* [PATCH v2 0/2] ovl: Fix null ptr dereference at realinode in rcu-walk
@ 2023-05-15 13:36 Zhihao Cheng
2023-05-15 13:36 ` [PATCH v2 1/2] ovl: ovl_permission: Fix null pointer dereference at realinode in rcu-walk mode Zhihao Cheng
2023-05-15 13:36 ` [PATCH v2 2/2] ovl: get_acl: " Zhihao Cheng
0 siblings, 2 replies; 14+ messages in thread
From: Zhihao Cheng @ 2023-05-15 13:36 UTC (permalink / raw)
To: miklos, amir73il, brauner; +Cc: linux-unionfs, linux-kernel, chengzhihao1
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/2] ovl: ovl_permission: Fix null pointer dereference at realinode in rcu-walk mode
2023-05-15 13:36 [PATCH v2 0/2] ovl: Fix null ptr dereference at realinode in rcu-walk Zhihao Cheng
@ 2023-05-15 13:36 ` Zhihao Cheng
2023-05-15 13:58 ` Christian Brauner
2023-05-15 13:36 ` [PATCH v2 2/2] ovl: get_acl: " Zhihao Cheng
1 sibling, 1 reply; 14+ messages in thread
From: Zhihao Cheng @ 2023-05-15 13:36 UTC (permalink / raw)
To: miklos, amir73il, brauner; +Cc: linux-unionfs, linux-kernel, chengzhihao1
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 2/2] ovl: get_acl: Fix null pointer dereference at realinode in rcu-walk mode
2023-05-15 13:36 [PATCH v2 0/2] ovl: Fix null ptr dereference at realinode in rcu-walk Zhihao Cheng
2023-05-15 13:36 ` [PATCH v2 1/2] ovl: ovl_permission: Fix null pointer dereference at realinode in rcu-walk mode Zhihao Cheng
@ 2023-05-15 13:36 ` Zhihao Cheng
2023-05-16 11:40 ` Amir Goldstein
1 sibling, 1 reply; 14+ messages in thread
From: Zhihao Cheng @ 2023-05-15 13:36 UTC (permalink / raw)
To: miklos, amir73il, brauner; +Cc: linux-unionfs, linux-kernel, chengzhihao1
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] ovl: ovl_permission: Fix null pointer dereference at realinode in rcu-walk mode
2023-05-15 13:36 ` [PATCH v2 1/2] ovl: ovl_permission: Fix null pointer dereference at realinode in rcu-walk mode Zhihao Cheng
@ 2023-05-15 13:58 ` Christian Brauner
2023-05-15 14:58 ` Amir Goldstein
0 siblings, 1 reply; 14+ messages in thread
From: Christian Brauner @ 2023-05-15 13:58 UTC (permalink / raw)
To: Zhihao Cheng; +Cc: miklos, amir73il, linux-unionfs, linux-kernel
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
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] ovl: ovl_permission: Fix null pointer dereference at realinode in rcu-walk mode
2023-05-15 13:58 ` Christian Brauner
@ 2023-05-15 14:58 ` Amir Goldstein
2023-05-15 15:16 ` Christian Brauner
0 siblings, 1 reply; 14+ messages in thread
From: Amir Goldstein @ 2023-05-15 14:58 UTC (permalink / raw)
To: Christian Brauner; +Cc: Zhihao Cheng, miklos, linux-unionfs, linux-kernel
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);
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] ovl: ovl_permission: Fix null pointer dereference at realinode in rcu-walk mode
2023-05-15 14:58 ` Amir Goldstein
@ 2023-05-15 15:16 ` Christian Brauner
2023-05-15 15:36 ` Amir Goldstein
0 siblings, 1 reply; 14+ messages in thread
From: Christian Brauner @ 2023-05-15 15:16 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Zhihao Cheng, miklos, linux-unionfs, linux-kernel
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...
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] ovl: ovl_permission: Fix null pointer dereference at realinode in rcu-walk mode
2023-05-15 15:16 ` Christian Brauner
@ 2023-05-15 15:36 ` Amir Goldstein
2023-05-16 1:16 ` Zhihao Cheng
0 siblings, 1 reply; 14+ messages in thread
From: Amir Goldstein @ 2023-05-15 15:36 UTC (permalink / raw)
To: Christian Brauner; +Cc: Zhihao Cheng, miklos, linux-unionfs, linux-kernel
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] ovl: ovl_permission: Fix null pointer dereference at realinode in rcu-walk mode
2023-05-15 15:36 ` Amir Goldstein
@ 2023-05-16 1:16 ` Zhihao Cheng
2023-05-16 11:45 ` Amir Goldstein
0 siblings, 1 reply; 14+ messages in thread
From: Zhihao Cheng @ 2023-05-16 1:16 UTC (permalink / raw)
To: Amir Goldstein, Christian Brauner; +Cc: miklos, linux-unionfs, linux-kernel
在 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.
> .
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] ovl: get_acl: Fix null pointer dereference at realinode in rcu-walk mode
2023-05-15 13:36 ` [PATCH v2 2/2] ovl: get_acl: " Zhihao Cheng
@ 2023-05-16 11:40 ` Amir Goldstein
2023-05-16 12:25 ` Zhihao Cheng
0 siblings, 1 reply; 14+ messages in thread
From: Amir Goldstein @ 2023-05-16 11:40 UTC (permalink / raw)
To: Zhihao Cheng; +Cc: miklos, brauner, linux-unionfs, linux-kernel
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
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] ovl: ovl_permission: Fix null pointer dereference at realinode in rcu-walk mode
2023-05-16 1:16 ` Zhihao Cheng
@ 2023-05-16 11:45 ` Amir Goldstein
2023-05-16 12:27 ` Zhihao Cheng
0 siblings, 1 reply; 14+ messages in thread
From: Amir Goldstein @ 2023-05-16 11:45 UTC (permalink / raw)
To: Zhihao Cheng; +Cc: Christian Brauner, miklos, linux-unionfs, linux-kernel
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] ovl: get_acl: Fix null pointer dereference at realinode in rcu-walk mode
2023-05-16 11:40 ` Amir Goldstein
@ 2023-05-16 12:25 ` Zhihao Cheng
2023-05-16 12:46 ` Amir Goldstein
0 siblings, 1 reply; 14+ messages in thread
From: Zhihao Cheng @ 2023-05-16 12:25 UTC (permalink / raw)
To: Amir Goldstein; +Cc: miklos, brauner, linux-unionfs, linux-kernel
在 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
>>
> .
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] ovl: ovl_permission: Fix null pointer dereference at realinode in rcu-walk mode
2023-05-16 11:45 ` Amir Goldstein
@ 2023-05-16 12:27 ` Zhihao Cheng
2023-05-16 12:35 ` Amir Goldstein
0 siblings, 1 reply; 14+ messages in thread
From: Zhihao Cheng @ 2023-05-16 12:27 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Christian Brauner, miklos, linux-unionfs, linux-kernel
在 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.
> .
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] ovl: ovl_permission: Fix null pointer dereference at realinode in rcu-walk mode
2023-05-16 12:27 ` Zhihao Cheng
@ 2023-05-16 12:35 ` Amir Goldstein
0 siblings, 0 replies; 14+ messages in thread
From: Amir Goldstein @ 2023-05-16 12:35 UTC (permalink / raw)
To: Zhihao Cheng; +Cc: Christian Brauner, miklos, linux-unionfs, linux-kernel
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] ovl: get_acl: Fix null pointer dereference at realinode in rcu-walk mode
2023-05-16 12:25 ` Zhihao Cheng
@ 2023-05-16 12:46 ` Amir Goldstein
0 siblings, 0 replies; 14+ messages in thread
From: Amir Goldstein @ 2023-05-16 12:46 UTC (permalink / raw)
To: Zhihao Cheng; +Cc: miklos, brauner, linux-unionfs, linux-kernel
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-05-16 12:47 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-15 13:36 [PATCH v2 0/2] ovl: Fix null ptr dereference at realinode in rcu-walk Zhihao Cheng
2023-05-15 13:36 ` [PATCH v2 1/2] ovl: ovl_permission: Fix null pointer dereference at realinode in rcu-walk mode Zhihao Cheng
2023-05-15 13:58 ` Christian Brauner
2023-05-15 14:58 ` Amir Goldstein
2023-05-15 15:16 ` Christian Brauner
2023-05-15 15:36 ` Amir Goldstein
2023-05-16 1:16 ` Zhihao Cheng
2023-05-16 11:45 ` Amir Goldstein
2023-05-16 12:27 ` Zhihao Cheng
2023-05-16 12:35 ` Amir Goldstein
2023-05-15 13:36 ` [PATCH v2 2/2] ovl: get_acl: " Zhihao Cheng
2023-05-16 11:40 ` Amir Goldstein
2023-05-16 12:25 ` Zhihao Cheng
2023-05-16 12:46 ` Amir Goldstein
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).