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

[Alex Ghiti]

Le 3/19/21 à 11:09 AM, Ben Dooks a écrit :
> 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.

I can't find your explanation, could you elaborate a bit more here on 
why this fix is not enough ?

Thanks !

> 
> 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 at 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 at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>>
> 
>