riscv: fix __user annotation in traps_misaligned.c

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

Posted by Ben Dooks 2 years, 1 month ago

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@codethink.co.uk>
---
 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;	\
 		__ret = 0;				\
 	}						\
 							\
-- 
2.37.2.352.g3c44437643

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

Posted by Clément Léger 2 years, 1 month ago

Hi Ben,

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

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@codethink.co.uk>
> ---
>  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;				\
>  	}						\
>  							\

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

Posted by Christoph Hellwig 2 years, 1 month ago

On Thu, Nov 23, 2023 at 02:16:17PM +0000, Ben Dooks wrote:
> @@ -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;

This is the wrong way to approach it.  Pass the untype unsigned long
from the caller instead and do a single round of casts from that
depending on the address_space.

And please also remove this horrible else after return entipattern
while you're at it.

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

Posted by Clément Léger 2 years, 1 month ago


On 24/11/2023 07:05, Christoph Hellwig wrote:
> On Thu, Nov 23, 2023 at 02:16:17PM +0000, Ben Dooks wrote:
>> @@ -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;
> 
> This is the wrong way to approach it.  Pass the untype unsigned long
> from the caller instead and do a single round of casts from that
> depending on the address_space.

I sent a similar patch two days ago with the same modification. I'm not
sure to get it. Why is it better to pass the "unsigned long" type from
the caller ? I mean, the resulting code would look like this right ?

static inline int store_u8(struct pt_regs *regs, unsigned long addr, u8 val)
{
	if (user_mode(regs)) {
		return __put_user(val, (u8 __user *)addr);
	} else {
		*addr = (u8 *)val;
		return 0;
	}
}

Is this better from a "semantic" point of view and be sure the casts are
done in a single place ?

> 
> And please also remove this horrible else after return entipattern
> while you're at it.

Acked,

Thanks,

> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

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

Posted by Christoph Hellwig 2 years, 1 month ago

On Fri, Nov 24, 2023 at 11:28:08AM +0100, Clément Léger wrote:
> I sent a similar patch two days ago with the same modification. I'm not
> sure to get it. Why is it better to pass the "unsigned long" type from
> the caller ? I mean, the resulting code would look like this right ?

Because you're legimitizing casting between address_space, which is a
horrible idea.  By casting either from the unsigned long you make it
very clear that deep magic is coming in and you make an informed
decisions based on the user_mode() predicate.  Witht a blind cast
to add/remove a __user you don't.

I'm actually surprised sparse even allows __user casts without __force.

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

Posted by Clément Léger 2 years, 1 month ago


On 24/11/2023 11:45, Christoph Hellwig wrote:
> On Fri, Nov 24, 2023 at 11:28:08AM +0100, Clément Léger wrote:
>> I sent a similar patch two days ago with the same modification. I'm not
>> sure to get it. Why is it better to pass the "unsigned long" type from
>> the caller ? I mean, the resulting code would look like this right ?
> 
> Because you're legimitizing casting between address_space, which is a
> horrible idea.  By casting either from the unsigned long you make it
> very clear that deep magic is coming in and you make an informed
> decisions based on the user_mode() predicate.  Witht a blind cast
> to add/remove a __user you don't.

Makes sense indeed, thanks !

Clément

> 
> I'm actually surprised sparse even allows __user casts without __force.