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

[PATCH v3 0/3] 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().
v2->v3:
  Modify original helper ovl_i_path_real() to return the realinode.

Zhihao Cheng (3):
  ovl: Let helper ovl_i_path_real() return the realinode
  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     | 17 ++++++++---------
 fs/overlayfs/overlayfs.h |  2 +-
 fs/overlayfs/util.c      |  7 ++++---
 3 files changed, 13 insertions(+), 13 deletions(-)

-- 
2.39.2

[PATCH v3 1/3] ovl: Let helper ovl_i_path_real() return the realinode

[Zhihao Cheng]

Let helper ovl_i_path_real() return the realinode to prepare for
checking non-null realinode in rcu walking path.

Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 fs/overlayfs/overlayfs.h | 2 +-
 fs/overlayfs/util.c      | 7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 4d0b278f5630..7398de332527 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -382,7 +382,7 @@ enum ovl_path_type ovl_path_type(struct dentry *dentry);
 void ovl_path_upper(struct dentry *dentry, struct path *path);
 void ovl_path_lower(struct dentry *dentry, struct path *path);
 void ovl_path_lowerdata(struct dentry *dentry, struct path *path);
-void ovl_i_path_real(struct inode *inode, struct path *path);
+struct inode *ovl_i_path_real(struct inode *inode, struct path *path);
 enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path);
 enum ovl_path_type ovl_path_realdata(struct dentry *dentry, struct path *path);
 struct dentry *ovl_dentry_upper(struct dentry *dentry);
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 923d66d131c1..00d31e38b57d 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -250,7 +250,7 @@ struct dentry *ovl_i_dentry_upper(struct inode *inode)
 	return ovl_upperdentry_dereference(OVL_I(inode));
 }
 
-void ovl_i_path_real(struct inode *inode, struct path *path)
+struct inode *ovl_i_path_real(struct inode *inode, struct path *path)
 {
 	path->dentry = ovl_i_dentry_upper(inode);
 	if (!path->dentry) {
@@ -259,6 +259,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;
 }
 
 struct inode *ovl_inode_upper(struct inode *inode)
@@ -1105,8 +1107,7 @@ void ovl_copyattr(struct inode *inode)
 	vfsuid_t vfsuid;
 	vfsgid_t vfsgid;
 
-	ovl_i_path_real(inode, &realpath);
-	realinode = d_inode(realpath.dentry);
+	realinode = ovl_i_path_real(inode, &realpath);
 	real_idmap = mnt_idmap(realpath.mnt);
 
 	vfsuid = i_uid_into_vfsuid(real_idmap, realinode);
-- 
2.39.2

[PATCH v3 2/3] 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].

Use the helper ovl_i_path_realinode() to get realinode and then do
non-nullptr checking.

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>
Suggested-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/inode.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 541cf3717fc2..ca56b1328a2c 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -288,8 +288,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 (!realinode) {
 		WARN_ON(!(mask & MAY_NOT_BLOCK));
 		return -ECHILD;
 	}
@@ -302,7 +302,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

[PATCH v3 3/3] 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].

Use the helper ovl_i_path_realinode() to get realinode and then do
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>
Suggested-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/inode.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index ca56b1328a2c..e7e888dea634 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -558,20 +558,20 @@ 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;
 
-	if (!IS_POSIXACL(realinode))
-		return NULL;
-
 	/* Careful in RCU walk mode */
