+ Kees because this is related to W^X memfd and security. On Fri, Jan 3, 2025 at 7:04 AM Jann Horn <jannh@xxxxxxxxxx> wrote: > > On Fri, Jan 3, 2025 at 12:32 AM Isaac J. Manjarres > <isaacmanjarres@xxxxxxxxxx> 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. > > 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? > > > 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@xxxxxxxxxx> > > --- > > 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? > > > + /* > > + * 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. > > return err; > > } > > -- > > 2.47.1.613.gc27f4b7a9f-goog > > > > > > >