| Message ID | 20171024002824.GL5483@magnolia (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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);
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