Re: [PATCH v10 13/40] arm64/mm: Map pages for guarded control stack

[Catalin Marinas <catalin.marinas@arm.com>]

On Thu, Aug 01, 2024 at 01:06:40PM +0100, Mark Brown wrote:
> Map pages flagged as being part of a GCS as such rather than using the
> full set of generic VM flags.
> 
> This is done using a conditional rather than extending the size of
> protection_map since that would make for a very sparse array.
> 
> Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/include/asm/mman.h |  9 +++++++++
>  arch/arm64/mm/mmap.c          | 10 +++++++++-
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/mman.h b/arch/arm64/include/asm/mman.h
> index c21849ffdd88..6d3fe6433a62 100644
> --- a/arch/arm64/include/asm/mman.h
> +++ b/arch/arm64/include/asm/mman.h
> @@ -61,6 +61,15 @@ static inline bool arch_validate_flags(unsigned long vm_flags)
>  			return false;
>  	}
>  
> +	if (system_supports_gcs() && (vm_flags & VM_SHADOW_STACK)) {
> +		/*
> +		 * An executable GCS isn't a good idea, and the mm
> +		 * core can't cope with a shared GCS.
> +		 */
> +		if (vm_flags & (VM_EXEC | VM_ARM64_BTI | VM_SHARED))
> +			return false;
> +	}

I wonder whether we should clear VM_MAYEXEC early on during the vma
creation. This way the mprotect() case will be handled in the core code.
At a quick look, do_mmap() seems to always set VM_MAYEXEC but discard it
for non-executable file mmap. Last time I looked (when doing MTE) there
wasn't a way for the arch code to clear specific VM_* flags, only to
validate them. But I think we should just clear VM_MAYEXEC and also
return an error for VM_EXEC in the core do_mmap() if VM_SHADOW_STACK. It
would cover the other architectures doing shadow stacks.

Regarding VM_SHARED, how do we even end up with this via the
map_shadow_stack() syscall? I can't see how one can pass MAP_SHARED to
do_mmap() on this path. I'm fine with a VM_WARN_ON() if you want the
check (and there's no way a user can trigger it).

Is there any arch restriction with setting BTI and GCS? It doesn't make
sense but curious if it matters. We block the exec permission anyway
(unless the BTI pages moved to PIE as well, I don't remember).

> +
>  	return true;
>  
>  }
> diff --git a/arch/arm64/mm/mmap.c b/arch/arm64/mm/mmap.c
> index 642bdf908b22..3ed63fc8cd0a 100644
> --- a/arch/arm64/mm/mmap.c
> +++ b/arch/arm64/mm/mmap.c
> @@ -83,9 +83,17 @@ arch_initcall(adjust_protection_map);
>  
>  pgprot_t vm_get_page_prot(unsigned long vm_flags)
>  {
> -	pteval_t prot = pgprot_val(protection_map[vm_flags &
> +	pteval_t prot;
> +
> +	/* Short circuit GCS to avoid bloating the table. */
> +	if (system_supports_gcs() && (vm_flags & VM_SHADOW_STACK)) {
> +		prot = _PAGE_GCS_RO;
> +	} else {
> +		prot = pgprot_val(protection_map[vm_flags &
>  				   (VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)]);
> +	}

This looks fine to me. Such page will become proper GCS on first access.

-- 
Catalin