* [PATCH] xfs: abort extended attribute list operation if btree is obviously weird
@ 2017-10-24 0:28 Darrick J. Wong
2017-10-24 12:53 ` Brian Foster
0 siblings, 1 reply; 3+ messages in thread
From: Darrick J. Wong @ 2017-10-24 0:28 UTC (permalink / raw)
To: xfs
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
@@ -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);
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] xfs: abort extended attribute list operation if btree is obviously weird
2017-10-24 0:28 [PATCH] xfs: abort extended attribute list operation if btree is obviously weird Darrick J. Wong
@ 2017-10-24 12:53 ` Brian Foster
2017-10-24 17:00 ` Darrick J. Wong
0 siblings, 1 reply; 3+ messages in thread
From: Brian Foster @ 2017-10-24 12:53 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: xfs
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] xfs: abort extended attribute list operation if btree is obviously weird
2017-10-24 12:53 ` Brian Foster
@ 2017-10-24 17:00 ` Darrick J. Wong
0 siblings, 0 replies; 3+ messages in thread
From: Darrick J. Wong @ 2017-10-24 17:00 UTC (permalink / raw)
To: Brian Foster; +Cc: xfs
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-10-24 17:00 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-24 0:28 [PATCH] xfs: abort extended attribute list operation if btree is obviously weird Darrick J. Wong
2017-10-24 12:53 ` Brian Foster
2017-10-24 17:00 ` Darrick J. Wong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).