Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] bpf, lsm: missing __nullable on file arg in bpf_lsm_path_getattr() - [Low] tomoyo: missing @file kernel-doc in tomoyo_path_getattr() --
commit b5ecf79579b44f0d69e46ecb8c8c91e302299f23 Author: Cai Xinchen <[email protected]> LSM: rename inode_getattr to path_getattr and pass struct file This commit renames the inode_getattr LSM hook and security_inode_getattr() wrapper to path_getattr and security_path_getattr(), and adds a struct file parameter passed by file descriptor-based stat operations while path-based callers pass NULL. It also updates LSM implementations, BPF LSM/tracing BTF IDs, and BPF selftests accordingly. [ ... ] > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h > index 96ae1b548e8d4..0f7712f516444 100644 > --- a/include/linux/lsm_hook_defs.h > +++ b/include/linux/lsm_hook_defs.h > @@ -153,7 +153,8 @@ LSM_HOOK(int, 0, inode_setattr, const struct mnt_idmap > *idmap, > const struct path *path, struct iattr *attr, struct file *file) > LSM_HOOK(void, LSM_RET_VOID, inode_post_setattr, const struct mnt_idmap > *idmap, > struct dentry *dentry, int ia_valid) > -LSM_HOOK(int, 0, inode_getattr, const struct path *path) > +LSM_HOOK(int, 0, path_getattr, const struct path *path, > + struct file *file) [Severity: High] Can this trigger a NULL pointer dereference when a BPF LSM program attaches to lsm/path_getattr or lsm.s/path_getattr? In fs/stat.c, both vfs_getattr() and vfs_statx() -> vfs_statx_path() pass NULL for the file argument when calling security_path_getattr(): retval = security_path_getattr(path, NULL); and: error = vfs_statx_path(&path, flags, stat, request_mask, NULL); In kernel/bpf/bpf_lsm.c, LSM_HOOK() generates the weak hook stub bpf_lsm_path_getattr(const struct path *path, struct file *file). Because bpf_lsm_path_getattr is not in untrusted_lsm_hooks, btf_ctx_access() in kernel/bpf/btf.c marks its pointer arguments as PTR_TO_BTF_ID | PTR_TRUSTED without PTR_MAYBE_NULL unless the parameter name has a __nullable suffix: 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 the file argument: 1. bpf_may_fault_on_deref(PTR_TO_BTF_ID | PTR_TRUSTED) returns false in include/linux/bpf_verifier.h, so direct loads from file (such as file->f_inode) are permitted without a NULL check and are not rewritten to BPF_PROBE_MEM by convert_ctx_accesses(). 2. Because bpf_lsm_path_getattr is in sleepable_lsm_hooks, a sleepable lsm.s/path_getattr program can pass file without a NULL check to kfuncs expecting a non-NULL trusted struct file * (such as bpf_get_file_xattr() in fs/bpf_fs_kfuncs.c), which immediately dereference file. Should bpf_lsm_path_getattr() have a strong override in kernel/bpf/bpf_lsm_proto.c with struct file *file__nullable, similar to bpf_lsm_mmap_file()? [ ... ] > diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c > index c66e02ed8ee3b..c67455e060f74 100644 > --- a/security/tomoyo/tomoyo.c > +++ b/security/tomoyo/tomoyo.c > @@ -112,13 +112,14 @@ static int tomoyo_bprm_check_security(struct > linux_binprm *bprm) > } > > /** > - * tomoyo_inode_getattr - Target for security_inode_getattr(). > + * tomoyo_path_getattr - Target for security_path_getattr(). > * > * @path: Pointer to "struct path". > * > * Returns 0 on success, negative value otherwise. > */ > -static int tomoyo_inode_getattr(const struct path *path) > +static int tomoyo_path_getattr(const struct path *path, > + struct file *file) [Severity: Low] Does the kernel-doc comment for tomoyo_path_getattr() also need a @file entry to avoid a kernel-doc warning under W=1 builds? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
