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

[Cyril Bur]

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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Signed-off-by: Cyril Bur<cyrilbur@xxxxxxxxxxxxxxx>
> > ---
> >   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