Re: [PATCH RFC] selinux: prevent truncation of status map

[Paul Moore]

On Sat, Jan 31, 2026 at 12:18 PM Christian Göttsche
<cgzones@xxxxxxxxxxxxxx> wrote:
> On Fri, 30 Jan 2026 at 21:46, Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > On Jan 30, 2026 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@xxxxxxxxxxxxx> wrote:
> > >
> > > Currently the SELinux status map can be truncated, given the necessary
> > > permissions, leading to foreign user space processes getting a bus error
> > > (SIGBUS) while concurrently making use of the status map.
> > > For example systemd can be killed that way, see [1].
> > >
> > > Override the setattr inode handler and check for O_TRUNC in the open
> > > handler to prevent truncations.
> > >
> > > Link [1]: https://github.com/systemd/systemd/issues/37349
> > >
> > > Closes: https://github.com/SELinuxProject/selinux/issues/475
> > > Signed-off-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>
> > > ---
> > >  security/selinux/selinuxfs.c | 43 ++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 41 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> > > index 896acad1f5f7..df079a35a02d 100644
> > > --- a/security/selinux/selinuxfs.c
> > > +++ b/security/selinux/selinuxfs.c
> > > @@ -214,10 +214,30 @@ static const struct file_operations sel_handle_unknown_ops = {
> > >       .llseek         = generic_file_llseek,
> > >  };
> > >
> > > +static int sel_setattr_handle_status(struct mnt_idmap *idmap,
> > > +                                  struct dentry *dentry,
> > > +                                  struct iattr *iattr)
> > > +{
> > > +     /* Prevent truncation to avoid raising SIGBUS */
> > > +     if (iattr->ia_valid & ATTR_SIZE)
> > > +             return -EINVAL;
> >
> > Do we want this as -EINVAL or -EPERM?  However, see my comments below
> > about how to handle the ATTR_SIZE case.
>
> I did not choose EPERM because it is not a matter of missing
> permissions of the caller.
> The status page should just not support truncation and EINVAL seemed
> more natural than ENOTSUP.

Okay, I can understand that argument.

> > > @@ -1995,6 +2014,26 @@ static int sel_fill_super(struct super_block *sb, struct fs_context *fc)
> > >       if (ret)
> > >               goto err;
> > >
> > > +     /* Create "status" separately to assign a custom inode_operations */
> > > +     {
> > > +             ret = -ENOMEM;
> > > +
> > > +             dentry = d_alloc_name(sb->s_root, "status");
> > > +             if (!dentry)
> > > +                     goto err;
> > > +             inode = new_inode(sb);
> > > +             if (!inode) {
> > > +                     dput(dentry);
> > > +                     goto err;
> > > +             }
> > > +             inode->i_mode = S_IFREG | 0444;
> > > +             simple_inode_init_ts(inode);
> > > +             inode->i_fop = &sel_handle_status_ops;
> > > +             inode->i_op = &sel_handle_status_iops;
> > > +             inode->i_ino = SEL_STATUS;
> > > +             d_add(dentry, inode);
> > > +     }
> >
> > I worry a little about duplicating and open coding the per-file loop from
> > simple_fill_super(), I can see things slowly getting out of sync and bad
> > things happening.  Unfortunately, I don't see anything in libfs that would
> > allow us to supply our own inode_operations either.
>
> We could ask the FS people if they would be fine with the following patch:

We probably need to give them another week or so to look at your
proposal and reply.   I'm very curious to hear what they have to say
on the issue.

> > What do you think about using selinux_inode_setattr() as shown below,
> > would this work?
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index feda34b18d83..8e4374f22a18 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -106,6 +106,7 @@
> >  #include "netlabel.h"
> >  #include "audit.h"
> >  #include "avc_ss.h"
> > +#include "selinuxfs.h"
> >
> >  #define SELINUX_INODE_INIT_XATTRS 1
> >
> > @@ -3291,6 +3292,12 @@ static int selinux_inode_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> >                         return 0;
> >         }
> >
> > +       if (inode->i_sb->s_magic == SELINUX_MAGIC) {
> > +               int rc = selinuxfs_inode_setattr(idmap, inode, iattr);
> > +               if (rc)
> > +                       return rc;
> > +       }
> > +
>
> I don't like that approach, since up to now selinux_inode_setattr() is
> only for SELinux permission checking and this would add some behavior
> modification into it.

I disagree with that characterization, but I'm not in love with that
solution either; special-casing specific filesystems always feels ugly
to me.  Let's see what vfs folks have to say about your proposal.

-- 
paul-moore.com