diff mbox series

[1/5] powerpc/64s: Guarded Userspace Access Prevention

Message ID 20181026063513.30806-2-ruscur@russell.cc (mailing list archive)
State Superseded
Headers show
Series Guarded Userspace Access Prevention on Radix | expand

Checks

Context Check Description
snowpatch_ozlabs/apply_patch success next/apply_patch Successfully applied
snowpatch_ozlabs/checkpatch fail Test checkpatch on branch next

Commit Message

Russell Currey Oct. 26, 2018, 6:35 a.m. UTC 
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(-)

Comments

kernel test robot Oct. 26, 2018, 8:20 a.m. UTC | #1 
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
Christophe Leroy Oct. 28, 2018, 5:57 p.m. UTC | #2 
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
kernel test robot Oct. 29, 2018, 1:27 p.m. UTC | #3 
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
Russell Currey Oct. 31, 2018, 4 a.m. UTC | #4 
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
> 
>
Christophe Leroy Oct. 31, 2018, 4:54 p.m. UTC | #5 
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
>>
>>
diff mbox series

Patch

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