| 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: <11229bf2-01f4-4219-b358-d5db57038838@rivosinc.com>
Date: Thu, 1 Feb 2024 14:46:30 +0100
From: Clément Léger <cleger@...osinc.com>
To: Paul Walmsley <paul.walmsley@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>,
linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org
Cc: Christoph Hellwig <hch@...radead.org>,
Ben Dooks <ben.dooks@...ethink.co.uk>, David Laight <David.Laight@...LAB.COM>
Subject: Re: [PATCH] riscv: misaligned: remove CONFIG_RISCV_M_MODE specific
code
Ping ?
On 28/11/2023 17:52, Clément Léger wrote:
> While reworking code to fix sparse errors, it appears that the
> RISCV_M_MODE specific could actually be removed and use the one for
> normal mode. Even though RISCV_M_MODE can do direct user memory access,
> using the user uaccess helpers is also going to work. Since there is no
> need anymore for specific accessors (load_u8()/store_u8()), we can
> directly use memcpy()/copy_{to/from}_user() and get rid of the copy
> loop entirely. __read_insn() is also fixed to use an unsigned long
> instead of a pointer which was cast in __user address space. The
> insn_addr parameter is now cast from unsigned long to the correct
> address space directly.
>
> Signed-off-by: Clément Léger <cleger@...osinc.com>
>
> ---
>
> Notes: This patch is a complete rework of a previous one [1] and thus is
> not a V3.
>
> [1] https://lore.kernel.org/linux-riscv/d156242a-f104-4925-9736-624a4ba8210d@rivosinc.com/
> ---
> arch/riscv/kernel/traps_misaligned.c | 108 +++++----------------------
> 1 file changed, 18 insertions(+), 90 deletions(-)
>
> diff --git a/arch/riscv/kernel/traps_misaligned.c b/arch/riscv/kernel/traps_misaligned.c
> index 5eba37147caa..38af97f4819c 100644
> --- a/arch/riscv/kernel/traps_misaligned.c
> +++ b/arch/riscv/kernel/traps_misaligned.c
> @@ -264,86 +264,14 @@ static unsigned long get_f32_rs(unsigned long insn, u8 fp_reg_offset,
> #define GET_F32_RS2C(insn, regs) (get_f32_rs(insn, 2, regs))
> #define GET_F32_RS2S(insn, regs) (get_f32_rs(RVC_RS2S(insn), 0, regs))
>
> -#ifdef CONFIG_RISCV_M_MODE
> -static inline int load_u8(struct pt_regs *regs, const u8 *addr, u8 *r_val)
> -{
> - u8 val;
> -
> - asm volatile("lbu %0, %1" : "=&r" (val) : "m" (*addr));
> - *r_val = val;
> -
> - return 0;
> -}
> -
> -static inline int store_u8(struct pt_regs *regs, u8 *addr, u8 val)
> -{
> - asm volatile ("sb %0, %1\n" : : "r" (val), "m" (*addr));
> -
> - return 0;
> -}
> -
> -static inline int get_insn(struct pt_regs *regs, ulong mepc, ulong *r_insn)
> -{
> - register ulong __mepc asm ("a2") = mepc;
> - ulong val, rvc_mask = 3, tmp;
> -
> - asm ("and %[tmp], %[addr], 2\n"
> - "bnez %[tmp], 1f\n"
> -#if defined(CONFIG_64BIT)
> - __stringify(LWU) " %[insn], (%[addr])\n"
> -#else
> - __stringify(LW) " %[insn], (%[addr])\n"
> -#endif
> - "and %[tmp], %[insn], %[rvc_mask]\n"
> - "beq %[tmp], %[rvc_mask], 2f\n"
> - "sll %[insn], %[insn], %[xlen_minus_16]\n"
> - "srl %[insn], %[insn], %[xlen_minus_16]\n"
> - "j 2f\n"
> - "1:\n"
> - "lhu %[insn], (%[addr])\n"
> - "and %[tmp], %[insn], %[rvc_mask]\n"
> - "bne %[tmp], %[rvc_mask], 2f\n"
> - "lhu %[tmp], 2(%[addr])\n"
> - "sll %[tmp], %[tmp], 16\n"
> - "add %[insn], %[insn], %[tmp]\n"
> - "2:"
> - : [insn] "=&r" (val), [tmp] "=&r" (tmp)
> - : [addr] "r" (__mepc), [rvc_mask] "r" (rvc_mask),
> - [xlen_minus_16] "i" (XLEN_MINUS_16));
> -
> - *r_insn = val;
> -
> - return 0;
> -}
> -#else
> -static inline int load_u8(struct pt_regs *regs, const u8 *addr, u8 *r_val)
> -{
> - if (user_mode(regs)) {
> - return __get_user(*r_val, addr);
> - } else {
> - *r_val = *addr;
> - return 0;
> - }
> -}
> -
> -static inline int store_u8(struct pt_regs *regs, u8 *addr, u8 val)
> -{
> - if (user_mode(regs)) {
> - return __put_user(val, addr);
> - } else {
> - *addr = val;
> - return 0;
> - }
> -}
> -
> -#define __read_insn(regs, insn, insn_addr) \
> +#define __read_insn(regs, insn, insn_addr, type) \
> ({ \
> int __ret; \
> \
> - if (user_mode(regs)) { \
> - __ret = __get_user(insn, insn_addr); \
> + if (user_mode(regs)) { \
> + __ret = __get_user(insn, (type __user *) insn_addr); \
> } else { \
> - insn = *insn_addr; \
> + insn = *(type *)insn_addr; \
> __ret = 0; \
> } \
> \
> @@ -356,9 +284,8 @@ static inline int get_insn(struct pt_regs *regs, ulong epc, ulong *r_insn)
>
> if (epc & 0x2) {
> ulong tmp = 0;
> - u16 __user *insn_addr = (u16 __user *)epc;
>
> - if (__read_insn(regs, insn, insn_addr))
> + if (__read_insn(regs, insn, epc, u16))
> return -EFAULT;
> /* __get_user() uses regular "lw" which sign extend the loaded
> * value make sure to clear higher order bits in case we "or" it
> @@ -369,16 +296,14 @@ static inline int get_insn(struct pt_regs *regs, ulong epc, ulong *r_insn)
> *r_insn = insn;
> return 0;
> }
> - insn_addr++;
> - if (__read_insn(regs, tmp, insn_addr))
> + epc += sizeof(u16);
> + if (__read_insn(regs, tmp, epc, u16))
> return -EFAULT;
> *r_insn = (tmp << 16) | insn;
>
> return 0;
> } else {
> - u32 __user *insn_addr = (u32 __user *)epc;
> -
> - if (__read_insn(regs, insn, insn_addr))
> + if (__read_insn(regs, insn, epc, u32))
> return -EFAULT;
> if ((insn & __INSN_LENGTH_MASK) == __INSN_LENGTH_32) {
> *r_insn = insn;
> @@ -390,7 +315,6 @@ static inline int get_insn(struct pt_regs *regs, ulong epc, ulong *r_insn)
> return 0;
> }
> }
> -#endif
>
> union reg_data {
> u8 data_bytes[8];
> @@ -409,7 +333,7 @@ int handle_misaligned_load(struct pt_regs *regs)
> unsigned long epc = regs->epc;
> unsigned long insn;
> unsigned long addr = regs->badaddr;
> - int i, fp = 0, shift = 0, len = 0;
> + int fp = 0, shift = 0, len = 0;
>
> perf_sw_event(PERF_COUNT_SW_ALIGNMENT_FAULTS, 1, regs, addr);
>
> @@ -490,9 +414,11 @@ int handle_misaligned_load(struct pt_regs *regs)
> return -EOPNOTSUPP;
>
> val.data_u64 = 0;
> - for (i = 0; i < len; i++) {
> - if (load_u8(regs, (void *)(addr + i), &val.data_bytes[i]))
> + if (user_mode(regs)) {
> + if (raw_copy_from_user(&val, (u8 __user *)addr, len))
> return -1;
> + } else {
> + memcpy(&val, (u8 *)addr, len);
> }
>
> if (!fp)
> @@ -513,7 +439,7 @@ int handle_misaligned_store(struct pt_regs *regs)
> unsigned long epc = regs->epc;
> unsigned long insn;
> unsigned long addr = regs->badaddr;
> - int i, len = 0, fp = 0;
> + int len = 0, fp = 0;
>
> perf_sw_event(PERF_COUNT_SW_ALIGNMENT_FAULTS, 1, regs, addr);
>
> @@ -588,9 +514,11 @@ int handle_misaligned_store(struct pt_regs *regs)
> if (!IS_ENABLED(CONFIG_FPU) && fp)
> return -EOPNOTSUPP;
>
> - for (i = 0; i < len; i++) {
> - if (store_u8(regs, (void *)(addr + i), val.data_bytes[i]))
> + if (user_mode(regs)) {
> + if (raw_copy_to_user((u8 __user *)addr, &val, len))
> return -1;
> + } else {
> + memcpy((u8 *)addr, &val, len);
> }
>
> regs->epc = epc + INSN_LEN(insn);
Powered by blists - more mailing lists