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] [day] [month] [year] [list]

Message-ID: <CAOQ4uxigfZhtfBv9VLvfcxcyj7NjYs9-rJycAep0xCNefnHm5w@mail.gmail.com>
Date:   Tue, 16 May 2023 15:46:46 +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 Tue, May 16, 2023 at 3:25 PM Zhihao Cheng <chengzhihao1@...wei.com> wrote:
>
> 在 2023/5/16 19:40, Amir Goldstein 写道:
> > 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.
> >
>
> Sure. I notice that there is a '[ Upstream commit xxx ]' field in 5.15
> patch, so may I backport it after the fix applied into mainline(6.4)?
>

Yes, you need to wait until the fix is merged to mainline before
posting a backport.

You'd usually prefix the patch subject with [PATCH 5.15] and send
it to stable@...nel.

It'd be good to mention the changes from upstream commit,
because this is not a trivial backport.

Thanks,
Amir.