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

[Arnd Bergmann]

On Thu, Mar 18, 2021 at 11:41 PM Ben Dooks <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'
> 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.
>
> Cc: Arnd Bergman <arnd at arndb.de>

Reviewed-by: Arnd Bergman <arnd at arndb.de>

Note: your Signed-off-by seems to be missing.

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

Side note: We should really change the first typeof to an __auto_type
here (and on all other architectures), but that's a separate change
and not strictly a bugfix.

        Arnd