Re: [PATCH v3 -next 06/18] LSM: pass struct path and struct file to the inode xattr hooks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] bpf: missing __nullable on file arg in inode xattr LSM hooks allows NULL deref
- [High] bpf: untrusted path->dentry in inode xattr LSM hooks breaks dentry kfuncs
- [Low] security: missing @idmap kernel-doc in cap_inode_removexattr()
--

commit e83206fa3b704333b0de2a23321bc120a419f251
Author: Cai Xinchen <caixinchen1@huawei.com>

LSM: pass struct path and struct file to the inode xattr hooks

This commit updates the inode_setxattr, inode_getxattr, inode_listxattr, and
inode_removexattr LSM hooks and their callers to take a const struct path
instead of a struct dentry, and passes the associated struct file when
available (or NULL for path-based and in-kernel operations). It also updates
the BPF LSM selftests attached to these hooks to pass path->dentry to the
dentry xattr kfuncs.

> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index ea6e0fbaa3b9c..2315961b6d713 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -156,14 +156,16 @@ LSM_HOOK(void, LSM_RET_VOID, inode_post_setattr, const struct mnt_idmap *idmap,
>  LSM_HOOK(int, 0, inode_getattr, const struct path *path)
>  LSM_HOOK(int, 0, inode_xattr_skipcap, const char *name)
>  LSM_HOOK(int, 0, inode_setxattr, const struct mnt_idmap *idmap,
> -	 struct dentry *dentry, const char *name, const void *value,
> -	 size_t size, int flags)
> +	 const struct path *path, const char *name, const void *value,
> +	 size_t size, int flags, struct file *file)
>  LSM_HOOK(void, LSM_RET_VOID, inode_post_setxattr, struct dentry *dentry,
>  	 const char *name, const void *value, size_t size, int flags)
> -LSM_HOOK(int, 0, inode_getxattr, struct dentry *dentry, const char *name)
> -LSM_HOOK(int, 0, inode_listxattr, struct dentry *dentry)
> +LSM_HOOK(int, 0, inode_getxattr, const struct path *path, const char *name,
> +	 struct file *file)
> +LSM_HOOK(int, 0, inode_listxattr, const struct path *path,
> +	 struct file *file)
>  LSM_HOOK(int, 0, inode_removexattr, const struct mnt_idmap *idmap,
> -	 struct dentry *dentry, const char *name)
> +	 const struct path *path, const char *name, struct file *file)

[Severity: Critical]
Since path-based xattr syscalls (filename_setxattr(), filename_getxattr(),
filename_listxattr(), and filename_removexattr() in fs/xattr.c) and in-kernel
callers pass file = NULL to these hooks, can omitting the __nullable suffix
on the new file parameter allow a BPF LSM program to trigger a kernel NULL
pointer dereference?

In kernel/bpf/btf.c:btf_ctx_access(), pointer arguments of trusted BPF LSM
hooks are marked as non-NULL PTR_TO_BTF_ID | PTR_TRUSTED unless the parameter
name ends with __nullable:

kernel/bpf/btf.c:btf_ctx_access() {
	...
	info->reg_type = PTR_TO_BTF_ID;
	if (prog_args_trusted(prog))
		info->reg_type |= PTR_TRUSTED;

	if (btf_param_match_suffix(btf, &args[arg], "__nullable"))
		info->reg_type |= PTR_MAYBE_NULL;
}

Without PTR_MAYBE_NULL on file, bpf_may_fault_on_deref() returns false, so
convert_ctx_accesses() does not rewrite loads from file (such as walking the
trusted field file->f_inode via BTF_TYPE_SAFE_TRUSTED(struct file)) to
BPF_PROBE_MEM. In addition, the verifier allows passing file without a NULL
check directly to kfuncs that require a non-NULL trusted struct file * and
immediately dereference it, such as bpf_get_file_xattr():

fs/bpf_fs_kfuncs.c:bpf_get_file_xattr() {
	struct dentry *dentry;

	dentry = file_dentry(file);
	return bpf_get_dentry_xattr(dentry, name__str, value_p);
}

Should these four hooks either name the parameter file__nullable or provide
strong bpf_lsm_* overrides with file__nullable in kernel/bpf/bpf_lsm_proto.c
(similar to bpf_lsm_mmap_file())?

