Messages in this thread | | | Date | Mon, 27 Nov 2023 13:46:31 +0100 | Subject | Re: [PATCH v2] riscv: fix incorrect use of __user pointer | From | Clément Léger <> |
| |
On 27/11/2023 13:02, Ben Dooks wrote: > On 24/11/2023 11:38, Clément Léger wrote: >> These warnings were reported by sparse and were due to lack of __user >> annotation as well as dereferencing such pointer: >> >> arch/riscv/kernel/traps_misaligned.c:361:21: warning: dereference of >> noderef expression >> arch/riscv/kernel/traps_misaligned.c:373:21: warning: dereference of >> noderef expression >> arch/riscv/kernel/traps_misaligned.c:381:21: warning: dereference of >> noderef expression >> arch/riscv/kernel/traps_misaligned.c:322:24: warning: incorrect type >> in initializer (different address spaces) >> arch/riscv/kernel/traps_misaligned.c:322:24: expected unsigned char >> const [noderef] __user *__gu_ptr >> arch/riscv/kernel/traps_misaligned.c:322:24: got unsigned char >> const [usertype] *addr >> arch/riscv/kernel/traps_misaligned.c:361:21: warning: dereference of >> noderef expression >> arch/riscv/kernel/traps_misaligned.c:373:21: warning: dereference of >> noderef expression >> arch/riscv/kernel/traps_misaligned.c:381:21: warning: dereference of >> noderef expression >> arch/riscv/kernel/traps_misaligned.c:332:24: warning: incorrect type >> in initializer (different address spaces) >> arch/riscv/kernel/traps_misaligned.c:332:24: expected unsigned char >> [noderef] __user *__gu_ptr >> arch/riscv/kernel/traps_misaligned.c:332:24: got unsigned char >> [usertype] *addr >> >> As suggested by Christoph Hellwig, casting pointers from an address >> space to another is not a good idea and we should rather cast the >> untyped unsigned long to their final address space. Fix the ones in >> load_u8()/store_u8()/__read_insn() by passing a unsigned long and then >> casting it to the appropriate type (__user of not) depending if used in >> kernel/ user mode. Also remove unneeded else construct in store_u8()/ >> load_u8(). >> >> Reported-by: kernel test robot <lkp@intel.com> >> Closes: >> https://lore.kernel.org/oe-kbuild-all/202311160606.obGOOwB3-lkp@intel.com/ >> Signed-off-by: Clément Léger <cleger@rivosinc.com> >> --- >> arch/riscv/kernel/traps_misaligned.c | 55 +++++++++++++--------------- >> 1 file changed, 25 insertions(+), 30 deletions(-) >> >> diff --git a/arch/riscv/kernel/traps_misaligned.c >> b/arch/riscv/kernel/traps_misaligned.c >> index 5eba37147caa..a92b88af855a 100644 >> --- a/arch/riscv/kernel/traps_misaligned.c >> +++ b/arch/riscv/kernel/traps_misaligned.c >> @@ -265,19 +265,19 @@ static unsigned long get_f32_rs(unsigned long >> insn, u8 fp_reg_offset, >> #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) >> +static inline int load_u8(struct pt_regs *regs, const unsigned long >> addr, u8 *r_val) >> { >> u8 val; >> - asm volatile("lbu %0, %1" : "=&r" (val) : "m" (*addr)); >> + asm volatile("lbu %0, %1" : "=&r" (val) : "m" (*(const u8 *)addr)); >> *r_val = val; >> return 0; >> } >> -static inline int store_u8(struct pt_regs *regs, u8 *addr, u8 val) >> +static inline int store_u8(struct pt_regs *regs, unsigned long addr, >> u8 val) >> { >> - asm volatile ("sb %0, %1\n" : : "r" (val), "m" (*addr)); >> + asm volatile ("sb %0, %1\n" : : "r" (val), "m" (*(u8 *)addr)); >> return 0; >> } >> @@ -316,34 +316,32 @@ static inline int get_insn(struct pt_regs *regs, >> ulong mepc, ulong *r_insn) >> return 0; >> } >> #else >> -static inline int load_u8(struct pt_regs *regs, const u8 *addr, u8 >> *r_val) >> +static inline int load_u8(struct pt_regs *regs, const unsigned long >> addr, u8 *r_val) >> { >> - if (user_mode(regs)) { >> - return __get_user(*r_val, addr); >> - } else { >> - *r_val = *addr; >> - return 0; >> - } >> + if (user_mode(regs)) >> + return __get_user(*r_val, (u8 __user *)addr); >> + >> + *r_val = *(const u8 *)addr; >> + return 0; >> } >> -static inline int store_u8(struct pt_regs *regs, u8 *addr, u8 val) >> +static inline int store_u8(struct pt_regs *regs, unsigned long addr, >> u8 val) >> { >> - if (user_mode(regs)) { >> - return __put_user(val, addr); >> - } else { >> - *addr = val; >> - return 0; >> - } >> + if (user_mode(regs)) >> + return __put_user(val, (u8 __user *)addr); >> + >> + *(u8 *)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); \ >> + __ret = __get_user(insn, (type __user *) insn_addr); \ >> } else { \ >> - insn = *insn_addr; \ >> + insn = *(type *)insn_addr; \ >> __ret = 0; \ >> } \ >> \ >> @@ -356,9 +354,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 +366,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; >> @@ -491,7 +486,7 @@ int handle_misaligned_load(struct pt_regs *regs) >> val.data_u64 = 0; >> for (i = 0; i < len; i++) { >> - if (load_u8(regs, (void *)(addr + i), &val.data_bytes[i])) >> + if (load_u8(regs, addr + i, &val.data_bytes[i])) >> return -1; >> } >> @@ -589,7 +584,7 @@ int handle_misaligned_store(struct pt_regs *regs) >> return -EOPNOTSUPP; >> for (i = 0; i < len; i++) { >> - if (store_u8(regs, (void *)(addr + i), val.data_bytes[i])) >> + if (store_u8(regs, addr + i, val.data_bytes[i])) >> > > Would it not be easier to have a switch here for memcpy or copy_to_user?
Most probably yes. I'll update the V3 with these modifications. We'll get rid of store_u8()/load_u8() entirely.
Thanks,
Clément
> > return -1; >> } >> >
| |