Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] ovl: bypass of underlying LSM checks in ovl_real_fileattr_get() and set - [Medium] vfs: vfs_fileattr_get() runs LSM hook before checking i_op->fileattr_get --
commit 2d42ade8215536e97143b2105a207c59391b323b Author: Cai Xinchen <[email protected]> LSM: pass struct path and struct file to the inode fileattr hooks This commit updates vfs_fileattr_get(), vfs_fileattr_set(), and the inode_file_getattr and inode_file_setattr security hooks to accept a const struct path and struct file. It also introduces unchecked __vfs_fileattr_get() and __vfs_fileattr_set() helpers and switches ecryptfs, overlayfs, and nfsd internal fileattr calls to use them. > diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c > index fcb5b4de897e4..2b8daf21aea85 100644 > --- a/fs/ecryptfs/inode.c > +++ b/fs/ecryptfs/inode.c > @@ -1077,7 +1077,7 @@ static int ecryptfs_removexattr(struct dentry *dentry, > struct inode *inode, > > static int ecryptfs_fileattr_get(struct dentry *dentry, struct file_kattr > *fa) > { > - return vfs_fileattr_get(ecryptfs_dentry_to_lower(dentry), fa); > + return __vfs_fileattr_get(ecryptfs_dentry_to_lower(dentry), fa); > } > > static int ecryptfs_fileattr_set(const struct mnt_idmap *idmap, > @@ -1086,7 +1086,7 @@ static int ecryptfs_fileattr_set(const struct mnt_idmap > *idmap, > struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry); > int rc; > > - rc = vfs_fileattr_set(&nop_mnt_idmap, lower_dentry, fa); > + rc = __vfs_fileattr_set(&nop_mnt_idmap, lower_dentry, fa); > fsstack_copy_attr_all(d_inode(dentry), d_inode(lower_dentry)); > > return rc; > diff --git a/fs/file_attr.c b/fs/file_attr.c > index 833e03810b24c..08b4cdbef9ffa 100644 > --- a/fs/file_attr.c > +++ b/fs/file_attr.c > @@ -75,27 +75,48 @@ void fileattr_fill_flags(struct file_kattr *fa, u32 flags) > EXPORT_SYMBOL(fileattr_fill_flags); > > /** > - * vfs_fileattr_get - retrieve miscellaneous file attributes > + * __vfs_fileattr_get - retrieve miscellaneous file attributes > * @dentry: the object to retrieve from > * @fa: fileattr pointer > * > - * Call i_op->fileattr_get() callback, if exists. > + * Call i_op->fileattr_get() callback, if exists. Unlike > + * vfs_fileattr_get(), no security hook is called: this is the variant to > + * use for internal forwards, where the LSM checks were already performed > + * on the user-facing path (e.g. by a stacking filesystem). > * > * Return: 0 on success, or a negative error on failure. > */ > -int vfs_fileattr_get(struct dentry *dentry, struct file_kattr *fa) > +int __vfs_fileattr_get(struct dentry *dentry, struct file_kattr *fa) > { > struct inode *inode = d_inode(dentry); > - int error; > > if (!inode->i_op->fileattr_get) > return -ENOIOCTLCMD; > > - error = security_inode_file_getattr(dentry, fa); > + return inode->i_op->fileattr_get(dentry, fa); > +} > +EXPORT_SYMBOL(__vfs_fileattr_get); > + > +/** > + * vfs_fileattr_get - retrieve miscellaneous file attributes > + * @path: the object to retrieve from > + * @fa: fileattr pointer > + * @file: file through which the attributes are retrieved, if any > + * > + * Call i_op->fileattr_get() callback, if exists. > + * > + * Return: 0 on success, or a negative error on failure. > + */ > +int vfs_fileattr_get(const struct path *path, struct file_kattr *fa, > + struct file *file) > +{ > + int error; > + > + error = security_inode_file_getattr(path, fa, file); > if (error) > return error; > > - return inode->i_op->fileattr_get(dentry, fa); > + return __vfs_fileattr_get(path->dentry, fa); > } [Severity: Medium] Should vfs_fileattr_get() check whether inode->i_op->fileattr_get is implemented before calling security_inode_file_getattr()? Previously, vfs_fileattr_get() checked !inode->i_op->fileattr_get and returned -ENOIOCTLCMD before invoking security_inode_file_getattr(), matching do_vfs_fileattr_set() in fs/file_attr.c: if (!inode->i_op->fileattr_set) return -ENOIOCTLCMD; With the check moved into __vfs_fileattr_get(), calling vfs_fileattr_get() on an inode whose filesystem does not implement i_op->fileattr_get (such as tmpfs, procfs, sysfs, NFS, FUSE without fileattr_get, or special files) runs security_inode_file_getattr() first. If an LSM denies the hook, vfs_fileattr_get() returns -EACCES (and generates audit denial records) instead of -ENOIOCTLCMD. Callers such as do_vfs_ioctl() -> ioctl_getflags() / ioctl_fsgetxattr(), file_getattr(), and ksmbd's smb2_get_info_filesystem(), ksmbd_vfs_get_compression(), and __ksmbd_vfs_set_compression() rely on -ENOIOCTLCMD to fall back to vfs_ioctl(), -EOPNOTSUPP, or default attributes: fs/smb/server/smb2pdu.c:smb2_get_info_filesystem() { ... err = vfs_fileattr_get(&path, &fa, NULL); if (err && err != -ENOIOCTLCMD && err != -ENOTTY && err != -EINVAL && err != -EOPNOTSUPP) { path_put(&path); return err; } ... } [ ... ] > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c > index eb408284df082..4f1d3077735bd 100644 > --- a/fs/overlayfs/inode.c > +++ b/fs/overlayfs/inode.c > @@ -608,47 +608,10 @@ static int ovl_fiemap(struct inode *inode, struct > fiemap_extent_info *fieinfo, > return realinode->i_op->fiemap(realinode, fieinfo, start, len); > } > > -/* > - * Work around the fact that security_file_ioctl() takes a file argument. > - * Introducing security_inode_fileattr_get/set() hooks would solve this issue > - * properly. > - */ > -static int ovl_security_fileattr(const struct path *realpath, struct > file_kattr *fa, > - bool set) > -{ > - struct file *file; > - unsigned int cmd; > - int err; > - unsigned int flags; > - > - flags = O_RDONLY; > - if (force_o_largefile()) > - flags |= O_LARGEFILE; > - > - file = dentry_open(realpath, flags, current_cred()); > - if (IS_ERR(file)) > - return PTR_ERR(file); > - > - if (set) > - cmd = fa->fsx_valid ? FS_IOC_FSSETXATTR : FS_IOC_SETFLAGS; > - else > - cmd = fa->fsx_valid ? FS_IOC_FSGETXATTR : FS_IOC_GETFLAGS; > - > - err = security_file_ioctl(file, cmd, 0); > - fput(file); > - > - return err; > -} > - > int ovl_real_fileattr_set(const struct path *realpath, struct file_kattr *fa) > { > - int err; > - > - err = ovl_security_fileattr(realpath, fa, true); > - if (err) > - return err; > - > - return vfs_fileattr_set(mnt_idmap(realpath->mnt), realpath->dentry, fa); > + return __vfs_fileattr_set(mnt_idmap(realpath->mnt), realpath->dentry, > + fa); > } > > int ovl_fileattr_set(const struct mnt_idmap *idmap, > @@ -718,11 +681,7 @@ int ovl_real_fileattr_get(const struct path *realpath, > struct file_kattr *fa) > { > int err; > > - err = ovl_security_fileattr(realpath, fa, false); > - if (err) > - return err; > - > - err = vfs_fileattr_get(realpath->dentry, fa); > + err = __vfs_fileattr_get(realpath->dentry, fa); > if (err == -ENOIOCTLCMD) > err = -ENOTTY; > return err; [Severity: High] Won't removing ovl_security_fileattr() while also switching ovl_real_fileattr_set() and ovl_real_fileattr_get() (as well as ecryptfs_fileattr_get() and ecryptfs_fileattr_set() in fs/ecryptfs/inode.c) to the unchecked __vfs_fileattr_set() and __vfs_fileattr_get() helpers bypass mandatory access control checks on the underlying layer? As described in Documentation/filesystems/overlayfs.rst ("Permission model"), overlayfs performs two permission checks on each access: a) check if current task is allowed access based on local DAC (owner, group, mode and posix acl), as well as MAC checks b) check if stashed credentials would be allowed real operation on lower or upper layer based on underlying filesystem permissions, again including MAC checks Other overlayfs metadata operations (such as ovl_do_notify_change(), ovl_xattr_get(), ovl_do_setxattr(), ovl_do_removexattr(), ovl_get_acl_path(), and ovl_do_set_acl()) invoke the hook-checked VFS helpers on the underlying realpath under with_ovl_creds() to enforce the second check. Commit 66dbfabf10d3 ("ovl: stack fileattr ops") added ovl_security_fileattr() under the mounter's credentials to check the underlying realpath when vfs_fileattr_get() and vfs_fileattr_set() lacked LSM hooks. Now that vfs_fileattr_get() and vfs_fileattr_set() invoke the security hooks on a struct path, using __vfs_fileattr_get() and __vfs_fileattr_set() in ovl_real_fileattr_get() and ovl_real_fileattr_set() skips the second MAC check on realpath altogether. For example, when an unprivileged user namespace mounts overlayfs with a context= mount option (or with override_creds) and calls path-based file_getattr() or file_setattr() on an upper file, the first check only evaluates the overlay mountpoint label, and __vfs_fileattr_get() / __vfs_fileattr_set() bypass SELinux FILE__GETATTR and FILE__SETATTR checks on the underlying inode. Similarly, in ecryptfs, the lower filesystem can have per-file SELinux xattr labels distinct from the ecryptfs genfscon label, and ecryptfs_setattr(), ecryptfs_setxattr(), and ecryptfs_set_acl() all call the hook-checked VFS helpers on the lower dentry/path. Should ovl_real_fileattr_get(), ovl_real_fileattr_set(), ecryptfs_fileattr_get(), and ecryptfs_fileattr_set() call vfs_fileattr_get() and vfs_fileattr_set() instead of the unchecked helpers? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=8
