Re: [syzbot] [nilfs?] general protection fault in nilfs_btree_insert (2)

[Ryusuke Konishi <konishi.ryusuke@xxxxxxxxx>]

On Mon, Sep 2, 2024 at 5:41 PM Lizhi Xu wrote:
>
> In nilfs_btree_do_lookup, if the number of children in the btree root node is 0,
> path[x].bp_bh will not be initialized by __nilfs_btree_get_block,
> which will result in uaf when executing nilfs-btree_get_nonroot_node
> in nilfs_btree_prepare_insert.
>
> In nilfs_bmap_do_insert will run bop_check_insert, so implement
> bop_check_insert and determine the number of children in the btree root
> node within it. If it is 0, return a negative value to avoid calling
> bop_intsert.

Thank you Lizhi for helping us solve this problem.

However, your proposed patch has a few issues (more on that later),
and this anomaly detection should be done when the root node is read,
not just before insertion.  So, instead of adding bop_check_insert, I
will modify the existing sanity check routine
nilfs_btree_root_broken() by adding the following failure condition:

diff --git a/fs/nilfs2/btree.c b/fs/nilfs2/btree.c
index 862bdf23120e..d390b8ba00d4 100644
--- a/fs/nilfs2/btree.c
+++ b/fs/nilfs2/btree.c
@@ -381,7 +381,8 @@ static int nilfs_btree_root_broken(const struct
nilfs_btree_node *node,
        if (unlikely(level < NILFS_BTREE_LEVEL_NODE_MIN ||
                     level >= NILFS_BTREE_LEVEL_MAX ||
                     nchildren < 0 ||
-                    nchildren > NILFS_BTREE_ROOT_NCHILDREN_MAX)) {
+                    nchildren > NILFS_BTREE_ROOT_NCHILDREN_MAX ||
+                    (nchildren == 0 && level > NILFS_BTREE_LEVEL_NODE_MIN))) {
                nilfs_crit(inode->i_sb,
                           "bad btree root (ino=%lu): level = %d,
flags = 0x%x, nchildren = %d",
                           inode->i_ino, level, flags, nchildren);

---
Going back to your proposed patch, there are two problems:

(1) nilfs_btree_node_get_nchildren(node) == 0 is a normal state in
itself, so it is not OK to return this as an error.

(2) Even if we use bop_check_insert to perform a sanity check, the
error code that should be returned is -EINVAL.
-ENOENT is an internal error code, but if bop_check_insert() returns
it, it may be wrongly exposed to userspace (i.e. system
calls).  If it is -EINVAL, it will be treated as metadata corruption
at the bmap layer with nilfs_bmap_convert_error(), and the error code
will be converted for return to userspace.

So this time I'll be fixing and testing it myself, and will mention
your help in the patch commit log.

Thanks,
Ryusuke Konishi

>
> #syz test
>
> diff --git a/fs/nilfs2/btree.c b/fs/nilfs2/btree.c
> index 862bdf23120e..d7fa4d914638 100644
> --- a/fs/nilfs2/btree.c
> +++ b/fs/nilfs2/btree.c
> @@ -1231,6 +1231,17 @@ static void nilfs_btree_commit_insert(struct nilfs_bmap *btree,
>                 nilfs_bmap_set_dirty(btree);
>  }
>
> +static int nilfs_btree_check_insert(const struct nilfs_bmap *btree, __u64 key)
> +{
> +       struct nilfs_btree_node *node;
> +       int level;
> +
> +       node = nilfs_btree_get_root(btree);
> +       level = nilfs_btree_node_get_level(node);
> +       return (level < NILFS_BTREE_LEVEL_NODE_MIN ||
> +               nilfs_btree_node_get_nchildren(node) <= 0) ? -ENOENT : 0;
> +}
> +
>  static int nilfs_btree_insert(struct nilfs_bmap *btree, __u64 key, __u64 ptr)
>  {
>         struct nilfs_btree_path *path;
> @@ -2385,7 +2396,7 @@ static const struct nilfs_bmap_operations nilfs_btree_ops = {
>         .bop_seek_key           =       nilfs_btree_seek_key,
>         .bop_last_key           =       nilfs_btree_last_key,
>
> -       .bop_check_insert       =       NULL,
> +       .bop_check_insert       =       nilfs_btree_check_insert,
>         .bop_check_delete       =       nilfs_btree_check_delete,
>         .bop_gather_data        =       nilfs_btree_gather_data,
>  };