| 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
| ||
|
Message-ID: <CAOQ4uxjageSA8G1UR=9KPCYm80=GFLrwVonEotjOf0tpZDBY+g@mail.gmail.com>
Date: Mon, 15 May 2023 18:36:27 +0300
From: Amir Goldstein <amir73il@...il.com>
To: Christian Brauner <brauner@...nel.org>
Cc: Zhihao Cheng <chengzhihao1@...wei.com>, miklos@...redi.hu,
linux-unionfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] ovl: ovl_permission: Fix null pointer dereference
at realinode in rcu-walk mode
On Mon, May 15, 2023 at 6:16 PM Christian Brauner <brauner@...nel.org> wrote:
>
> On Mon, May 15, 2023 at 05:58:55PM +0300, Amir Goldstein wrote:
> > On Mon, May 15, 2023 at 4:58 PM Christian Brauner <brauner@...nel.org> wrote:
> > >
> > > On Mon, May 15, 2023 at 09:36:28PM +0800, Zhihao Cheng wrote:
> > > > Following process:
> > > > P1 P2
> > > > path_lookupat
> > > > link_path_walk
> > > > inode_permission
> > > > ovl_permission
> > > > ovl_i_path_real(inode, &realpath)
> > > > path->dentry = ovl_i_dentry_upper(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
> > > > realinode = d_inode(realpath.dentry) // return NULL
> > > > inode_permission(realinode)
> > > > inode->i_sb // NULL pointer dereference
> > > > , will trigger an null pointer dereference at realinode:
> > > > [ 335.664979] BUG: kernel NULL pointer dereference,
> > > > address: 0000000000000002
> > > > [ 335.668032] CPU: 0 PID: 2592 Comm: ls Not tainted 6.3.0
> > > > [ 335.669956] RIP: 0010:inode_permission+0x33/0x2c0
> > > > [ 335.678939] Call Trace:
> > > > [ 335.679165] <TASK>
> > > > [ 335.679371] ovl_permission+0xde/0x320
> > > > [ 335.679723] inode_permission+0x15e/0x2c0
> > > > [ 335.680090] link_path_walk+0x115/0x550
> > > > [ 335.680771] path_lookupat.isra.0+0xb2/0x200
> > > > [ 335.681170] filename_lookup+0xda/0x240
> > > > [ 335.681922] vfs_statx+0xa6/0x1f0
> > > > [ 335.682233] vfs_fstatat+0x7b/0xb0
> > > >
> > > > Fetch a reproducer in [Link].
> > > >
> > > > Add a new helper ovl_i_path_realinode() to get realpath and real inode
> > > > after non-nullptr checking, use the helper to replace the current realpath
> > > > getting logic.
> > > >
> > > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217405
> > > > Fixes: 4b7791b2e958 ("ovl: handle idmappings in ovl_permission()")
> > > > Signed-off-by: Zhihao Cheng <chengzhihao1@...wei.com>
> > > > Suggested-by: Christian Brauner <brauner@...nel.org>
> > > > ---
> > > > fs/overlayfs/inode.c | 31 ++++++++++++++++++++++++-------
> > > > 1 file changed, 24 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> > > > index 541cf3717fc2..cc3ef5a6666a 100644
> > > > --- a/fs/overlayfs/inode.c
> > > > +++ b/fs/overlayfs/inode.c
> > > > @@ -278,6 +278,26 @@ int ovl_getattr(struct mnt_idmap *idmap, const struct path *path,
> > > > return err;
> > > > }
> > > >
> > > > +static inline int ovl_i_path_realinode(struct inode *inode,
> > > > + struct path *realpath,
> > > > + struct inode **realinode, int rcu)
> > > > +{
> > > > + /* Careful in RCU walk mode */
> > > > + ovl_i_path_real(inode, realpath);
> > > > + if (!realpath->dentry) {
> > > > + WARN_ON(!rcu);
> > > > + return -ECHILD;
> > > > + }
> > > > +
> > > > + *realinode = d_inode(realpath->dentry);
> > > > + if (!*realinode) {
> > > > + WARN_ON(!rcu);
> > > > + return -ECHILD;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > >
> > > If you want to return the inode wouldn't it possibly make more sense to
> > > return the inode from the function directly? But not fuzzed. Maybe Amir
> > > has a preference. As I said, I'm even fine with the original approach.
> >
> > Sorry for not reviewing v1, I was traveling, even though it is hard to use
> > this excuse when I was traveling with Christian who did review v1 :)
>
> Well, I did only do it this morning. :)
>
> >
> > >
> > > static inline struct inode *ovl_i_path_realinode(struct inode *inode,
> > > struct path *realpath,
> > > int rcu)
> > > {
> > > struct inode *realinode;
> > >
> > > /* Careful in RCU walk mode */
> > > ovl_i_path_real(inode, realpath);
> > > if (!realpath->dentry) {
> > > WARN_ON(!rcu);
> > > return ERR_PTR(-ECHILD);
> > > }
> > >
> > > realinode = d_inode(realpath->dentry);
> > > if (!realinode) {
> > > WARN_ON(!rcu);
> > > return ERR_PTR(-ECHILD);
> > > }
> > >
> > > return realinode;
> > > }
> > >
> >
> > I think this helper is over engineered ;-)
>
> Yes. As mentioned before, I would've been happy even with v1 that didn't
> have any helper.
>
> > The idea for a helper that returns inode is good,
> > but I see no reason to mix RCU walk in this helper
> > and don't even need a new helper (see untested patch below).
>
> Looks good to me too.
>
> >
> > Thanks,
> > Amir.
> >
> > ---
> > -void ovl_i_path_real(struct inode *inode, struct path *path)
> > +struct inode *ovl_i_path_real(struct inode *inode, struct path *path)
> > {
> > struct ovl_path *lowerpath = ovl_lowerpath(OVL_I_E(inode));
> >
> > @@ -342,6 +342,8 @@ void ovl_i_path_real(struct inode *inode, struct path *path)
> > } else {
> > path->mnt = ovl_upper_mnt(OVL_FS(inode->i_sb));
> > }
> > +
> > + return path->dentry ? d_inode(path->dentry) : NULL;
> > }
> >
> > @@ -295,8 +295,8 @@ int ovl_permission(struct mnt_idmap *idmap,
> > int err;
> >
> > /* Careful in RCU walk mode */
> > - ovl_i_path_real(inode, &realpath);
> > - if (!realpath.dentry) {
> > + realinode = ovl_i_path_real(inode, &realpath);
> > + if (!realpath.dentry || !realinode) {
> > WARN_ON(!(mask & MAY_NOT_BLOCK));
> > return -ECHILD;
> > }
> > @@ -309,7 +309,6 @@ int ovl_permission(struct mnt_idmap *idmap,
> >
> > if (err)
> > return err;
> >
> > - realinode = d_inode(realpath.dentry);
> > old_cred = ovl_override_creds(inode->i_sb);
>
> Btw, if the reproducer that Zhihao has posted in the bugzilla link:
>
> #!/bin/bash
>
> mkdir -p /root/tmp/merge /root/tmp/upper/dir /root/tmp/lower /root/tmp/work
> touch /root/tmp/upper/dir/file
> chown freg:freg -R /root/tmp/upper/dir
> mount -t overlay none -oupperdir=/root/tmp/upper,lowerdir=/root/tmp/lower,workdir=/root/tmp/work /root/tmp/merge
> ls /root/tmp/merge/dir/file &
> echo 3 > /proc/sys/vm/drop_caches
>
> is reliable we should add it to xfstests...
If only it was that easy to trigger this race.
Look at the debug kernel patch named 'diff' in bugzilla...
Thanks,
Amir.
Powered by blists - more mailing lists