Re: [PATCH] RFC: riscv: evaluate put_user() arg before enabling user access

[Ben Dooks <ben.dooks@codethink.co.uk>]

On 19/03/2021 15:03, Alex Ghiti wrote:
> Le 3/18/21 à 6:41 PM, Ben Dooks a écrit :
>> The <asm/uaccess.h> header has a problem with
>> put_user(a, ptr) if the 'a' is not a simple
>> variable, such as a function. This can lead
>> to the compiler producing code as so:
>>
>> 1:    enable_user_access()
>> 2:    evaluate 'a'
>> 3:    put 'a' to 'ptr'
>> 4:    disable_user_acess()
>>
>> The issue is that 'a' is now being evaluated
>> with the user memory protections disabled. So
>> we try and force the evaulation by assinging
>> 'x' to __val at the start, and hoping the
>> compiler barriers in enable_user_access()
>> do the job of ordering step 2 before step 1.
>>
>> This has shown up in a bug where 'a' sleeps
>> and thus schedules out and loses the SR_SUM
>> flag. This isn't sufficient to fully fix, but
>> should reduce the window of opportunity.
> 
> I would say this patch is enough to fix the issue because it only 
> happens when 'a' *explicitly schedules* when in 
> __enable_user_access()/__disable_user_access(). Otherwise, I see 2 cases:
> 
> - user memory is correctly mapped and nothing stops the current process.
> - an exception (interrupt or trap) is triggered: in those cases, the 
> exception path correctly saves and restores SR_SUM.

This fixes part of the other issue.

I did point out in the other email there could be longer cases
where the protections are disabled. The saving of the flags over
switch_to() is still necessary.

Also, I am not sure if this will guarantee ordering. It does
seem to fix it for the cases I checked

>>
>> Cc: Arnd Bergman <arnd@arndb.de>
>> ---
>>   arch/riscv/include/asm/uaccess.h | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/riscv/include/asm/uaccess.h 
>> b/arch/riscv/include/asm/uaccess.h
>> index 824b2c9da75b..7bf90d462ec9 100644
>> --- a/arch/riscv/include/asm/uaccess.h
>> +++ b/arch/riscv/include/asm/uaccess.h
>> @@ -306,7 +306,10 @@ do {                                \
>>    * data types like structures or arrays.
>>    *
>>    * @ptr must have pointer-to-simple-variable type, and @x must be 
>> assignable
>> - * to the result of dereferencing @ptr.
>> + * to the result of dereferencing @ptr. The @x is copied inside the 
>> macro
>> + * to avoid code re-ordering where @x gets evaulated within the block 
>> that
>> + * enables user-space access (thus possibly bypassing some of the 
>> protection
>> + * this feautre provides).
>>    *
>>    * Caller must check the pointer with access_ok() before calling this
>>    * function.
>> @@ -316,12 +319,13 @@ do {                                \
>>   #define __put_user(x, ptr)                    \
>>   ({                                \
>>       __typeof__(*(ptr)) __user *__gu_ptr = (ptr);        \
>> +    __typeof__(*__gu_ptr) __val = (x);            \
>>       long __pu_err = 0;                    \
>>                                   \
>>       __chk_user_ptr(__gu_ptr);                \
>>                                   \
>>       __enable_user_access();                    \
>> -    __put_user_nocheck(x, __gu_ptr, __pu_err);        \
>> +    __put_user_nocheck(__val, __gu_ptr, __pu_err);        \
>>       __disable_user_access();                \
>>                                   \
>>       __pu_err;                        \
>>
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
> 


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv