Re: [PATCH v5 10/10] powerpc/mm: Detect bad KUAP faults

[Michael Ellerman <mpe@ellerman.id.au>]

Christophe Leroy <christophe.leroy@c-s.fr> writes:
> Le 08/03/2019 à 02:16, Michael Ellerman a écrit :
>> When KUAP is enabled we have logic to detect page faults that occur
>> outside of a valid user access region and are blocked by the AMR.
>> 
>> What we don't have at the moment is logic to detect a fault *within* a
>> valid user access region, that has been incorrectly blocked by AMR.
>> This is not meant to ever happen, but it can if we incorrectly
>> save/restore the AMR, or if the AMR was overwritten for some other
>> reason.
>> 
>> Currently if that happens we assume it's just a regular fault that
>> will be corrected by handling the fault normally, so we just return.
>> But there is nothing the fault handling code can do to fix it, so the
>> fault just happens again and we spin forever, leading to soft lockups.
>> 
>> So add some logic to detect that case and WARN() if we ever see it.
>> Arguably it should be a BUG(), but it's more polite to fail the access
>> and let the kernel continue, rather than taking down the box. There
>> should be no data integrity issue with failing the fault rather than
>> BUG'ing, as we're just going to disallow an access that should have
>> been allowed.
>> 
>> To make the code a little easier to follow, unroll the condition at
>> the end of bad_kernel_fault() and comment each case, before adding the
>> call to bad_kuap_fault().
>> 
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>> ---
>> 
>> v5: New.
>> 
>>   .../powerpc/include/asm/book3s/64/kup-radix.h | 12 +++++++++
>>   arch/powerpc/include/asm/kup.h                |  1 +
>>   arch/powerpc/mm/fault.c                       | 25 ++++++++++++++++---
>>   3 files changed, 35 insertions(+), 3 deletions(-)
>> 
>> diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h b/arch/powerpc/include/asm/book3s/64/kup-radix.h
>> index 3d60b04fc3f6..8d2ddc61e92e 100644
>> --- a/arch/powerpc/include/asm/book3s/64/kup-radix.h
>> +++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h
>> @@ -100,6 +100,18 @@ static inline void prevent_user_access(void __user *to, const void __user *from,
>>   	set_kuap(AMR_KUAP_BLOCKED);
>>   }
>>   
>> +static inline bool bad_kuap_fault(struct pt_regs *regs, bool is_write)
>> +{
>> +	if (mmu_has_feature(MMU_FTR_RADIX_KUAP) &&
>> +	    ((is_write && (regs->kuap & AMR_KUAP_BLOCK_WRITE)) ||
>> +	     (!is_write && (regs->kuap & AMR_KUAP_BLOCK_READ)))) {
>
> Should this { go on the previous line ?

Yeah I guess.

>> +		WARN(true, "Bug: %s fault blocked by AMR!", is_write ? "Write" : "Read");
>> +		return true;
>
> Could just be
> 	return WARN(true, ....)
>
> Or even
> 	return WARN(mmu_has_feature(MMU_FTR_RADIX_KUAP) &&
> 	    ((is_write && (regs->kuap & AMR_KUAP_BLOCK_WRITE)) ||
> 	     (!is_write && (regs->kuap & AMR_KUAP_BLOCK_READ))), ...);

That's not any more readable IMO.


>> diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h
>> index f79d4d970852..ccbd2a249575 100644
>> --- a/arch/powerpc/include/asm/kup.h
>> +++ b/arch/powerpc/include/asm/kup.h
>> @@ -28,6 +28,7 @@ static inline void prevent_user_access(void __user *to, const void __user *from,
>>   				       unsigned long size) { }
>>   static inline void allow_read_from_user(const void __user *from, unsigned long size) {}
>>   static inline void allow_write_to_user(void __user *to, unsigned long size) {}
>> +static inline bool bad_kuap_fault(struct pt_regs *regs, bool is_write) { return false; }
>>   #endif /* CONFIG_PPC_KUAP */
>>   
>>   static inline void prevent_read_from_user(const void __user *from, unsigned long size)
>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>> index 463d1e9d026e..b5d3578d9f65 100644
>> --- a/arch/powerpc/mm/fault.c
>> +++ b/arch/powerpc/mm/fault.c
>> @@ -224,7 +225,7 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr,
>>   
>>   /* Is this a bad kernel fault ? */
>>   static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
>> -			     unsigned long address)
>> +			     unsigned long address, bool is_write)
>
> We have regs, do we need is_write in addition ?

It comes from error_code, which we also have. But I don't see any harm
passing it as we already have it calculated and sitting in a GPR.

cheers