xfs: abort extended attribute list operation if btree is obviously weird
diff mbox

Message ID 20171024002824.GL5483@magnolia
State New
Headers show

Commit Message

Darrick J. Wong Oct. 24, 2017, 12:28 a.m. UTC
Abort an attribute list operation if the attr btree has obvious problems
like loops back to the root or pointers don't point down the tree.
Found by fuzzing btree[0].before to zero in xfs/402.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_attr_list.c |   54 ++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 11 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Brian Foster Oct. 24, 2017, 12:53 p.m. UTC | #1
On Mon, Oct 23, 2017 at 05:28:24PM -0700, Darrick J. Wong wrote:
> Abort an attribute list operation if the attr btree has obvious problems
> like loops back to the root or pointers don't point down the tree.
> Found by fuzzing btree[0].before to zero in xfs/402.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_attr_list.c |   54 ++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 43 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
> index 5816786..9f6fcc6 100644
> --- a/fs/xfs/xfs_attr_list.c
> +++ b/fs/xfs/xfs_attr_list.c
...
> @@ -302,6 +306,30 @@ xfs_attr_node_list(xfs_attr_list_context_t *context)
>  			}
>  
>  			dp->d_ops->node_hdr_from_disk(&nodehdr, node);
> +
> +			/* Tree taller than we can handle; bail out! */
> +			if (nodehdr.level >= XFS_DA_NODE_MAXDEPTH) {
> +				xfs_trans_brelse(context->tp, bp);
> +				return -EFSCORRUPTED;
> +			}
> +
> +			if (cursor->blkno == 0) {
> +				/*
> +				 * This is the root node, set up for the
> +				 * next level we want to see.
> +				 */
> +				expected_level = nodehdr.level - 1;
> +			} else if (expected_level != nodehdr.level) {
> +				/*
> +				 * Not the level we were expecting, which
> +				 * implies that the tree is bad.
> +				 */
> +				xfs_trans_brelse(context->tp, bp);
> +				return -EFSCORRUPTED;
> +			} else {
> +				expected_level--;
> +			}
> +

It looks like we wouldn't catch the case of a two level tree having a
bad level, since we'd break out on leaf magic check above. Perhaps we
should check or assert that expected_level reaches 0? Otherwise looks Ok
to me.

Brian

>  			btree = dp->d_ops->node_tree_p(node);
>  			for (i = 0; i < nodehdr.count; btree++, i++) {
>  				if (cursor->hashval
> @@ -317,6 +345,10 @@ xfs_attr_node_list(xfs_attr_list_context_t *context)
>  				return 0;
>  			}
>  			xfs_trans_brelse(context->tp, bp);
> +
> +			/* We can't point back to the root. */
> +			if (cursor->blkno == 0)
> +				return -EFSCORRUPTED;
>  		}
>  	}
>  	ASSERT(bp != NULL);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darrick J. Wong Oct. 24, 2017, 5 p.m. UTC | #2
On Tue, Oct 24, 2017 at 08:53:36AM -0400, Brian Foster wrote:
> On Mon, Oct 23, 2017 at 05:28:24PM -0700, Darrick J. Wong wrote:
> > Abort an attribute list operation if the attr btree has obvious problems
> > like loops back to the root or pointers don't point down the tree.
> > Found by fuzzing btree[0].before to zero in xfs/402.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/xfs_attr_list.c |   54 ++++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 43 insertions(+), 11 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
> > index 5816786..9f6fcc6 100644
> > --- a/fs/xfs/xfs_attr_list.c
> > +++ b/fs/xfs/xfs_attr_list.c
> ...
> > @@ -302,6 +306,30 @@ xfs_attr_node_list(xfs_attr_list_context_t *context)
> >  			}
> >  
> >  			dp->d_ops->node_hdr_from_disk(&nodehdr, node);
> > +
> > +			/* Tree taller than we can handle; bail out! */
> > +			if (nodehdr.level >= XFS_DA_NODE_MAXDEPTH) {
> > +				xfs_trans_brelse(context->tp, bp);
> > +				return -EFSCORRUPTED;
> > +			}
> > +
> > +			if (cursor->blkno == 0) {
> > +				/*
> > +				 * This is the root node, set up for the
> > +				 * next level we want to see.
> > +				 */
> > +				expected_level = nodehdr.level - 1;
> > +			} else if (expected_level != nodehdr.level) {
> > +				/*
> > +				 * Not the level we were expecting, which
> > +				 * implies that the tree is bad.
> > +				 */
> > +				xfs_trans_brelse(context->tp, bp);
> > +				return -EFSCORRUPTED;
> > +			} else {
> > +				expected_level--;
> > +			}
> > +
> 
> It looks like we wouldn't catch the case of a two level tree having a
> bad level, since we'd break out on leaf magic check above. Perhaps we
> should check or assert that expected_level reaches 0? Otherwise looks Ok
> to me.

