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

[Casey Schaufler <casey@schaufler-ca.com>]

On 4/25/2025 10:21 AM, Casey Schaufler wrote:
> On 4/25/2025 8:14 AM, Stephen Smalley wrote:
>> On Fri, Apr 25, 2025 at 5:20 AM Christian Brauner <brauner@kernel.org> wrote:
>>> 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.
>> Prior to the coreutils change noted in [1], no one would have had
>> reason to notice. I might also be wrong about the point where it was
>> first introduced - I didn't verify via testing the old commit, just
>> looked for when tmpfs gained its own xattr handlers that didn't call
>> security_inode_listsecurity().
>>
>> [1] https://lore.kernel.org/selinux/CAEjxPJ6ocwsAAdT8cHGLQ77Z=+HOXg2KkaKNP8w9CruFj2ChoA@mail.gmail.com/T/#t
>>
>>>> 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.
>> SELinux has always returned a result for getxattr(...,
>> "security.selinux", ...) regardless of whether one has been set by
>> userspace or fetched from backing store because it assigns a label to
>> all inodes for use in permission checks, regardless.
> Smack has the same behavior. Any strict subject+object+access scheme
> can be expected to do this.
>
>> And likewise returned "security.selinux" in listxattr() for all inodes
>> using either the vfs fallback or in the per-filesystem handlers prior
>> to the introduction of xattr handlers for tmpfs and later
>> sysfs/kernfs. SELinux labels were always a bit different than regular
>> xattrs; the original implementation didn't use xattrs but we were
>> directed to use them instead of our own MAC labeling scheme.
> There aren't a complete set of "rules" for filesystems supporting
> xattrs. As a result, LSMs have to be creative when a filesystem does
> not cooperate, or does so in a peculiar manner.
>
>
>>>> 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?
> Nope.

Oops. I'm wrong. More below ..


>>> Afaict, bpf is always active and has a hook for this.
>>> So the LSMs trample over each other filling the buffer?
> The bpf hook exists, but had better be a NOP if either SELinux
> or Smack is active. There are multiple cases where bpf, with its
> "all hooks defined" strategy can disrupt system behavior. The bpf
> LSM was known to be unsafe in this regard when it was accepted.
>
>> There are a number of residual challenges to supporting full stacking
>> of arbitrary LSMs; this is just one instance. Why one would stack
>> SELinux with Smack though I can't imagine, and that's the only
>> combination that would break (and already doesn't work, so no change
>> here).
> There's an amusing scenario where one can use Smack to separate SELinux
> containers, but it requires patches that I've been pushing slowly up the
> mountain for quite some time. The change to inode_listsecurity hooks
> won't be too bad, although I admit I've missed it so far. The change to
> security_inode_listsecurity() is going to be a bit awkward, but no more
> (or less) so than what needs done for security_secid_to_secctx().

Turns out I spoke too soon. The existing implementation of
security_inode_listsecurity() works correctly today, even in the
face of multiple LSMs (e.g. SELinux and Smack) being active. As
for security_inode_getsecurity(), there's no issue as the attribute
name desired is passed.