Re: [EXT] Re: [PATCH v3 1/4] riscv: implement user_access_begin() and families

[Cyril Bur <cyrilbur@tenstorrent.com>]

On 15/3/2025 12:28 am, Alexandre Ghiti wrote:
> Hi Cyril,
> 
> On 21/02/2025 01:09, Cyril Bur wrote:
>> From: Jisheng Zhang<jszhang@kernel.org>
>>
>> Currently, when a function like strncpy_from_user() is called,
>> the userspace access protection is disabled and enabled
>> for every word read.
>>
>> By implementing user_access_begin() and families, the protection
>> is disabled at the beginning of the copy and enabled at the end.
>>
>> The __inttype macro is borrowed from x86 implementation.
>>
>> Signed-off-by: Jisheng Zhang<jszhang@kernel.org>
>> Signed-off-by: Cyril Bur<cyrilbur@tenstorrent.com>
>> ---
>>   arch/riscv/include/asm/uaccess.h | 63 ++++++++++++++++++++++++++++++++
>>   1 file changed, 63 insertions(+)
>>
>> diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/ 
>> asm/uaccess.h
>> index fee56b0c8058..43db1d9c2f99 100644
>> --- a/arch/riscv/include/asm/uaccess.h
>> +++ b/arch/riscv/include/asm/uaccess.h
>> @@ -61,6 +61,19 @@ static inline unsigned long 
>> __untagged_addr_remote(struct mm_struct *mm, unsigne
>>   #define __disable_user_access()                            \
>>       __asm__ __volatile__ ("csrc sstatus, %0" : : "r" (SR_SUM) : 
>> "memory")
>> +/*
>> + * This is the smallest unsigned integer type that can fit a value
>> + * (up to 'long long')
>> + */
>> +#define __inttype(x) __typeof__(        \
>> +    __typefits(x,char,            \
>> +      __typefits(x,short,            \
>> +        __typefits(x,int,            \
>> +          __typefits(x,long,0ULL)))))
>> +
>> +#define __typefits(x,type,not) \
>> +    __builtin_choose_expr(sizeof(x)<=sizeof(type),(unsigned type)0,not)
>> +
>>   /*
>>    * The exception table consists of pairs of addresses: the first is the
>>    * address of an instruction that is allowed to fault, and the 
>> second is
>> @@ -368,6 +381,56 @@ do {                                    \
>>           goto err_label;                        \
>>   } while (0)
>> +static __must_check __always_inline bool user_access_begin(const void 
>> __user *ptr, size_t len)
>> +{
>> +    if (unlikely(!access_ok(ptr,len)))
>> +        return 0;
>> +    __enable_user_access();
>> +    return 1;
>> +}
>> +#define user_access_begin(a,b)    user_access_begin(a,b)
> 
> 
> Nit: no need for (a,b) here
> 
> 
>> +#define user_access_end()    __disable_user_access()
>> +
>> +static inline unsigned long user_access_save(void) { return 0UL; }
>> +static inline void user_access_restore(unsigned long enabled) { }
>> +
>> +/*
>> + * We want the unsafe accessors to always be inlined and use
>> + * the error labels - thus the macro games.
>> + */
>> +#define unsafe_put_user(x, ptr, label)    do {                \
>> +    long __err = 0;                            \
>> +    __put_user_nocheck(x, (ptr), __err);                \
>> +    if (__err) goto label;                        \
>> +} while (0)
>> +
>> +#define unsafe_get_user(x, ptr, label)    do {                \
>> +    long __err = 0;                            \
>> +    __inttype(*(ptr)) __gu_val;                    \
>> +    __get_user_nocheck(__gu_val, (ptr), __err);            \
>> +    (x) = (__force __typeof__(*(ptr)))__gu_val;            \
>> +    if (__err) goto label;                        \
>> +} while (0)
>> +
>> +#define unsafe_copy_loop(dst, src, len, type, label)                \
>> +    while (len >= sizeof(type)) {                        \
>> +        unsafe_put_user(*(type *)(src),(type __user *)(dst),label);    \
>> +        dst += sizeof(type);                        \
>> +        src += sizeof(type);                        \
>> +        len -= sizeof(type);                        \
>> +    }
>> +
>> +#define unsafe_copy_to_user(_dst,_src,_len,label)            \
>> +do {                                    \
>> +    char __user *__ucu_dst = (_dst);                \
>> +    const char *__ucu_src = (_src);                    \
>> +    size_t __ucu_len = (_len);                    \
>> +    unsafe_copy_loop(__ucu_dst, __ucu_src, __ucu_len, u64, label);    \
>> +    unsafe_copy_loop(__ucu_dst, __ucu_src, __ucu_len, u32, label);    \
>> +    unsafe_copy_loop(__ucu_dst, __ucu_src, __ucu_len, u16, label);    \
>> +    unsafe_copy_loop(__ucu_dst, __ucu_src, __ucu_len, u8, label);    \
>> +} while (0)
>> +
>>   #else /* CONFIG_MMU */
>>   #include <asm-generic/uaccess.h>
>>   #endif /* CONFIG_MMU */
> 
> There is a bunch of checkpatch errors to fix, see https:// 
> gist.github.com/linux-riscv-bot/98f23fd1b04d6da7c23c6cb18245a158
> 

Oops, yeah will fix.

> Why isn't there an implementation for unsafe_copy_from_user()? Let's 
> take the following example:
> 
> user_access_begin()
> unsafe_copy_from_user()
> unsafe_get_user() <==== This one will fail since unsafe_copy_from_user() 
> -> raw_copy_from_user() -> __asm_vector_usercopy() which enables and 
> disables the SUM bit.
> user_access_end()

I'll have to look into that - thanks for the feedback.

> 
> Another thing is that with this patch, we lose the vectorized user 
> access functions, can you fix that too?

I'll have a look at this also.

> 
> Thanks,
> 
> Alex
> 
> 
>