lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAKFNMom+we=aMXZz_f4zVGR6VRO3Zj+y1GC=KZSwLq8MdxO7BQ@mail.gmail.com>
Date: Tue, 3 Sep 2024 04:40:21 +0900
From: Ryusuke Konishi <konishi.ryusuke@...il.com>
To: Lizhi Xu <lizhi.xu@...driver.com>
Cc: syzbot+9bff4c7b992038a7409f@...kaller.appspotmail.com, 
	linux-kernel@...r.kernel.org, linux-nilfs@...r.kernel.org, 
	syzkaller-bugs@...glegroups.com
Subject: Re: [syzbot] [nilfs?] general protection fault in nilfs_btree_insert (2)

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,
>  };

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