On Fri, Apr 25, 2025 at 11:14:46AM -0400, 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. > 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. Interesting, thanks for the background. > > > > > > 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. A follow-up cleanup would be very much appreciated. > > > > > > + > > > 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 > > >
