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

[Ard Biesheuvel]

On Tue, 12 Mar 2024 at 20:53, Catalin Marinas <catalin.marinas at arm.com> wrote:
>
> On Wed, Feb 14, 2024 at 01:29:28PM +0100, Ard Biesheuvel wrote:
> > From: Ard Biesheuvel <ardb at 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 at chromium.org>
> > Signed-off-by: Ard Biesheuvel <ardb at 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?

I did not consider arch_validate_flags() at all because I was looking
at ways to validate PROT_ flags not VM_ flags. But if
PROT_WRITE+PROT_EXEC implies VM_WRITE+VM_EXEC, I don't see why we
wouldn't be able to change this. That way, we can drop this patch
entirely afaict.

> 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).
>

Yes. Looking at it now, it would be better to add a single arch hook
to map_deny_write_exec(), and use that instead.

> 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.
>

Indeed.

MDWE looks like a good match in principle, but combining the two seems tricky:
- EL1 is going to flip between WXN protection on and off depending on
which setting it is using for EL0;
- context switching SCTLR_EL1.WXN may become costly in terms of TLB
maintenance, unless we cheat and ignore the kernel mappings (which
should work as expected regardless of the value of SCTLR_EL1.WXN);

If we can find a reasonable strategy to manage the TLB maintenance
that does not leave EL1 behind, I'm happy to explore this further. But
perhaps WXN should simply be treated as MDWE always-on for all
processes.

> Sorry for not catching this early.
>

No worries - it's more likely to be useful if we get this right.