Ok, will do.

--D

> 
> Brian
> 
> >  			btree = dp->d_ops->node_tree_p(node);
> >  			for (i = 0; i < nodehdr.count; btree++, i++) {
> >  				if (cursor->hashval
> > @@ -317,6 +345,10 @@ xfs_attr_node_list(xfs_attr_list_context_t *context)
> >  				return 0;
> >  			}
> >  			xfs_trans_brelse(context->tp, bp);
> > +
> > +			/* We can't point back to the root. */
> > +			if (cursor->blkno == 0)
> > +				return -EFSCORRUPTED;
> >  		}
> >  	}
> >  	ASSERT(bp != NULL);
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
index 5816786..9f6fcc6 100644
--- a/fs/xfs/xfs_attr_list.c
+++ b/fs/xfs/xfs_attr_list.c
@@ -205,18 +205,21 @@  xfs_attr_shortform_list(xfs_attr_list_context_t *context)
 }
 
 STATIC int
-xfs_attr_node_list(xfs_attr_list_context_t *context)
+xfs_attr_node_list(
+	struct xfs_attr_list_context	*context)
 {
-	attrlist_cursor_kern_t *cursor;
-	xfs_attr_leafblock_t *leaf;
-	xfs_da_intnode_t *node;
-	struct xfs_attr3_icleaf_hdr leafhdr;
-	struct xfs_da3_icnode_hdr nodehdr;
-	struct xfs_da_node_entry *btree;
-	int error, i;
-	struct xfs_buf *bp;
-	struct xfs_inode	*dp = context->dp;
-	struct xfs_mount	*mp = dp->i_mount;
+	struct xfs_attr3_icleaf_hdr	leafhdr;
+	struct xfs_da3_icnode_hdr	nodehdr;
+	struct attrlist_cursor_kern	*cursor;
+	struct xfs_attr_leafblock	*leaf;
+	struct xfs_da_intnode		*node;
+	struct xfs_da_node_entry	*btree;
+	struct xfs_buf			*bp;
+	struct xfs_inode		*dp = context->dp;
+	struct xfs_mount		*mp = dp->i_mount;
+	int				i;
+	unsigned int			expected_level;
+	int				error;
 
 	trace_xfs_attr_node_list(context);
 
@@ -277,6 +280,7 @@  xfs_attr_node_list(xfs_attr_list_context_t *context)
 	 * Note that start of node block is same as start of leaf block.
 	 */
 	if (bp == NULL) {
+		expected_level = -1U;
 		cursor->blkno = 0;
 		for (;;) {
 			uint16_t magic;
@@ -302,6 +306,30 @@  xfs_attr_node_list(xfs_attr_list_context_t *context)
 			}
 
 			dp->d_ops->node_hdr_from_disk(&nodehdr, node);
+
+			/* Tree taller than we can handle; bail out! */
+			if (nodehdr.level >= XFS_DA_NODE_MAXDEPTH) {
+				xfs_trans_brelse(context->tp, bp);
+				return -EFSCORRUPTED;
+			}
+
+			if (cursor->blkno == 0) {
+				/*
+				 * This is the root node, set up for the
+				 * next level we want to see.
+				 */
+				expected_level = nodehdr.level - 1;
+			} else if (expected_level != nodehdr.level) {
+				/*
+				 * Not the level we were expecting, which
+				 * implies that the tree is bad.
+				 */
+				xfs_trans_brelse(context->tp, bp);
+				return -EFSCORRUPTED;
+			} else {
+				expected_level--;
+			}
+
 			btree = dp->d_ops->node_tree_p(node);
 			for (i = 0; i < nodehdr.count; btree++, i++) {
 				if (cursor->hashval
@@ -317,6 +345,10 @@  xfs_attr_node_list(xfs_attr_list_context_t *context)
 				return 0;
 			}
 			xfs_trans_brelse(context->tp, bp);
+
+			/* We can't point back to the root. */
+			if (cursor->blkno == 0)
+				return -EFSCORRUPTED;
 		}
 	}
 	ASSERT(bp != NULL);