Re: [PATCH 1/5] powerpc/64s: Kernel Hypervisor Restricted Access Prevention

[Nicholas Piggin <npiggin@gmail.com>]

On Wed, 17 Oct 2018 17:44:19 +1100
Russell Currey <ruscur@russell.cc> wrote:

> Kernel Hypervisor Restricted Access Prevention (KHRAP) 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
> 4% slower (performing 1GB of 1 byte read()/write() syscalls), and is gated
> behind the CONFIG_PPC_RADIX_KHRAP 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>
> ---
> More detailed benchmarks soon, there's more optimisations here as well.

Nice, this turned out to be a lot neater than I feared! Good stuff.

> @@ -240,6 +240,22 @@ BEGIN_FTR_SECTION_NESTED(941)						\
>  	mtspr	SPRN_PPR,ra;						\
>  END_FTR_SECTION_NESTED(CPU_FTR_HAS_PPR,CPU_FTR_HAS_PPR,941)
>  
> +#define LOCK_AMR(reg)							\
> +BEGIN_MMU_FTR_SECTION_NESTED(69)						\
> +	LOAD_REG_IMMEDIATE(reg,AMR_LOCKED);				\
> +	isync;				    				\
> +	mtspr	SPRN_AMR,reg;						\
> +	isync;								\
> +END_MMU_FTR_SECTION_NESTED(MMU_FTR_RADIX_KHRAP,MMU_FTR_RADIX_KHRAP,69)
> +
> +#define UNLOCK_AMR(reg)							\
> +BEGIN_MMU_FTR_SECTION_NESTED(420)						\
> +	li	reg,0;							\
> +	isync;				    				\
> +	mtspr	SPRN_AMR,reg;						\
> +	isync;								\
> +END_MMU_FTR_SECTION_NESTED(MMU_FTR_RADIX_KHRAP,MMU_FTR_RADIX_KHRAP,420)

I wonder if you can skip the first isync on the way in and the second
isync on the way out because the interrupt and return should be context
synchronizing. Might not make a difference though.

What do you think about making the name match the C code a bit more.
Like AMR_LOCK_USER_ACCESS()?

Thanks,
Nick