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

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

[Eric Sandeen]

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>
---

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

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

[Eric Sandeen]

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...

NAK - this needs a bit more love; if we catch this and fail,
the caller may still do something crazy with this data.

V2 coming in a bit.

-Eric

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

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

[Eric Sandeen]

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

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

[Mark Tinguely]

On 11/19/14 16:12, 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>
> ---

Looks good.

I did a little playing with sending the try lock failure (EAGAIN?) and 
EFSCORRUPT error status up the stack. It looked straight forward and 
could save a xfs_buf allocation when we know it is not necessary.

Reviewed-by: Mark Tinguely <tinguely@sgi.com>

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