[7/7] powerpc/64s: Implement KUAP for Radix MMU

Kernel Userspace Access Prevention utilises a feature of the Radix MMU
which disallows read and write access to userspace addresses. By
utilising this, the kernel is prevented from accessing user data from
outside of trusted paths that perform proper safety checks, such as
copy_{to/from}_user() and friends.

Userspace access is disabled from early boot and is only enabled when
performing an operation like copy_{to/from}_user().  The register that
controls this (AMR) does not prevent userspace from accessing other
userspace, so there is no need to save and restore when entering and
exiting userspace.

This feature has a slight performance impact which I roughly measured
to be 3% slower in the worst case (performing 1GB of 1 byte
read()/write() syscalls), and is gated behind the CONFIG_PPC_KUAP
option for performance-critical builds.

This feature can be tested by using the lkdtm driver (CONFIG_LKDTM=y)
and performing the following:

  # (echo ACCESS_USERSPACE) > [debugfs]/provoke-crash/DIRECT

If enabled, this should send SIGSEGV to the thread.

A big limitation of the current implementation is that user access
is left unlocked if an exception is taken while user access is unlocked
(i.e. if an interrupt is taken during copy_to_user()). This should be
resolved in future, and is why the state is tracked in the PACA even
though nothing currently uses it.

Signed-off-by: Russell Currey <ruscur@russell.cc>
---
 .../powerpc/include/asm/book3s/64/kup-radix.h | 36 +++++++++++++++++++
 arch/powerpc/include/asm/kup.h                |  4 +++
 arch/powerpc/include/asm/mmu.h                |  9 ++++-
 arch/powerpc/include/asm/reg.h                |  1 +
 arch/powerpc/mm/pgtable-radix.c               | 16 +++++++++
 arch/powerpc/mm/pkeys.c                       |  7 ++--
 arch/powerpc/platforms/Kconfig.cputype        |  1 +
 7 files changed, 71 insertions(+), 3 deletions(-)
 create mode 100644 arch/powerpc/include/asm/book3s/64/kup-radix.h

Russell Currey's on February 21, 2019 7:36 pm:
> Kernel Userspace Access Prevention utilises a feature of the Radix MMU
> which disallows read and write access to userspace addresses. By
> utilising this, the kernel is prevented from accessing user data from
> outside of trusted paths that perform proper safety checks, such as
> copy_{to/from}_user() and friends.
> 
> Userspace access is disabled from early boot and is only enabled when
> performing an operation like copy_{to/from}_user().  The register that
> controls this (AMR) does not prevent userspace from accessing other
> userspace, so there is no need to save and restore when entering and
> exiting userspace.
> 
> This feature has a slight performance impact which I roughly measured
> to be 3% slower in the worst case (performing 1GB of 1 byte
> read()/write() syscalls), and is gated behind the CONFIG_PPC_KUAP
> option for performance-critical builds.
> 
> This feature can be tested by using the lkdtm driver (CONFIG_LKDTM=y)
> and performing the following:
> 
>   # (echo ACCESS_USERSPACE) > [debugfs]/provoke-crash/DIRECT
> 
> If enabled, this should send SIGSEGV to the thread.
> 
> A big limitation of the current implementation is that user access
> is left unlocked if an exception is taken while user access is unlocked
> (i.e. if an interrupt is taken during copy_to_user()). This should be
> resolved in future, and is why the state is tracked in the PACA even
> though nothing currently uses it.

Did you have an implementation for this in an earlier series?
What's happened to that? If the idea is to add things incrementally
that's fine.

> Signed-off-by: Russell Currey <ruscur@russell.cc>
> ---
>  .../powerpc/include/asm/book3s/64/kup-radix.h | 36 +++++++++++++++++++
>  arch/powerpc/include/asm/kup.h                |  4 +++
>  arch/powerpc/include/asm/mmu.h                |  9 ++++-
>  arch/powerpc/include/asm/reg.h                |  1 +
>  arch/powerpc/mm/pgtable-radix.c               | 16 +++++++++
>  arch/powerpc/mm/pkeys.c                       |  7 ++--
>  arch/powerpc/platforms/Kconfig.cputype        |  1 +
>  7 files changed, 71 insertions(+), 3 deletions(-)
>  create mode 100644 arch/powerpc/include/asm/book3s/64/kup-radix.h
> 
> diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h b/arch/powerpc/include/asm/book3s/64/kup-radix.h
> new file mode 100644
> index 000000000000..5cfdea954418
> --- /dev/null
> +++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_POWERPC_KUP_RADIX_H
> +#define _ASM_POWERPC_KUP_RADIX_H
> +
> +#ifndef __ASSEMBLY__
> +#ifdef CONFIG_PPC_KUAP
> +#include <asm/reg.h>
> +/*
> + * We do have the ability to individually lock/unlock reads and writes rather
> + * than both at once, however it's a significant performance hit due to needing
> + * to do a read-modify-write, which adds a mfspr, which is slow.  As a result,
> + * locking/unlocking both at once is preferred.
> + */
> +static inline void unlock_user_access(void __user *to, const void __user *from,
> +				      unsigned long size)
> +{
> +	if (!mmu_has_feature(MMU_FTR_RADIX_KUAP))
> +		return;
> +
> +	mtspr(SPRN_AMR, 0);
> +	isync();
> +	get_paca()->user_access_allowed = 1;

I think this is going to get corrupted when you context switch isn't
it? I would have thought a per thread flag would be easier, but maybe 
that's difficult in your exception code... If you've got more code to 
deal with it in a later patch, might be worth just moving all the
user_access_allowed stuff there.

Possibly you could add some debug warnings to catch double lock or 
unpaired unlock? That could be removed or put under a CONFIG option 
after it gets more testing.

> +}
> +
> +static inline void lock_user_access(void __user *to, const void __user *from,
> +				    unsigned long size)
> +{
> +	if (!mmu_has_feature(MMU_FTR_RADIX_KUAP))
> +		return;
> +
> +	mtspr(SPRN_AMR, RADIX_AMR_LOCKED);
> +	get_paca()->user_access_allowed = 0;

Without the isync here gives you some small window to execute user 
accesses without faulting I think. If that's for performance I won't 
complain, but a comment would be good.

Looks good though, no real complaints about the series.

Thanks,
Nick