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. 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. > > > 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? > Afaict, bpf is always active and has a hook for this. > So the LSMs trample over each other filling the buffer? 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). > > > + 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 > >