> diff --git a/security/commoncap.c b/security/commoncap.c
> index 565c893c6d4b4..0a94b9cd231fa 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
[ ... ]
> @@ -1044,25 +1048,21 @@ int cap_inode_setxattr(struct dentry *dentry, const char *name,
>  /**
>   * cap_inode_removexattr - Determine whether an xattr may be removed
>   *
> - * @idmap:	idmap of the mount the inode was found from
> - * @dentry:	The inode/dentry being altered
> + * @path:	The inode/path being altered
>   * @name:	The name of the xattr to be changed
> + * @file:	the file through which the xattr removal is requested, if any
>   *
>   * Determine whether an xattr may be removed from an inode, returning 0 if
>   * permission is granted, -ve if denied.
>   *
> - * If the inode has been found through an idmapped mount the idmap of
> - * the vfsmount must be passed through @idmap. This function will then
> - * take care to map the inode according to @idmap before checking
> - * permissions. On non-idmapped mounts or if permission checking is to be
> - * performed on the raw inode simply pass @nop_mnt_idmap.
> - *
>   * This is used to make sure security xattrs don't get removed by those who
>   * aren't privileged to remove them.
>   */
>  int cap_inode_removexattr(const struct mnt_idmap *idmap,
> -			  struct dentry *dentry, const char *name)
> +			  const struct path *path, const char *name,
> +			  struct file *file)

[Severity: Low]
Was the @idmap kernel-doc description above cap_inode_removexattr() dropped
by mistake? Since const struct mnt_idmap *idmap is still a parameter of
cap_inode_removexattr(), removing @idmap from the comment block will trigger
a scripts/kernel-doc warning.

> diff --git a/tools/testing/selftests/bpf/progs/test_get_xattr.c b/tools/testing/selftests/bpf/progs/test_get_xattr.c
> index 54305f4c9f2dd..97361ff10e9f1 100644
> --- a/tools/testing/selftests/bpf/progs/test_get_xattr.c
> +++ b/tools/testing/selftests/bpf/progs/test_get_xattr.c
> @@ -56,7 +56,8 @@ int BPF_PROG(test_file_open, struct file *f)
>  }
>  
>  SEC("lsm.s/inode_getxattr")
> -int BPF_PROG(test_inode_getxattr, struct dentry *dentry, char *name)
> +int BPF_PROG(test_inode_getxattr, const struct path *path, char *name,
> +	     struct file *file)
>  {
>  	struct bpf_dynptr value_ptr;
>  	__u32 pid;
> @@ -69,7 +70,7 @@ int BPF_PROG(test_inode_getxattr, struct dentry *dentry, char *name)
>  	bpf_dynptr_from_mem(value2, sizeof(value2), 0, &value_ptr);
>  
>  	for (i = 0; i < ARRAY_SIZE(xattr_names); i++) {
> -		ret = bpf_get_dentry_xattr(dentry, xattr_names[i], &value_ptr);
> +		ret = bpf_get_dentry_xattr(path->dentry, xattr_names[i], &value_ptr);
>  		if (ret == sizeof(expected_value))
>  			break;
>  	}

[Severity: High]
Will the BPF verifier reject loading test_inode_getxattr() here (as well as
test_inode_getxattr() and test_inode_setxattr() in
tools/testing/selftests/bpf/progs/test_set_remove_xattr.c) when passing
path->dentry to bpf_get_dentry_xattr(), bpf_set_dentry_xattr(), and
bpf_remove_dentry_xattr()?

Previously, dentry was a direct argument to the inode_getxattr and
inode_setxattr hooks, so btf_ctx_access() marked it as
PTR_TO_BTF_ID | PTR_TRUSTED.

With the hook argument changed to const struct path *path, dereferencing
path->dentry goes through check_ptr_to_btf_access(), which only preserves
PTR_TRUSTED if type_is_trusted() or type_is_trusted_or_null() returns true:

kernel/bpf/verifier.c:check_ptr_to_btf_access() {
	...
	if (flag & PTR_TRUSTED) {
		if (type_is_trusted(env, reg, field_name, btf_id)) {
			flag |= PTR_TRUSTED;
		} else if (type_is_trusted_or_null(env, reg, field_name, btf_id)) {
			flag |= PTR_TRUSTED | PTR_MAYBE_NULL;
		} else if (in_rcu_cs(env) && !type_may_be_null(reg->type)) {
			clear_trusted_flags(&flag);
			flag |= MEM_RCU;
		} else {
			flag = PTR_UNTRUSTED;
		}
	}
}

Because struct path is not listed in BTF_TYPE_SAFE_TRUSTED in
kernel/bpf/verifier.c:type_is_trusted(), walking path->dentry drops
PTR_TRUSTED (marking the register PTR_UNTRUSTED in sleepable lsm.s programs).
Since bpf_get_dentry_xattr(), bpf_set_dentry_xattr(), and
bpf_remove_dentry_xattr() require a trusted struct dentry * argument, the
verifier rejects the call with -EINVAL ("R1 must be referenced or trusted").

Does struct path need to be added to BTF_TYPE_SAFE_TRUSTED in
kernel/bpf/verifier.c so that path->dentry and path->mnt remain trusted when
accessed from LSM hooks?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027121151.1887650-1-caixinchen1@huawei.com?part=6