Re: [PATCH v4 24/26] mm: add arch hook to validate mmap() prot flags

[Kees Cook <keescook@chromium.org>]

On Mon, Jun 13, 2022 at 04:45:48PM +0200, Ard Biesheuvel wrote:
> 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.
> 
> 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 58b3abd457a3..53ac72310ce0 100644
> --- a/include/linux/mman.h
> +++ b/include/linux/mman.h
> @@ -120,6 +120,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 61e6135c54ef..4a585879937d 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1437,6 +1437,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;

I assume yes, but just to be clear, the existing userspace programs that
can switch modes are checking for EACCES? (Or are just just checking for
failure generally?) It looks like, for example, SELinux returns EACCES
too, so this looks correct. (Looking at the mmap man page, it seems the
ship has sailed for this to be EPERM, which looks more correct to me,
but so be it.)

> +
>  	/* force arch specific MAP_FIXED handling in get_unmapped_area */
>  	if (flags & MAP_FIXED_NOREPLACE)
>  		flags |= MAP_FIXED;
> -- 
> 2.30.2
> 

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook