| Message ID | 1652722802-66170-1-git-send-email-lyz_cs@pku.edu.cn (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | commoncap: check return value to avoid null pointer dereference | expand |
Yongzhi Liu <lyz_cs@pku.edu.cn> writes: > 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") I don't see how this fixes anything. The dentry should be non-negative so d_backing_inode should always return true. > 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; How can dentry->d_inode be valid and d_backing_inode not be valid? That would seem to be a bug elsewhere in the code if it actually happens. > error = __vfs_getxattr(dentry, inode, XATTR_NAME_CAPS, NULL, 0); > return error > 0; > } > @@ -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)) Same with this one. Short of a negative dentry I don't see how d_backing_inode can be NULL, and we are talking about negative dentries that should have been handled long before these two functions are called. Eric
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.
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; } @@ -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))
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(-)