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

[christophe leroy <christophe.leroy@c-s.fr>]

Le 08/03/2019 à 09:53, Christophe Leroy a écrit :
> 
> 
> 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 ?
> 
>> +        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))), ...);

Could also be simplified as follows since (is_write && ...) and 
(!is_write && ...) are mutually exclusive:

mmu_has_feature(MMU_FTR_RADIX_KUAP) &&
(regs->kuap & (is_write ? AMR_KUAP_BLOCK_WRITE : AMR_KUAP_BLOCK_READ))

Christophe

> 
>> +    }
>> +
>> +    return false;
>> +}
>>   #endif /* CONFIG_PPC_KUAP */
>>   #endif /* __ASSEMBLY__ */
>> 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
>> @@ -44,6 +44,7 @@
>>   #include <asm/mmu_context.h>
>>   #include <asm/siginfo.h>
>>   #include <asm/debug.h>
>> +#include <asm/kup.h>
>>   static inline bool notify_page_fault(struct pt_regs *regs)
>>   {
>> @@ -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 ?
> 
> Christophe
> 
>>   {
>>       int is_exec = TRAP(regs) == 0x400;
>> @@ -235,6 +236,9 @@ static bool bad_kernel_fault(struct pt_regs *regs, 
>> unsigned long error_code,
>>                       address >= TASK_SIZE ? "exec-protected" : "user",
>>                       address,
>>                       from_kuid(&init_user_ns, current_uid()));
>> +
>> +        // Kernel exec fault is always bad
>> +        return true;
>>       }
>>       if (!is_exec && address < TASK_SIZE && (error_code & 
>> DSISR_PROTFAULT) &&
>> @@ -244,7 +248,22 @@ static bool bad_kernel_fault(struct pt_regs 
>> *regs, unsigned long error_code,
>>                       from_kuid(&init_user_ns, current_uid()));
>>       }
>> -    return is_exec || (address >= TASK_SIZE) || 
>> !search_exception_tables(regs->nip);
>> +    // Kernel fault on kernel address is bad
>> +    if (address >= TASK_SIZE)
>> +        return true;
>> +
>> +    // Fault on user outside of certain regions (eg. 
>> copy_tofrom_user()) is bad
>> +    if (!search_exception_tables(regs->nip))
>> +        return true;
>> +
>> +    // Read/write fault in a valid region (the exception table search 
>> passed
>> +    // above), but blocked by KUAP is bad, it can never succeed.
>> +    if (bad_kuap_fault(regs, is_write))
>> +        return true;
>> +
>> +    // What's left? Kernel fault on user in well defined regions 
>> (extable
>> +    // matched), and allowed by KUAP in the faulting context.
>> +    return false;
>>   }
>>   static bool bad_stack_expansion(struct pt_regs *regs, unsigned long 
>> address,
>> @@ -467,7 +486,7 @@ static int __do_page_fault(struct pt_regs *regs, 
>> unsigned long address,
>>        * take a page fault to a kernel address or a page fault to a user
>>        * address outside of dedicated places
>>        */
>> -    if (unlikely(!is_user && bad_kernel_fault(regs, error_code, 
>> address)))
>> +    if (unlikely(!is_user && bad_kernel_fault(regs, error_code, 
>> address, is_write)))
>>           return SIGSEGV;
>>       /*
>>

---
L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast.
https://www.avast.com/antivirus