[PATCH 1/5] powerpc/64s: Guarded Userspace Access Prevention
LEROY Christophe
christophe.leroy at c-s.fr
Mon Oct 29 04:57:06 AEDT 2018
Russell Currey <ruscur at russell.cc> a écrit :
> Guarded Userspace Access Prevention (GUAP) 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:
>
> - exiting the kernel and entering userspace
> - performing an operation like copy_{to/from}_user()
> - context switching to a process that has access enabled
>
> and similarly, access is disabled again when exiting userspace and entering
> the kernel.
>
> 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_RADIX_GUAP 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.
>
> Signed-off-by: Russell Currey <ruscur at russell.cc>
I think this patch should be split in at least two parts:
First part for implementing the generic part, including the changes to
futex and csum, and a second part implementing the radix part.
> ---
> Since the previous version of this patchset (named KHRAP) there have been
> several changes, some of which include:
>
> - macro naming, suggested by Nick
> - builds should be fixed outside of 64s
> - no longer unlock heading out to userspace
> - removal of unnecessary isyncs
> - more config option testing
> - removal of save/restore
> - use pr_crit() and reword message on fault
>
> arch/powerpc/include/asm/exception-64e.h | 3 ++
> arch/powerpc/include/asm/exception-64s.h | 19 +++++++-
> arch/powerpc/include/asm/mmu.h | 7 +++
> arch/powerpc/include/asm/paca.h | 3 ++
> arch/powerpc/include/asm/reg.h | 1 +
> arch/powerpc/include/asm/uaccess.h | 57 ++++++++++++++++++++----
> arch/powerpc/kernel/asm-offsets.c | 1 +
> arch/powerpc/kernel/dt_cpu_ftrs.c | 4 ++
> arch/powerpc/kernel/entry_64.S | 17 ++++++-
> arch/powerpc/mm/fault.c | 12 +++++
> arch/powerpc/mm/pgtable-radix.c | 2 +
> arch/powerpc/mm/pkeys.c | 7 ++-
> arch/powerpc/platforms/Kconfig.cputype | 15 +++++++
> 13 files changed, 135 insertions(+), 13 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/exception-64e.h
> b/arch/powerpc/include/asm/exception-64e.h
> index 555e22d5e07f..bf25015834ee 100644
> --- a/arch/powerpc/include/asm/exception-64e.h
> +++ b/arch/powerpc/include/asm/exception-64e.h
> @@ -215,5 +215,8 @@ exc_##label##_book3e:
> #define RFI_TO_USER \
> rfi
>
> +#define UNLOCK_USER_ACCESS(reg)
> +#define LOCK_USER_ACCESS(reg)
> +
> #endif /* _ASM_POWERPC_EXCEPTION_64E_H */
>
> diff --git a/arch/powerpc/include/asm/exception-64s.h
> b/arch/powerpc/include/asm/exception-64s.h
> index 3b4767ed3ec5..0cac5bd380ca 100644
> --- a/arch/powerpc/include/asm/exception-64s.h
> +++ b/arch/powerpc/include/asm/exception-64s.h
> @@ -264,6 +264,19 @@ BEGIN_FTR_SECTION_NESTED(943) \
> std ra,offset(r13); \
> END_FTR_SECTION_NESTED(ftr,ftr,943)
>
> +#define LOCK_USER_ACCESS(reg) \
> +BEGIN_MMU_FTR_SECTION_NESTED(944) \
> + LOAD_REG_IMMEDIATE(reg,AMR_LOCKED); \
> + mtspr SPRN_AMR,reg; \
> +END_MMU_FTR_SECTION_NESTED(MMU_FTR_RADIX_GUAP,MMU_FTR_RADIX_GUAP,944)
> +
> +#define UNLOCK_USER_ACCESS(reg) \
> +BEGIN_MMU_FTR_SECTION_NESTED(945) \
> + li reg,0; \
> + mtspr SPRN_AMR,reg; \
> + isync \
> +END_MMU_FTR_SECTION_NESTED(MMU_FTR_RADIX_GUAP,MMU_FTR_RADIX_GUAP,945)
> +
> #define EXCEPTION_PROLOG_0(area) \
> GET_PACA(r13); \
> std r9,area+EX_R9(r13); /* save r9 */ \
> @@ -500,7 +513,11 @@ END_FTR_SECTION_NESTED(ftr,ftr,943)
> beq 4f; /* if from kernel mode */ \
> ACCOUNT_CPU_USER_ENTRY(r13, r9, r10); \
> SAVE_PPR(area, r9); \
> -4: EXCEPTION_PROLOG_COMMON_2(area) \
> +4: lbz r9,PACA_USER_ACCESS_ALLOWED(r13); \
> + cmpwi cr1,r9,0; \
> + beq 5f; \
> + LOCK_USER_ACCESS(r9); \
> +5: EXCEPTION_PROLOG_COMMON_2(area) \
> EXCEPTION_PROLOG_COMMON_3(n) \
> ACCOUNT_STOLEN_TIME
>
> diff --git a/arch/powerpc/include/asm/mmu.h b/arch/powerpc/include/asm/mmu.h
> index eb20eb3b8fb0..3b31ed702785 100644
> --- a/arch/powerpc/include/asm/mmu.h
> +++ b/arch/powerpc/include/asm/mmu.h
> @@ -107,6 +107,10 @@
> */
> #define MMU_FTR_1T_SEGMENT ASM_CONST(0x40000000)
>
> +/* Supports GUAP (key 0 controlling userspace addresses) on radix
> + */
> +#define MMU_FTR_RADIX_GUAP ASM_CONST(0x80000000)
> +
> /* MMU feature bit sets for various CPUs */
> #define MMU_FTRS_DEFAULT_HPTE_ARCH_V2 \
> MMU_FTR_HPTE_TABLE | MMU_FTR_PPCAS_ARCH_V2
> @@ -143,6 +147,9 @@ enum {
> MMU_FTR_KERNEL_RO | MMU_FTR_68_BIT_VA |
> #ifdef CONFIG_PPC_RADIX_MMU
> MMU_FTR_TYPE_RADIX |
> +#endif
> +#ifdef CONFIG_PPC_RADIX_GUAP
> + MMU_FTR_RADIX_GUAP |
Can this exist without MMT_FTR_TYPE_RADIX ?
> #endif
> 0,
> };
> diff --git a/arch/powerpc/include/asm/paca.h
> b/arch/powerpc/include/asm/paca.h
> index e843bc5d1a0f..e905f09b2d38 100644
> --- a/arch/powerpc/include/asm/paca.h
> +++ b/arch/powerpc/include/asm/paca.h
> @@ -169,6 +169,9 @@ struct paca_struct {
> u64 saved_r1; /* r1 save for RTAS calls or PM or EE=0 */
> u64 saved_msr; /* MSR saved here by enter_rtas */
> u16 trap_save; /* Used when bad stack is encountered */
> +#ifdef CONFIG_PPC_RADIX_GUAP
> + u8 user_access_allowed; /* set when AMR allows user accesses */
> +#endif
> u8 irq_soft_mask; /* mask for irq soft masking */
> u8 irq_happened; /* irq happened while soft-disabled */
> u8 io_sync; /* writel() needs spin_unlock sync */
> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
> index 640a4d818772..b994099a906b 100644
> --- a/arch/powerpc/include/asm/reg.h
> +++ b/arch/powerpc/include/asm/reg.h
> @@ -246,6 +246,7 @@
> #define SPRN_DSCR 0x11
> #define SPRN_CFAR 0x1c /* Come From Address Register */
> #define SPRN_AMR 0x1d /* Authority Mask Register */
> +#define AMR_LOCKED 0xC000000000000000ULL /* Read & Write disabled */
Why ULL ? mtspr() takes unsigned long arg.
> #define SPRN_UAMOR 0x9d /* User Authority Mask Override Register */
> #define SPRN_AMOR 0x15d /* Authority Mask Override Register */
> #define SPRN_ACOP 0x1F /* Available Coprocessor Register */
> diff --git a/arch/powerpc/include/asm/uaccess.h
> b/arch/powerpc/include/asm/uaccess.h
> index 15bea9a0f260..209bfc47c340 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -62,6 +62,27 @@ static inline int __access_ok(unsigned long addr,
> unsigned long size,
>
> #endif
>
> +static inline void unlock_user_access(void)
> +{
> +#ifdef CONFIG_PPC_RADIX_GUAP
> + if (mmu_has_feature(MMU_FTR_RADIX_GUAP)) {
You need to include the .h which provides mmu_has_feature()
I think uaccess.h should only include the empty function for when
CONFIG_PPC_GUAP is not defined. Radix guap function should go in a
radix header file.
> + mtspr(SPRN_AMR, 0);
> + isync();
> + get_paca()->user_access_allowed = 1;
> + }
> +#endif
> +}
> +
> +static inline void lock_user_access(void)
> +{
> +#ifdef CONFIG_PPC_RADIX_GUAP
> + if (mmu_has_feature(MMU_FTR_RADIX_GUAP)) {
> + mtspr(SPRN_AMR, AMR_LOCKED);
> + get_paca()->user_access_allowed = 0;
> + }
> +#endif
> +}
> +
> #define access_ok(type, addr, size) \
> (__chk_user_ptr(addr), \
> __access_ok((__force unsigned long)(addr), (size), get_fs()))
> @@ -141,6 +162,7 @@ extern long __put_user_bad(void);
> #define __put_user_size(x, ptr, size, retval) \
> do { \
> retval = 0; \
> + unlock_user_access(); \
> switch (size) { \
> case 1: __put_user_asm(x, ptr, retval, "stb"); break; \
> case 2: __put_user_asm(x, ptr, retval, "sth"); break; \
> @@ -148,6 +170,7 @@ do { \
> case 8: __put_user_asm2(x, ptr, retval); break; \
> default: __put_user_bad(); \
> } \
> + lock_user_access(); \
> } while (0)
>
> #define __put_user_nocheck(x, ptr, size) \
> @@ -240,6 +263,7 @@ do { \
> __chk_user_ptr(ptr); \
> if (size > sizeof(x)) \
> (x) = __get_user_bad(); \
> + unlock_user_access(); \
> switch (size) { \
> case 1: __get_user_asm(x, ptr, retval, "lbz"); break; \
> case 2: __get_user_asm(x, ptr, retval, "lhz"); break; \
> @@ -247,6 +271,7 @@ do { \
> case 8: __get_user_asm2(x, ptr, retval); break; \
> default: (x) = __get_user_bad(); \
> } \
> + lock_user_access(); \
> } while (0)
>
> /*
> @@ -306,15 +331,20 @@ extern unsigned long __copy_tofrom_user(void
> __user *to,
> static inline unsigned long
> raw_copy_in_user(void __user *to, const void __user *from, unsigned long n)
> {
> - return __copy_tofrom_user(to, from, n);
> + unsigned long ret;
> + unlock_user_access(); \
> + ret = __copy_tofrom_user(to, from, n); \
> + lock_user_access(); \
> + return ret; \
> }
> #endif /* __powerpc64__ */
>
> static inline unsigned long raw_copy_from_user(void *to,
> const void __user *from, unsigned long n)
> {
> + unsigned long ret;
> if (__builtin_constant_p(n) && (n <= 8)) {
> - unsigned long ret = 1;
> + ret = 1;
>
> switch (n) {
> case 1:
> @@ -339,14 +369,18 @@ static inline unsigned long
> raw_copy_from_user(void *to,
> }
>
> barrier_nospec();
> - return __copy_tofrom_user((__force void __user *)to, from, n);
> + unlock_user_access();
> + ret = __copy_tofrom_user((__force void __user *)to, from, n);
> + lock_user_access();
> + return ret;
> }
>
> static inline unsigned long raw_copy_to_user(void __user *to,
> const void *from, unsigned long n)
> {
> + unsigned long ret;
> if (__builtin_constant_p(n) && (n <= 8)) {
> - unsigned long ret = 1;
> + ret = 1;
>
> switch (n) {
> case 1:
> @@ -366,17 +400,24 @@ static inline unsigned long
> raw_copy_to_user(void __user *to,
> return 0;
> }
>
> - return __copy_tofrom_user(to, (__force const void __user *)from, n);
> + unlock_user_access();
> + ret = __copy_tofrom_user(to, (__force const void __user *)from, n);
> + lock_user_access();
> + return ret;
> }
>
> extern unsigned long __clear_user(void __user *addr, unsigned long size);
>
> static inline unsigned long clear_user(void __user *addr, unsigned
> long size)
> {
> + unsigned long ret = size;
> might_fault();
> - if (likely(access_ok(VERIFY_WRITE, addr, size)))
> - return __clear_user(addr, size);
> - return size;
> + if (likely(access_ok(VERIFY_WRITE, addr, size))) {
> + unlock_user_access();
> + ret = __clear_user(addr, size);
> + lock_user_access();
> + }
> + return ret;
> }
>
> extern long strncpy_from_user(char *dst, const char __user *src,
> long count);
> diff --git a/arch/powerpc/kernel/asm-offsets.c
> b/arch/powerpc/kernel/asm-offsets.c
> index 10ef2e4db2fd..5050f15ad2f5 100644
> --- a/arch/powerpc/kernel/asm-offsets.c
> +++ b/arch/powerpc/kernel/asm-offsets.c
> @@ -260,6 +260,7 @@ int main(void)
> OFFSET(ACCOUNT_STARTTIME_USER, paca_struct, accounting.starttime_user);
> OFFSET(ACCOUNT_USER_TIME, paca_struct, accounting.utime);
> OFFSET(ACCOUNT_SYSTEM_TIME, paca_struct, accounting.stime);
> + OFFSET(PACA_USER_ACCESS_ALLOWED, paca_struct, user_access_allowed);
> OFFSET(PACA_TRAP_SAVE, paca_struct, trap_save);
> OFFSET(PACA_NAPSTATELOST, paca_struct, nap_state_lost);
> OFFSET(PACA_SPRG_VDSO, paca_struct, sprg_vdso);
> diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c
> b/arch/powerpc/kernel/dt_cpu_ftrs.c
> index f432054234a4..df4716624840 100644
> --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
> +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
> @@ -337,6 +337,10 @@ static int __init feat_enable_mmu_radix(struct
> dt_cpu_feature *f)
> cur_cpu_spec->mmu_features |= MMU_FTRS_HASH_BASE;
> cur_cpu_spec->cpu_user_features |= PPC_FEATURE_HAS_MMU;
>
> +#ifdef CONFIG_PPC_RADIX_GUAP
> + cur_cpu_spec->mmu_features |= MMU_FTR_RADIX_GUAP;
> +#endif
> +
> return 1;
> #endif
> return 0;
> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> index 7b1693adff2a..23f0944185d3 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -297,7 +297,12 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> b . /* prevent speculative execution */
>
> /* exit to kernel */
> -1: ld r2,GPR2(r1)
> +1: /* if the AMR was unlocked before, unlock it again */
> + lbz r2,PACA_USER_ACCESS_ALLOWED(r13)
> + cmpwi cr1,0
> + bne 2f
> + UNLOCK_USER_ACCESS(r2)
> +2: ld r2,GPR2(r1)
> ld r1,GPR1(r1)
> mtlr r4
> mtcr r5
> @@ -965,6 +970,7 @@ BEGIN_FTR_SECTION
> ld r2,_PPR(r1)
> mtspr SPRN_PPR,r2
> END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> +
> ACCOUNT_CPU_USER_EXIT(r13, r2, r4)
> REST_GPR(13, r1)
>
> @@ -983,7 +989,14 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> RFI_TO_USER
> b . /* prevent speculative execution */
>
> -1: mtspr SPRN_SRR1,r3
> +1: /* exit to kernel */
> + /* if the AMR was unlocked before, unlock it again */
> + lbz r2,PACA_USER_ACCESS_ALLOWED(r13)
> + cmpwi cr1,0
> + bne 2f
> + UNLOCK_USER_ACCESS(r2)
> +
> +2: mtspr SPRN_SRR1,r3
>
> ld r2,_CCR(r1)
> mtcrf 0xFF,r2
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index d51cf5f4e45e..17fd8c6b055b 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -462,6 +462,18 @@ static int __do_page_fault(struct pt_regs
> *regs, unsigned long address,
> return bad_key_fault_exception(regs, address,
> get_mm_addr_key(mm, address));
>
> +#ifdef CONFIG_PPC_RADIX_SMAP
SMAP ?
> + if (mmu_has_feature(MMU_FTR_RADIX_GUAP)) {
> + if (unlikely(!is_user &&
> + (error_code & DSISR_PROTFAULT) &&
> + (mfspr(SPRN_AMR) & AMR_LOCKED))) {
Do you mean that in case of fault in user copy, we leave the
protection open for handling the exception ? What is the purpose of
the new paca flag then ?
> + pr_crit("Kernel attempted to access user data"
> + " unsafely, possible exploit attempt\n");
> + return bad_area_nosemaphore(regs, address);
> + }
> + }
Are we sure it is an access to user data ?
> +#endif
> +
> /*
> * We want to do this outside mmap_sem, because reading code around nip
> * can result in fault, which will cause a deadlock when called with
> diff --git a/arch/powerpc/mm/pgtable-radix.c
> b/arch/powerpc/mm/pgtable-radix.c
> index c879979faa73..9e5b98887a05 100644
> --- a/arch/powerpc/mm/pgtable-radix.c
> +++ b/arch/powerpc/mm/pgtable-radix.c
> @@ -29,6 +29,7 @@
> #include <asm/powernv.h>
> #include <asm/sections.h>
> #include <asm/trace.h>
> +#include <asm/uaccess.h>
>
> #include <trace/events/thp.h>
>
> @@ -608,6 +609,7 @@ void __init radix__early_init_mmu(void)
> mtspr(SPRN_LPCR, lpcr | LPCR_UPRT | LPCR_HR);
> radix_init_partition_table();
> radix_init_amor();
> + lock_user_access();
> } else {
> radix_init_pseries();
> }
> diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> index b271b283c785..0b9bc320138c 100644
> --- a/arch/powerpc/mm/pkeys.c
> +++ b/arch/powerpc/mm/pkeys.c
> @@ -7,6 +7,7 @@
>
> #include <asm/mman.h>
> #include <asm/setup.h>
> +#include <asm/uaccess.h>
> #include <linux/pkeys.h>
> #include <linux/of_device.h>
>
> @@ -266,7 +267,8 @@ int __arch_set_user_pkey_access(struct
> task_struct *tsk, int pkey,
>
> void thread_pkey_regs_save(struct thread_struct *thread)
> {
> - if (static_branch_likely(&pkey_disabled))
> + if (static_branch_likely(&pkey_disabled) &&
> + !mmu_has_feature(MMU_FTR_RADIX_GUAP))
> return;
>
> /*
> @@ -280,7 +282,8 @@ void thread_pkey_regs_save(struct thread_struct *thread)
> void thread_pkey_regs_restore(struct thread_struct *new_thread,
> struct thread_struct *old_thread)
> {
> - if (static_branch_likely(&pkey_disabled))
> + if (static_branch_likely(&pkey_disabled) &&
> + !mmu_has_feature(MMU_FTR_RADIX_GUAP))
> return;
>
> if (old_thread->amr != new_thread->amr)
> diff --git a/arch/powerpc/platforms/Kconfig.cputype
> b/arch/powerpc/platforms/Kconfig.cputype
> index f4e2c5729374..6617d3e415a7 100644
> --- a/arch/powerpc/platforms/Kconfig.cputype
> +++ b/arch/powerpc/platforms/Kconfig.cputype
> @@ -351,6 +351,21 @@ config PPC_RADIX_MMU_DEFAULT
>
> If you're unsure, say Y.
>
> +config PPC_RADIX_GUAP
> + bool "Guarded Userspace Access Prevention on Radix"
> + depends on PPC_RADIX_MMU
> + default y
> + help
> + Enable support for Guarded Userspace Access Prevention (GUAP)
> + when using the Radix MMU. GUAP is a security feature
> + preventing the kernel from directly accessing userspace data
> + without going through the proper checks.
> +
> + GUAP has a minor performance impact on context switching and can be
> + disabled at boot time using the "nosmap" kernel command line option.
> +
> + If you're unsure, say Y.
> +
I think this should be a named in a generic way without the radix thing.
Then one day it will be reused by the 8xx
Christophe
> config ARCH_ENABLE_HUGEPAGE_MIGRATION
> def_bool y
> depends on PPC_BOOK3S_64 && HUGETLB_PAGE && MIGRATION
> --
> 2.19.1
More information about the Linuxppc-dev
mailing list