| 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
| ||
|
Message-ID: <a1150501-d15c-4844-b5d9-39ded3979e2d@ghiti.fr>
Date: Tue, 22 Apr 2025 12:36:28 +0200
From: Alexandre Ghiti <alex@...ti.fr>
To: Cyril Bur <cyrilbur@...storrent.com>, palmer@...belt.com,
aou@...s.berkeley.edu, paul.walmsley@...ive.com, charlie@...osinc.com,
jrtc27@...c27.com, ben.dooks@...ethink.co.uk
Cc: linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
jszhang@...nel.org
Subject: Re: [PATCH v6 4/5] riscv: uaccess: use 'asm goto' for put_user()
On 10/04/2025 09:05, Cyril Bur wrote:
> From: Jisheng Zhang <jszhang@...nel.org>
>
> With 'asm goto' we don't need to test the error etc, the exception just
> jumps to the error handling directly.
>
> Because there are no output clobbers which could trigger gcc bugs [1]
> the use of asm_goto_output() macro is not necessary here. Not using
> asm_goto_output() is desirable as the generated output asm will be
> cleaner.
>
> Use of the volatile keyword is redundant as per gcc 14.2.0 manual section
> 6.48.2.7 Goto Labels:
>> Also note that an asm goto statement is always implicitly considered
> volatile.
>
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113921 # 1
>
> Signed-off-by: Jisheng Zhang <jszhang@...nel.org>
> [Cyril Bur: Rewritten commit message]
> Signed-off-by: Cyril Bur <cyrilbur@...storrent.com>
> ---
> arch/riscv/include/asm/uaccess.h | 71 +++++++++++++++-----------------
> 1 file changed, 33 insertions(+), 38 deletions(-)
>
> diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
> index da36057847f0..719c9179a751 100644
> --- a/arch/riscv/include/asm/uaccess.h
> +++ b/arch/riscv/include/asm/uaccess.h
> @@ -214,61 +214,66 @@ do { \
> ((x) = (__force __typeof__(x))0, -EFAULT); \
> })
>
> -#define __put_user_asm(insn, x, ptr, err) \
> +#define __put_user_asm(insn, x, ptr, label) \
> do { \
> __typeof__(*(ptr)) __x = x; \
> - __asm__ __volatile__ ( \
> + asm goto( \
> "1:\n" \
> - " " insn " %z1, %2\n" \
> - "2:\n" \
> - _ASM_EXTABLE_UACCESS_ERR(1b, 2b, %0) \
> - : "+r" (err) \
> - : "rJ" (__x), "m"(*(ptr))); \
> + " " insn " %z0, %1\n" \
> + _ASM_EXTABLE(1b, %l2) \
> + : : "rJ" (__x), "m"(*(ptr)) : : label); \
> } while (0)
>
> #ifdef CONFIG_64BIT
> -#define __put_user_8(x, ptr, err) \
> - __put_user_asm("sd", x, ptr, err)
> +#define __put_user_8(x, ptr, label) \
> + __put_user_asm("sd", x, ptr, label)
> #else /* !CONFIG_64BIT */
> -#define __put_user_8(x, ptr, err) \
> +#define __put_user_8(x, ptr, label) \
> do { \
> u32 __user *__ptr = (u32 __user *)(ptr); \
> u64 __x = (__typeof__((x)-(x)))(x); \
> - __asm__ __volatile__ ( \
> + asm goto( \
> "1:\n" \
> - " sw %z1, %3\n" \
> + " sw %z0, %2\n" \
> "2:\n" \
> - " sw %z2, %4\n" \
> - "3:\n" \
> - _ASM_EXTABLE_UACCESS_ERR(1b, 3b, %0) \
> - _ASM_EXTABLE_UACCESS_ERR(2b, 3b, %0) \
> - : "+r" (err) \
> - : "rJ" (__x), "rJ" (__x >> 32), \
> + " sw %z1, %3\n" \
> + _ASM_EXTABLE(1b, %l4) \
> + _ASM_EXTABLE(2b, %l4) \
> + : : "rJ" (__x), "rJ" (__x >> 32), \
> "m" (__ptr[__LSW]), \
> - "m" (__ptr[__MSW])); \
> + "m" (__ptr[__MSW]) : : label); \
> } while (0)
> #endif /* CONFIG_64BIT */
>
> -#define __put_user_nocheck(x, __gu_ptr, __pu_err) \
> +#define __put_user_nocheck(x, __gu_ptr, label) \
> do { \
> switch (sizeof(*__gu_ptr)) { \
> case 1: \
> - __put_user_asm("sb", (x), __gu_ptr, __pu_err); \
> + __put_user_asm("sb", (x), __gu_ptr, label); \
> break; \
> case 2: \
> - __put_user_asm("sh", (x), __gu_ptr, __pu_err); \
> + __put_user_asm("sh", (x), __gu_ptr, label); \
> break; \
> case 4: \
> - __put_user_asm("sw", (x), __gu_ptr, __pu_err); \
> + __put_user_asm("sw", (x), __gu_ptr, label); \
> break; \
> case 8: \
> - __put_user_8((x), __gu_ptr, __pu_err); \
> + __put_user_8((x), __gu_ptr, label); \
> break; \
> default: \
> BUILD_BUG(); \
> } \
> } while (0)
>
> +#define __put_user_error(x, ptr, err) \
> +do { \
> + __label__ err_label; \
> + __put_user_nocheck(x, ptr, err_label); \
> + break; \
> +err_label: \
> + (err) = -EFAULT; \
> +} while (0)
> +
> /**
> * __put_user: - Write a simple value into user space, with less checking.
> * @x: Value to copy to user space.
> @@ -299,7 +304,7 @@ do { \
> __chk_user_ptr(__gu_ptr); \
> \
> __enable_user_access(); \
> - __put_user_nocheck(__val, __gu_ptr, __pu_err); \
> + __put_user_error(__val, __gu_ptr, __pu_err); \
> __disable_user_access(); \
> \
> __pu_err; \
> @@ -373,13 +378,7 @@ do { \
> } while (0)
>
> #define __put_kernel_nofault(dst, src, type, err_label) \
> -do { \
> - long __kr_err = 0; \
> - \
> - __put_user_nocheck(*((type *)(src)), (type *)(dst), __kr_err); \
> - if (unlikely(__kr_err)) \
> - goto err_label; \
> -} while (0)
> + __put_user_nocheck(*((type *)(src)), (type *)(dst), err_label)
>
> static __must_check __always_inline bool user_access_begin(const void __user *ptr, size_t len)
> {
> @@ -398,12 +397,8 @@ 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_put_user(x, ptr, label) \
> + __put_user_nocheck(x, (ptr), label)
>
> #define unsafe_get_user(x, ptr, label) do { \
> long __err = 0; \
Reviewed-by: Alexandre Ghiti <alexghiti@...osinc.com>
Thanks,
Alex
Powered by blists - more mailing lists