Re: [PATCH] riscv: fix __user annotation in traps_misaligned.c

From: Clément Léger
Date: Fri Nov 24 2023 - 05:21:32 EST


Hi Ben,

I sent a similar patch two days ago
(https://lore.kernel.org/linux-riscv/20231122135141.2936663-1-cleger@xxxxxxxxxxxx/).

On 23/11/2023 15:16, Ben Dooks wrote:
> The instruction reading code can read from either user or kernel addresses
> and thus the use of __user on pointers to instructions depends on which
> context. Fix a few sparse warnings by using __user for user-accesses and
> remove it when not.
>
> Fixes:
>
> 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
>
> Fixes: 7c83232161f60 ("riscv: add support for misaligned trap handling in S-mode")
> Signed-off-by: Ben Dooks <ben.dooks@xxxxxxxxxxxxxxx>
> ---
> arch/riscv/kernel/traps_misaligned.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/riscv/kernel/traps_misaligned.c b/arch/riscv/kernel/traps_misaligned.c
> index 5eba37147caa..446e3d4eeea9 100644
> --- a/arch/riscv/kernel/traps_misaligned.c
> +++ b/arch/riscv/kernel/traps_misaligned.c
> @@ -319,7 +319,7 @@ static inline int get_insn(struct pt_regs *regs, ulong mepc, ulong *r_insn)
> 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);
> + return __get_user(*r_val, (u8 __user *)addr);
> } else {
> *r_val = *addr;
> return 0;
> @@ -329,7 +329,7 @@ static inline int load_u8(struct pt_regs *regs, const u8 *addr, u8 *r_val)
> static inline int store_u8(struct pt_regs *regs, u8 *addr, u8 val)
> {
> if (user_mode(regs)) {
> - return __put_user(val, addr);
> + return __put_user(val, (u8 __user *)addr);
> } else {
> *addr = val;
> return 0;
> @@ -343,7 +343,7 @@ static inline int store_u8(struct pt_regs *regs, u8 *addr, u8 val)
> if (user_mode(regs)) { \
> __ret = __get_user(insn, insn_addr); \
> } else { \
> - insn = *insn_addr; \
> + insn = *(__force u16 *)insn_addr; \

__read_insn() is called with either a u32 or a u16 pointer which is why
this macros did not used a specific type. Doing so would result in
loading half of what is needed. My patch addresses that.

Thanks

> __ret = 0; \
> } \
> \