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

[Paul Moore <paul@paul-moore.com>]

On Jan 30, 2026 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@seltendoof.de> 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@googlemail.com>
> ---
>  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.

> +	return simple_setattr(idmap, dentry, iattr);
> +}
> +
> +static const struct inode_operations sel_handle_status_iops = {
> +	.setattr	= sel_setattr_handle_status,
> +};
> +
>  static int sel_open_handle_status(struct inode *inode, struct file *filp)
>  {
> -	struct page    *status = selinux_kernel_status_page();
> +	struct page *status;
>  
> +	/* Prevent truncation to avoid raising SIGBUS */
> +	if (filp->f_flags & O_TRUNC)
> +		return -EINVAL;

Same as above, should this be -EPERM?

> +	status = selinux_kernel_status_page();
>  	if (!status)
>  		return -ENOMEM;
>  
> @@ -1980,7 +2000,6 @@ static int sel_fill_super(struct super_block *sb, struct fs_context *fc)
>  		[SEL_CHECKREQPROT] = {"checkreqprot", &sel_checkreqprot_ops, S_IRUGO|S_IWUSR},
>  		[SEL_REJECT_UNKNOWN] = {"reject_unknown", &sel_handle_unknown_ops, S_IRUGO},
>  		[SEL_DENY_UNKNOWN] = {"deny_unknown", &sel_handle_unknown_ops, S_IRUGO},
> -		[SEL_STATUS] = {"status", &sel_handle_status_ops, S_IRUGO},
>  		[SEL_POLICY] = {"policy", &sel_policy_ops, S_IRUGO},
>  		[SEL_VALIDATE_TRANS] = {"validatetrans", &sel_transition_ops,
>  					S_IWUGO},
> @@ -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.

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;
+       }
+
        if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID |
                        ATTR_ATIME_SET | ATTR_MTIME_SET | ATTR_TIMES_SET))
                return dentry_has_perm(cred, dentry, FILE__SETATTR);
diff --git a/security/selinux/include/selinuxfs.h b/security/selinux/include/selinuxfs.h
new file mode 100644
index 000000000000..f5e6da02833b
--- /dev/null
+++ b/security/selinux/include/selinuxfs.h
@@ -0,0 +1,2 @@
+int selinuxfs_inode_setattr(struct mnt_idmap *idmap, struct inode *inode,
+                            struct iattr *iattr);
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 896acad1f5f7..5af5e1dee743 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -43,6 +43,7 @@
 #include "objsec.h"
 #include "conditional.h"
 #include "ima.h"
+#include "selinuxfs.h"
 
 enum sel_inos {
        SEL_ROOT_INO = 2,
@@ -1954,6 +1955,20 @@ static struct dentry *sel_make_swapover_dir(struct super_block *sb,
        return dentry;  // borrowed
 }
 
+int selinuxfs_inode_setattr(struct mnt_idmap *idmap, struct inode *inode,
+                           struct iattr *iattr)
+{
+       switch (inode->i_ino) {
+       case SEL_STATUS:
+               if (iattr->ia_valid & ATTR_SIZE)
+                       return -EPERM;
+       default:
+               break;
+       }
+
+       return 0;
+}

--
paul-moore.com