Message-ID: <20250428-zielgebiet-zinspolitik-5a500ebb76c7@brauner>
Date: Mon, 28 Apr 2025 10:53:43 +0200
From: Christian Brauner <brauner@...nel.org>
To: Stephen Smalley <stephen.smalley.work@...il.com>
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
Subject: Re: [PATCH] fs/xattr.c: fix simple_xattr_list to always include
 security.* xattrs

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@...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.
> 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@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?
> > 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
> > >

Powered by blists - more mailing lists