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@xxxxxxxxxx> 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@xxxxxxxxxxxxxx/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@xxxxxxxxxxxxxx/ >>>> Link: https://lore.kernel.org/selinux/20250423175728.3185-2-stephen.smalley.work@xxxxxxxxx/ >>>> Signed-off-by: Stephen Smalley <stephen.smalley.work@xxxxxxxxx> >>>> --- >>>> 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.
