linux-kernel - Re: [PATCH 2/2] riscv: uaccess: do not do misaligned accesses in get/put

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <718fb1d7-9f0c-4637-abe8-5a34305c3ae2@ghiti.fr>
Date: Sat, 31 May 2025 14:35:58 +0200
From: Alexandre Ghiti <alex@...ti.fr>
To: Clément Léger <cleger@...osinc.com>,
 linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org
Cc: Paul Walmsley <paul.walmsley@...ive.com>,
 Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>
Subject: Re: [PATCH 2/2] riscv: uaccess: do not do misaligned accesses in
 get/put_user()

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@...osinc.com>
> ---
>   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


> +		}						\
> +	}							\
>   	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.


> +			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)
>   {