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@xxxxxxxxxx> > --- > 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.).
