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 <[email protected]>
---
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;
}
@@ -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))
--
2.7.4
Yongzhi Liu <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
> ---
> 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.