[PATCH] fs/xattr.c: fix simple_xattr_list to always include security.* xattrs

[PATCH] fs/xattr.c: fix simple_xattr_list to always include security.* xattrs

[Stephen Smalley]

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

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@gmail.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);
+	if (err < 0)
+		return err;
+
+	if (buffer) {
+		if (remaining_size < err)
+			return -ERANGE;
+		buffer += err;
+	}
+	remaining_size -= err;
+
 	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

Re: [PATCH] fs/xattr.c: fix simple_xattr_list to always include security.* xattrs

[Stephen Smalley]

On Thu, Apr 24, 2025 at 11:28 AM Stephen Smalley
<stephen.smalley.work@gmail.com> 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
> 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.
>
> 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@gmail.com>

Fixes: b09e0fa4b4ea66266058ee ("tmpfs: implement generic xattr support")

> ---
>  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);
> +       if (err < 0)
> +               return err;
> +
> +       if (buffer) {
> +               if (remaining_size < err)
> +                       return -ERANGE;
> +               buffer += err;
> +       }
> +       remaining_size -= err;
> +
>         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
>

Re: [PATCH] fs/xattr.c: fix simple_xattr_list to always include security.* xattrs

[Christian Brauner]

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.

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

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

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

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

Re: [PATCH] fs/xattr.c: fix simple_xattr_list to always include security.* xattrs

[Stephen Smalley]

On Fri, Apr 25, 2025 at 5:20 AM Christian Brauner <brauner@kernel.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@gmail.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
> >

Re: [PATCH] fs/xattr.c: fix simple_xattr_list to always include security.* xattrs

[Casey Schaufler]

On 4/25/2025 8:14 AM, Stephen Smalley wrote:
> On Fri, Apr 25, 2025 at 5:20 AM Christian Brauner <brauner@kernel.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.

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

Nope.

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

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

Re: [PATCH] fs/xattr.c: fix simple_xattr_list to always include security.* xattrs

[Casey Schaufler]

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

Re: [PATCH] fs/xattr.c: fix simple_xattr_list to always include security.* xattrs

[Paul Moore]

On Fri, Apr 25, 2025 at 11:14 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Fri, Apr 25, 2025 at 5:20 AM Christian Brauner <brauner@kernel.org> wrote:
> > On Thu, Apr 24, 2025 at 11:28:20AM -0400, Stephen Smalley wrote:

...

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

We talked about moving to xattr_list_one() in the other RFC thread
earlier this week and as previously mentioned I think it's the right
thing to do.  However, considering the issue with the new coreutils
release, I think it's best to keep this patch limited to the fixes
necessary to restore the desired behavior with the recent coreutils;
this should make life easier for distro and stable backports.  We can
address the LSM hook cleanup/rework in a second patch{set} afterwards.

--
paul-moore.com

Re: [PATCH] fs/xattr.c: fix simple_xattr_list to always include security.* xattrs

[Christian Brauner]

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@kernel.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@gmail.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
> > >

Re: [PATCH] fs/xattr.c: fix simple_xattr_list to always include security.* xattrs

[Christian Brauner]

On Thu, 24 Apr 2025 11:28:20 -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
> 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.
> 
> [...]

Applied to the vfs.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[1/1] fs/xattr.c: fix simple_xattr_list to always include security.* xattrs
      https://git.kernel.org/vfs/vfs/c/8b0ba61df5a1