[PATCH] xfs: don't perform lookups on zero-height btrees

[PATCH] xfs: don't perform lookups on zero-height btrees

[Darrick J. Wong]

If the caller passes in a cursor to a zero-height btree (which is
impossible), we never set block to anything but NULL, which causes the
later dereference of it to crash.  Instead, just return -EFSCORRUPTED.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_btree.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
index 64fd847..4bffea4 100644
--- a/fs/xfs/libxfs/xfs_btree.c
+++ b/fs/xfs/libxfs/xfs_btree.c
@@ -1814,6 +1814,10 @@ xfs_btree_lookup(
 
 	XFS_BTREE_STATS_INC(cur, lookup);
 
+	/* No such thing as a zero-level tree. */
+	if (cur->bc_nlevels == 0)
+		return -EFSCORRUPTED;
+
 	block = NULL;
 	keyno = 0;
 

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

Re: [PATCH] xfs: don't perform lookups on zero-height btrees

[Brian Foster]

On Fri, Aug 19, 2016 at 01:30:22PM -0700, Darrick J. Wong wrote:
> If the caller passes in a cursor to a zero-height btree (which is
> impossible), we never set block to anything but NULL, which causes the
> later dereference of it to crash.  Instead, just return -EFSCORRUPTED.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Did something actually cause this to happen?

Brian

>  fs/xfs/libxfs/xfs_btree.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
> index 64fd847..4bffea4 100644
> --- a/fs/xfs/libxfs/xfs_btree.c
> +++ b/fs/xfs/libxfs/xfs_btree.c
> @@ -1814,6 +1814,10 @@ xfs_btree_lookup(
>  
>  	XFS_BTREE_STATS_INC(cur, lookup);
>  
> +	/* No such thing as a zero-level tree. */
> +	if (cur->bc_nlevels == 0)
> +		return -EFSCORRUPTED;
> +
>  	block = NULL;
>  	keyno = 0;
>  
> 
> _______________________________________________
> 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: don't perform lookups on zero-height btrees

[Darrick J. Wong]

On Wed, Aug 24, 2016 at 02:01:55PM -0400, Brian Foster wrote:
> On Fri, Aug 19, 2016 at 01:30:22PM -0700, Darrick J. Wong wrote:
> > If the caller passes in a cursor to a zero-height btree (which is
> > impossible), we never set block to anything but NULL, which causes the
> > later dereference of it to crash.  Instead, just return -EFSCORRUPTED.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> 
> Did something actually cause this to happen?

Well, if you took a reflink xfs that was formatted before we added the
rmap/refcount btree block counters to the AGF and then tried to mount
it after will blow up like this, because the refcount_level field got
moved to somewhere that's zero in the old image.

Anyone being malicious with xfs_db can also do this to any other _level field.

(Yay automounting!)

--D

> 
> Brian
> 
> >  fs/xfs/libxfs/xfs_btree.c |    4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
> > index 64fd847..4bffea4 100644
> > --- a/fs/xfs/libxfs/xfs_btree.c
> > +++ b/fs/xfs/libxfs/xfs_btree.c
> > @@ -1814,6 +1814,10 @@ xfs_btree_lookup(
> >  
> >  	XFS_BTREE_STATS_INC(cur, lookup);
> >  
> > +	/* No such thing as a zero-level tree. */
> > +	if (cur->bc_nlevels == 0)
> > +		return -EFSCORRUPTED;
> > +
> >  	block = NULL;
> >  	keyno = 0;
> >  
> > 
> > _______________________________________________
> > 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: don't perform lookups on zero-height btrees

[Christoph Hellwig]

On Fri, Aug 19, 2016 at 01:30:22PM -0700, Darrick J. Wong wrote:
> If the caller passes in a cursor to a zero-height btree (which is
> impossible), we never set block to anything but NULL, which causes the
> later dereference of it to crash.  Instead, just return -EFSCORRUPTED.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

maybe also throw in an unlikely notation, though..

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