Re: [PATCH 2/2] riscv: uaccess: do not do misaligned accesses in get/put_user()

[Alexandre Ghiti]

Hi Clément,

On 6/2/25 09:37, Clément Léger wrote:
> On 31/05/2025 14:35, Alexandre Ghiti wrote:
> > On 5/30/25 22:56, Clément Léger wrote:
> > > Doing misaligned access to userspace memory would make a trap on
> > > platform where it is emulated. Latest fixes removed the kernel
> > > capability to do unaligned accesses to userspace memory safely since
> > > interrupts are kept disabled at all time during that. Thus doing so
> > > would crash the kernel.
> > >
> > > Such behavior was detected with GET_UNALIGN_CTL() that was doing
> > > a put_user() with an unsigned long* address that should have been an
> > > unsigned int*. Reenabling kernel misaligned access emulation is a bit
> > > risky and it would also degrade performances. Rather than doing that,
> > > we will try to avoid any misaligned accessed by using copy_from/to_user()
> > > which does not do any misaligned accesses. This can be done only for
> > > !CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS and thus allows to only generate
> > > a bit more code for this config.
> > >
> > > Signed-off-by: Clément Léger <cleger@xxxxxxxxxxxx>
> > > ---
> > >    arch/riscv/include/asm/uaccess.h | 28 ++++++++++++++++++++++------
> > >    1 file changed, 22 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/
> > > asm/uaccess.h
> > > index 046de7ced09c..b542c05f394f 100644
> > > --- a/arch/riscv/include/asm/uaccess.h
> > > +++ b/arch/riscv/include/asm/uaccess.h
> > > @@ -169,8 +169,21 @@ do {                                \
> > >      #endif /* CONFIG_64BIT */
> > >    +unsigned long __must_check __asm_copy_to_user(void __user *to,
> > > +    const void *from, unsigned long n);
> > > +unsigned long __must_check __asm_copy_from_user(void *to,
> > > +    const void __user *from, unsigned long n);
> > > +
> > >    #define __get_user_nocheck(x, __gu_ptr, label)            \
> > >    do {                                \
> > > +    if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS))
> > > {            \
> > > +        if (!IS_ALIGNED((uintptr_t)__gu_ptr, sizeof(*__gu_ptr)))
> > > {        \
> >
> > Nit: I would use && instead of 2 ifs.
> >
> >
> > > +            if (__asm_copy_from_user(&(x), __gu_ptr,
> > > sizeof(*__gu_ptr)))    \
> > > +                goto label;            \
> > > +            else                    \
> > > +                break;                \
> >
> > Here I would remove the else
> Hi Alex,
>
> The "else" is needed to break from the outer do/while loop or it will go
> though the next switch case (and it will crash due to misaligned accesses).


I meant only the "else", not the "break"!

Thanks,

Alex


> > > +        }                        \
> > > +    }                            \
> > >        switch (sizeof(*__gu_ptr)) {                \
> > >        case 1:                            \
> > >            __get_user_asm("lb", (x), __gu_ptr, label);    \
> > > @@ -297,6 +310,15 @@ do {                                \
> > >      #define __put_user_nocheck(x, __gu_ptr, label)            \
> > >    do {                                \
> > > +    if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS))
> > > {            \
> > > +        if (!IS_ALIGNED((uintptr_t)__gu_ptr, sizeof(*__gu_ptr)))
> > > {        \
> > > +            unsigned long val = (unsigned long)(x);                \
> >
> > Here it sems like __inttype(*(__gu_ptr)) is more accurate than unsigned
> > long, even though I think unsigned long works fine too.
> Wasn't aware of __inttype, but it sounds good.
>
> Thanks,
>
> Clément
>
> > > +            if (__asm_copy_to_user(__gu_ptr, &(val),
> > > sizeof(*__gu_ptr)))    \
> > > +                goto label;            \
> > > +            else                    \
> > > +                break;                \
> > > +        }                        \
> > > +    }                            \
> > >        switch (sizeof(*__gu_ptr)) {                \
> > >        case 1:                            \
> > >            __put_user_asm("sb", (x), __gu_ptr, label);    \
> > > @@ -385,12 +407,6 @@ err_label:                            \
> > >            -EFAULT;                    \
> > >    })
> > >    -
> > > -unsigned long __must_check __asm_copy_to_user(void __user *to,
> > > -    const void *from, unsigned long n);
> > > -unsigned long __must_check __asm_copy_from_user(void *to,
> > > -    const void __user *from, unsigned long n);
> > > -
> > >    static inline unsigned long
> > >    raw_copy_from_user(void *to, const void __user *from, unsigned long n)
> > >    {
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-riscv