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

[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@xxxxxxxxx>
>> Closes:
>> https://lore.kernel.org/oe-kbuild-all/202311160606.obGOOwB3-lkp@xxxxxxxxx/
>> Signed-off-by: Clément Léger <cleger@xxxxxxxxxxxx>
>> ---
>>   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;
>>       }
>>   
>