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

[sashiko-bot@kernel.org]

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 <caixinchen1@huawei.com>

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/20261027121151.1887650-1-caixinchen1@huawei.com?part=8