-	ovl_i_path_real(inode, &realpath);
-	if (!realpath.dentry) {
+	realinode = ovl_i_path_real(inode, &realpath);
+	if (!realinode) {
 		WARN_ON(!rcu);
 		return ERR_PTR(-ECHILD);
 	}
 
+	if (!IS_POSIXACL(realinode))
+		return NULL;
+
 	if (rcu) {
 		/*
 		 * If the layer is idmapped drop out of RCU path walk
-- 
2.39.2

Re: [PATCH v3 0/3] ovl: Fix null ptr dereference at realinode in rcu-walk

[Christian Brauner]

On Tue, May 16, 2023 at 10:16:16PM +0800, Zhihao Cheng wrote:
> v1->v2:
>   Extract a helper to get realpath and real inode from ovl inode.
>   Get realinode from realpath in do_ovl_get_acl().
> v2->v3:
>   Modify original helper ovl_i_path_real() to return the realinode.
> 
> Zhihao Cheng (3):
>   ovl: Let helper ovl_i_path_real() return the realinode
>   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     | 17 ++++++++---------
>  fs/overlayfs/overlayfs.h |  2 +-
>  fs/overlayfs/util.c      |  7 ++++---
>  3 files changed, 13 insertions(+), 13 deletions(-)

Looks good to me,
Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH v3 1/3] ovl: Let helper ovl_i_path_real() return the realinode

[Amir Goldstein]

On Tue, May 16, 2023 at 5:19 PM Zhihao Cheng <chengzhihao1@huawei.com> wrote:
>
> Let helper ovl_i_path_real() return the realinode to prepare for
> checking non-null realinode in rcu walking path.
>
> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  fs/overlayfs/overlayfs.h | 2 +-
>  fs/overlayfs/util.c      | 7 ++++---
>  2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 4d0b278f5630..7398de332527 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -382,7 +382,7 @@ enum ovl_path_type ovl_path_type(struct dentry *dentry);
>  void ovl_path_upper(struct dentry *dentry, struct path *path);
>  void ovl_path_lower(struct dentry *dentry, struct path *path);
>  void ovl_path_lowerdata(struct dentry *dentry, struct path *path);
> -void ovl_i_path_real(struct inode *inode, struct path *path);
> +struct inode *ovl_i_path_real(struct inode *inode, struct path *path);
>  enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path);
>  enum ovl_path_type ovl_path_realdata(struct dentry *dentry, struct path *path);
>  struct dentry *ovl_dentry_upper(struct dentry *dentry);
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 923d66d131c1..00d31e38b57d 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -250,7 +250,7 @@ struct dentry *ovl_i_dentry_upper(struct inode *inode)
>         return ovl_upperdentry_dereference(OVL_I(inode));
>  }
>
> -void ovl_i_path_real(struct inode *inode, struct path *path)
> +struct inode *ovl_i_path_real(struct inode *inode, struct path *path)
>  {
>         path->dentry = ovl_i_dentry_upper(inode);
>         if (!path->dentry) {
> @@ -259,6 +259,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;
>  }
>
>  struct inode *ovl_inode_upper(struct inode *inode)
> @@ -1105,8 +1107,7 @@ void ovl_copyattr(struct inode *inode)
>         vfsuid_t vfsuid;
>         vfsgid_t vfsgid;
>
> -       ovl_i_path_real(inode, &realpath);
> -       realinode = d_inode(realpath.dentry);
> +       realinode = ovl_i_path_real(inode, &realpath);
>         real_idmap = mnt_idmap(realpath.mnt);
>
>         vfsuid = i_uid_into_vfsuid(real_idmap, realinode);
> --
> 2.39.2
>

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

[Amir Goldstein]

On Tue, May 16, 2023 at 5:19 PM Zhihao Cheng <chengzhihao1@huawei.com> 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].
>
> Use the helper ovl_i_path_realinode() to get realinode and then do
> non-nullptr checking.
>
> 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>
> Suggested-by: Amir Goldstein <amir73il@gmail.com>

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  fs/overlayfs/inode.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 541cf3717fc2..ca56b1328a2c 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -288,8 +288,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 (!realinode) {
>                 WARN_ON(!(mask & MAY_NOT_BLOCK));
>                 return -ECHILD;
>         }
> @@ -302,7 +302,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 v3 3/3] ovl: get_acl: Fix null pointer dereference at realinode in rcu-walk mode

[Amir Goldstein]

On Tue, May 16, 2023 at 5:19 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].
>
> Use the helper ovl_i_path_realinode() to get realinode and then do
> 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>
> Suggested-by: Amir Goldstein <amir73il@gmail.com>

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  fs/overlayfs/inode.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index ca56b1328a2c..e7e888dea634 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -558,20 +558,20 @@ 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;
>
> -       if (!IS_POSIXACL(realinode))
> -               return NULL;
> -
>         /* Careful in RCU walk mode */
> -       ovl_i_path_real(inode, &realpath);
> -       if (!realpath.dentry) {
> +       realinode = ovl_i_path_real(inode, &realpath);
> +       if (!realinode) {
>                 WARN_ON(!rcu);
>                 return ERR_PTR(-ECHILD);
>         }
>
> +       if (!IS_POSIXACL(realinode))
> +               return NULL;
> +
>         if (rcu) {
>                 /*
>                  * If the layer is idmapped drop out of RCU path walk
> --
> 2.39.2
>