| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJ2a_Dddd=u9zGB0KB2-YeAbteByVa__4+S21mYxcPJi0DFm3g@mail.gmail.com>
Date: Sat, 31 Jan 2026 18:18:47 +0100
From: Christian Göttsche <cgzones@...glemail.com>
To: selinux@...r.kernel.org
Cc: Paul Moore <paul@...l-moore.com>, Stephen Smalley <stephen.smalley.work@...il.com>,
Ondrej Mosnacek <omosnace@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC] selinux: prevent truncation of status map
On Fri, 30 Jan 2026 at 21:46, Paul Moore <paul@...l-moore.com> wrote:
>
> On Jan 30, 2026 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@...tendoof.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@...glemail.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.
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.
>
> > + 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.
We could ask the FS people if they would be fine with the following patch:
diff --git a/fs/libfs.c b/fs/libfs.c
index 9264523be85c..76f7fec136cb 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -1089,6 +1089,7 @@ int simple_fill_super(struct super_block *s,
unsigned long magic,
}
inode->i_mode = S_IFREG | files->mode;
simple_inode_init_ts(inode);
+ inode->i_op = files->iops;
inode->i_fop = files->ops;
inode->i_ino = i;
d_make_persistent(dentry, inode);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 04ceeca12a0d..9f1a9f0a9b48 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3225,7 +3225,7 @@ extern const struct file_operations simple_dir_operations;
extern const struct inode_operations simple_dir_inode_operations;
extern void make_empty_dir_inode(struct inode *inode);
extern bool is_empty_dir_inode(struct inode *inode);
-struct tree_descr { const char *name; const struct file_operations
*ops; int mode; };
+struct tree_descr { const char *name; const struct file_operations
*ops; int mode; const struct inode_operations *iops; };
struct dentry *d_alloc_name(struct dentry *, const char *);
extern int simple_fill_super(struct super_block *, unsigned long,
const struct tree_descr *);
and then adding the hook would just be
- [SEL_STATUS] = {"status", &sel_handle_status_ops, S_IRUGO},
+ [SEL_STATUS] = {"status", &sel_handle_status_ops,
S_IRUGO, &sel_handle_status_iops},
>
> 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.
> 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
Powered by blists - more mailing lists