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

[Ben Dooks]

On 30/03/2021 06:47, Palmer Dabbelt wrote:
> On Mon, 29 Mar 2021 02:57:49 PDT (-0700), ben.dooks at codethink.co.uk wrote:
>> 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' into register 'r'
>> 3:    put 'r' 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 assigning
>> '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. The first instance of this
>> we found is in scheudle_tail() where the code does:
>>
>> $ less -N kernel/sched/core.c
>>
>> 4263  if (current->set_child_tid)
>> 4264         put_user(task_pid_vnr(current), current->set_child_tid);
>>
>> Here, the task_pid_vnr(current) is called within the block that has
>> enabled the user memory access. This can be made worse with KASAN
>> which makes task_pid_vnr() a rather large call with plenty of
>> opportunity to sleep.
>>
>> Signed-off-by: Ben Dooks <ben.dooks at codethink.co.uk>
>> Reported-by: syzbot+e74b94fe601ab9552d69 at syzkaller.appspotmail.com
>> Suggested-by: Arnd Bergman <arnd at arndb.de>
> 
> Thanks, this is on fixes.

Great. I still think we need a discussion on whether __switch_to()
also needs to save some of the flags from sstatus. I did some basic
tests and so far I think the SR_SUM is the only thing that should be
saved.

I'll try and finish writing this bug-trail up and seeing what else
needs to be done. I expect there may be other architectures that
have similar issues with put_user() and friends

At least the riscv stress testing can move forward with this in
place.

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

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