Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] vfs: do_fchmodat(), do_fchownat(), and do_utimes_path() reject O_PATH fds - [High] lsm: missing __nullable on file parameter of inode_setattr hook - [High] fs: AT_EMPTY_PATH regressions in do_utimes_path(), do_fchmodat(), and do_fchownat() - [Medium] lsm: untrusted path->dentry breaks bpf_lsm_inode_setattr() dentry kfuncs --
commit 5205e882fa7eb32587d3ca77d8a0f86441ec4e69 Author: Cai Xinchen <[email protected]> LSM: pass struct path and struct file to the inode_setattr hook This commit updates the inode_setattr LSM hook and its callers to pass a struct path and an optional struct file alongside the mnt_idmap. It also dispatches AT_EMPTY_PATH operations in do_fchmodat(), do_fchownat(), and do_utimes_path() to the file descriptor-based helpers so the struct file can be threaded through notify_change(). > diff --git a/fs/open.c b/fs/open.c > index 294d0ea7488e6..882985d4008bf 100644 > --- a/fs/open.c > +++ b/fs/open.c [ ... ] > @@ -724,12 +729,22 @@ static int do_fchmodat(int dfd, const char __user > *filename, umode_t mode, > if (unlikely(flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH))) > return -EINVAL; > > + CLASS(filename_maybe_null, name)(filename, flags); > + > + if (!name && dfd >= 0) { > + CLASS(fd, f)(dfd); > + > + if (fd_empty(f)) > + return -EBADF; > + > + return vfs_fchmod(fd_file(f), mode); > + } [ ... ] > @@ -844,14 +860,24 @@ int do_fchownat(int dfd, const char __user *filename, > uid_t user, gid_t group, > if ((flag & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0) > return -EINVAL; > > + CLASS(filename_maybe_null, name)(filename, flag); > + > + if (!name && dfd >= 0) { > + CLASS(fd, f)(dfd); > + > + if (fd_empty(f)) > + return -EBADF; > + > + return vfs_fchown(fd_file(f), user, group); > + } [Severity: High] Does using CLASS(fd, f)(dfd) here and in do_fchmodat() and do_utimes_path() break AT_EMPTY_PATH on O_PATH file descriptors? Previously, an empty path with AT_EMPTY_PATH went through filename_lookup() -> path_lookupat() -> path_init(), which looks up nd->dfd using CLASS(fd_raw, f)(nd->dfd) (just as vfs_fstat() does for vfs_fstatat()): fs/namei.c:path_init() { ... CLASS(fd_raw, f)(nd->dfd); struct dentry *dentry; if (fd_empty(f)) return ERR_PTR(-EBADF); ... } Supporting O_PATH file descriptors without /proc/self/fd was the reason AT_EMPTY_PATH was added to fchmodat2(2) in commit 5daeb41a6fc9 ("fchmodat2: add support for AT_EMPTY_PATH"), and is documented UAPI for fchownat(2) and utimensat(2), since fchmod(2), fchown(2), and futimens(3) reject O_PATH descriptors with EBADF. By switching to CLASS(fd, f)(dfd), which calls fdget(dfd) -> __fget_light(dfd, FMODE_PATH), fd_empty(f) is true whenever FMODE_PATH is set, causing fchmodat2(dfd, "", mode, AT_EMPTY_PATH), fchownat(dfd, "", user, group, AT_EMPTY_PATH), and utimensat(dfd, "", times, AT_EMPTY_PATH) on an O_PATH fd to fail with -EBADF. In addition, in do_dentry_open(), O_PATH files return before security_file_open() is called: fs/open.c:do_dentry_open() { ... if (unlikely(f->f_flags & O_PATH)) { f->f_mode = FMODE_PATH | FMODE_OPENED; file_set_fsnotify_mode(f, FMODE_NONOTIFY); f->f_op = &empty_fops; return 0; } ... } Because an O_PATH struct file was never authorized at open(2) time, should these callers use CLASS(fd_raw, f)(dfd) and ensure that LSMs checking the struct file in security_inode_setattr() do not treat an FMODE_PATH file as carrying open-time permissions? > diff --git a/fs/utimes.c b/fs/utimes.c > index 5193d504d289f..6021c481e742f 100644 > --- a/fs/utimes.c > +++ b/fs/utimes.c [ ... ] > @@ -88,15 +89,25 @@ static int do_utimes_path(int dfd, const char __user > *filename, > if (flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) > return -EINVAL; > > + CLASS(filename_maybe_null, name)(filename, flags); > + > + if (!name && dfd >= 0) { > + CLASS(fd, f)(dfd); > + > + if (fd_empty(f)) > + return -EBADF; > + > + return vfs_utimes(&fd_file(f)->f_path, times, fd_file(f)); > + } [Severity: High] Beyond the CLASS(fd, f)(dfd) issue with O_PATH file descriptors (which also makes the FMODE_PATH check for hook_path_setattr in commit "landlock: Add READ_METADATA and WRITE_METADATA access rights" unreachable), can switching do_utimes_path() from CLASS(filename_uflags, name) to CLASS(filename_maybe_null, name) cause utimensat(AT_FDCWD, NULL, times, AT_EMPTY_PATH) to modify the current working directory instead of returning -EFAULT? In do_utimes(), a NULL filename with dfd != AT_FDCWD is routed to do_utimes_fd() (which rejects non-zero flags like AT_EMPTY_PATH with -EINVAL), whereas dfd == AT_FDCWD is routed to do_utimes_path(): fs/utimes.c:do_utimes() { if (filename == NULL && dfd != AT_FDCWD) return do_utimes_fd(dfd, times, flags); return do_utimes_path(dfd, filename, times, flags); } When filename == NULL, dfd == AT_FDCWD, and flags == AT_EMPTY_PATH, CLASS(filename_uflags, name) previously returned ERR_PTR(-EFAULT). With CLASS(filename_maybe_null, name), name is NULL, the (!name && dfd >= 0) check is skipped because AT_FDCWD < 0, and filename_lookup(AT_FDCWD, NULL, ...) treats NULL as an empty path and updates the current working directory. Also, when (!name && dfd >= 0) is taken in do_utimes_path(), it bypasses filename_lookup() (which called audit_inode()) and calls vfs_utimes(). Unlike vfs_fchmod() and vfs_fchown(), vfs_utimes() does not call audit_file(). Should audit_file(fd_file(f)) be called before vfs_utimes() on the fd paths? > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h > index 539525648b303..ea6e0fbaa3b9c 100644 > --- a/include/linux/lsm_hook_defs.h > +++ b/include/linux/lsm_hook_defs.h > @@ -141,8 +141,16 @@ LSM_HOOK(int, 0, inode_follow_link, struct dentry > *dentry, struct inode *inode, > bool rcu) > LSM_HOOK(int, 0, inode_permission, const struct mnt_idmap *idmap, > struct inode *inode, int mask) > -LSM_HOOK(int, 0, inode_setattr, const struct mnt_idmap *idmap, struct dentry > *dentry, > - struct iattr *attr) > +/* > + * For the hooks that pass a struct path: the VFS only sanctions > + * passing the path to VFS helpers taking a struct path (e.g., > + * notify_change(), the *xattr() helpers); it makes no guarantees > + * with regard to path->mnt, whose layout and implementation details > + * are subject to change. Implementations must not rely on its > + * properties. > + */ > +LSM_HOOK(int, 0, inode_setattr, const struct mnt_idmap *idmap, > + const struct path *path, struct iattr *attr, struct file *file) [Severity: High] Could omitting a __nullable annotation on the struct file *file parameter of inode_setattr allow a BPF LSM program to trigger a kernel NULL pointer dereference when notify_change() is called with file == NULL? All path-based and internal callers of notify_change() (such as chmod_common(), chown_common(), vfs_utimes(), do_truncate(), and __remove_privs()) pass file == NULL, which do_notify_change() forwards to security_inode_setattr(idmap, path, attr, NULL). In btf_ctx_access(), arguments of trusted BPF LSM hooks are marked PTR_TO_BTF_ID | PTR_TRUSTED, and PTR_MAYBE_NULL is only added if 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 __nullable (either in lsm_hook_defs.h or via a strong override in kernel/bpf/bpf_lsm_proto.c like bpf_lsm_mmap_file()), the BPF verifier treats file as a non-NULL trusted pointer and permits passing it without a NULL check to kfuncs such as bpf_get_file_xattr() or bpf_real_data_inode(), which immediately dereference file->f_path.dentry. [Severity: Medium] Does replacing struct dentry *dentry with const struct path *path in inode_setattr also break sleepable BPF LSM programs that pass the dentry to bpf_get_dentry_xattr(), bpf_set_dentry_xattr(), or bpf_remove_dentry_xattr()? bpf_lsm_inode_setattr is listed in d_inode_locked_hooks in fs/bpf_fs_kfuncs.c specifically so sleepable BPF LSM programs attached to inode_setattr can call those dentry xattr kfuncs: fs/bpf_fs_kfuncs.c { BTF_SET_START(d_inode_locked_hooks) ... BTF_ID(func, bpf_lsm_inode_setattr) ... BTF_SET_END(d_inode_locked_hooks) } However, struct path is not listed in BTF_TYPE_SAFE_TRUSTED in type_is_trusted() in kernel/bpf/verifier.c. When a BPF program walks path->dentry, check_ptr_to_btf_access() marks the resulting dentry pointer as PTR_UNTRUSTED, causing the BPF verifier to reject calls to the dentry xattr kfuncs. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
