| 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: <92255d5e-7e0a-6ca3-3169-114ae7f6247f@google.com>
Date: Mon, 15 Sep 2025 22:06:52 -0700 (PDT)
From: Hugh Dickins <hughd@...gle.com>
To: Paul Moore <paul@...l-moore.com>
cc: Thiébaud Weksteen <tweek@...gle.com>,
James Morris <jmorris@...ei.org>,
Stephen Smalley <stephen.smalley.work@...il.com>,
Hugh Dickins <hughd@...gle.com>, Jeff Vander Stoep <jeffv@...gle.com>,
Nick Kralevich <nnk@...gle.com>, Jeff Xu <jeffxu@...gle.com>,
Baolin Wang <baolin.wang@...ux.alibaba.com>, linux-kernel@...r.kernel.org,
linux-security-module@...r.kernel.org, selinux@...r.kernel.org,
linux-mm@...ck.org
Subject: Re: [PATCH] memfd,selinux: call security_inode_init_security_anon
On Wed, 3 Sep 2025, Paul Moore wrote:
> On Aug 25, 2025 "=?UTF-8?q?Thi=C3=A9baud=20Weksteen?=" <tweek@...gle.com> wrote:
> >
> > Prior to this change, no security hooks were called at the creation of a
> > memfd file. It means that, for SELinux as an example, it will receive
> > the default type of the filesystem that backs the in-memory inode. In
> > most cases, that would be tmpfs, but if MFD_HUGETLB is passed, it will
> > be hugetlbfs. Both can be considered implementation details of memfd.
> >
> > It also means that it is not possible to differentiate between a file
> > coming from memfd_create and a file coming from a standard tmpfs mount
> > point.
> >
> > Additionally, no permission is validated at creation, which differs from
> > the similar memfd_secret syscall.
> >
> > Call security_inode_init_security_anon during creation. This ensures
> > that the file is setup similarly to other anonymous inodes. On SELinux,
> > it means that the file will receive the security context of its task.
> >
> > The ability to limit fexecve on memfd has been of interest to avoid
> > potential pitfalls where /proc/self/exe or similar would be executed
> > [1][2]. Reuse the "execute_no_trans" and "entrypoint" access vectors,
> > similarly to the file class. These access vectors may not make sense for
> > the existing "anon_inode" class. Therefore, define and assign a new
> > class "memfd_file" to support such access vectors.
> >
> > Guard these changes behind a new policy capability named "memfd_class".
> >
> > [1] https://crbug.com/1305267
> > [2] https://lore.kernel.org/lkml/20221215001205.51969-1-jeffxu@google.com/
> >
> > Signed-off-by: Thiébaud Weksteen <tweek@...gle.com>
> > Acked-by: Stephen Smalley <stephen.smalley.work@...il.com>
> > Tested-by: Stephen Smalley <stephen.smalley.work@...il.com>
> > ---
> > Changes since RFC:
> > - Remove enum argument, simply compare the anon inode name
> > - Introduce a policy capability for compatility
> > - Add validation of class in selinux_bprm_creds_for_exec
> >
> > include/linux/memfd.h | 2 ++
> > mm/memfd.c | 14 +++++++++--
> > security/selinux/hooks.c | 27 ++++++++++++++++++----
> > security/selinux/include/classmap.h | 2 ++
> > security/selinux/include/policycap.h | 1 +
> > security/selinux/include/policycap_names.h | 1 +
> > security/selinux/include/security.h | 5 ++++
> > 7 files changed, 46 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/linux/memfd.h b/include/linux/memfd.h
> > index 6f606d9573c3..cc74de3dbcfe 100644
> > --- a/include/linux/memfd.h
> > +++ b/include/linux/memfd.h
> > @@ -4,6 +4,8 @@
> >
> > #include <linux/file.h>
> >
> > +#define MEMFD_ANON_NAME "[memfd]"
> > +
> > #ifdef CONFIG_MEMFD_CREATE
> > extern long memfd_fcntl(struct file *file, unsigned int cmd, unsigned int arg);
> > struct folio *memfd_alloc_folio(struct file *memfd, pgoff_t idx);
> > diff --git a/mm/memfd.c b/mm/memfd.c
> > index bbe679895ef6..63b439eb402a 100644
> > --- a/mm/memfd.c
> > +++ b/mm/memfd.c
> > @@ -433,6 +433,8 @@ static struct file *alloc_file(const char *name, unsigned int flags)
> > {
> > unsigned int *file_seals;
> > struct file *file;
> > + struct inode *inode;
> > + int err = 0;
> >
> > if (flags & MFD_HUGETLB) {
> > file = hugetlb_file_setup(name, 0, VM_NORESERVE,
> > @@ -444,12 +446,20 @@ static struct file *alloc_file(const char *name, unsigned int flags)
> > }
> > if (IS_ERR(file))
> > return file;
> > +
> > + inode = file_inode(file);
> > + err = security_inode_init_security_anon(inode,
> > + &QSTR(MEMFD_ANON_NAME), NULL);
> > + if (err) {
> > + fput(file);
> > + file = ERR_PTR(err);
> > + return file;
> > + }
> > +
> > file->f_mode |= FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE;
> > file->f_flags |= O_LARGEFILE;
> >
> > if (flags & MFD_NOEXEC_SEAL) {
> > - struct inode *inode = file_inode(file);
> > -
> > inode->i_mode &= ~0111;
> > file_seals = memfd_file_seals_ptr(file);
> > if (file_seals) {
>
> Hugh, Baolin, and shmem/mm folks, are you okay with the changes above? If
> so it would be nice to get an ACK from one of you.
So far as I can tell, seems okay to me:
Acked-by: Hugh Dickins <hughd@...gle.com>
If I'd responded earlier (sorry), I would have asked for it just to use
&QSTR("[memfd]") directly in the call, rather than indirecting through
unnecessary #define MEMFD_ANON_NAME "[memfd]"; never mind, that's all.
Please do take this, along with the rest, through your security tree:
mm.git contains no conflicting change to mm/memfd.c at present.
Thanks,
Hugh
>
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index c95a5874bf7d..429b2269b35a 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -93,6 +93,7 @@
> > #include <linux/fanotify.h>
> > #include <linux/io_uring/cmd.h>
> > #include <uapi/linux/lsm.h>
> > +#include <linux/memfd.h>
> >
> > #include "avc.h"
> > #include "objsec.h"
> > @@ -2366,9 +2367,12 @@ static int selinux_bprm_creds_for_exec(struct linux_binprm *bprm)
> > ad.type = LSM_AUDIT_DATA_FILE;
> > ad.u.file = bprm->file;
> >
> > + if (isec->sclass != SECCLASS_FILE && isec->sclass != SECCLASS_MEMFD_FILE)
> > + return -EPERM;
>
> In the interest of failing fast, this should probably be moved up in the
> function to just after where @isec is set. There are also a number of
> checks that happen prior to this placement, but after the isec assignment.
> While I don't think any of those checks should be an issue, I'd rather
> not to have to worry about those and just fail the non-FILE/MEMFD_FILE
> case as soon as we can in selinux_bprm_creds_for_exec().
>
> > if (new_tsec->sid == old_tsec->sid) {
> > - rc = avc_has_perm(old_tsec->sid, isec->sid,
> > - SECCLASS_FILE, FILE__EXECUTE_NO_TRANS, &ad);
> > + rc = avc_has_perm(old_tsec->sid, isec->sid, isec->sclass,
> > + FILE__EXECUTE_NO_TRANS, &ad);
> > if (rc)
> > return rc;
> > } else {
> > @@ -2378,8 +2382,8 @@ static int selinux_bprm_creds_for_exec(struct linux_binprm *bprm)
> > if (rc)
> > return rc;
> >
> > - rc = avc_has_perm(new_tsec->sid, isec->sid,
> > - SECCLASS_FILE, FILE__ENTRYPOINT, &ad);
> > + rc = avc_has_perm(new_tsec->sid, isec->sid, isec->sclass,
> > + FILE__ENTRYPOINT, &ad);
> > if (rc)
> > return rc;
> >
> > @@ -2974,10 +2978,18 @@ static int selinux_inode_init_security_anon(struct inode *inode,
> > struct common_audit_data ad;
> > struct inode_security_struct *isec;
> > int rc;
> > + bool is_memfd = false;
> >
> > if (unlikely(!selinux_initialized()))
> > return 0;
> >
> > + if (name != NULL && name->name != NULL &&
> > + !strcmp(name->name, MEMFD_ANON_NAME)) {
> > + if (!selinux_policycap_memfd_class())
> > + return 0;
> > + is_memfd = true;
> > + }
> > +
> > isec = selinux_inode(inode);
> >
> > /*
> > @@ -2996,6 +3008,13 @@ static int selinux_inode_init_security_anon(struct inode *inode,
> >
> > isec->sclass = context_isec->sclass;
> > isec->sid = context_isec->sid;
> > + } else if (is_memfd) {
> > + isec->sclass = SECCLASS_MEMFD_FILE;
> > + rc = security_transition_sid(
> > + sid, sid,
> > + isec->sclass, name, &isec->sid);
> > + if (rc)
> > + return rc;
> > } else {
> > isec->sclass = SECCLASS_ANON_INODE;
> > rc = security_transition_sid(
>
> We're duplicating the security_transition_sid() call which seems less
> than ideal, how about something like this?
>
> if (context_inode) {
> /* ... existing stuff ... */
> } else {
> if (is_memfd)
> isec->sclass = SECCLASS_MEMFD_FILE;
> else
> isec->sclass = SECCLASS_ANON_INODE;
> rc = security_transition_sid(...);
> if (rc)
> return rc;
> }
>
> --
> paul-moore.com
Powered by blists - more mailing lists