[PATCH 3/4] powerpc/uaccess: evaluate macro arguments once, before user access is allowed

[Nicholas Piggin]

Christophe Leroy's on March 27, 2020 5:21 pm:
> 
> 
> Le 27/03/2020 à 08:02, Nicholas Piggin a écrit :
>> get/put_user can be called with nontrivial arguments. fs/proc/page.c
>> has a good example:
>> 
>>      if (put_user(stable_page_flags(ppage), out)) {
>> 
>> stable_page_flags is quite a lot of code, including spin locks in the
>> page allocator.
>> 
>> Ensure these arguments are evaluated before user access is allowed.
>> This improves security by reducing code with access to userspace, but
>> it also fixes a PREEMPT bug with KUAP on powerpc/64s:
>> stable_page_flags is currently called with AMR set to allow writes,
>> it ends up calling spin_unlock(), which can call preempt_schedule. But
>> the task switch code can not be called with AMR set (it relies on
>> interrupts saving the register), so this blows up.
>> 
>> It's fine if the code inside allow_user_access is preemptible, because
>> a timer or IPI will save the AMR, but it's not okay to explicitly
>> cause a reschedule.
>> 
>> Signed-off-by: Nicholas Piggin <npiggin at gmail.com>
>> ---
>>   arch/powerpc/include/asm/uaccess.h | 97 ++++++++++++++++++------------
>>   1 file changed, 59 insertions(+), 38 deletions(-)
>> 
>> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
>> index 670910df3cc7..1cf8595aeef1 100644
>> --- a/arch/powerpc/include/asm/uaccess.h
>> +++ b/arch/powerpc/include/asm/uaccess.h
>> @@ -162,36 +162,48 @@ do {								\
>>   	prevent_write_to_user(ptr, size);			\
>>   } while (0)
>>   
>> -#define __put_user_nocheck(x, ptr, size, do_allow)			\
>> +#define __put_user_nocheck(x, ptr, size, do_allow)		\
> 
> No need to touch this line. Anyway at the end, you still have several \ 
> which are not aligned.
> 
>>   ({								\
>>   	long __pu_err;						\
>>   	__typeof__(*(ptr)) __user *__pu_addr = (ptr);		\
>> +	__typeof__(*(ptr)) __pu_val = (x);			\
>> +	__typeof__(size) __pu_size = (size);			\
>> +								\
>>   	if (!is_kernel_addr((unsigned long)__pu_addr))		\
>>   		might_fault();					\
>> -	__chk_user_ptr(ptr);					\
>> -	if (do_allow)								\
> 
> No need to touch that line
> 
>> -		__put_user_size((x), __pu_addr, (size), __pu_err);		\
>> -	else									\
> 
> No need to touch that line

Okay fair point I can redo the patch.

Thanks,
Nick