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

[Russell Currey]

On Wed, 2018-10-17 at 22:59 +1000, Nicholas Piggin wrote:
> On Wed, 17 Oct 2018 17:44:19 +1100
> Russell Currey <ruscur at 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 at 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.

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