linux-kernel - Re: [PATCH v2 2/2] ovl: get_acl: Fix null pointer dereference at realinode in rcu-walk mode

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]

Date:   Tue, 16 May 2023 14:40:52 +0300
From:   Amir Goldstein <amir73il@...il.com>
To:     Zhihao Cheng <chengzhihao1@...wei.com>
Cc:     miklos@...redi.hu, brauner@...nel.org,
        linux-unionfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] ovl: get_acl: Fix null pointer dereference at
 realinode in rcu-walk mode

On Mon, May 15, 2023 at 4:39 PM Zhihao Cheng <chengzhihao1@...wei.com> wrote:
>
> Following process:
>          P1                     P2
>  path_openat
>   link_path_walk
>    may_lookup
>     inode_permission(rcu)
>      ovl_permission
>       acl_permission_check
>        check_acl
>         get_cached_acl_rcu
>          ovl_get_inode_acl
>           realinode = ovl_inode_real(ovl_inode)
>                               drop_cache
>                                __dentry_kill(ovl_dentry)
>                                 iput(ovl_inode)
>                                  ovl_destroy_inode(ovl_inode)
>                                   dput(oi->__upperdentry)
>                                    dentry_kill(upperdentry)
>                                     dentry_unlink_inode
>                                      upperdentry->d_inode = NULL
>             ovl_inode_upper
>              upperdentry = ovl_i_dentry_upper(ovl_inode)
>              d_inode(upperdentry) // returns NULL
>           IS_POSIXACL(realinode) // NULL pointer dereference
> , will trigger an null pointer dereference at realinode:
>   [  205.472797] BUG: kernel NULL pointer dereference, address:
>                  0000000000000028
>   [  205.476701] CPU: 2 PID: 2713 Comm: ls Not tainted
>                  6.3.0-12064-g2edfa098e750-dirty #1216
>   [  205.478754] RIP: 0010:do_ovl_get_acl+0x5d/0x300
>   [  205.489584] Call Trace:
>   [  205.489812]  <TASK>
>   [  205.490014]  ovl_get_inode_acl+0x26/0x30
>   [  205.490466]  get_cached_acl_rcu+0x61/0xa0
>   [  205.490908]  generic_permission+0x1bf/0x4e0
>   [  205.491447]  ovl_permission+0x79/0x1b0
>   [  205.491917]  inode_permission+0x15e/0x2c0
>   [  205.492425]  link_path_walk+0x115/0x550
>   [  205.493311]  path_lookupat.isra.0+0xb2/0x200
>   [  205.493803]  filename_lookup+0xda/0x240
>   [  205.495747]  vfs_fstatat+0x7b/0xb0
>
> Fetch a reproducer in [Link].
>
> Fix it by using helper ovl_i_path_realinode() to get realpath and real
> inode after non-nullptr checking.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217404
> Fixes: 332f606b32b6 ("ovl: enable RCU'd ->get_acl()")

Note that this bug is also in 5.15.y, in method ovl_get_acl().
I hope you will be able to follow up with a simple backport for 5.15 -
i.e. only need to add a check for NULL realinode at the beginning.
There was no realpath back then.

AFAICT, both your patches should apply cleanly to 6.1.y, so should
be picked up automatically by stable kernel bots.

Thanks,
Amir.

> Signed-off-by: Zhihao Cheng <chengzhihao1@...wei.com>
> Suggested-by: Christian Brauner <brauner@...nel.org>
> ---
>  fs/overlayfs/inode.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index cc3ef5a6666a..b2021eada8be 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -576,20 +576,18 @@ struct posix_acl *do_ovl_get_acl(struct mnt_idmap *idmap,
>                                  struct inode *inode, int type,
>                                  bool rcu, bool noperm)
>  {
> -       struct inode *realinode = ovl_inode_real(inode);
> +       struct inode *realinode;
>         struct posix_acl *acl;
>         struct path realpath;
> +       int err;
> +
> +       err = ovl_i_path_realinode(inode, &realpath, &realinode, rcu);
> +       if (err)
> +               return ERR_PTR(err);
>
>         if (!IS_POSIXACL(realinode))
>                 return NULL;
>
> -       /* Careful in RCU walk mode */
> -       ovl_i_path_real(inode, &realpath);
> -       if (!realpath.dentry) {
> -               WARN_ON(!rcu);
> -               return ERR_PTR(-ECHILD);
> -       }
> -
>         if (rcu) {
>                 /*
>                  * If the layer is idmapped drop out of RCU path walk
> --
> 2.39.2
>