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

[Lukas Czerner]

On Wed, 7 Sep 2011, Lukas Czerner wrote:

> On Wed, 7 Sep 2011, Christoph Hellwig wrote:
> 
> > On Wed, Sep 07, 2011 at 12:05:14PM +0200, Lukas Czerner wrote:
> > > > > +	if (len > max_blks)
> > > > > +		len = max_blks - start;
> > > > 
> > > > Is this really the correct check?
> > > > 
> > > > Shouldn't it be
> > > > 
> > > > 	if (start + len > max_blks)
> > > > 		len = max_blks - start;
> > > > 
> > > > I'd also just use the mp->m_sb.sb_dblocks value directly instead
> > > > of assigning it to a local variable.
> > > > 
> > > 
> > > Agh, you're right. I am bit too hasty I guess. I thought that
> > > 
> > > if (start_agno >= mp->m_sb.sb_agcount)
> > > 	return -XFS_ERROR(EINVAL);
> > > 
> > > will cover us from the unreasonably big start, however if the file
> > > system has really huge number of AGs than it will fail to prevent the
> > > overflow, I am not sure if that is possible to happen, but what you
> > > proposed is definitely better.
> > 
> > The problem is that start could be very far into the fs, so checking
> > len alone won't help very much.  And we probably want a check if 
> > start + len is overflowing, too.
> 
> I do not think that start + len can overflow since we are doing
> XFS_B_TO_FSBT() on it first. Am I missing something ?
> 
> The commit description is a bit misleading since the overflow can happen
> when storing the value of XFS_FSB_TO_AGNO() rather than start
> + len alone. I'll update it as well.
> 
> Also this is wrong:
> 
> 	start_agno = XFS_FSB_TO_AGNO(mp, start);
> 	if (start_agno >= mp->m_sb.sb_agcount)
> 		return -XFS_ERROR(EINVAL);
> 
> because XFS_FSB_TO_AGNO() might overflow as I mentioned above. This is
> very similar problem to ext4 as well, but it is kind of hard to test
> since the both block size and AF might be different, moreover in XFS
> AG differs with the different fs size.
> 
> > 
> > Care to update the test case to cover these cases as well?
> > 
> 
> 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.
> 
> Thanks!
> -Lukas
> 
> Anyway what about this patch ?

Hi Christoph,

have you had a chance to look at this patch ?

Thanks!
-Lukas

> 
> Subject: [PATCH] xfs: fix possible overflow in xfs_ioc_trim()
> 
> In xfs_ioc_trim it is possible that computing the last allocation group
> to discard might overflow for big start & len values, because the result
> might be bigger then xfs_agnumber_t which is 32 bit long. Fix this by not
> allowing the start and end block of the range to be beyond the end of the
> file system.
> 
> Note that if the start is beyond the end of the file system we have to
> return -EINVAL, but in the "end" case we have to truncate it to the fs
> size.
> 
> Also introduce "end" variable, rather than using start+len which which
> might be more confusing to get right as this bug shows.
> 
> Signed-off-by: Lukas Czerner <lczerner at redhat.com>
> ---
>  fs/xfs/xfs_discard.c |   20 +++++++++++---------
>  1 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
> index 244e797..5ef3568 100644
> --- a/fs/xfs/xfs_discard.c
> +++ b/fs/xfs/xfs_discard.c
> @@ -38,7 +38,7 @@ xfs_trim_extents(
>  	struct xfs_mount	*mp,
>  	xfs_agnumber_t		agno,
>  	xfs_fsblock_t		start,
> -	xfs_fsblock_t		len,
> +	xfs_fsblock_t		end,
>  	xfs_fsblock_t		minlen,
>  	__uint64_t		*blocks_trimmed)
>  {
> @@ -100,7 +100,7 @@ xfs_trim_extents(
>  		 * down partially overlapping ranges for now.
>  		 */
>  		if (XFS_AGB_TO_FSB(mp, agno, fbno) + flen < start ||
> -		    XFS_AGB_TO_FSB(mp, agno, fbno) >= start + len) {
> +		    XFS_AGB_TO_FSB(mp, agno, fbno) > end) {
>  			trace_xfs_discard_exclude(mp, agno, fbno, flen);
>  			goto next_extent;
>  		}
> @@ -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, len, minlen;
> +	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);
> -	len = XFS_B_TO_FSBT(mp, range.len);
> +	end = start + XFS_B_TO_FSBT(mp, range.len) - 1;
>  	minlen = XFS_B_TO_FSB(mp, max_t(u64, granularity, range.minlen));
>  
> -	start_agno = XFS_FSB_TO_AGNO(mp, start);
> -	if (start_agno >= mp->m_sb.sb_agcount)
> +	if (start >= mp->m_sb.sb_dblocks)
>  		return -XFS_ERROR(EINVAL);
> +	start_agno = XFS_FSB_TO_AGNO(mp, start);
>  
> -	end_agno = XFS_FSB_TO_AGNO(mp, start + len);
> -	if (end_agno >= mp->m_sb.sb_agcount)
> +	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);
>  
>  	for (agno = start_agno; agno <= end_agno; agno++) {
> -		error = -xfs_trim_extents(mp, agno, start, len, minlen,
> +		error = -xfs_trim_extents(mp, agno, start, end, minlen,
>  					  &blocks_trimmed);
>  		if (error)
>  			last_error = error;
> 

--