Message-ID: <CAEjxPJ5otOTmP44QcYXpqcAyg1JU+CTPmS6UgPugnQbFJ_9=dw@mail.gmail.com>
Date: Mon, 11 Aug 2025 09:08:53 -0400
From: Stephen Smalley <stephen.smalley.work@...il.com>
To: Thiébaud Weksteen <tweek@...gle.com>
Cc: Paul Moore <paul@...l-moore.com>, James Morris <jmorris@...ei.org>, 
	Hugh Dickins <hughd@...gle.com>, Jeff Vander Stoep <jeffv@...gle.com>, Nick Kralevich <nnk@...gle.com>, 
	Jeff Xu <jeffxu@...gle.com>, linux-kernel@...r.kernel.org, 
	linux-security-module@...r.kernel.org, selinux@...r.kernel.org
Subject: Re: [RFC PATCH 2/2] memfd: call security_inode_init_security_anon

On Mon, Aug 11, 2025 at 1:34 AM Thiébaud Weksteen <tweek@...gle.com> wrote:
>
> On Fri, Aug 8, 2025 at 10:41 PM Stephen Smalley
> <stephen.smalley.work@...il.com> wrote:
> >
> > On Fri, Aug 8, 2025 at 7:57 AM Stephen Smalley
> > <stephen.smalley.work@...il.com> wrote:
>
> Thanks for the review Stephen.
>
> > >
> > > On Thu, Aug 7, 2025 at 3:57 AM Thiébaud Weksteen <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.
> > >
> > > 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@...gle.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.

Powered by blists - more mailing lists