[PATCH v2] xfs: fix possible overflow in xfs_ioc_trim()

[Lukas Czerner]

On Tue, 20 Sep 2011, Christoph Hellwig wrote:

> > 
> > I am not sure what do you mean ? There already is a check when both
> > start and len are huge numbers. I am not sure if we can do more without
> > significantly complicating the test to cover various start, or len
> > numbers where can the fsblock->group_number overflow for various file
> > systems.
> 
> Add a testcase where start is a relatively small number (smaller than an
> AG/BG), but start + len is outside the fs.

Already done in the second version of the xfstests patch.

> 
> > @@ -145,7 +145,7 @@ xfs_ioc_trim(
> >  	struct request_queue	*q = mp->m_ddev_targp->bt_bdev->bd_disk->queue;
> >  	unsigned int		granularity = q->limits.discard_granularity;
> >  	struct fstrim_range	range;
> > +	xfs_fsblock_t		start, end, minlen;
> >  	xfs_agnumber_t		start_agno, end_agno, agno;
> >  	__uint64_t		blocks_trimmed = 0;
> >  	int			error, last_error = 0;
> > @@ -165,19 +165,21 @@ xfs_ioc_trim(
> >  	 * matter as trimming blocks is an advisory interface.
> >  	 */
> >  	start = XFS_B_TO_FSBT(mp, range.start);
> > +	end = start + XFS_B_TO_FSBT(mp, range.len) - 1;
> >  	minlen = XFS_B_TO_FSB(mp, max_t(u64, granularity, range.minlen));
> >  
> > +	if (start >= mp->m_sb.sb_dblocks)
> >  		return -XFS_ERROR(EINVAL);
> > +	start_agno = XFS_FSB_TO_AGNO(mp, start);
> >  
> > +	if (end >= mp->m_sb.sb_dblocks) {
> > +		end = mp->m_sb.sb_dblocks - 1;
> >  		end_agno = mp->m_sb.sb_agcount - 1;
> > +	} else
> > +		end_agno = XFS_FSB_TO_AGNO(mp, end);
> 
> I'd rather do something like:
> 
> 	if (start >= mp->m_sb.sb_dblocks)
>   		return -XFS_ERROR(EINVAL);
> 	if (end > mp->m_sb.sb_dblocks - 1)
> 		end = mp->m_sb.sb_dblocks - 1;
> 
> 
> 	start_agno = XFS_FSB_TO_AGNO(mp, start);
> 	end_agno = XFS_FSB_TO_AGNO(mp, end)
> 
> here.
> 
> Otherwise the patch looks fine.
> 

Thanks!
-Lukas