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 <[email protected]> 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/[email protected]?part=6
