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

[Alex Ghiti]

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.

> 
> 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;						\
>