Re: [PATCH] memfd,selinux: call security_inode_init_security_anon
From: Paul Moore <paul@paul-moore.com>
Date: 2025-09-03 21:26:24
Also in:
linux-mm, lkml, selinux
On Aug 25, 2025 "=?UTF-8?q?Thi=C3=A9baud=20Weksteen?=" [off-list ref] wrote:
quoted hunk ↗ jump to hunk
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/ (local) Signed-off-by: Thiébaud Weksteen <redacted> Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com> Tested-by: Stephen Smalley <stephen.smalley.work@gmail.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.
quoted hunk ↗ jump to hunk
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().
quoted hunk ↗ jump to hunk
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