* [PATCH 1/2] ovl: get_acl: Fix null pointer dereference at realinode in rcu-walk mode
2023-05-05 12:24 [PATCH 0/2] ovl: Fix null ptr dereference at realinode in rcu-walk Zhihao Cheng
@ 2023-05-05 12:24 ` Zhihao Cheng
2023-05-15 11:08 ` Christian Brauner
2023-05-05 12:24 ` [PATCH 2/2] ovl: ovl_permission: " Zhihao Cheng
2023-05-15 11:04 ` [PATCH 0/2] ovl: Fix null ptr dereference at realinode in rcu-walk Christian Brauner
2 siblings, 1 reply; 5+ messages in thread
From: Zhihao Cheng @ 2023-05-05 12:24 UTC (permalink / raw)
To: miklos, amir73il, brauner; +Cc: linux-unionfs, linux-kernel
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 checking realinode whether to be NULL before accessing it.
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>
---
fs/overlayfs/inode.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 541cf3717fc2..38cfdf9e2b44 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -563,16 +563,16 @@ struct posix_acl *do_ovl_get_acl(struct mnt_idmap *idmap,
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) {
+ if (!realpath.dentry || !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.31.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH 1/2] ovl: get_acl: Fix null pointer dereference at realinode in rcu-walk mode
2023-05-05 12:24 ` [PATCH 1/2] ovl: get_acl: Fix null pointer dereference at realinode in rcu-walk mode Zhihao Cheng
@ 2023-05-15 11:08 ` Christian Brauner
0 siblings, 0 replies; 5+ messages in thread
From: Christian Brauner @ 2023-05-15 11:08 UTC (permalink / raw)
To: Zhihao Cheng; +Cc: miklos, amir73il, linux-unionfs, linux-kernel
On Fri, May 05, 2023 at 08:24:51PM +0800, Zhihao Cheng 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 checking realinode whether to be NULL before accessing it.
>
> 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>
> ---
> fs/overlayfs/inode.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 541cf3717fc2..38cfdf9e2b44 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -563,16 +563,16 @@ struct posix_acl *do_ovl_get_acl(struct mnt_idmap *idmap,
> 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) {
> + if (!realpath.dentry || !realinode) {
> WARN_ON(!rcu);
> return ERR_PTR(-ECHILD);
> }
I think the logic here is now a bit strange. I would just not bother
calling ovl_inode_real() anymore and simply use the same logic as in
ovl_permission() (Thus my comment about using a tiny helper.).
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/2] ovl: ovl_permission: Fix null pointer dereference at realinode in rcu-walk mode
2023-05-05 12:24 [PATCH 0/2] ovl: Fix null ptr dereference at realinode in rcu-walk Zhihao Cheng
2023-05-05 12:24 ` [PATCH 1/2] ovl: get_acl: Fix null pointer dereference at realinode in rcu-walk mode Zhihao Cheng
@ 2023-05-05 12:24 ` Zhihao Cheng
2023-05-15 11:04 ` [PATCH 0/2] ovl: Fix null ptr dereference at realinode in rcu-walk Christian Brauner
2 siblings, 0 replies; 5+ messages in thread
From: Zhihao Cheng @ 2023-05-05 12:24 UTC (permalink / raw)
To: miklos, amir73il, brauner; +Cc: linux-unionfs, linux-kernel
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].
Fix it by checking realinode whether to be NULL before accessing it.
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>
---
fs/overlayfs/inode.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 38cfdf9e2b44..a4c56584b8c2 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -293,6 +293,11 @@ int ovl_permission(struct mnt_idmap *idmap,
WARN_ON(!(mask & MAY_NOT_BLOCK));
return -ECHILD;
}
+ realinode = d_inode(realpath.dentry);
+ if (!realinode) {
+ WARN_ON(!(mask & MAY_NOT_BLOCK));
+ return -ECHILD;
+ }
/*
* Check overlay inode with the creds of task and underlying inode
@@ -302,7 +307,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.31.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH 0/2] ovl: Fix null ptr dereference at realinode in rcu-walk
2023-05-05 12:24 [PATCH 0/2] ovl: Fix null ptr dereference at realinode in rcu-walk Zhihao Cheng
2023-05-05 12:24 ` [PATCH 1/2] ovl: get_acl: Fix null pointer dereference at realinode in rcu-walk mode Zhihao Cheng
2023-05-05 12:24 ` [PATCH 2/2] ovl: ovl_permission: " Zhihao Cheng
@ 2023-05-15 11:04 ` Christian Brauner
2 siblings, 0 replies; 5+ messages in thread
From: Christian Brauner @ 2023-05-15 11:04 UTC (permalink / raw)
To: Zhihao Cheng; +Cc: miklos, amir73il, linux-unionfs, linux-kernel
On Fri, May 05, 2023 at 08:24:50PM +0800, Zhihao Cheng wrote:
> Zhihao Cheng (2):
> ovl: get_acl: Fix null pointer dereference at realinode in rcu-walk
> mode
> ovl: ovl_permission: Fix null pointer dereference at realinode in
> rcu-walk mode
>
> fs/overlayfs/inode.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
Seems fine to me. Thanks for fixing this,
Reviewed-by: Christian Brauner <brauner@kernel.org>
(One thing that might be possible is to move this into a tiny static
inline helper but might not be worth it.)
^ permalink raw reply [flat|nested] 5+ messages in thread