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

On Wed, 2018-10-17 at 22:59 +1000, Nicholas Piggin wrote:

On Wed, 17 Oct 2018 17:44:19 +1100
Russell Currey [off-list ref] wrote:

quoted

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 <redacted>
---
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.

quoted

@@ -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.

Ben thought we wouldn't need at least one of them, but it's
implementation dependent, so there might be some concern with future
chips actually needing both isyncs or something.  There weren't any
consequences to leaving out isyncs, I'll do some quick benchmarking to
see if it's any meaningful speedup to leave one out.

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

That is a good idea.

Thanks,
Nick