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

[Ben Dooks]

On 19/03/2021 16:12, Alex Ghiti wrote:
> 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 ?


I would have to check if this current applies to riscv, but there is
code that does the following (fs/select.c does this):

                 if (!user_read_access_begin(from, sizeof(*from)))
                         return -EFAULT;
                 unsafe_get_user(to->p, &from->p, Efault);
                 unsafe_get_user(to->size, &from->size, Efault);
                 user_read_access_end();

My argument for fixing with both is:

- cover more than the put_user() case
- try and avoid any future bug
- ensure we do not leak SR_SUM elsewhere

I may also have a quick check to see if we don't also leak other
flags during these swaps. It might be we should save and restore
all flags.

I do see that this fix is going to hit a good proportion of the
cases we've seen so far. I could try and run a stress test with
just this in over the weekend (so far syz-stress has been running
for over 24hrs with minimal issues)


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

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