* [PATCH 0/5] Guarded Userspace Access Prevention on Radix @ 2018-10-26 6:35 Russell Currey 2018-10-26 6:35 ` [PATCH 1/5] powerpc/64s: Guarded Userspace Access Prevention Russell Currey ` (5 more replies) 0 siblings, 6 replies; 23+ messages in thread From: Russell Currey @ 2018-10-26 6:35 UTC (permalink / raw) To: linuxppc-dev; +Cc: mikey, Russell Currey, npiggin Guarded Userspace Access Prevention is a security mechanism that prevents the kernel from being able to read and write userspace addresses outside of the allowed paths, most commonly copy_{to/from}_user(). At present, the only CPU that supports this is POWER9, and only while using the Radix MMU. Privileged reads and writes cannot access user data when key 0 of the AMR is set. This is described in the "Radix Tree Translation Storage Protection" section of the POWER ISA as of version 3.0. GUAP code sets key 0 of the AMR (thus disabling accesses of user data) early during boot, and only ever "unlocks" access prior to certain operations, like copy_{to/from}_user(), futex ops, etc. Setting this does not prevent unprivileged access, so userspace can operate fine while access is locked. There is a performance impact, although I don't consider it heavy. Running a worst-case benchmark of a 1GB copy 1 byte at a time (and thus constant read(1) write(1) syscalls), I found enabling GUAP to be 3.5% slower than when disabled. In most cases, the difference is negligible. The main performance impact is the mtspr instruction, which is quite slow. There are a few caveats with this series that could be improved upon in future. Right now there is no saving and restoring of the AMR value - there is no userspace exploitation of the AMR on Radix in POWER9, but if this were to change in future, saving and restoring the value would be necessary. No attempt to optimise cases of repeated calls - for example, if some code was repeatedly calling copy_to_user() for small sizes very frequently, it would be slower than the equivalent of wrapping that code in an unlock and lock and only having to modify the AMR once. There are some interesting cases that I've attempted to handle, such as if the AMR is unlocked (i.e. because a copy_{to_from}_user is in progress)... - and an exception is taken, the kernel would then be running with the AMR unlocked and freely able to access userspace again. I am working around this by storing a flag in the PACA to indicate if the AMR is unlocked (to save a costly SPR read), and if so, locking the AMR in the exception entry path and unlocking it on the way out. - and gets context switched out, goes into a path that locks the AMR, then context switches back, access will be disabled and will fault. As a result, I context switch the AMR between tasks as if it was used by userspace like hash (which already implements this). Another consideration is use of the isync instruction. Without an isync following the mtspr instruction, there is no guarantee that the change takes effect. The issue is that isync is very slow, and so I tried to avoid them wherever necessary. In this series, the only place an isync gets used is after *unlocking* the AMR, because if an access takes place and access is still prevented, the kernel will fault. On the flipside, a slight delay in unlocking caused by skipping an isync potentially allows a small window of vulnerability. It is my opinion that this window is practically impossible to exploit, but if someone thinks otherwise, please do share. This series is my first attempt at POWER assembly so all feedback is very welcome. The official theme song of this series can be found here: https://www.youtube.com/watch?v=QjTrnKAcYjE Russell Currey (5): powerpc/64s: Guarded Userspace Access Prevention powerpc/futex: GUAP support for futex ops powerpc/lib: checksum GUAP support powerpc/64s: Disable GUAP with nosmap option powerpc/64s: Document that PPC supports nosmap .../admin-guide/kernel-parameters.txt | 2 +- arch/powerpc/include/asm/exception-64e.h | 3 + arch/powerpc/include/asm/exception-64s.h | 19 ++++++- arch/powerpc/include/asm/futex.h | 6 ++ 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/lib/checksum_wrappers.c | 6 +- arch/powerpc/mm/fault.c | 9 +++ arch/powerpc/mm/init_64.c | 15 +++++ arch/powerpc/mm/pgtable-radix.c | 2 + arch/powerpc/mm/pkeys.c | 7 ++- arch/powerpc/platforms/Kconfig.cputype | 15 +++++ 17 files changed, 158 insertions(+), 16 deletions(-) -- 2.19.1 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 1/5] powerpc/64s: Guarded Userspace Access Prevention 2018-10-26 6:35 [PATCH 0/5] Guarded Userspace Access Prevention on Radix Russell Currey @ 2018-10-26 6:35 ` Russell Currey 2018-10-26 8:20 ` kbuild test robot ` (2 more replies) 2018-10-26 6:35 ` [PATCH 2/5] powerpc/futex: GUAP support for futex ops Russell Currey ` (4 subsequent siblings) 5 siblings, 3 replies; 23+ messages in thread From: Russell Currey @ 2018-10-26 6:35 UTC (permalink / raw) To: linuxppc-dev; +Cc: mikey, Russell Currey, npiggin 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@russell.cc> --- 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 | #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 */ #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)) { + 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 + if (mmu_has_feature(MMU_FTR_RADIX_GUAP)) { + if (unlikely(!is_user && + (error_code & DSISR_PROTFAULT) && + (mfspr(SPRN_AMR) & AMR_LOCKED))) { + pr_crit("Kernel attempted to access user data" + " unsafely, possible exploit attempt\n"); + return bad_area_nosemaphore(regs, address); + } + } +#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. + config ARCH_ENABLE_HUGEPAGE_MIGRATION def_bool y depends on PPC_BOOK3S_64 && HUGETLB_PAGE && MIGRATION -- 2.19.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 1/5] powerpc/64s: Guarded Userspace Access Prevention 2018-10-26 6:35 ` [PATCH 1/5] powerpc/64s: Guarded Userspace Access Prevention Russell Currey @ 2018-10-26 8:20 ` kbuild test robot 2018-10-28 17:57 ` LEROY Christophe 2018-10-29 13:27 ` kbuild test robot 2 siblings, 0 replies; 23+ messages in thread From: kbuild test robot @ 2018-10-26 8:20 UTC (permalink / raw) To: Russell Currey; +Cc: mikey, linuxppc-dev, kbuild-all, npiggin [-- Attachment #1: Type: text/plain, Size: 2778 bytes --] Hi Russell, I love your patch! Yet something to improve: [auto build test ERROR on powerpc/next] [also build test ERROR on next-20181019] [cannot apply to v4.19] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Russell-Currey/Guarded-Userspace-Access-Prevention-on-Radix/20181026-145017 base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next config: powerpc-allyesconfig (attached as .config) compiler: powerpc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=7.2.0 make.cross ARCH=powerpc All error/warnings (new ones prefixed by >>): arch/powerpc/kernel/entry_64.S: Assembler messages: >> arch/powerpc/kernel/entry_64.S:304: Error: junk at end of line: `9452:.pushsection __ftr_alt_945,"a"' >> arch/powerpc/kernel/entry_64.S:304: Warning: .popsection without corresponding .pushsection; ignored >> arch/powerpc/kernel/entry_64.S:304: Error: backward ref to unknown label "9452:" >> arch/powerpc/kernel/entry_64.S:304: Error: backward ref to unknown label "9452:" >> arch/powerpc/kernel/entry_64.S:304: Error: non-constant expression in ".if" statement arch/powerpc/kernel/entry_64.S:997: Error: junk at end of line: `9452:.pushsection __ftr_alt_945,"a"' arch/powerpc/kernel/entry_64.S:997: Warning: .popsection without corresponding .pushsection; ignored arch/powerpc/kernel/entry_64.S:997: Error: backward ref to unknown label "9452:" arch/powerpc/kernel/entry_64.S:997: Error: backward ref to unknown label "9452:" arch/powerpc/kernel/entry_64.S:997: Error: non-constant expression in ".if" statement vim +304 arch/powerpc/kernel/entry_64.S 288 289 ld r13,GPR13(r1) /* only restore r13 if returning to usermode */ 290 ld r2,GPR2(r1) 291 ld r1,GPR1(r1) 292 mtlr r4 293 mtcr r5 294 mtspr SPRN_SRR0,r7 295 mtspr SPRN_SRR1,r8 296 RFI_TO_USER 297 b . /* prevent speculative execution */ 298 299 /* exit to kernel */ 300 1: /* if the AMR was unlocked before, unlock it again */ 301 lbz r2,PACA_USER_ACCESS_ALLOWED(r13) 302 cmpwi cr1,0 303 bne 2f > 304 UNLOCK_USER_ACCESS(r2) 305 2: ld r2,GPR2(r1) 306 ld r1,GPR1(r1) 307 mtlr r4 308 mtcr r5 309 mtspr SPRN_SRR0,r7 310 mtspr SPRN_SRR1,r8 311 RFI_TO_KERNEL 312 b . /* prevent speculative execution */ 313 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 58726 bytes --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/5] powerpc/64s: Guarded Userspace Access Prevention 2018-10-26 6:35 ` [PATCH 1/5] powerpc/64s: Guarded Userspace Access Prevention Russell Currey 2018-10-26 8:20 ` kbuild test robot @ 2018-10-28 17:57 ` LEROY Christophe 2018-10-31 4:00 ` Russell Currey 2018-10-29 13:27 ` kbuild test robot 2 siblings, 1 reply; 23+ messages in thread From: LEROY Christophe @ 2018-10-28 17:57 UTC (permalink / raw) To: Russell Currey; +Cc: mikey, linuxppc-dev, npiggin Russell Currey <ruscur@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@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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/5] powerpc/64s: Guarded Userspace Access Prevention 2018-10-28 17:57 ` LEROY Christophe @ 2018-10-31 4:00 ` Russell Currey 2018-10-31 16:54 ` LEROY Christophe 0 siblings, 1 reply; 23+ messages in thread From: Russell Currey @ 2018-10-31 4:00 UTC (permalink / raw) To: LEROY Christophe; +Cc: mikey, linuxppc-dev, npiggin On Sun, 2018-10-28 at 18:57 +0100, LEROY Christophe wrote: > Russell Currey <ruscur@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@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. I'll see how I go making generic handlers - I am concerned about the implementation becoming more complex than it needs to be just to accommodate potential future changes that could end up having different requirements anyway, rather than something simple that works today. > > --- > > 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,9 > > 44) > > + > > +#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,9 > > 45) > > + > > #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 ? No, no it can't. > > > #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 ? Whoops, leftover. Good catch. > > > + 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 ? No. The protection doesn't get left open for handling the exception - in fact the opposite, the protection gets enforced again on entry. The PACA flag is to make sure that on exception exit we unlock the AMR on the way back out during usercopy, without it there is no way of knowing whether we're supposed to go back to the kernel locked or unlocked. > > > + 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 ? No, this condition could in theory be hit by another kind of PROTFAULT hit in the kernel. I haven't seen that happen, but I should try checking the address or something. > > > +#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 I agree in theory, will have to play with making it more generic. Thanks for the review. - Russell > > Christophe > > > config ARCH_ENABLE_HUGEPAGE_MIGRATION > > def_bool y > > depends on PPC_BOOK3S_64 && HUGETLB_PAGE && MIGRATION > > -- > > 2.19.1 > > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/5] powerpc/64s: Guarded Userspace Access Prevention 2018-10-31 4:00 ` Russell Currey @ 2018-10-31 16:54 ` LEROY Christophe 0 siblings, 0 replies; 23+ messages in thread From: LEROY Christophe @ 2018-10-31 16:54 UTC (permalink / raw) To: Russell Currey; +Cc: mikey, linuxppc-dev, npiggin Russell Currey <ruscur@russell.cc> a écrit : > On Sun, 2018-10-28 at 18:57 +0100, LEROY Christophe wrote: >> Russell Currey <ruscur@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@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. > > I'll see how I go making generic handlers - I am concerned about the > implementation becoming more complex than it needs to be just to > accommodate potential future changes that could end up having different > requirements anyway, rather than something simple that works today. I think we can do something quite simple. I'll draft something next week and send it to get your feedback. > >> > --- >> > 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,9 >> > 44) >> > + >> > +#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,9 >> > 45) >> > + >> > #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 ? > > No, no it can't. > >> >> > #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 ? > > Whoops, leftover. Good catch. > >> >> > + 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 ? > > No. The protection doesn't get left open for handling the exception - > in fact the opposite, the protection gets enforced again on entry. The > PACA flag is to make sure that on exception exit we unlock the AMR on > the way back out during usercopy, without it there is no way of knowing > whether we're supposed to go back to the kernel locked or unlocked. But then the above test checking SPRN_AMR will always be true. > >> >> > + 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 ? > > No, this condition could in theory be hit by another kind of PROTFAULT > hit in the kernel. I haven't seen that happen, but I should try > checking the address or something. What about checking search_exception_table() ? Christophe > >> >> > +#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 > > I agree in theory, will have to play with making it more generic. > > Thanks for the review. > > - Russell > >> >> Christophe >> >> > config ARCH_ENABLE_HUGEPAGE_MIGRATION >> > def_bool y >> > depends on PPC_BOOK3S_64 && HUGETLB_PAGE && MIGRATION >> > -- >> > 2.19.1 >> >> ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/5] powerpc/64s: Guarded Userspace Access Prevention 2018-10-26 6:35 ` [PATCH 1/5] powerpc/64s: Guarded Userspace Access Prevention Russell Currey 2018-10-26 8:20 ` kbuild test robot 2018-10-28 17:57 ` LEROY Christophe @ 2018-10-29 13:27 ` kbuild test robot 2 siblings, 0 replies; 23+ messages in thread From: kbuild test robot @ 2018-10-29 13:27 UTC (permalink / raw) To: Russell Currey; +Cc: mikey, linuxppc-dev, kbuild-all, npiggin [-- Attachment #1: Type: text/plain, Size: 2873 bytes --] Hi Russell, I love your patch! Yet something to improve: [auto build test ERROR on powerpc/next] [also build test ERROR on next-20181029] [cannot apply to v4.19] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Russell-Currey/Guarded-Userspace-Access-Prevention-on-Radix/20181026-145017 base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next config: powerpc-ppc64e_defconfig (attached as .config) compiler: powerpc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=7.2.0 make.cross ARCH=powerpc All error/warnings (new ones prefixed by >>): In file included from arch/powerpc/kernel/asm-offsets.c:31:0: arch/powerpc/kernel/asm-offsets.c: In function 'main': >> include/linux/compiler_types.h:229:35: error: 'struct paca_struct' has no member named 'user_access_allowed' #define __compiler_offsetof(a, b) __builtin_offsetof(a, b) ^ include/linux/kbuild.h:6:62: note: in definition of macro 'DEFINE' asm volatile("\n.ascii \"->" #sym " %0 " #val "\"" : : "i" (val)) ^~~ include/linux/stddef.h:17:32: note: in expansion of macro '__compiler_offsetof' #define offsetof(TYPE, MEMBER) __compiler_offsetof(TYPE, MEMBER) ^~~~~~~~~~~~~~~~~~~ >> include/linux/kbuild.h:11:14: note: in expansion of macro 'offsetof' DEFINE(sym, offsetof(struct str, mem)) ^~~~~~~~ >> arch/powerpc/kernel/asm-offsets.c:263:2: note: in expansion of macro 'OFFSET' OFFSET(PACA_USER_ACCESS_ALLOWED, paca_struct, user_access_allowed); ^~~~~~ make[2]: *** [arch/powerpc/kernel/asm-offsets.s] Error 1 make[2]: Target '__build' not remade because of errors. make[1]: *** [prepare0] Error 2 make[1]: Target 'prepare' not remade because of errors. make: *** [sub-make] Error 2 vim +229 include/linux/compiler_types.h 815f0ddb Nick Desaulniers 2018-08-22 228 815f0ddb Nick Desaulniers 2018-08-22 @229 #define __compiler_offsetof(a, b) __builtin_offsetof(a, b) 815f0ddb Nick Desaulniers 2018-08-22 230 :::::: The code at line 229 was first introduced by commit :::::: 815f0ddb346c196018d4d8f8f55c12b83da1de3f include/linux/compiler*.h: make compiler-*.h mutually exclusive :::::: TO: Nick Desaulniers <ndesaulniers@google.com> :::::: CC: Linus Torvalds <torvalds@linux-foundation.org> --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 21124 bytes --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/5] powerpc/futex: GUAP support for futex ops 2018-10-26 6:35 [PATCH 0/5] Guarded Userspace Access Prevention on Radix Russell Currey 2018-10-26 6:35 ` [PATCH 1/5] powerpc/64s: Guarded Userspace Access Prevention Russell Currey @ 2018-10-26 6:35 ` Russell Currey 2018-10-26 16:32 ` LEROY Christophe 2018-10-26 6:35 ` [PATCH 3/5] powerpc/lib: checksum GUAP support Russell Currey ` (3 subsequent siblings) 5 siblings, 1 reply; 23+ messages in thread From: Russell Currey @ 2018-10-26 6:35 UTC (permalink / raw) To: linuxppc-dev; +Cc: mikey, Russell Currey, npiggin Wrap the futex operations in GUAP locks and unlocks. Signed-off-by: Russell Currey <ruscur@russell.cc> --- arch/powerpc/include/asm/futex.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/arch/powerpc/include/asm/futex.h b/arch/powerpc/include/asm/futex.h index 94542776a62d..3aed640ee9ef 100644 --- a/arch/powerpc/include/asm/futex.h +++ b/arch/powerpc/include/asm/futex.h @@ -35,6 +35,7 @@ static inline int arch_futex_atomic_op_inuser(int op, int oparg, int *oval, { int oldval = 0, ret; + unlock_user_access(); pagefault_disable(); switch (op) { @@ -62,6 +63,7 @@ static inline int arch_futex_atomic_op_inuser(int op, int oparg, int *oval, if (!ret) *oval = oldval; + lock_user_access(); return ret; } @@ -75,6 +77,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32))) return -EFAULT; + unlock_user_access(); __asm__ __volatile__ ( PPC_ATOMIC_ENTRY_BARRIER "1: lwarx %1,0,%3 # futex_atomic_cmpxchg_inatomic\n\ @@ -95,6 +98,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, : "cc", "memory"); *uval = prev; + lock_user_access(); return ret; } -- 2.19.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 2/5] powerpc/futex: GUAP support for futex ops 2018-10-26 6:35 ` [PATCH 2/5] powerpc/futex: GUAP support for futex ops Russell Currey @ 2018-10-26 16:32 ` LEROY Christophe 2018-10-29 1:08 ` Russell Currey 0 siblings, 1 reply; 23+ messages in thread From: LEROY Christophe @ 2018-10-26 16:32 UTC (permalink / raw) To: Russell Currey; +Cc: mikey, linuxppc-dev, npiggin Russell Currey <ruscur@russell.cc> a écrit : > Wrap the futex operations in GUAP locks and unlocks. Does it means futex doesn't work anymore once only patch 1 is applied ? If so, then you should split patch 1 in two parts and reorder patches so that guap can only be activated once all necessary changes are done. Otherwise the serie won't be bisectable Christophe > > Signed-off-by: Russell Currey <ruscur@russell.cc> > --- > arch/powerpc/include/asm/futex.h | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/powerpc/include/asm/futex.h > b/arch/powerpc/include/asm/futex.h > index 94542776a62d..3aed640ee9ef 100644 > --- a/arch/powerpc/include/asm/futex.h > +++ b/arch/powerpc/include/asm/futex.h > @@ -35,6 +35,7 @@ static inline int arch_futex_atomic_op_inuser(int > op, int oparg, int *oval, > { > int oldval = 0, ret; > > + unlock_user_access(); > pagefault_disable(); > > switch (op) { > @@ -62,6 +63,7 @@ static inline int arch_futex_atomic_op_inuser(int > op, int oparg, int *oval, > if (!ret) > *oval = oldval; > > + lock_user_access(); > return ret; > } > > @@ -75,6 +77,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, > if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32))) > return -EFAULT; > > + unlock_user_access(); > __asm__ __volatile__ ( > PPC_ATOMIC_ENTRY_BARRIER > "1: lwarx %1,0,%3 # futex_atomic_cmpxchg_inatomic\n\ > @@ -95,6 +98,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, > : "cc", "memory"); > > *uval = prev; > + lock_user_access(); > return ret; > } > > -- > 2.19.1 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/5] powerpc/futex: GUAP support for futex ops 2018-10-26 16:32 ` LEROY Christophe @ 2018-10-29 1:08 ` Russell Currey 0 siblings, 0 replies; 23+ messages in thread From: Russell Currey @ 2018-10-29 1:08 UTC (permalink / raw) To: LEROY Christophe; +Cc: mikey, linuxppc-dev, npiggin On Fri, 2018-10-26 at 18:32 +0200, LEROY Christophe wrote: > Russell Currey <ruscur@russell.cc> a écrit : > > > Wrap the futex operations in GUAP locks and unlocks. > > Does it means futex doesn't work anymore once only patch 1 is > applied > ? If so, then you should split patch 1 in two parts and reorder > patches so that guap can only be activated once all necessary > changes > are done. Otherwise the serie won't be bisectable Yeah, I agree. I just wanted to remove some amount of breadth from what already is one gigantic patch. Bisectability is more important than that, however. - Russell > > Christophe > > > Signed-off-by: Russell Currey <ruscur@russell.cc> > > --- > > arch/powerpc/include/asm/futex.h | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/arch/powerpc/include/asm/futex.h > > b/arch/powerpc/include/asm/futex.h > > index 94542776a62d..3aed640ee9ef 100644 > > --- a/arch/powerpc/include/asm/futex.h > > +++ b/arch/powerpc/include/asm/futex.h > > @@ -35,6 +35,7 @@ static inline int > > arch_futex_atomic_op_inuser(int > > op, int oparg, int *oval, > > { > > int oldval = 0, ret; > > > > + unlock_user_access(); > > pagefault_disable(); > > > > switch (op) { > > @@ -62,6 +63,7 @@ static inline int > > arch_futex_atomic_op_inuser(int > > op, int oparg, int *oval, > > if (!ret) > > *oval = oldval; > > > > + lock_user_access(); > > return ret; > > } > > > > @@ -75,6 +77,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 > > __user *uaddr, > > if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32))) > > return -EFAULT; > > > > + unlock_user_access(); > > __asm__ __volatile__ ( > > PPC_ATOMIC_ENTRY_BARRIER > > "1: lwarx %1,0,%3 # futex_atomic_cmpxchg_inatomic\n\ > > @@ -95,6 +98,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 > > __user *uaddr, > > : "cc", "memory"); > > > > *uval = prev; > > + lock_user_access(); > > return ret; > > } > > > > -- > > 2.19.1 > > ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 3/5] powerpc/lib: checksum GUAP support 2018-10-26 6:35 [PATCH 0/5] Guarded Userspace Access Prevention on Radix Russell Currey 2018-10-26 6:35 ` [PATCH 1/5] powerpc/64s: Guarded Userspace Access Prevention Russell Currey 2018-10-26 6:35 ` [PATCH 2/5] powerpc/futex: GUAP support for futex ops Russell Currey @ 2018-10-26 6:35 ` Russell Currey 2018-10-26 16:33 ` LEROY Christophe 2018-10-26 6:35 ` [PATCH 4/5] powerpc/64s: Disable GUAP with nosmap option Russell Currey ` (2 subsequent siblings) 5 siblings, 1 reply; 23+ messages in thread From: Russell Currey @ 2018-10-26 6:35 UTC (permalink / raw) To: linuxppc-dev; +Cc: mikey, Russell Currey, npiggin Wrap the checksumming code in GUAP locks and unlocks. Signed-off-by: Russell Currey <ruscur@russell.cc> --- arch/powerpc/lib/checksum_wrappers.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/arch/powerpc/lib/checksum_wrappers.c b/arch/powerpc/lib/checksum_wrappers.c index a0cb63fb76a1..c67db0a6e18b 100644 --- a/arch/powerpc/lib/checksum_wrappers.c +++ b/arch/powerpc/lib/checksum_wrappers.c @@ -28,6 +28,7 @@ __wsum csum_and_copy_from_user(const void __user *src, void *dst, { unsigned int csum; + unlock_user_access(); might_sleep(); *err_ptr = 0; @@ -60,6 +61,7 @@ __wsum csum_and_copy_from_user(const void __user *src, void *dst, } out: + lock_user_access(); return (__force __wsum)csum; } EXPORT_SYMBOL(csum_and_copy_from_user); @@ -69,6 +71,7 @@ __wsum csum_and_copy_to_user(const void *src, void __user *dst, int len, { unsigned int csum; + unlock_user_access(); might_sleep(); *err_ptr = 0; @@ -97,6 +100,7 @@ __wsum csum_and_copy_to_user(const void *src, void __user *dst, int len, } out: + lock_user_access(); return (__force __wsum)csum; } EXPORT_SYMBOL(csum_and_copy_to_user); -- 2.19.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 3/5] powerpc/lib: checksum GUAP support 2018-10-26 6:35 ` [PATCH 3/5] powerpc/lib: checksum GUAP support Russell Currey @ 2018-10-26 16:33 ` LEROY Christophe 0 siblings, 0 replies; 23+ messages in thread From: LEROY Christophe @ 2018-10-26 16:33 UTC (permalink / raw) To: Russell Currey; +Cc: mikey, linuxppc-dev, npiggin Same comment as for futex Christophe Russell Currey <ruscur@russell.cc> a écrit : > Wrap the checksumming code in GUAP locks and unlocks. > > Signed-off-by: Russell Currey <ruscur@russell.cc> > --- > arch/powerpc/lib/checksum_wrappers.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/powerpc/lib/checksum_wrappers.c > b/arch/powerpc/lib/checksum_wrappers.c > index a0cb63fb76a1..c67db0a6e18b 100644 > --- a/arch/powerpc/lib/checksum_wrappers.c > +++ b/arch/powerpc/lib/checksum_wrappers.c > @@ -28,6 +28,7 @@ __wsum csum_and_copy_from_user(const void __user > *src, void *dst, > { > unsigned int csum; > > + unlock_user_access(); > might_sleep(); > > *err_ptr = 0; > @@ -60,6 +61,7 @@ __wsum csum_and_copy_from_user(const void __user > *src, void *dst, > } > > out: > + lock_user_access(); > return (__force __wsum)csum; > } > EXPORT_SYMBOL(csum_and_copy_from_user); > @@ -69,6 +71,7 @@ __wsum csum_and_copy_to_user(const void *src, void > __user *dst, int len, > { > unsigned int csum; > > + unlock_user_access(); > might_sleep(); > > *err_ptr = 0; > @@ -97,6 +100,7 @@ __wsum csum_and_copy_to_user(const void *src, > void __user *dst, int len, > } > > out: > + lock_user_access(); > return (__force __wsum)csum; > } > EXPORT_SYMBOL(csum_and_copy_to_user); > -- > 2.19.1 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 4/5] powerpc/64s: Disable GUAP with nosmap option 2018-10-26 6:35 [PATCH 0/5] Guarded Userspace Access Prevention on Radix Russell Currey ` (2 preceding siblings ...) 2018-10-26 6:35 ` [PATCH 3/5] powerpc/lib: checksum GUAP support Russell Currey @ 2018-10-26 6:35 ` Russell Currey 2018-10-26 6:35 ` [PATCH 5/5] powerpc/64s: Document that PPC supports nosmap Russell Currey 2018-10-26 16:29 ` [PATCH 0/5] Guarded Userspace Access Prevention on Radix LEROY Christophe 5 siblings, 0 replies; 23+ messages in thread From: Russell Currey @ 2018-10-26 6:35 UTC (permalink / raw) To: linuxppc-dev; +Cc: mikey, Russell Currey, npiggin GUAP is similar to SMAP on x86 platforms, so implement support for the same kernel parameter. Signed-off-by: Russell Currey <ruscur@russell.cc> --- arch/powerpc/mm/init_64.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/arch/powerpc/mm/init_64.c b/arch/powerpc/mm/init_64.c index 7a9886f98b0c..b26641df36f2 100644 --- a/arch/powerpc/mm/init_64.c +++ b/arch/powerpc/mm/init_64.c @@ -312,6 +312,7 @@ void register_page_bootmem_memmap(unsigned long section_nr, #ifdef CONFIG_PPC_BOOK3S_64 static bool disable_radix = !IS_ENABLED(CONFIG_PPC_RADIX_MMU_DEFAULT); +static bool disable_guap = !IS_ENABLED(CONFIG_PPC_RADIX_GUAP); static int __init parse_disable_radix(char *p) { @@ -328,6 +329,18 @@ static int __init parse_disable_radix(char *p) } early_param("disable_radix", parse_disable_radix); +static int __init parse_nosmap(char *p) +{ + /* + * nosmap is an existing option on x86 where it doesn't return -EINVAL + * if the parameter is set to something, so even though it's different + * to disable_radix, don't return an error for compatibility. + */ + disable_guap = true; + return 0; +} +early_param("nosmap", parse_nosmap); + /* * If we're running under a hypervisor, we need to check the contents of * /chosen/ibm,architecture-vec-5 to see if the hypervisor is willing to do @@ -381,6 +394,8 @@ void __init mmu_early_init_devtree(void) /* Disable radix mode based on kernel command line. */ if (disable_radix) cur_cpu_spec->mmu_features &= ~MMU_FTR_TYPE_RADIX; + if (disable_radix || disable_guap) + cur_cpu_spec->mmu_features &= ~MMU_FTR_RADIX_GUAP; /* * Check /chosen/ibm,architecture-vec-5 if running as a guest. -- 2.19.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 5/5] powerpc/64s: Document that PPC supports nosmap 2018-10-26 6:35 [PATCH 0/5] Guarded Userspace Access Prevention on Radix Russell Currey ` (3 preceding siblings ...) 2018-10-26 6:35 ` [PATCH 4/5] powerpc/64s: Disable GUAP with nosmap option Russell Currey @ 2018-10-26 6:35 ` Russell Currey 2018-10-26 16:35 ` LEROY Christophe 2018-10-26 16:29 ` [PATCH 0/5] Guarded Userspace Access Prevention on Radix LEROY Christophe 5 siblings, 1 reply; 23+ messages in thread From: Russell Currey @ 2018-10-26 6:35 UTC (permalink / raw) To: linuxppc-dev; +Cc: mikey, Russell Currey, npiggin Signed-off-by: Russell Currey <ruscur@russell.cc> --- Documentation/admin-guide/kernel-parameters.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index a5ad67d5cb16..8f78e75965f0 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -2764,7 +2764,7 @@ noexec=on: enable non-executable mappings (default) noexec=off: disable non-executable mappings - nosmap [X86] + nosmap [X86,PPC] Disable SMAP (Supervisor Mode Access Prevention) even if it is supported by processor. -- 2.19.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 5/5] powerpc/64s: Document that PPC supports nosmap 2018-10-26 6:35 ` [PATCH 5/5] powerpc/64s: Document that PPC supports nosmap Russell Currey @ 2018-10-26 16:35 ` LEROY Christophe 2018-10-29 1:06 ` Russell Currey 0 siblings, 1 reply; 23+ messages in thread From: LEROY Christophe @ 2018-10-26 16:35 UTC (permalink / raw) To: Russell Currey; +Cc: mikey, linuxppc-dev, npiggin Why not call our new functionnality SMAP instead of calling it GUAP ? Christophe Russell Currey <ruscur@russell.cc> a écrit : > Signed-off-by: Russell Currey <ruscur@russell.cc> > --- > Documentation/admin-guide/kernel-parameters.txt | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt > b/Documentation/admin-guide/kernel-parameters.txt > index a5ad67d5cb16..8f78e75965f0 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -2764,7 +2764,7 @@ > noexec=on: enable non-executable mappings (default) > noexec=off: disable non-executable mappings > > - nosmap [X86] > + nosmap [X86,PPC] > Disable SMAP (Supervisor Mode Access Prevention) > even if it is supported by processor. > > -- > 2.19.1 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 5/5] powerpc/64s: Document that PPC supports nosmap 2018-10-26 16:35 ` LEROY Christophe @ 2018-10-29 1:06 ` Russell Currey 2018-10-31 17:06 ` LEROY Christophe 0 siblings, 1 reply; 23+ messages in thread From: Russell Currey @ 2018-10-29 1:06 UTC (permalink / raw) To: LEROY Christophe; +Cc: mikey, linuxppc-dev, npiggin On Fri, 2018-10-26 at 18:35 +0200, LEROY Christophe wrote: > Why not call our new functionnality SMAP instead of calling it GUAP ? mpe wasn't a fan of using the same terminology as other architectures. Having a separate term does avoid some assumptions about how things work or are implemented, but sharing compatibility with an existing parameter is nice. Personally I don't really care too much about the name. - Russell > > Christophe > > Russell Currey <ruscur@russell.cc> a écrit : > > > Signed-off-by: Russell Currey <ruscur@russell.cc> > > --- > > Documentation/admin-guide/kernel-parameters.txt | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt > > b/Documentation/admin-guide/kernel-parameters.txt > > index a5ad67d5cb16..8f78e75965f0 100644 > > --- a/Documentation/admin-guide/kernel-parameters.txt > > +++ b/Documentation/admin-guide/kernel-parameters.txt > > @@ -2764,7 +2764,7 @@ > > noexec=on: enable non-executable mappings > > (default) > > noexec=off: disable non-executable mappings > > > > - nosmap [X86] > > + nosmap [X86,PPC] > > Disable SMAP (Supervisor Mode Access > > Prevention) > > even if it is supported by processor. > > > > -- > > 2.19.1 > > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 5/5] powerpc/64s: Document that PPC supports nosmap 2018-10-29 1:06 ` Russell Currey @ 2018-10-31 17:06 ` LEROY Christophe 0 siblings, 0 replies; 23+ messages in thread From: LEROY Christophe @ 2018-10-31 17:06 UTC (permalink / raw) To: Russell Currey; +Cc: mikey, linuxppc-dev, npiggin Russell Currey <ruscur@russell.cc> a écrit : > On Fri, 2018-10-26 at 18:35 +0200, LEROY Christophe wrote: >> Why not call our new functionnality SMAP instead of calling it GUAP ? > > mpe wasn't a fan of using the same terminology as other architectures. I don't like too much the word 'guarded' because it means something different for powerpc MMUs. What about something like 'user space access protection' ? Christophe > Having a separate term does avoid some assumptions about how things > work or are implemented, but sharing compatibility with an existing > parameter is nice. > > Personally I don't really care too much about the name. > > - Russell > >> >> Christophe >> >> Russell Currey <ruscur@russell.cc> a écrit : >> >> > Signed-off-by: Russell Currey <ruscur@russell.cc> >> > --- >> > Documentation/admin-guide/kernel-parameters.txt | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/Documentation/admin-guide/kernel-parameters.txt >> > b/Documentation/admin-guide/kernel-parameters.txt >> > index a5ad67d5cb16..8f78e75965f0 100644 >> > --- a/Documentation/admin-guide/kernel-parameters.txt >> > +++ b/Documentation/admin-guide/kernel-parameters.txt >> > @@ -2764,7 +2764,7 @@ >> > noexec=on: enable non-executable mappings >> > (default) >> > noexec=off: disable non-executable mappings >> > >> > - nosmap [X86] >> > + nosmap [X86,PPC] >> > Disable SMAP (Supervisor Mode Access >> > Prevention) >> > even if it is supported by processor. >> > >> > -- >> > 2.19.1 >> >> ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/5] Guarded Userspace Access Prevention on Radix 2018-10-26 6:35 [PATCH 0/5] Guarded Userspace Access Prevention on Radix Russell Currey ` (4 preceding siblings ...) 2018-10-26 6:35 ` [PATCH 5/5] powerpc/64s: Document that PPC supports nosmap Russell Currey @ 2018-10-26 16:29 ` LEROY Christophe 2018-10-31 3:53 ` Russell Currey 5 siblings, 1 reply; 23+ messages in thread From: LEROY Christophe @ 2018-10-26 16:29 UTC (permalink / raw) To: Russell Currey; +Cc: mikey, linuxppc-dev, npiggin Russell Currey <ruscur@russell.cc> a écrit : > Guarded Userspace Access Prevention is a security mechanism that prevents > the kernel from being able to read and write userspace addresses outside of > the allowed paths, most commonly copy_{to/from}_user(). > > At present, the only CPU that supports this is POWER9, and only while using > the Radix MMU. Privileged reads and writes cannot access user data when > key 0 of the AMR is set. This is described in the "Radix Tree Translation > Storage Protection" section of the POWER ISA as of version 3.0. It is not right that only power9 can support that. The 8xx has mmu access protection registers which can serve the same purpose. Today on the 8xx kernel space is group 0 and user space is group 1. Group 0 is set to "page defined access permission" in MD_AP and MI_AP registers, and group 1 is set to "all accesses done with supervisor rights". By setting group 1 to "user and supervisor interpretation swapped" we can forbid kernel access to user space while still allowing user access to it. Then by simply changing group 1 mode at dedicated places we can lock/unlock kernel access to user space. Could you implement something as generic as possible having that in mind for a future patch ? Christophe > > GUAP code sets key 0 of the AMR (thus disabling accesses of user data) > early during boot, and only ever "unlocks" access prior to certain > operations, like copy_{to/from}_user(), futex ops, etc. Setting this does > not prevent unprivileged access, so userspace can operate fine while access > is locked. > > There is a performance impact, although I don't consider it heavy. Running > a worst-case benchmark of a 1GB copy 1 byte at a time (and thus constant > read(1) write(1) syscalls), I found enabling GUAP to be 3.5% slower than > when disabled. In most cases, the difference is negligible. The main > performance impact is the mtspr instruction, which is quite slow. > > There are a few caveats with this series that could be improved upon in > future. Right now there is no saving and restoring of the AMR value - > there is no userspace exploitation of the AMR on Radix in POWER9, but if > this were to change in future, saving and restoring the value would be > necessary. > > No attempt to optimise cases of repeated calls - for example, if some > code was repeatedly calling copy_to_user() for small sizes very frequently, > it would be slower than the equivalent of wrapping that code in an unlock > and lock and only having to modify the AMR once. > > There are some interesting cases that I've attempted to handle, such as if > the AMR is unlocked (i.e. because a copy_{to_from}_user is in progress)... > > - and an exception is taken, the kernel would then be running with the > AMR unlocked and freely able to access userspace again. I am working > around this by storing a flag in the PACA to indicate if the AMR is > unlocked (to save a costly SPR read), and if so, locking the AMR in > the exception entry path and unlocking it on the way out. > > - and gets context switched out, goes into a path that locks the AMR, > then context switches back, access will be disabled and will fault. > As a result, I context switch the AMR between tasks as if it was used > by userspace like hash (which already implements this). > > Another consideration is use of the isync instruction. Without an isync > following the mtspr instruction, there is no guarantee that the change > takes effect. The issue is that isync is very slow, and so I tried to > avoid them wherever necessary. In this series, the only place an isync > gets used is after *unlocking* the AMR, because if an access takes place > and access is still prevented, the kernel will fault. > > On the flipside, a slight delay in unlocking caused by skipping an isync > potentially allows a small window of vulnerability. It is my opinion > that this window is practically impossible to exploit, but if someone > thinks otherwise, please do share. > > This series is my first attempt at POWER assembly so all feedback is very > welcome. > > The official theme song of this series can be found here: > https://www.youtube.com/watch?v=QjTrnKAcYjE > > Russell Currey (5): > powerpc/64s: Guarded Userspace Access Prevention > powerpc/futex: GUAP support for futex ops > powerpc/lib: checksum GUAP support > powerpc/64s: Disable GUAP with nosmap option > powerpc/64s: Document that PPC supports nosmap > > .../admin-guide/kernel-parameters.txt | 2 +- > arch/powerpc/include/asm/exception-64e.h | 3 + > arch/powerpc/include/asm/exception-64s.h | 19 ++++++- > arch/powerpc/include/asm/futex.h | 6 ++ > 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/lib/checksum_wrappers.c | 6 +- > arch/powerpc/mm/fault.c | 9 +++ > arch/powerpc/mm/init_64.c | 15 +++++ > arch/powerpc/mm/pgtable-radix.c | 2 + > arch/powerpc/mm/pkeys.c | 7 ++- > arch/powerpc/platforms/Kconfig.cputype | 15 +++++ > 17 files changed, 158 insertions(+), 16 deletions(-) > > -- > 2.19.1 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/5] Guarded Userspace Access Prevention on Radix 2018-10-26 16:29 ` [PATCH 0/5] Guarded Userspace Access Prevention on Radix LEROY Christophe @ 2018-10-31 3:53 ` Russell Currey 2018-10-31 16:58 ` LEROY Christophe 0 siblings, 1 reply; 23+ messages in thread From: Russell Currey @ 2018-10-31 3:53 UTC (permalink / raw) To: LEROY Christophe; +Cc: mikey, linuxppc-dev, npiggin On Fri, 2018-10-26 at 18:29 +0200, LEROY Christophe wrote: > Russell Currey <ruscur@russell.cc> a écrit : > > > Guarded Userspace Access Prevention is a security mechanism that > > prevents > > the kernel from being able to read and write userspace addresses > > outside of > > the allowed paths, most commonly copy_{to/from}_user(). > > > > At present, the only CPU that supports this is POWER9, and only > > while using > > the Radix MMU. Privileged reads and writes cannot access user data > > when > > key 0 of the AMR is set. This is described in the "Radix Tree > > Translation > > Storage Protection" section of the POWER ISA as of version 3.0. > > It is not right that only power9 can support that. It's true that not only P9 can support it, but there are more considerations under hash than radix, implementing this for radix is a first step. > > The 8xx has mmu access protection registers which can serve the > same > purpose. Today on the 8xx kernel space is group 0 and user space is > group 1. Group 0 is set to "page defined access permission" in > MD_AP > and MI_AP registers, and group 1 is set to "all accesses done with > supervisor rights". By setting group 1 to "user and supervisor > interpretation swapped" we can forbid kernel access to user space > while still allowing user access to it. Then by simply changing > group > 1 mode at dedicated places we can lock/unlock kernel access to user > space. > > Could you implement something as generic as possible having that in > mind for a future patch ? I don't think anything in this series is particularly problematic in relation to future work for hash. I am interested in doing a hash implementation in future too. - Russell > > Christophe > > > GUAP code sets key 0 of the AMR (thus disabling accesses of user > > data) > > early during boot, and only ever "unlocks" access prior to certain > > operations, like copy_{to/from}_user(), futex ops, etc. Setting > > this does > > not prevent unprivileged access, so userspace can operate fine > > while access > > is locked. > > > > There is a performance impact, although I don't consider it > > heavy. Running > > a worst-case benchmark of a 1GB copy 1 byte at a time (and thus > > constant > > read(1) write(1) syscalls), I found enabling GUAP to be 3.5% slower > > than > > when disabled. In most cases, the difference is negligible. The > > main > > performance impact is the mtspr instruction, which is quite slow. > > > > There are a few caveats with this series that could be improved > > upon in > > future. Right now there is no saving and restoring of the AMR > > value - > > there is no userspace exploitation of the AMR on Radix in POWER9, > > but if > > this were to change in future, saving and restoring the value would > > be > > necessary. > > > > No attempt to optimise cases of repeated calls - for example, if > > some > > code was repeatedly calling copy_to_user() for small sizes very > > frequently, > > it would be slower than the equivalent of wrapping that code in an > > unlock > > and lock and only having to modify the AMR once. > > > > There are some interesting cases that I've attempted to handle, > > such as if > > the AMR is unlocked (i.e. because a copy_{to_from}_user is in > > progress)... > > > > - and an exception is taken, the kernel would then be running > > with the > > AMR unlocked and freely able to access userspace again. I am > > working > > around this by storing a flag in the PACA to indicate if the > > AMR is > > unlocked (to save a costly SPR read), and if so, locking the > > AMR in > > the exception entry path and unlocking it on the way out. > > > > - and gets context switched out, goes into a path that locks > > the AMR, > > then context switches back, access will be disabled and will > > fault. > > As a result, I context switch the AMR between tasks as if it > > was used > > by userspace like hash (which already implements this). > > > > Another consideration is use of the isync instruction. Without an > > isync > > following the mtspr instruction, there is no guarantee that the > > change > > takes effect. The issue is that isync is very slow, and so I tried > > to > > avoid them wherever necessary. In this series, the only place an > > isync > > gets used is after *unlocking* the AMR, because if an access takes > > place > > and access is still prevented, the kernel will fault. > > > > On the flipside, a slight delay in unlocking caused by skipping an > > isync > > potentially allows a small window of vulnerability. It is my > > opinion > > that this window is practically impossible to exploit, but if > > someone > > thinks otherwise, please do share. > > > > This series is my first attempt at POWER assembly so all feedback > > is very > > welcome. > > > > The official theme song of this series can be found here: > > https://www.youtube.com/watch?v=QjTrnKAcYjE > > > > Russell Currey (5): > > powerpc/64s: Guarded Userspace Access Prevention > > powerpc/futex: GUAP support for futex ops > > powerpc/lib: checksum GUAP support > > powerpc/64s: Disable GUAP with nosmap option > > powerpc/64s: Document that PPC supports nosmap > > > > .../admin-guide/kernel-parameters.txt | 2 +- > > arch/powerpc/include/asm/exception-64e.h | 3 + > > arch/powerpc/include/asm/exception-64s.h | 19 ++++++- > > arch/powerpc/include/asm/futex.h | 6 ++ > > 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/lib/checksum_wrappers.c | 6 +- > > arch/powerpc/mm/fault.c | 9 +++ > > arch/powerpc/mm/init_64.c | 15 +++++ > > arch/powerpc/mm/pgtable-radix.c | 2 + > > arch/powerpc/mm/pkeys.c | 7 ++- > > arch/powerpc/platforms/Kconfig.cputype | 15 +++++ > > 17 files changed, 158 insertions(+), 16 deletions(-) > > > > -- > > 2.19.1 > > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/5] Guarded Userspace Access Prevention on Radix 2018-10-31 3:53 ` Russell Currey @ 2018-10-31 16:58 ` LEROY Christophe 2018-11-01 3:54 ` Russell Currey 0 siblings, 1 reply; 23+ messages in thread From: LEROY Christophe @ 2018-10-31 16:58 UTC (permalink / raw) To: Russell Currey; +Cc: mikey, linuxppc-dev, npiggin Russell Currey <ruscur@russell.cc> a écrit : > On Fri, 2018-10-26 at 18:29 +0200, LEROY Christophe wrote: >> Russell Currey <ruscur@russell.cc> a écrit : >> >> > Guarded Userspace Access Prevention is a security mechanism that >> > prevents >> > the kernel from being able to read and write userspace addresses >> > outside of >> > the allowed paths, most commonly copy_{to/from}_user(). >> > >> > At present, the only CPU that supports this is POWER9, and only >> > while using >> > the Radix MMU. Privileged reads and writes cannot access user data >> > when >> > key 0 of the AMR is set. This is described in the "Radix Tree >> > Translation >> > Storage Protection" section of the POWER ISA as of version 3.0. >> >> It is not right that only power9 can support that. > > It's true that not only P9 can support it, but there are more > considerations under hash than radix, implementing this for radix is a > first step. I don't know much about hash, but I was talking about the 8xx which is a nohash ppc32. I'll see next week if I can do something with it on top of your serie. > >> >> The 8xx has mmu access protection registers which can serve the >> same >> purpose. Today on the 8xx kernel space is group 0 and user space is >> group 1. Group 0 is set to "page defined access permission" in >> MD_AP >> and MI_AP registers, and group 1 is set to "all accesses done with >> supervisor rights". By setting group 1 to "user and supervisor >> interpretation swapped" we can forbid kernel access to user space >> while still allowing user access to it. Then by simply changing >> group >> 1 mode at dedicated places we can lock/unlock kernel access to user >> space. >> >> Could you implement something as generic as possible having that in >> mind for a future patch ? > > I don't think anything in this series is particularly problematic in > relation to future work for hash. I am interested in doing a hash > implementation in future too. I think we have to look at that carrefuly to avoid uggly ifdefs Christophe > > - Russell > >> >> Christophe >> >> > GUAP code sets key 0 of the AMR (thus disabling accesses of user >> > data) >> > early during boot, and only ever "unlocks" access prior to certain >> > operations, like copy_{to/from}_user(), futex ops, etc. Setting >> > this does >> > not prevent unprivileged access, so userspace can operate fine >> > while access >> > is locked. >> > >> > There is a performance impact, although I don't consider it >> > heavy. Running >> > a worst-case benchmark of a 1GB copy 1 byte at a time (and thus >> > constant >> > read(1) write(1) syscalls), I found enabling GUAP to be 3.5% slower >> > than >> > when disabled. In most cases, the difference is negligible. The >> > main >> > performance impact is the mtspr instruction, which is quite slow. >> > >> > There are a few caveats with this series that could be improved >> > upon in >> > future. Right now there is no saving and restoring of the AMR >> > value - >> > there is no userspace exploitation of the AMR on Radix in POWER9, >> > but if >> > this were to change in future, saving and restoring the value would >> > be >> > necessary. >> > >> > No attempt to optimise cases of repeated calls - for example, if >> > some >> > code was repeatedly calling copy_to_user() for small sizes very >> > frequently, >> > it would be slower than the equivalent of wrapping that code in an >> > unlock >> > and lock and only having to modify the AMR once. >> > >> > There are some interesting cases that I've attempted to handle, >> > such as if >> > the AMR is unlocked (i.e. because a copy_{to_from}_user is in >> > progress)... >> > >> > - and an exception is taken, the kernel would then be running >> > with the >> > AMR unlocked and freely able to access userspace again. I am >> > working >> > around this by storing a flag in the PACA to indicate if the >> > AMR is >> > unlocked (to save a costly SPR read), and if so, locking the >> > AMR in >> > the exception entry path and unlocking it on the way out. >> > >> > - and gets context switched out, goes into a path that locks >> > the AMR, >> > then context switches back, access will be disabled and will >> > fault. >> > As a result, I context switch the AMR between tasks as if it >> > was used >> > by userspace like hash (which already implements this). >> > >> > Another consideration is use of the isync instruction. Without an >> > isync >> > following the mtspr instruction, there is no guarantee that the >> > change >> > takes effect. The issue is that isync is very slow, and so I tried >> > to >> > avoid them wherever necessary. In this series, the only place an >> > isync >> > gets used is after *unlocking* the AMR, because if an access takes >> > place >> > and access is still prevented, the kernel will fault. >> > >> > On the flipside, a slight delay in unlocking caused by skipping an >> > isync >> > potentially allows a small window of vulnerability. It is my >> > opinion >> > that this window is practically impossible to exploit, but if >> > someone >> > thinks otherwise, please do share. >> > >> > This series is my first attempt at POWER assembly so all feedback >> > is very >> > welcome. >> > >> > The official theme song of this series can be found here: >> > https://www.youtube.com/watch?v=QjTrnKAcYjE >> > >> > Russell Currey (5): >> > powerpc/64s: Guarded Userspace Access Prevention >> > powerpc/futex: GUAP support for futex ops >> > powerpc/lib: checksum GUAP support >> > powerpc/64s: Disable GUAP with nosmap option >> > powerpc/64s: Document that PPC supports nosmap >> > >> > .../admin-guide/kernel-parameters.txt | 2 +- >> > arch/powerpc/include/asm/exception-64e.h | 3 + >> > arch/powerpc/include/asm/exception-64s.h | 19 ++++++- >> > arch/powerpc/include/asm/futex.h | 6 ++ >> > 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/lib/checksum_wrappers.c | 6 +- >> > arch/powerpc/mm/fault.c | 9 +++ >> > arch/powerpc/mm/init_64.c | 15 +++++ >> > arch/powerpc/mm/pgtable-radix.c | 2 + >> > arch/powerpc/mm/pkeys.c | 7 ++- >> > arch/powerpc/platforms/Kconfig.cputype | 15 +++++ >> > 17 files changed, 158 insertions(+), 16 deletions(-) >> > >> > -- >> > 2.19.1 >> >> ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/5] Guarded Userspace Access Prevention on Radix 2018-10-31 16:58 ` LEROY Christophe @ 2018-11-01 3:54 ` Russell Currey 2018-11-08 17:52 ` Christophe LEROY 0 siblings, 1 reply; 23+ messages in thread From: Russell Currey @ 2018-11-01 3:54 UTC (permalink / raw) To: LEROY Christophe; +Cc: mikey, linuxppc-dev, npiggin On Wed, 2018-10-31 at 17:58 +0100, LEROY Christophe wrote: > Russell Currey <ruscur@russell.cc> a écrit : > > > On Fri, 2018-10-26 at 18:29 +0200, LEROY Christophe wrote: > > > Russell Currey <ruscur@russell.cc> a écrit : > > > > > > > Guarded Userspace Access Prevention is a security mechanism > > > > that > > > > prevents > > > > the kernel from being able to read and write userspace > > > > addresses > > > > outside of > > > > the allowed paths, most commonly copy_{to/from}_user(). > > > > > > > > At present, the only CPU that supports this is POWER9, and only > > > > while using > > > > the Radix MMU. Privileged reads and writes cannot access user > > > > data > > > > when > > > > key 0 of the AMR is set. This is described in the "Radix Tree > > > > Translation > > > > Storage Protection" section of the POWER ISA as of version 3.0. > > > > > > It is not right that only power9 can support that. > > > > It's true that not only P9 can support it, but there are more > > considerations under hash than radix, implementing this for radix > > is a > > first step. > > I don't know much about hash, but I was talking about the 8xx which > is > a nohash ppc32. I'll see next week if I can do something with it on > top of your serie. My small brain saw the number 8 and assumed you were talking about POWER8, I didn't know what 8xx was until now. Working on a refactor to make things a bit more generic, and removing the radix name and dependency from the config option. > > > > The 8xx has mmu access protection registers which can serve the > > > same > > > purpose. Today on the 8xx kernel space is group 0 and user space > > > is > > > group 1. Group 0 is set to "page defined access permission" in > > > MD_AP > > > and MI_AP registers, and group 1 is set to "all accesses done > > > with > > > supervisor rights". By setting group 1 to "user and supervisor > > > interpretation swapped" we can forbid kernel access to user space > > > while still allowing user access to it. Then by simply changing > > > group > > > 1 mode at dedicated places we can lock/unlock kernel access to > > > user > > > space. > > > > > > Could you implement something as generic as possible having that > > > in > > > mind for a future patch ? > > > > I don't think anything in this series is particularly problematic > > in > > relation to future work for hash. I am interested in doing a hash > > implementation in future too. > > I think we have to look at that carrefuly to avoid uggly ifdefs > > Christophe > > > - Russell > > > > > Christophe > > > > > > > GUAP code sets key 0 of the AMR (thus disabling accesses of > > > > user > > > > data) > > > > early during boot, and only ever "unlocks" access prior to > > > > certain > > > > operations, like copy_{to/from}_user(), futex ops, > > > > etc. Setting > > > > this does > > > > not prevent unprivileged access, so userspace can operate fine > > > > while access > > > > is locked. > > > > > > > > There is a performance impact, although I don't consider it > > > > heavy. Running > > > > a worst-case benchmark of a 1GB copy 1 byte at a time (and thus > > > > constant > > > > read(1) write(1) syscalls), I found enabling GUAP to be 3.5% > > > > slower > > > > than > > > > when disabled. In most cases, the difference is > > > > negligible. The > > > > main > > > > performance impact is the mtspr instruction, which is quite > > > > slow. > > > > > > > > There are a few caveats with this series that could be improved > > > > upon in > > > > future. Right now there is no saving and restoring of the AMR > > > > value - > > > > there is no userspace exploitation of the AMR on Radix in > > > > POWER9, > > > > but if > > > > this were to change in future, saving and restoring the value > > > > would > > > > be > > > > necessary. > > > > > > > > No attempt to optimise cases of repeated calls - for example, > > > > if > > > > some > > > > code was repeatedly calling copy_to_user() for small sizes very > > > > frequently, > > > > it would be slower than the equivalent of wrapping that code in > > > > an > > > > unlock > > > > and lock and only having to modify the AMR once. > > > > > > > > There are some interesting cases that I've attempted to handle, > > > > such as if > > > > the AMR is unlocked (i.e. because a copy_{to_from}_user is in > > > > progress)... > > > > > > > > - and an exception is taken, the kernel would then be > > > > running > > > > with the > > > > AMR unlocked and freely able to access userspace again. I > > > > am > > > > working > > > > around this by storing a flag in the PACA to indicate if > > > > the > > > > AMR is > > > > unlocked (to save a costly SPR read), and if so, locking > > > > the > > > > AMR in > > > > the exception entry path and unlocking it on the way out. > > > > > > > > - and gets context switched out, goes into a path that > > > > locks > > > > the AMR, > > > > then context switches back, access will be disabled and > > > > will > > > > fault. > > > > As a result, I context switch the AMR between tasks as if > > > > it > > > > was used > > > > by userspace like hash (which already implements this). > > > > > > > > Another consideration is use of the isync instruction. Without > > > > an > > > > isync > > > > following the mtspr instruction, there is no guarantee that the > > > > change > > > > takes effect. The issue is that isync is very slow, and so I > > > > tried > > > > to > > > > avoid them wherever necessary. In this series, the only place > > > > an > > > > isync > > > > gets used is after *unlocking* the AMR, because if an access > > > > takes > > > > place > > > > and access is still prevented, the kernel will fault. > > > > > > > > On the flipside, a slight delay in unlocking caused by skipping > > > > an > > > > isync > > > > potentially allows a small window of vulnerability. It is my > > > > opinion > > > > that this window is practically impossible to exploit, but if > > > > someone > > > > thinks otherwise, please do share. > > > > > > > > This series is my first attempt at POWER assembly so all > > > > feedback > > > > is very > > > > welcome. > > > > > > > > The official theme song of this series can be found here: > > > > https://www.youtube.com/watch?v=QjTrnKAcYjE > > > > > > > > Russell Currey (5): > > > > powerpc/64s: Guarded Userspace Access Prevention > > > > powerpc/futex: GUAP support for futex ops > > > > powerpc/lib: checksum GUAP support > > > > powerpc/64s: Disable GUAP with nosmap option > > > > powerpc/64s: Document that PPC supports nosmap > > > > > > > > .../admin-guide/kernel-parameters.txt | 2 +- > > > > arch/powerpc/include/asm/exception-64e.h | 3 + > > > > arch/powerpc/include/asm/exception-64s.h | 19 ++++++- > > > > arch/powerpc/include/asm/futex.h | 6 ++ > > > > 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/lib/checksum_wrappers.c | 6 +- > > > > arch/powerpc/mm/fault.c | 9 +++ > > > > arch/powerpc/mm/init_64.c | 15 +++++ > > > > arch/powerpc/mm/pgtable-radix.c | 2 + > > > > arch/powerpc/mm/pkeys.c | 7 ++- > > > > arch/powerpc/platforms/Kconfig.cputype | 15 +++++ > > > > 17 files changed, 158 insertions(+), 16 deletions(-) > > > > > > > > -- > > > > 2.19.1 > > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/5] Guarded Userspace Access Prevention on Radix 2018-11-01 3:54 ` Russell Currey @ 2018-11-08 17:52 ` Christophe LEROY 2018-11-08 20:09 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 23+ messages in thread From: Christophe LEROY @ 2018-11-08 17:52 UTC (permalink / raw) To: Russell Currey; +Cc: mikey, linuxppc-dev, npiggin Le 01/11/2018 à 04:54, Russell Currey a écrit : > On Wed, 2018-10-31 at 17:58 +0100, LEROY Christophe wrote: >> Russell Currey <ruscur@russell.cc> a écrit : >> >>> On Fri, 2018-10-26 at 18:29 +0200, LEROY Christophe wrote: >>>> Russell Currey <ruscur@russell.cc> a écrit : >>>> >>>>> Guarded Userspace Access Prevention is a security mechanism >>>>> that >>>>> prevents >>>>> the kernel from being able to read and write userspace >>>>> addresses >>>>> outside of >>>>> the allowed paths, most commonly copy_{to/from}_user(). >>>>> >>>>> At present, the only CPU that supports this is POWER9, and only >>>>> while using >>>>> the Radix MMU. Privileged reads and writes cannot access user >>>>> data >>>>> when >>>>> key 0 of the AMR is set. This is described in the "Radix Tree >>>>> Translation >>>>> Storage Protection" section of the POWER ISA as of version 3.0. >>>> >>>> It is not right that only power9 can support that. >>> >>> It's true that not only P9 can support it, but there are more >>> considerations under hash than radix, implementing this for radix >>> is a >>> first step. >> >> I don't know much about hash, but I was talking about the 8xx which >> is >> a nohash ppc32. I'll see next week if I can do something with it on >> top of your serie. > > My small brain saw the number 8 and assumed you were talking about > POWER8, I didn't know what 8xx was until now. > > Working on a refactor to make things a bit more generic, and removing > the radix name and dependency from the config option. In signal_32.c and signal_64.c, save_user_regs() calls __put_user() to modify code, then calls flush_icache_range() on user addresses. Shouldn't flush_icache_range() be performed with userspace access protection unlocked ? Christophe ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/5] Guarded Userspace Access Prevention on Radix 2018-11-08 17:52 ` Christophe LEROY @ 2018-11-08 20:09 ` Benjamin Herrenschmidt 0 siblings, 0 replies; 23+ messages in thread From: Benjamin Herrenschmidt @ 2018-11-08 20:09 UTC (permalink / raw) To: Christophe LEROY, Russell Currey; +Cc: mikey, linuxppc-dev, npiggin On Thu, 2018-11-08 at 18:52 +0100, Christophe LEROY wrote: > > In signal_32.c and signal_64.c, save_user_regs() calls __put_user() to > modify code, then calls flush_icache_range() on user addresses. > > Shouldn't flush_icache_range() be performed with userspace access > protection unlocked ? Thankfully this code is pretty much never used these days... Russell: To trigger that, you need to disable the VDSO. This brings back the idea however of having a way to "bulk" open the gate during the whole signal sequence... Cheers, Ben. ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2018-11-08 20:13 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-26 6:35 [PATCH 0/5] Guarded Userspace Access Prevention on Radix Russell Currey 2018-10-26 6:35 ` [PATCH 1/5] powerpc/64s: Guarded Userspace Access Prevention Russell Currey 2018-10-26 8:20 ` kbuild test robot 2018-10-28 17:57 ` LEROY Christophe 2018-10-31 4:00 ` Russell Currey 2018-10-31 16:54 ` LEROY Christophe 2018-10-29 13:27 ` kbuild test robot 2018-10-26 6:35 ` [PATCH 2/5] powerpc/futex: GUAP support for futex ops Russell Currey 2018-10-26 16:32 ` LEROY Christophe 2018-10-29 1:08 ` Russell Currey 2018-10-26 6:35 ` [PATCH 3/5] powerpc/lib: checksum GUAP support Russell Currey 2018-10-26 16:33 ` LEROY Christophe 2018-10-26 6:35 ` [PATCH 4/5] powerpc/64s: Disable GUAP with nosmap option Russell Currey 2018-10-26 6:35 ` [PATCH 5/5] powerpc/64s: Document that PPC supports nosmap Russell Currey 2018-10-26 16:35 ` LEROY Christophe 2018-10-29 1:06 ` Russell Currey 2018-10-31 17:06 ` LEROY Christophe 2018-10-26 16:29 ` [PATCH 0/5] Guarded Userspace Access Prevention on Radix LEROY Christophe 2018-10-31 3:53 ` Russell Currey 2018-10-31 16:58 ` LEROY Christophe 2018-11-01 3:54 ` Russell Currey 2018-11-08 17:52 ` Christophe LEROY 2018-11-08 20:09 ` Benjamin Herrenschmidt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).