Re: [PATCH 1/2] arm64: Support execute-only permissions with Enhanced PAN

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

On Thu, Nov 19, 2020 at 06:52:05PM +0000, Dave P Martin wrote:
> On Thu, Nov 19, 2020 at 01:39:52PM +0000, Vladimir Murzin wrote:
> > Enhanced Privileged Access Never (EPAN) allows Privileged Access Never
> > to be used with Execute-only mappings.
> > 
> > Absence of such support was a reason for 24cecc377463 ("arm64: Revert
> > support for execute-only user mappings"). Thus now it can be revisited
> > and re-enabled.
> > 
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
> > ---
> >  arch/arm64/Kconfig                    | 17 +++++++++++++++++
> >  arch/arm64/include/asm/cpucaps.h      |  5 +++--
> >  arch/arm64/include/asm/pgtable-prot.h |  5 +++--
> >  arch/arm64/include/asm/pgtable.h      | 14 +++++++++++++-
> >  arch/arm64/include/asm/sysreg.h       |  1 +
> >  arch/arm64/kernel/cpufeature.c        | 21 +++++++++++++++++++++
> >  arch/arm64/mm/fault.c                 |  3 +++
> >  7 files changed, 61 insertions(+), 5 deletions(-)
> > 
> 
> [...]
> 
> > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> > index 1ee9400..b93222e 100644
> > --- a/arch/arm64/mm/fault.c
> > +++ b/arch/arm64/mm/fault.c
> > @@ -467,6 +467,9 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
> >  	if (faulthandler_disabled() || !mm)
> >  		goto no_context;
> >  
> > +	if (cpus_have_const_cap(ARM64_HAS_EPAN))
> > +		vm_flags &= ~VM_EXEC;
> > +
> 
> IIUC, this would be telling __do_page_fault() that the access would have
> succeeded with any kind of permissions except for write access, which
> doesn't seem right.

I always have trouble remembering what the vm_flags does. So
__do_page_fault() checks vma->vm_flags & vm_flags and returns an error
if the intersection is empty. We start with all rwx permission but
modify it further down in the in do_page_fault(): if it was an exec
fault, we set vm_flags to VM_EXEC only as that's what we want to check
against vma->vm_flags; similarly, if it was a write fault, we want to
check VM_WRITE only. If it's neither exec nor a write fault (i.e. a
read), we leave it as rwx since both write and exec (prior to EPAN)
imply read.

With the EPAN patches, exec no longer implies read, so if it's neither
an exec nor a write fault, we want vm_flags to be VM_READ|VM_WRITE since
only write now implies read.

> Also, isn't vm_flags just overwritten by the code after the hunk?
> 
> The logic in __do_page_fault() looks like might not have been written
> with the assumption that there might be more than a single set bit in
> vm_flags.

I think it was, it's checking the intersection. We could do with some
comments in this code, otherwise next time someone asks I'll spend
another 30 min reading the code ;).

-- 
Catalin

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