Re: [PATCH] fs/xattr.c: fix simple_xattr_list to always include security.* xattrs

[Christian Brauner <brauner@kernel.org>]

On Thu, Apr 24, 2025 at 11:28:20AM -0400, Stephen Smalley wrote:
> The vfs has long had a fallback to obtain the security.* xattrs from the
> LSM when the filesystem does not implement its own listxattr, but
> shmem/tmpfs and kernfs later gained their own xattr handlers to support
> other xattrs. Unfortunately, as a side effect, tmpfs and kernfs-based

This change is from 2011. So no living soul has ever cared at all for
at least 14 years. Surprising that this is an issue now.

> filesystems like sysfs no longer return the synthetic security.* xattr
> names via listxattr unless they are explicitly set by userspace or
> initially set upon inode creation after policy load. coreutils has
> recently switched from unconditionally invoking getxattr for security.*
> for ls -Z via libselinux to only doing so if listxattr returns the xattr
> name, breaking ls -Z of such inodes.

So no xattrs have been set on a given inode and we lie to userspace by
listing them anyway. Well ok then.

> Before:
> $ getfattr -m.* /run/initramfs
> <no output>
> $ getfattr -m.* /sys/kernel/fscaps
> <no output>
> $ setfattr -n user.foo /run/initramfs
> $ getfattr -m.* /run/initramfs
> user.foo
> 
> After:
> $ getfattr -m.* /run/initramfs
> security.selinux
> $ getfattr -m.* /sys/kernel/fscaps
> security.selinux
> $ setfattr -n user.foo /run/initramfs
> $ getfattr -m.* /run/initramfs
> security.selinux
> user.foo
> 
> Link: https://lore.kernel.org/selinux/CAFqZXNtF8wDyQajPCdGn=iOawX4y77ph0EcfcqcUUj+T87FKyA@mail.gmail.com/
> Link: https://lore.kernel.org/selinux/20250423175728.3185-2-stephen.smalley.work@gmail.com/
> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> ---
>  fs/xattr.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/fs/xattr.c b/fs/xattr.c
> index 02bee149ad96..2fc314b27120 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -1428,6 +1428,15 @@ static bool xattr_is_trusted(const char *name)
>  	return !strncmp(name, XATTR_TRUSTED_PREFIX, XATTR_TRUSTED_PREFIX_LEN);
>  }
>  
> +static bool xattr_is_maclabel(const char *name)
> +{
> +	const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
> +
> +	return !strncmp(name, XATTR_SECURITY_PREFIX,
> +			XATTR_SECURITY_PREFIX_LEN) &&
> +		security_ismaclabel(suffix);
> +}
> +
>  /**
>   * simple_xattr_list - list all xattr objects
>   * @inode: inode from which to get the xattrs
> @@ -1460,6 +1469,17 @@ ssize_t simple_xattr_list(struct inode *inode, struct simple_xattrs *xattrs,
>  	if (err)
>  		return err;
>  
> +	err = security_inode_listsecurity(inode, buffer, remaining_size);

Is that supposed to work with multiple LSMs?
Afaict, bpf is always active and has a hook for this.
So the LSMs trample over each other filling the buffer?

> +	if (err < 0)
> +		return err;
> +
> +	if (buffer) {
> +		if (remaining_size < err)
> +			return -ERANGE;
> +		buffer += err;
> +	}
> +	remaining_size -= err;

Really unpleasant code duplication in here. We have xattr_list_one() for
that. security_inode_listxattr() should probably receive a pointer to
&remaining_size?

> +
>  	read_lock(&xattrs->lock);
>  	for (rbp = rb_first(&xattrs->rb_root); rbp; rbp = rb_next(rbp)) {
>  		xattr = rb_entry(rbp, struct simple_xattr, rb_node);
> @@ -1468,6 +1488,10 @@ ssize_t simple_xattr_list(struct inode *inode, struct simple_xattrs *xattrs,
>  		if (!trusted && xattr_is_trusted(xattr->name))
>  			continue;
>  
> +		/* skip MAC labels; these are provided by LSM above */
> +		if (xattr_is_maclabel(xattr->name))
> +			continue;
> +
>  		err = xattr_list_one(&buffer, &remaining_size, xattr->name);
>  		if (err)
>  			break;
> -- 
> 2.49.0
>