Re: [PATCH v2] riscv: fix incorrect use of __user pointer

[Ben Dooks <ben.dooks@codethink.co.uk>]

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?

    			return -1;
>   	}
>   

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html