[PATCH v2 0/2] ovl: Fix null ptr dereference at realinode in rcu-walk

[PATCH v2 0/2] ovl: Fix null ptr dereference at realinode in rcu-walk

[Zhihao Cheng]

v1->v2:
  Extract a helper to get realpath and real inode from ovl inode.
  Get realinode from realpath in do_ovl_get_acl().

Zhihao Cheng (2):
  ovl: ovl_permission: Fix null pointer dereference at realinode in
    rcu-walk mode
  ovl: get_acl: Fix null pointer dereference at realinode in rcu-walk
    mode

 fs/overlayfs/inode.c | 45 +++++++++++++++++++++++++++++---------------
 1 file changed, 30 insertions(+), 15 deletions(-)

-- 
2.39.2

[PATCH v2 1/2] ovl: ovl_permission: Fix null pointer dereference at realinode in rcu-walk mode

[Zhihao Cheng]

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@huawei.com>
Suggested-by: Christian Brauner <brauner@kernel.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;
+}
+
 int ovl_permission(struct mnt_idmap *idmap,
 		   struct inode *inode, int mask)
 {
@@ -287,12 +307,10 @@ int ovl_permission(struct mnt_idmap *idmap,
 	const struct cred *old_cred;
 	int err;
 
-	/* Careful in RCU walk mode */
-	ovl_i_path_real(inode, &realpath);
-	if (!realpath.dentry) {
-		WARN_ON(!(mask & MAY_NOT_BLOCK));
-		return -ECHILD;
-	}
+	err = ovl_i_path_realinode(inode, &realpath, &realinode,
+				   mask & MAY_NOT_BLOCK);
+	if (err)
+		return err;
 
 	/*
 	 * Check overlay inode with the creds of task and underlying inode
@@ -302,7 +320,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);
 	if (!upperinode &&
 	    !special_file(realinode->i_mode) && mask & MAY_WRITE) {
-- 
2.39.2

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

[Christian Brauner]

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@huawei.com>
> Suggested-by: Christian Brauner <brauner@kernel.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.

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

> +
>  int ovl_permission(struct mnt_idmap *idmap,
>  		   struct inode *inode, int mask)
>  {
> @@ -287,12 +307,10 @@ int ovl_permission(struct mnt_idmap *idmap,
>  	const struct cred *old_cred;
>  	int err;
>  
> -	/* Careful in RCU walk mode */
> -	ovl_i_path_real(inode, &realpath);
> -	if (!realpath.dentry) {
> -		WARN_ON(!(mask & MAY_NOT_BLOCK));
> -		return -ECHILD;
> -	}
> +	err = ovl_i_path_realinode(inode, &realpath, &realinode,
> +				   mask & MAY_NOT_BLOCK);
> +	if (err)
> +		return err;
>  
>  	/*
>  	 * Check overlay inode with the creds of task and underlying inode
> @@ -302,7 +320,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);
>  	if (!upperinode &&
>  	    !special_file(realinode->i_mode) && mask & MAY_WRITE) {
> -- 
> 2.39.2
> 

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

[Amir Goldstein]

On Mon, May 15, 2023 at 4:58 PM Christian Brauner <brauner@kernel.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@huawei.com>
> > Suggested-by: Christian Brauner <brauner@kernel.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 :)

>
> 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 ;-)
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).

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);

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

[Christian Brauner]

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@kernel.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@huawei.com>
> > > Suggested-by: Christian Brauner <brauner@kernel.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...

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

[Amir Goldstein]

On Mon, May 15, 2023 at 6:16 PM Christian Brauner <brauner@kernel.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@kernel.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@huawei.com>
> > > > Suggested-by: Christian Brauner <brauner@kernel.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.

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

[Zhihao Cheng]

在 2023/5/15 23:36, Amir Goldstein 写道:
> On Mon, May 15, 2023 at 6:16 PM Christian Brauner <brauner@kernel.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@kernel.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@huawei.com>
>>>>> Suggested-by: Christian Brauner <brauner@kernel.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);
>>

Thanks for reviewings and helpful discussion from Amir and Christian.

>> 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...

Yes, I can hardly reproduce the problem without the time delay in kernel.

> 
> Thanks,
> Amir.
> .
> 

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

[Amir Goldstein]

On Tue, May 16, 2023 at 4:16 AM Zhihao Cheng <chengzhihao1@huawei.com> wrote:
>
> 在 2023/5/15 23:36, Amir Goldstein 写道:
> > On Mon, May 15, 2023 at 6:16 PM Christian Brauner <brauner@kernel.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@kernel.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@huawei.com>
> >>>>> Suggested-by: Christian Brauner <brauner@kernel.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);
> >>
>
> Thanks for reviewings and helpful discussion from Amir and Christian.
>

You're welcome.

Note to self: there is be a trivial conflict with the change to
ovl_i_path_real()
return type and this patch from my ovl-lazy-lowerdata series:
https://lore.kernel.org/linux-unionfs/20230427130539.2798797-7-amir73il@gmail.com/

After Miklos takes your fixes, I will rebase.

Thanks,
Amir.

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

[Zhihao Cheng]

在 2023/5/16 19:45, Amir Goldstein 写道:
> On Tue, May 16, 2023 at 4:16 AM Zhihao Cheng <chengzhihao1@huawei.com> wrote:
>>
>> 在 2023/5/15 23:36, Amir Goldstein 写道:
>>> On Mon, May 15, 2023 at 6:16 PM Christian Brauner <brauner@kernel.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@kernel.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@huawei.com>
>>>>>>> Suggested-by: Christian Brauner <brauner@kernel.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);
>>>>
>>
>> Thanks for reviewings and helpful discussion from Amir and Christian.
>>
> 
> You're welcome.
> 
> Note to self: there is be a trivial conflict with the change to
> ovl_i_path_real()
> return type and this patch from my ovl-lazy-lowerdata series:
> https://lore.kernel.org/linux-unionfs/20230427130539.2798797-7-amir73il@gmail.com/
> 
> After Miklos takes your fixes, I will rebase.

Thanks for rebasing, so you will take v2 with modified helper 
ovl_i_path_real(return realinode)?

> 
> Thanks,
> Amir.
> .
> 

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

[Amir Goldstein]

On Tue, May 16, 2023 at 3:27 PM Zhihao Cheng <chengzhihao1@huawei.com> wrote:
>
> 在 2023/5/16 19:45, Amir Goldstein 写道:
> > On Tue, May 16, 2023 at 4:16 AM Zhihao Cheng <chengzhihao1@huawei.com> wrote:
> >>
> >> 在 2023/5/15 23:36, Amir Goldstein 写道:
> >>> On Mon, May 15, 2023 at 6:16 PM Christian Brauner <brauner@kernel.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@kernel.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@huawei.com>
> >>>>>>> Suggested-by: Christian Brauner <brauner@kernel.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);
> >>>>
> >>
> >> Thanks for reviewings and helpful discussion from Amir and Christian.
> >>
> >
> > You're welcome.
> >
> > Note to self: there is be a trivial conflict with the change to
> > ovl_i_path_real()
> > return type and this patch from my ovl-lazy-lowerdata series:
> > https://lore.kernel.org/linux-unionfs/20230427130539.2798797-7-amir73il@gmail.com/
> >
> > After Miklos takes your fixes, I will rebase.
>
> Thanks for rebasing, so you will take v2 with modified helper
> ovl_i_path_real(return realinode)?
>

Please post v3 with the modified helper for Miklos to take.

Thanks,
Amir.

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

[Zhihao Cheng]

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()")
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
Suggested-by: Christian Brauner <brauner@kernel.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

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

[Amir Goldstein]

On Mon, May 15, 2023 at 4:39 PM Zhihao Cheng <chengzhihao1@huawei.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@huawei.com>
> Suggested-by: Christian Brauner <brauner@kernel.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
>

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

[Zhihao Cheng]

在 2023/5/16 19:40, Amir Goldstein 写道:
> On Mon, May 15, 2023 at 4:39 PM Zhihao Cheng <chengzhihao1@huawei.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)?

> 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@huawei.com>
>> Suggested-by: Christian Brauner <brauner@kernel.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
>>
> .
> 

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

[Amir Goldstein]

On Tue, May 16, 2023 at 3:25 PM Zhihao Cheng <chengzhihao1@huawei.com> wrote:
>
> 在 2023/5/16 19:40, Amir Goldstein 写道:
> > On Mon, May 15, 2023 at 4:39 PM Zhihao Cheng <chengzhihao1@huawei.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@kernel.

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

Thanks,
Amir.