On Mon, Aug 07, 2023 at 11:00:24PM +0100, Mark Brown wrote:
> diff --git a/arch/arm64/mm/gcs.c b/arch/arm64/mm/gcs.c
> index b0a67efc522b..1e059c37088d 100644
> --- a/arch/arm64/mm/gcs.c
> +++ b/arch/arm64/mm/gcs.c
> @@ -8,6 +8,62 @@
> #include <asm/cpufeature.h>
> #include <asm/page.h>
>
> +static unsigned long alloc_gcs(unsigned long addr, unsigned long size,
> + unsigned long token_offset, bool set_res_tok)
> +{
> + int flags = MAP_ANONYMOUS | MAP_PRIVATE;
> + struct mm_struct *mm = current->mm;
> + unsigned long mapped_addr, unused;
> +
> + if (addr)
> + flags |= MAP_FIXED_NOREPLACE;
> +
> + mmap_write_lock(mm);
> + mapped_addr = do_mmap(NULL, addr, size, PROT_READ, flags,
> + VM_SHADOW_STACK | VM_WRITE, 0, &unused, NULL);
Why not PROT_WRITE as well? I guess I need to check the x86 patches
since the do_mmap() called here has a different prototype than what's in
mainline.
This gets confusing since currently the VM_* flags are derived from the
PROT_* flags passed to mmap(). But you skip the PROT_WRITE in favour of
adding VM_WRITE directly.
I haven't followed the x86 discussion but did we run out of PROT_* bits
for a PROT_SHADOW_STACK?
> + mmap_write_unlock(mm);
> +
> + return mapped_addr;
> +}
> +
> +static unsigned long gcs_size(unsigned long size)
> +{
> + if (size)
> + return PAGE_ALIGN(size);
> +
> + /* Allocate RLIMIT_STACK with limits of PAGE_SIZE..4G */
> + size = PAGE_ALIGN(min_t(unsigned long long,
> + rlimit(RLIMIT_STACK), SZ_4G));
> + return max(PAGE_SIZE, size);
> +}
I saw Szabolcs commenting on the default size as well. Maybe we should
go for RLIMIT_STACK/2 but let's see how the other sub-thread is going.
> +
> +unsigned long gcs_alloc_thread_stack(struct task_struct *tsk,
> + unsigned long clone_flags, size_t size)
> +{
> + unsigned long addr;
> +
> + if (!system_supports_gcs())
> + return 0;
> +
> + if (!task_gcs_el0_enabled(tsk))
> + return 0;
> +
> + if ((clone_flags & (CLONE_VFORK | CLONE_VM)) != CLONE_VM)
> + return 0;
Is it safe for CLONE_VFORK not to get a new shadow stack? A syscall for
exec could push something to the stack. I guess the GCS pointer in the
parent stays the same, so it wouldn't matter.
That said, I think this check should be somewhere higher up in the
caller of gcs_alloc_thread_stack(). The copy_thread_gcs() function
already does most of the above checks. Is the GCS allocation called from
elsewhere as well?
--
Catalin
