Message-ID: <CAEjxPJ4vntQ5cCo_=KN0d+5FDPRwStjXUimE4iHXJkz9oeuVCw@mail.gmail.com>
Date: Fri, 25 Apr 2025 11:14:46 -0400
From: Stephen Smalley <stephen.smalley.work@...il.com>
To: 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
Subject: Re: [PATCH] fs/xattr.c: fix simple_xattr_list to always include
 security.* xattrs

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.

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

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