| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAC_TJvdVhGW+4y0JHqRVTdqAWpQRDOgWW8b1TAK3V9zdnmw0ow@mail.gmail.com>
Date: Fri, 6 Dec 2024 09:49:35 -0800
From: Kalesh Singh <kaleshsingh@...gle.com>
To: "Isaac J. Manjarres" <isaacmanjarres@...gle.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, Jeff Layton <jlayton@...nel.org>,
Chuck Lever <chuck.lever@...cle.com>, Alexander Aring <alex.aring@...il.com>,
"Liam R. Howlett" <Liam.Howlett@...cle.com>, Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
Vlastimil Babka <vbabka@...e.cz>, Jann Horn <jannh@...gle.com>, Shuah Khan <shuah@...nel.org>,
kernel-team@...roid.com, linux-mm@...ck.org, linux-kernel@...r.kernel.org,
linux-fsdevel@...r.kernel.org, linux-kselftest@...r.kernel.org,
Suren Baghdasaryan <surenb@...gle.com>, John Stultz <jstultz@...gle.com>
Subject: Re: [RFC PATCH v1 1/2] mm/memfd: Add support for F_SEAL_FUTURE_EXEC
to memfd
On Thu, Dec 5, 2024 at 5:09 PM 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.
> We are 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. Although, 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.
>
> 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
>
> Cc: Suren Baghdasaryan <surenb@...gle.com>
> Cc: Kalesh Singh <kaleshsingh@...gle.com>
> Cc: John Stultz <jstultz@...gle.com>
> Signed-off-by: Isaac J. Manjarres <isaacmanjarres@...gle.com>
> ---
> include/linux/mm.h | 5 +++++
> include/uapi/linux/fcntl.h | 1 +
> mm/memfd.c | 1 +
> mm/mmap.c | 11 +++++++++++
> 4 files changed, 18 insertions(+)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 4eb8e62d5c67..40c03a491e45 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -4096,6 +4096,11 @@ static inline bool is_write_sealed(int seals)
> return seals & (F_SEAL_WRITE | F_SEAL_FUTURE_WRITE);
> }
>
> +static inline bool is_exec_sealed(int seals)
> +{
> + return seals & F_SEAL_FUTURE_EXEC;
> +}
> +
> /**
> * is_readonly_sealed - Checks whether write-sealed but mapped read-only,
> * in which case writes should be disallowing moving
> 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 35a370d75c9a..77b49995a044 100644
> --- a/mm/memfd.c
> +++ b/mm/memfd.c
> @@ -184,6 +184,7 @@ 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 | \
> diff --git a/mm/mmap.c b/mm/mmap.c
> index b1b2a24ef82e..c7b96b057fda 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -375,6 +375,17 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
> if (!file_mmap_ok(file, inode, pgoff, len))
> return -EOVERFLOW;
>
> + if (is_exec_sealed(seals)) {
> + /* No new executable mappings if the file is exec sealed. */
> + if (prot & PROT_EXEC)
> + return -EACCES;
I think this should be -EPERM to be consistent with seal_check_write()
and mmap(2) man page:
" EPERM The operation was prevented by a file seal; see fcntl(2)."
Thanks,
Kalesh
> + /*
> + * Prevent an initially non-executable mapping from
> + * later becoming executable via mprotect().
> + */
> + vm_flags &= ~VM_MAYEXEC;
> + }
> +
> flags_mask = LEGACY_MAP_MASK;
> if (file->f_op->fop_flags & FOP_MMAP_SYNC)
> flags_mask |= MAP_SYNC;
> --
> 2.47.0.338.g60cca15819-goog
>
Powered by blists - more mailing lists