Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230515133629.1974610-1-chengzhihao1@huawei.com>
 <20230515133629.1974610-3-chengzhihao1@huawei.com> <CAOQ4uxj-j-ugHrvvfS+XF=rzE3NH_NaZgf4_rWBxvaGYO3iN-w@mail.gmail.com>
 <1f11e2f3-bdf8-553c-82fa-b24619e28e73@huawei.com>
In-Reply-To: <1f11e2f3-bdf8-553c-82fa-b24619e28e73@huawei.com>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Tue, 16 May 2023 15:46:46 +0300
Message-ID: <CAOQ4uxigfZhtfBv9VLvfcxcyj7NjYs9-rJycAep0xCNefnHm5w@mail.gmail.com>
Subject: Re: [PATCH v2 2/2] ovl: get_acl: Fix null pointer dereference at
 realinode in rcu-walk mode
To:     Zhihao Cheng <chengzhihao1@huawei.com>
Cc:     miklos@szeredi.hu, brauner@kernel.org,
        linux-unionfs@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, May 16, 2023 at 3:25=E2=80=AFPM Zhihao Cheng <chengzhihao1@huawei.c=
om> wrote:
>
> =E5=9C=A8 2023/5/16 19:40, Amir Goldstein =E5=86=99=E9=81=93:
> > On Mon, May 15, 2023 at 4:39=E2=80=AFPM Zhihao Cheng <chengzhihao1@huaw=
ei.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 =3D 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 =3D NULL
> >>              ovl_inode_upper
> >>               upperdentry =3D 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=3D217404
> >> 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@kernel.

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

Thanks,
Amir.