Message-ID: <Z3yCzcpTHnW671WL@google.com>
Date: Mon, 6 Jan 2025 17:26:37 -0800
From: Isaac Manjarres <isaacmanjarres@...gle.com>
To: Jeff Xu <jeffxu@...omium.org>
Cc: Jann Horn <jannh@...gle.com>, Kees Cook <keescook@...omium.org>,
	lorenzo.stoakes@...cle.com, Jeff Layton <jlayton@...nel.org>,
	Chuck Lever <chuck.lever@...cle.com>,
	Alexander Aring <alex.aring@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Shuah Khan <shuah@...nel.org>, surenb@...gle.com,
	kaleshsingh@...gle.com, jstultz@...gle.com, aliceryhl@...gle.com,
	jeffxu@...gle.com, kees@...nel.org, kernel-team@...roid.com,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-mm@...ck.org, linux-kselftest@...r.kernel.org
Subject: Re: [RFC PATCH RESEND v2 1/2] mm/memfd: Add support for
 F_SEAL_FUTURE_EXEC to memfd

On Mon, Jan 06, 2025 at 09:35:09AM -0800, Jeff Xu wrote:
> + Kees because this is related to W^X memfd and security.
> 
> On Fri, Jan 3, 2025 at 7:04 AM Jann Horn <jannh@...gle.com> wrote:
> >
> > On Fri, Jan 3, 2025 at 12:32 AM Isaac J. Manjarres
> > <isaacmanjarres@...gle.com> wrote:
> > > Android currently uses the ashmem driver [1] for creating shared memory
> > > regions between processes. Ashmem buffers can initially be mapped with
> > > PROT_READ, PROT_WRITE, and PROT_EXEC. Processes can then use the
> > > ASHMEM_SET_PROT_MASK ioctl command to restrict--never add--the
> > > permissions that the buffer can be mapped with.
> > >
> > > Processes can remove the ability to map ashmem buffers as executable to
> > > ensure that those buffers cannot be exploited to run unintended code.
> >
> > Is there really code out there that first maps an ashmem buffer with
> > PROT_EXEC, then uses the ioctl to remove execute permission for future
> > mappings? I don't see why anyone would do that.
> >
> > > For instance, suppose process A allocates a memfd that is meant to be
> > > read and written by itself and another process, call it B.
> > >
> > > Process A shares the buffer with process B, but process B injects code
> > > into the buffer, and compromises process A, such that it makes A map
> > > the buffer with PROT_EXEC. This provides an opportunity for process A
> > > to run the code that process B injected into the buffer.
> > >
> > > If process A had the ability to seal the buffer against future
> > > executable mappings before sharing the buffer with process B, this
> > > attack would not be possible.
> >
> > I think if you want to enforce such restrictions in a scenario where
> > the attacker can already make the target process perform
> > semi-arbitrary syscalls, it would probably be more reliable to enforce
> > rules on executable mappings with something like SELinux policy and/or
> > F_SEAL_EXEC.
> >
> I would like to second on the suggestion of  making this as part of F_SEAL_EXEC.

Thanks for taking a look at this patch Jeff! Can you please elaborate
some more on how F_SEAL_EXEC should be extended to restricting executable
mappings?

I understand that if a memfd file is non-executable (either because it
was made non-executable via fchmod() or by being created with
MFD_NOEXEC_SEAL) one could argue that applying F_SEAL_EXEC to that file
would also mean preventing any executable mappings. However, it is not
clear to me if we should tie a file's executable permissions to whether
or not if it can be mapped as executable. For example, shared object
files don't have to have executable permissions, but processes should
be able to map them as executable.

The case where we apply F_SEAL_EXEC on an executable memfd also seems
awkward to me, since memfds can be mapped as executable by default
so what would happen in that scenario?

I also shared the same concerns in my response to Jann in [1].

> > > diff --git a/mm/memfd.c b/mm/memfd.c
> > > index 5f5a23c9051d..cfd62454df5e 100644
> > > --- a/mm/memfd.c
> > > +++ b/mm/memfd.c
> > > @@ -184,6 +184,7 @@ static unsigned int *memfd_file_seals_ptr(struct file *file)
> > >  }
> > >
> > >  #define F_ALL_SEALS (F_SEAL_SEAL | \
> > > +                    F_SEAL_FUTURE_EXEC |\
> > >                      F_SEAL_EXEC | \
> > >                      F_SEAL_SHRINK | \
> > >                      F_SEAL_GROW | \
> > > @@ -357,14 +358,50 @@ static int check_write_seal(unsigned long *vm_flags_ptr)
> > >         return 0;
> > >  }
> > >
> > > +static inline bool is_exec_sealed(unsigned int seals)
> > > +{
> > > +       return seals & F_SEAL_FUTURE_EXEC;
> > > +}
> > > +
> > > +static int check_exec_seal(unsigned long *vm_flags_ptr)
> > > +{
> > > +       unsigned long vm_flags = *vm_flags_ptr;
> > > +       unsigned long mask = vm_flags & (VM_SHARED | VM_EXEC);
> > > +
> > > +       /* Executability is not a concern for private mappings. */
> > > +       if (!(mask & VM_SHARED))
> > > +               return 0;
> >
> > Why is it not a concern for private mappings?
> >
> > > +       /*
> > > +        * New PROT_EXEC and MAP_SHARED mmaps are not allowed when exec seal
> > > +        * is active.
> > > +        */
> > > +       if (mask & VM_EXEC)
> > > +               return -EPERM;
> > > +
> > > +       /*
> > > +        * Prevent mprotect() from making an exec-sealed mapping executable in
> > > +        * the future.
> > > +        */
> > > +       *vm_flags_ptr &= ~VM_MAYEXEC;
> > > +
> > > +       return 0;
> > > +}
> > > +
> > >  int memfd_check_seals_mmap(struct file *file, unsigned long *vm_flags_ptr)
> > >  {
> > >         int err = 0;
> > >         unsigned int *seals_ptr = memfd_file_seals_ptr(file);
> > >         unsigned int seals = seals_ptr ? *seals_ptr : 0;
> > >
> > > -       if (is_write_sealed(seals))
> > > +       if (is_write_sealed(seals)) {
> > >                 err = check_write_seal(vm_flags_ptr);
> > > +               if (err)
> > > +                       return err;
> > > +       }
> > > +
> > > +       if (is_exec_sealed(seals))
> > > +               err = check_exec_seal(vm_flags_ptr);
> > >
> memfd_check_seals_mmap is only for mmap() path, right ?
> 
> How about the mprotect()  path ? i.e.  An attacker can first create a
> RW VMA mapping for the memfd and later mprotect the VMA to be
> executable.
> 
> Similar to the check_write_seal call , we might want to block mprotect
> for write seal as well.
>

So when memfd_check_seals_mmap() is called, if the file is exec_sealed,
check_exec_seal() will not only just check that VM_EXEC is not set,
but it will also clear VM_MAYEXEC, which will prevent the mapping from
being changed to executable via mprotect() later.

[1] https://lore.kernel.org/all/Z3x_8uFn2e0EpDqM@google.com/

Thanks,
Isaac

Powered by blists - more mailing lists