Re: [PATCH v8 42/43] mm: add arch hook to validate mmap() prot flags

From: Catalin Marinas <catalin.marinas@arm.com>
To: Ard Biesheuvel <ardb+git@google.com>
Cc: linux-arm-kernel@lists.infradead.org,
	Ard Biesheuvel <ardb@kernel.org>, Will Deacon <will@kernel.org>,
	Marc Zyngier <maz@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Ryan Roberts <ryan.roberts@arm.com>,
	Anshuman Khandual <anshuman.khandual@arm.com>,
	Kees Cook <keescook@chromium.org>
Subject: Re: [PATCH v8 42/43] mm: add arch hook to validate mmap() prot flags
Date: Tue, 12 Mar 2024 19:53:01 +0000	[thread overview]
Message-ID: <ZfCynRRCg5Bvcekl@arm.com> (raw)
In-Reply-To: <20240214122845.2033971-87-ardb+git@google.com>

On Wed, Feb 14, 2024 at 01:29:28PM +0100, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
> 
> Add a hook to permit architectures to perform validation on the prot
> flags passed to mmap(), like arch_validate_prot() does for mprotect().
> This will be used by arm64 to reject PROT_WRITE+PROT_EXEC mappings on
> configurations that run with WXN enabled.
> 
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  include/linux/mman.h | 15 +++++++++++++++
>  mm/mmap.c            |  3 +++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/include/linux/mman.h b/include/linux/mman.h
> index dc7048824be8..ec5e7f606e43 100644
> --- a/include/linux/mman.h
> +++ b/include/linux/mman.h
> @@ -124,6 +124,21 @@ static inline bool arch_validate_flags(unsigned long flags)
>  #define arch_validate_flags arch_validate_flags
>  #endif
>  
> +#ifndef arch_validate_mmap_prot
> +/*
> + * This is called from mmap(), which ignores unknown prot bits so the default
> + * is to accept anything.
> + *
> + * Returns true if the prot flags are valid
> + */
> +static inline bool arch_validate_mmap_prot(unsigned long prot,
> +					   unsigned long addr)
> +{
> +	return true;
> +}
> +#define arch_validate_mmap_prot arch_validate_mmap_prot
> +#endif
> +
>  /*
>   * Optimisation macro.  It is equivalent to:
>   *      (x & bit1) ? bit2 : 0
> diff --git a/mm/mmap.c b/mm/mmap.c
> index d89770eaab6b..977a8c3fd9f5 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1229,6 +1229,9 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
>  		if (!(file && path_noexec(&file->f_path)))
>  			prot |= PROT_EXEC;
>  
> +	if (!arch_validate_mmap_prot(prot, addr))
> +		return -EACCES;
> +
>  	/* force arch specific MAP_FIXED handling in get_unmapped_area */
>  	if (flags & MAP_FIXED_NOREPLACE)
>  		flags |= MAP_FIXED;

While writing the pull request for Linus (and looking to justify this
change), I realised that we already have arch_validate_flags() that can
do a similar check but on VM_* flags instead of PROT_* (we use it for
VM_MTE checks). What was the reason for adding a new hook? The only
difference is that here it returns -EACCESS while on
arch_validate_flags() failure it would return -EINVAL. It probably makes
more to return -EACCESS as it matches map_deny_write_exec() but with
your patches we are inconsistent between mmap() and mprotect() errors
(the latter is still -EINVAL).

It also got me thinking on whether we could use this as a hardened
version of the MDWE feature instead a CPU feature (though we'd end up
context-switching this SCTLR_EL1 bit). I think your patches have been
around before the MDWE feature was added to the kernel.

Sorry for not catching this early.

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel