Re: [PATCH] xfs: catch invalid negative blknos in _xfs_buf_find()

[Eric Sandeen <sandeen@sandeen.net>]

On 11/19/14 4:12 PM, Eric Sandeen wrote:
> Here blkno is a daddr_t, which is a __s64; it's possible to hold
> a value which is negative, and thus pass the (blkno >= eofs)
> test.  Then we try to do a xfs_perag_get() for a ridiculous
> agno via xfs_daddr_to_agno(), and bad things happen when that
> fails, and returns a null pag which is dereferenced shortly
> thereafter.
> 
> Found via a user-supplied fuzzed image...
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Ok, so after further digging, I think this patch is fine on its own,
and could use review.  How and if and when this function returns errors,
and whether errors should get this far, etc, are all subjects for other
patches.  IF we're range-checking here, we should do both halves
of the range checking properly so ... review appreciated.

Thanks,
-Eric

> ---
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 24b4ebe..f54a497 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -463,7 +463,7 @@ _xfs_buf_find(
>  	 * have to check that the buffer falls within the filesystem bounds.
>  	 */
>  	eofs = XFS_FSB_TO_BB(btp->bt_mount, btp->bt_mount->m_sb.sb_dblocks);
> -	if (blkno >= eofs) {
> +	if (blkno < 0 || blkno >= eofs) {
>  		/*
>  		 * XXX (dgc): we should really be returning -EFSCORRUPTED here,
>  		 * but none of the higher level infrastructure supports
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs