Message-ID: <184c3ed7-5581-4bdf-99ea-083e28e530a8@schaufler-ca.com>
Date: Fri, 25 Apr 2025 10:21:00 -0700
From: Casey Schaufler <casey@...aufler-ca.com>
To: Stephen Smalley <stephen.smalley.work@...il.com>,
 Christian Brauner <brauner@...nel.org>
Cc: paul@...l-moore.com, omosnace@...hat.com, selinux@...r.kernel.org,
 linux-security-module@...r.kernel.org,
 Alexander Viro <viro@...iv.linux.org.uk>, Jan Kara <jack@...e.cz>,
 linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
 Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH] fs/xattr.c: fix simple_xattr_list to always include
 security.* xattrs

On 4/25/2025 8:14 AM, Stephen Smalley wrote:
> On Fri, Apr 25, 2025 at 5:20 AM Christian Brauner <brauner@...nel.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@...il.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.

>> 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().

>>> +     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?
> Not sure how to avoid the duplication, but willing to take it inside
> of security_inode_listsecurity() and change its hook interface if
> desired.

>
>>> +
>>>       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
>>>

Powered by blists - more mailing lists