* [RFC PATCH 2/2] memfd: call security_inode_init_security_anon
@ 2025-08-07 7:57 Thiébaud Weksteen
2025-08-08 11:57 ` Stephen Smalley
0 siblings, 1 reply; 6+ messages in thread
From: Thiébaud Weksteen @ 2025-08-07 7:57 UTC (permalink / raw)
To: Paul Moore, James Morris, Stephen Smalley, Hugh Dickins,
Jeff Vander Stoep, Nick Kralevich, Jeff Xu
Cc: Thiébaud Weksteen, linux-kernel, linux-security-module,
selinux
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.
[1] https://crbug.com/1305267
[2] https://lore.kernel.org/lkml/20221215001205.51969-1-jeffxu@google.com/
Signed-off-by: Thiébaud Weksteen <tweek@google.com>
---
mm/memfd.c | 16 ++++++++++++++--
security/selinux/hooks.c | 15 +++++++++++----
security/selinux/include/classmap.h | 2 ++
3 files changed, 27 insertions(+), 6 deletions(-)
diff --git a/mm/memfd.c b/mm/memfd.c
index bbe679895ef6..13bff0e91816 100644
--- a/mm/memfd.c
+++ b/mm/memfd.c
@@ -433,6 +433,9 @@ static struct file *alloc_file(const char *name, unsigned int flags)
{
unsigned int *file_seals;
struct file *file;
+ struct inode *inode;
+ int err = 0;
+ const char *anon_name = "[memfd]";
if (flags & MFD_HUGETLB) {
file = hugetlb_file_setup(name, 0, VM_NORESERVE,
@@ -444,12 +447,21 @@ 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,
+ LSM_ANON_INODE_MEMFD,
+ &QSTR(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) {
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 8d36d5ebb6e5..49742930e706 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2367,8 +2367,8 @@ static int selinux_bprm_creds_for_exec(struct linux_binprm *bprm)
ad.u.file = bprm->file;
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 +2378,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;
@@ -2997,6 +2997,13 @@ static int selinux_inode_init_security_anon(struct inode *inode,
isec->sclass = context_isec->sclass;
isec->sid = context_isec->sid;
+ } else if (type == LSM_ANON_INODE_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(
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 5665aa5e7853..3ec85142771f 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -179,6 +179,8 @@ const struct security_class_mapping secclass_map[] = {
{ "anon_inode", { COMMON_FILE_PERMS, NULL } },
{ "io_uring", { "override_creds", "sqpoll", "cmd", "allowed", NULL } },
{ "user_namespace", { "create", NULL } },
+ { "memfd_file",
+ { COMMON_FILE_PERMS, "execute_no_trans", "entrypoint", NULL } },
/* last one */ { NULL, {} }
};
--
2.50.1.703.g449372360f-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH 2/2] memfd: call security_inode_init_security_anon
2025-08-07 7:57 [RFC PATCH 2/2] memfd: call security_inode_init_security_anon Thiébaud Weksteen
@ 2025-08-08 11:57 ` Stephen Smalley
2025-08-08 12:41 ` Stephen Smalley
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Smalley @ 2025-08-08 11:57 UTC (permalink / raw)
To: Thiébaud Weksteen
Cc: Paul Moore, James Morris, Hugh Dickins, Jeff Vander Stoep,
Nick Kralevich, Jeff Xu, linux-kernel, linux-security-module,
selinux
On Thu, Aug 7, 2025 at 3:57 AM Thiébaud Weksteen <tweek@google.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.
To provide backward compatibility, I would anticipate that you will
need to define a new SELinux policy capability and make this change
conditional on it being enabled, see:
https://github.com/SELinuxProject/selinux-kernel/wiki/Getting-Started#adding-a-new-selinux-policy-capability
for instructions and links to examples.
Otherwise, see below.
>
> [1] https://crbug.com/1305267
> [2] https://lore.kernel.org/lkml/20221215001205.51969-1-jeffxu@google.com/
>
> Signed-off-by: Thiébaud Weksteen <tweek@google.com>
> ---
> mm/memfd.c | 16 ++++++++++++++--
> security/selinux/hooks.c | 15 +++++++++++----
> security/selinux/include/classmap.h | 2 ++
> 3 files changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/mm/memfd.c b/mm/memfd.c
> index bbe679895ef6..13bff0e91816 100644
> --- a/mm/memfd.c
> +++ b/mm/memfd.c
> @@ -433,6 +433,9 @@ static struct file *alloc_file(const char *name, unsigned int flags)
> {
> unsigned int *file_seals;
> struct file *file;
> + struct inode *inode;
> + int err = 0;
> + const char *anon_name = "[memfd]";
>
> if (flags & MFD_HUGETLB) {
> file = hugetlb_file_setup(name, 0, VM_NORESERVE,
> @@ -444,12 +447,21 @@ 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,
> + LSM_ANON_INODE_MEMFD,
> + &QSTR(anon_name), NULL);
Since the anon_name already indicates that this is a memfd, so can't
you already distinguish these via name-based type_transition rules?
Why do we need the enum argument?
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 8d36d5ebb6e5..49742930e706 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2367,8 +2367,8 @@ static int selinux_bprm_creds_for_exec(struct linux_binprm *bprm)
> ad.u.file = bprm->file;
>
> 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);
Here and below I am a little concerned that we could end up reaching
this code on an inode with an isec->sclass that does not define the
execute_no_trans and entrypoint permissions. We should do something to
make that never happens, or check for it and always deny in that case.
> if (rc)
> return rc;
> } else {
> @@ -2378,8 +2378,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;
>
> @@ -2997,6 +2997,13 @@ static int selinux_inode_init_security_anon(struct inode *inode,
>
> isec->sclass = context_isec->sclass;
> isec->sid = context_isec->sid;
> + } else if (type == LSM_ANON_INODE_MEMFD) {
> + isec->sclass = SECCLASS_MEMFD_FILE;
> + rc = security_transition_sid(
> + sid, sid,
> + isec->sclass, name, &isec->sid);
Again, name-based type_transitions ought to be able to distinguish
memfd based on the name argument IIUC.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH 2/2] memfd: call security_inode_init_security_anon
2025-08-08 11:57 ` Stephen Smalley
@ 2025-08-08 12:41 ` Stephen Smalley
2025-08-11 5:34 ` Thiébaud Weksteen
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Smalley @ 2025-08-08 12:41 UTC (permalink / raw)
To: Thiébaud Weksteen
Cc: Paul Moore, James Morris, Hugh Dickins, Jeff Vander Stoep,
Nick Kralevich, Jeff Xu, linux-kernel, linux-security-module,
selinux
On Fri, Aug 8, 2025 at 7:57 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Thu, Aug 7, 2025 at 3:57 AM Thiébaud Weksteen <tweek@google.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.
>
> To provide backward compatibility, I would anticipate that you will
> need to define a new SELinux policy capability and make this change
> conditional on it being enabled, see:
> https://github.com/SELinuxProject/selinux-kernel/wiki/Getting-Started#adding-a-new-selinux-policy-capability
> for instructions and links to examples.
>
> Otherwise, see below.
>
> >
> > [1] https://crbug.com/1305267
> > [2] https://lore.kernel.org/lkml/20221215001205.51969-1-jeffxu@google.com/
> >
> > Signed-off-by: Thiébaud Weksteen <tweek@google.com>
> > ---
> > mm/memfd.c | 16 ++++++++++++++--
> > security/selinux/hooks.c | 15 +++++++++++----
> > security/selinux/include/classmap.h | 2 ++
> > 3 files changed, 27 insertions(+), 6 deletions(-)
> >
> > diff --git a/mm/memfd.c b/mm/memfd.c
> > index bbe679895ef6..13bff0e91816 100644
> > --- a/mm/memfd.c
> > +++ b/mm/memfd.c
> > @@ -433,6 +433,9 @@ static struct file *alloc_file(const char *name, unsigned int flags)
> > {
> > unsigned int *file_seals;
> > struct file *file;
> > + struct inode *inode;
> > + int err = 0;
> > + const char *anon_name = "[memfd]";
> >
> > if (flags & MFD_HUGETLB) {
> > file = hugetlb_file_setup(name, 0, VM_NORESERVE,
> > @@ -444,12 +447,21 @@ 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,
> > + LSM_ANON_INODE_MEMFD,
> > + &QSTR(anon_name), NULL);
>
> Since the anon_name already indicates that this is a memfd, so can't
> you already distinguish these via name-based type_transition rules?
> Why do we need the enum argument?
On second thought, I see that you are distinguishing not just the
security context/type but also the security class, but the question
remains: can't you compare the name to make this determination?
>
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 8d36d5ebb6e5..49742930e706 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -2367,8 +2367,8 @@ static int selinux_bprm_creds_for_exec(struct linux_binprm *bprm)
> > ad.u.file = bprm->file;
> >
> > 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);
>
> Here and below I am a little concerned that we could end up reaching
> this code on an inode with an isec->sclass that does not define the
> execute_no_trans and entrypoint permissions. We should do something to
> make that never happens, or check for it and always deny in that case.
>
> > if (rc)
> > return rc;
> > } else {
> > @@ -2378,8 +2378,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;
> >
> > @@ -2997,6 +2997,13 @@ static int selinux_inode_init_security_anon(struct inode *inode,
> >
> > isec->sclass = context_isec->sclass;
> > isec->sid = context_isec->sid;
> > + } else if (type == LSM_ANON_INODE_MEMFD) {
> > + isec->sclass = SECCLASS_MEMFD_FILE;
> > + rc = security_transition_sid(
> > + sid, sid,
> > + isec->sclass, name, &isec->sid);
>
> Again, name-based type_transitions ought to be able to distinguish
> memfd based on the name argument IIUC.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH 2/2] memfd: call security_inode_init_security_anon
2025-08-08 12:41 ` Stephen Smalley
@ 2025-08-11 5:34 ` Thiébaud Weksteen
2025-08-11 13:08 ` Stephen Smalley
0 siblings, 1 reply; 6+ messages in thread
From: Thiébaud Weksteen @ 2025-08-11 5:34 UTC (permalink / raw)
To: Stephen Smalley
Cc: Paul Moore, James Morris, Hugh Dickins, Jeff Vander Stoep,
Nick Kralevich, Jeff Xu, linux-kernel, linux-security-module,
selinux
On Fri, Aug 8, 2025 at 10:41 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Fri, Aug 8, 2025 at 7:57 AM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
Thanks for the review Stephen.
> >
> > On Thu, Aug 7, 2025 at 3:57 AM Thiébaud Weksteen <tweek@google.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.
> >
> > To provide backward compatibility, I would anticipate that you will
> > need to define a new SELinux policy capability and make this change
> > conditional on it being enabled, see:
> > https://github.com/SELinuxProject/selinux-kernel/wiki/Getting-Started#adding-a-new-selinux-policy-capability
> > for instructions and links to examples.
> >
I agree. Thanks, I'll add this to the next patchset.
> > Otherwise, see below.
> >
> > >
> > > [1] https://crbug.com/1305267
> > > [2] https://lore.kernel.org/lkml/20221215001205.51969-1-jeffxu@google.com/
> > >
> > > Signed-off-by: Thiébaud Weksteen <tweek@google.com>
> > > ---
> > > mm/memfd.c | 16 ++++++++++++++--
> > > security/selinux/hooks.c | 15 +++++++++++----
> > > security/selinux/include/classmap.h | 2 ++
> > > 3 files changed, 27 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/mm/memfd.c b/mm/memfd.c
> > > index bbe679895ef6..13bff0e91816 100644
> > > --- a/mm/memfd.c
> > > +++ b/mm/memfd.c
> > > @@ -433,6 +433,9 @@ static struct file *alloc_file(const char *name, unsigned int flags)
> > > {
> > > unsigned int *file_seals;
> > > struct file *file;
> > > + struct inode *inode;
> > > + int err = 0;
> > > + const char *anon_name = "[memfd]";
> > >
> > > if (flags & MFD_HUGETLB) {
> > > file = hugetlb_file_setup(name, 0, VM_NORESERVE,
> > > @@ -444,12 +447,21 @@ 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,
> > > + LSM_ANON_INODE_MEMFD,
> > > + &QSTR(anon_name), NULL);
> >
> > Since the anon_name already indicates that this is a memfd, so can't
> > you already distinguish these via name-based type_transition rules?
> > Why do we need the enum argument?
>
> On second thought, I see that you are distinguishing not just the
> security context/type but also the security class, but the question
> remains: can't you compare the name to make this determination?
>
I definitely can. My initial thought was that a string comparison
would not be adequate here (performance-wise), but I guess calls to
this method are infrequent enough that a strncmp would work here?
On the option of just relying on name-based type transitions, this has
an impact on the size of the policy: for Android, the transition from
ashmem to memfd means that virtually all domains will rely on such a
file. It means that we would need to define a type transition and a
new type for all our domains. (This is an argument on top of the need
for execute_no_trans, as I described in the commit message).
> >
> > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > index 8d36d5ebb6e5..49742930e706 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -2367,8 +2367,8 @@ static int selinux_bprm_creds_for_exec(struct linux_binprm *bprm)
> > > ad.u.file = bprm->file;
> > >
> > > 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);
> >
> > Here and below I am a little concerned that we could end up reaching
> > this code on an inode with an isec->sclass that does not define the
> > execute_no_trans and entrypoint permissions. We should do something to
> > make that never happens, or check for it and always deny in that case.
I agree. I can add a condition to make sure only these 2 classes (file
or memfd_file) are used here.
> >
> > > if (rc)
> > > return rc;
> > > } else {
> > > @@ -2378,8 +2378,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;
> > >
> > > @@ -2997,6 +2997,13 @@ static int selinux_inode_init_security_anon(struct inode *inode,
> > >
> > > isec->sclass = context_isec->sclass;
> > > isec->sid = context_isec->sid;
> > > + } else if (type == LSM_ANON_INODE_MEMFD) {
> > > + isec->sclass = SECCLASS_MEMFD_FILE;
> > > + rc = security_transition_sid(
> > > + sid, sid,
> > > + isec->sclass, name, &isec->sid);
> >
> > Again, name-based type_transitions ought to be able to distinguish
> > memfd based on the name argument IIUC.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH 2/2] memfd: call security_inode_init_security_anon
2025-08-11 5:34 ` Thiébaud Weksteen
@ 2025-08-11 13:08 ` Stephen Smalley
2025-08-14 2:24 ` Thiébaud Weksteen
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Smalley @ 2025-08-11 13:08 UTC (permalink / raw)
To: Thiébaud Weksteen
Cc: Paul Moore, James Morris, Hugh Dickins, Jeff Vander Stoep,
Nick Kralevich, Jeff Xu, linux-kernel, linux-security-module,
selinux
On Mon, Aug 11, 2025 at 1:34 AM Thiébaud Weksteen <tweek@google.com> wrote:
>
> On Fri, Aug 8, 2025 at 10:41 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> >
> > On Fri, Aug 8, 2025 at 7:57 AM Stephen Smalley
> > <stephen.smalley.work@gmail.com> wrote:
>
> Thanks for the review Stephen.
>
> > >
> > > On Thu, Aug 7, 2025 at 3:57 AM Thiébaud Weksteen <tweek@google.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.
> > >
> > > To provide backward compatibility, I would anticipate that you will
> > > need to define a new SELinux policy capability and make this change
> > > conditional on it being enabled, see:
> > > https://github.com/SELinuxProject/selinux-kernel/wiki/Getting-Started#adding-a-new-selinux-policy-capability
> > > for instructions and links to examples.
> > >
>
> I agree. Thanks, I'll add this to the next patchset.
>
> > > Otherwise, see below.
> > >
> > > >
> > > > [1] https://crbug.com/1305267
> > > > [2] https://lore.kernel.org/lkml/20221215001205.51969-1-jeffxu@google.com/
> > > >
> > > > Signed-off-by: Thiébaud Weksteen <tweek@google.com>
> > > > ---
> > > > mm/memfd.c | 16 ++++++++++++++--
> > > > security/selinux/hooks.c | 15 +++++++++++----
> > > > security/selinux/include/classmap.h | 2 ++
> > > > 3 files changed, 27 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/mm/memfd.c b/mm/memfd.c
> > > > index bbe679895ef6..13bff0e91816 100644
> > > > --- a/mm/memfd.c
> > > > +++ b/mm/memfd.c
> > > > @@ -433,6 +433,9 @@ static struct file *alloc_file(const char *name, unsigned int flags)
> > > > {
> > > > unsigned int *file_seals;
> > > > struct file *file;
> > > > + struct inode *inode;
> > > > + int err = 0;
> > > > + const char *anon_name = "[memfd]";
> > > >
> > > > if (flags & MFD_HUGETLB) {
> > > > file = hugetlb_file_setup(name, 0, VM_NORESERVE,
> > > > @@ -444,12 +447,21 @@ 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,
> > > > + LSM_ANON_INODE_MEMFD,
> > > > + &QSTR(anon_name), NULL);
> > >
> > > Since the anon_name already indicates that this is a memfd, so can't
> > > you already distinguish these via name-based type_transition rules?
> > > Why do we need the enum argument?
> >
> > On second thought, I see that you are distinguishing not just the
> > security context/type but also the security class, but the question
> > remains: can't you compare the name to make this determination?
> >
>
> I definitely can. My initial thought was that a string comparison
> would not be adequate here (performance-wise), but I guess calls to
> this method are infrequent enough that a strncmp would work here?
Open to data showing otherwise, but would think that a fixed-size
small string comparison would be in the noise here.
>
> On the option of just relying on name-based type transitions, this has
> an impact on the size of the policy: for Android, the transition from
> ashmem to memfd means that virtually all domains will rely on such a
> file. It means that we would need to define a type transition and a
> new type for all our domains. (This is an argument on top of the need
> for execute_no_trans, as I described in the commit message).
>
> > >
> > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > > index 8d36d5ebb6e5..49742930e706 100644
> > > > --- a/security/selinux/hooks.c
> > > > +++ b/security/selinux/hooks.c
> > > > @@ -2367,8 +2367,8 @@ static int selinux_bprm_creds_for_exec(struct linux_binprm *bprm)
> > > > ad.u.file = bprm->file;
> > > >
> > > > 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);
> > >
> > > Here and below I am a little concerned that we could end up reaching
> > > this code on an inode with an isec->sclass that does not define the
> > > execute_no_trans and entrypoint permissions. We should do something to
> > > make that never happens, or check for it and always deny in that case.
>
> I agree. I can add a condition to make sure only these 2 classes (file
> or memfd_file) are used here.
>
> > >
> > > > if (rc)
> > > > return rc;
> > > > } else {
> > > > @@ -2378,8 +2378,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;
> > > >
> > > > @@ -2997,6 +2997,13 @@ static int selinux_inode_init_security_anon(struct inode *inode,
> > > >
> > > > isec->sclass = context_isec->sclass;
> > > > isec->sid = context_isec->sid;
> > > > + } else if (type == LSM_ANON_INODE_MEMFD) {
> > > > + isec->sclass = SECCLASS_MEMFD_FILE;
> > > > + rc = security_transition_sid(
> > > > + sid, sid,
> > > > + isec->sclass, name, &isec->sid);
> > >
> > > Again, name-based type_transitions ought to be able to distinguish
> > > memfd based on the name argument IIUC.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH 2/2] memfd: call security_inode_init_security_anon
2025-08-11 13:08 ` Stephen Smalley
@ 2025-08-14 2:24 ` Thiébaud Weksteen
0 siblings, 0 replies; 6+ messages in thread
From: Thiébaud Weksteen @ 2025-08-14 2:24 UTC (permalink / raw)
To: Stephen Smalley
Cc: Paul Moore, James Morris, Hugh Dickins, Jeff Vander Stoep,
Nick Kralevich, Jeff Xu, linux-kernel, linux-security-module,
selinux
On Mon, Aug 11, 2025 at 11:09 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Mon, Aug 11, 2025 at 1:34 AM Thiébaud Weksteen <tweek@google.com> wrote:
> >
> > On Fri, Aug 8, 2025 at 10:41 PM Stephen Smalley
> > <stephen.smalley.work@gmail.com> wrote:
> > >
> > > On Fri, Aug 8, 2025 at 7:57 AM Stephen Smalley
> > > <stephen.smalley.work@gmail.com> wrote:
> >
> > Thanks for the review Stephen.
> >
> > > >
> > > > On Thu, Aug 7, 2025 at 3:57 AM Thiébaud Weksteen <tweek@google.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.
> > > >
> > > > To provide backward compatibility, I would anticipate that you will
> > > > need to define a new SELinux policy capability and make this change
> > > > conditional on it being enabled, see:
> > > > https://github.com/SELinuxProject/selinux-kernel/wiki/Getting-Started#adding-a-new-selinux-policy-capability
> > > > for instructions and links to examples.
> > > >
> >
> > I agree. Thanks, I'll add this to the next patchset.
> >
> > > > Otherwise, see below.
> > > >
> > > > >
> > > > > [1] https://crbug.com/1305267
> > > > > [2] https://lore.kernel.org/lkml/20221215001205.51969-1-jeffxu@google.com/
> > > > >
> > > > > Signed-off-by: Thiébaud Weksteen <tweek@google.com>
> > > > > ---
> > > > > mm/memfd.c | 16 ++++++++++++++--
> > > > > security/selinux/hooks.c | 15 +++++++++++----
> > > > > security/selinux/include/classmap.h | 2 ++
> > > > > 3 files changed, 27 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/mm/memfd.c b/mm/memfd.c
> > > > > index bbe679895ef6..13bff0e91816 100644
> > > > > --- a/mm/memfd.c
> > > > > +++ b/mm/memfd.c
> > > > > @@ -433,6 +433,9 @@ static struct file *alloc_file(const char *name, unsigned int flags)
> > > > > {
> > > > > unsigned int *file_seals;
> > > > > struct file *file;
> > > > > + struct inode *inode;
> > > > > + int err = 0;
> > > > > + const char *anon_name = "[memfd]";
> > > > >
> > > > > if (flags & MFD_HUGETLB) {
> > > > > file = hugetlb_file_setup(name, 0, VM_NORESERVE,
> > > > > @@ -444,12 +447,21 @@ 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,
> > > > > + LSM_ANON_INODE_MEMFD,
> > > > > + &QSTR(anon_name), NULL);
> > > >
> > > > Since the anon_name already indicates that this is a memfd, so can't
> > > > you already distinguish these via name-based type_transition rules?
> > > > Why do we need the enum argument?
> > >
> > > On second thought, I see that you are distinguishing not just the
> > > security context/type but also the security class, but the question
> > > remains: can't you compare the name to make this determination?
> > >
> >
> > I definitely can. My initial thought was that a string comparison
> > would not be adequate here (performance-wise), but I guess calls to
> > this method are infrequent enough that a strncmp would work here?
>
> Open to data showing otherwise, but would think that a fixed-size
> small string comparison would be in the noise here.
>
I agree.
> >
> > On the option of just relying on name-based type transitions, this has
> > an impact on the size of the policy: for Android, the transition from
> > ashmem to memfd means that virtually all domains will rely on such a
> > file. It means that we would need to define a type transition and a
> > new type for all our domains. (This is an argument on top of the need
> > for execute_no_trans, as I described in the commit message).
> >
> > > >
> > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > > > index 8d36d5ebb6e5..49742930e706 100644
> > > > > --- a/security/selinux/hooks.c
> > > > > +++ b/security/selinux/hooks.c
> > > > > @@ -2367,8 +2367,8 @@ static int selinux_bprm_creds_for_exec(struct linux_binprm *bprm)
> > > > > ad.u.file = bprm->file;
> > > > >
> > > > > 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);
> > > >
> > > > Here and below I am a little concerned that we could end up reaching
> > > > this code on an inode with an isec->sclass that does not define the
> > > > execute_no_trans and entrypoint permissions. We should do something to
> > > > make that never happens, or check for it and always deny in that case.
> >
> > I agree. I can add a condition to make sure only these 2 classes (file
> > or memfd_file) are used here.
> >
> > > >
> > > > > if (rc)
> > > > > return rc;
> > > > > } else {
> > > > > @@ -2378,8 +2378,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;
> > > > >
> > > > > @@ -2997,6 +2997,13 @@ static int selinux_inode_init_security_anon(struct inode *inode,
> > > > >
> > > > > isec->sclass = context_isec->sclass;
> > > > > isec->sid = context_isec->sid;
> > > > > + } else if (type == LSM_ANON_INODE_MEMFD) {
> > > > > + isec->sclass = SECCLASS_MEMFD_FILE;
> > > > > + rc = security_transition_sid(
> > > > > + sid, sid,
> > > > > + isec->sclass, name, &isec->sid);
> > > >
> > > > Again, name-based type_transitions ought to be able to distinguish
> > > > memfd based on the name argument IIUC.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-08-14 2:25 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-07 7:57 [RFC PATCH 2/2] memfd: call security_inode_init_security_anon Thiébaud Weksteen
2025-08-08 11:57 ` Stephen Smalley
2025-08-08 12:41 ` Stephen Smalley
2025-08-11 5:34 ` Thiébaud Weksteen
2025-08-11 13:08 ` Stephen Smalley
2025-08-14 2:24 ` Thiébaud Weksteen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).