Messages in this thread | | | Date | Tue, 17 May 2022 21:43:38 +0000 | From | Al Viro <> | Subject | Re: [PATCH] commoncap: check return value to avoid null pointer dereference |
| |
On Mon, May 16, 2022 at 10:40:02AM -0700, Yongzhi Liu wrote: > The pointer inode is dereferenced before a null pointer > check on inode, hence if inode is actually null we will > get a null pointer dereference. Fix this by only dereferencing > inode after the null pointer check on inode. > > Fixes: c6f493d631c ("VFS: security/: d_backing_inode() annotations") > Fixes: 8db6c34 ("Introduce v3 namespaced file capabilities") > > Signed-off-by: Yongzhi Liu <lyz_cs@pku.edu.cn> > --- > security/commoncap.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/security/commoncap.c b/security/commoncap.c > index 5fc8986..978f011 100644 > --- a/security/commoncap.c > +++ b/security/commoncap.c > @@ -298,6 +298,8 @@ int cap_inode_need_killpriv(struct dentry *dentry) > struct inode *inode = d_backing_inode(dentry); > int error; > > + if (!inode) > + return 0; > error = __vfs_getxattr(dentry, inode, XATTR_NAME_CAPS, NULL, 0); > return error > 0; > }
Huh? AFAICS, that has all of two callers - notify_change() and dentry_needs_remove_privs(). Both of them should never be called on negative dentries and both would have blown up well before the call of security_inode_need_killpriv().
> @@ -545,11 +547,13 @@ int cap_convert_nscap(struct user_namespace *mnt_userns, struct dentry *dentry, > const struct vfs_cap_data *cap = *ivalue; > __u32 magic, nsmagic; > struct inode *inode = d_backing_inode(dentry); > - struct user_namespace *task_ns = current_user_ns(), > - *fs_ns = inode->i_sb->s_user_ns; > + struct user_namespace *task_ns = current_user_ns(), *fs_ns; > kuid_t rootid; > size_t newsize; > > + if (!inode) > + return -EINVAL; > + fs_ns = inode->i_sb->s_user_ns; > if (!*ivalue) > return -EINVAL; > if (!validheader(size, cap))
... and that one come from vfs_setxattr(). Again, calling that on a negative dentry is an oopsable bug. If you have any reproducers, please post the stack traces so it would be possible to deal with the underlying problem.
| |