Message-ID: <Z3x_8uFn2e0EpDqM@google.com>
Date: Mon, 6 Jan 2025 17:14:26 -0800
From: Isaac Manjarres <isaacmanjarres@...gle.com>
To: Jann Horn <jannh@...gle.com>
Cc: 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 Fri, Jan 03, 2025 at 04:03:44PM +0100, Jann Horn 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.

Hi Jann,

Thanks for your feedback and for taking the time to review these
patches!

Not that I'm aware of. The reason why I made this seal have semantics
where it prevents future executable mappings is because there are
existing applications that allocate an ashmem buffer (default
permissions are RWX), map the buffer as RW, and then restrict
the permissions to just R.

When the buffer is mapped as RW, do_mmap() unconditionally sets
VM_MAYEXEC on the VMA for the mapping, which means that the mapping
could later be mapped as executable via mprotect(). Therefore, having
the semantics of the seal be that it prevents any executable mappings
would break existing code that has already been released. It would
make transitioning clients to memfd difficult, because to amend that,
the ashmem users would have to first restrict the permissions of the
buffer to be RW, then map it as RW, and then restrict the permissions
again to be just R, which also means an additional system call.

> > 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.
>

For SELinux policy, do you mean to not allow execmem permissions? What
about scenarios where a process wants to use JIT compilation, but
doesn't want memfd data buffers to be executable? My thought was to use
this new seal to have a finer granularity to control what buffers can
be mapped as executable. If not, could you please clarify?

Also, F_SEAL_EXEC just seals the memfd's current executable permissions,
and doesn't affect the mapping permissions at all. Are you saying that
F_SEAL_EXEC should be extended to cover mappings as well? If so, it is
not clear to me what the semantics of that would be.

For instance, if a memfd is non-executable and F_SEAL_EXEC is applied,
we can also prevent any executable mappings at that point. I'm not sure
if that's the right thing to do though. For instance, there are shared
object files that don't have executable permissions, but their code
sections should be mapped as executable. So, drawing from that, I'm not
sure if it makes sense to tie the file execution permissions to the
mapping permissions.

There's also the case where F_SEAL_EXEC is invoked on an executable
memfd. In that case, there doesn't seem to be anything to do from a
mapping perspective since memfds can be mapped as executable by
default?

> > Android is currently trying to replace ashmem with memfd. However, memfd
> > does not have a provision to permanently remove the ability to map a
> > buffer as executable, and leaves itself open to the type of attack
> > described earlier. However, this should be something that can be
> > achieved via a new file seal.
> >
> > There are known usecases (e.g. CursorWindow [2]) where a process
> > maps a buffer with read/write permissions before restricting the buffer
> > to being mapped as read-only for future mappings.
> 
> Here you're talking about write permission, but the patch is about
> execute permission?
> 

Sorry, I used this example about write permission to show why I implemented
the seal with support for preventing future mappings, since the writable
mappings that get created can become executable in the future, as
described later in the commit text.

> > The resulting VMA from the writable mapping has VM_MAYEXEC set, meaning
> > that mprotect() can change the mapping to be executable. Therefore,
> > implementing the seal similar to F_SEAL_WRITE would not be appropriate,
> > since it would not work with the CursorWindow usecase. This is because
> > the CursorWindow process restricts the mapping permissions to read-only
> > after the writable mapping is created. So, adding a file seal for
> > executable mappings that operates like F_SEAL_WRITE would fail.
> >
> > Therefore, add support for F_SEAL_FUTURE_EXEC, which is handled
> > similarly to F_SEAL_FUTURE_WRITE. This ensures that CursorWindow can
> > continue to create a writable mapping initially, and then restrict the
> > permissions on the buffer to be mappable as read-only by using both
> > F_SEAL_FUTURE_WRITE and F_SEAL_FUTURE_EXEC. After the seal is
> > applied, any calls to mmap() with PROT_EXEC will fail.
> >
> > [1] https://cs.android.com/android/kernel/superproject/+/common-android-mainline:common/drivers/staging/android/ashmem.c
> > [2] https://developer.android.com/reference/android/database/CursorWindow
> >
> > Signed-off-by: Isaac J. Manjarres <isaacmanjarres@...gle.com>
> > ---
> >  include/uapi/linux/fcntl.h |  1 +
> >  mm/memfd.c                 | 39 +++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 39 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> > index 6e6907e63bfc..ef066e524777 100644
> > --- a/include/uapi/linux/fcntl.h
> > +++ b/include/uapi/linux/fcntl.h
> > @@ -49,6 +49,7 @@
> >  #define F_SEAL_WRITE   0x0008  /* prevent writes */
> >  #define F_SEAL_FUTURE_WRITE    0x0010  /* prevent future writes while mapped */
> >  #define F_SEAL_EXEC    0x0020  /* prevent chmod modifying exec bits */
> > +#define F_SEAL_FUTURE_EXEC     0x0040 /* prevent future executable mappings */
> >  /* (1U << 31) is reserved for signed error codes */
> >
> >  /*
> > 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?
>
I didn't consider private mappings since it wasn't clear as to how
they could be a threat to another process. A process can copy the
contents of the buffer into another executable region of memory
and just run it from there? Or are you saying that because it
can do that, is there any value in differentiating between
shared and private mappings?

Thanks,
Isaac

Powered by blists - more mailing lists