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 <[email protected]>
Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
Signed-off-by: Clément Léger <[email protected]>
---
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]))
return -1;
}
--
2.42.0
On Fri, Nov 24, 2023 at 12:38:03PM +0100, Cl?ment L?ger wrote:
>
> #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)
Please avoid the overly long line here. Or just drop the const as that
is rather pointless for a scalaer (unlike the pointer).
Otherwise looks good:
Reviewed-by: Christoph Hellwig <[email protected]>
...
> @@ -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;
> }
I'd really have thought that you'd want to pull the kernel/user
check way outside the loop?
In any case, for a misaligned read why not just read (addr & ~7)[0]
and (if needed) (addr & ~7)[1] and then ahift and or together?
clang will do it for misaligned structure members with known
misalignment, but it is almost certainly also better for reads
with unknown misalignment.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On 25/11/2023 16:37, David Laight wrote:
> ...
>> @@ -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;
>> }
>
> I'd really have thought that you'd want to pull the kernel/user
> check way outside the loop?
Hi David,
I hope the compiler is able to extract that 'if' out of the loop since
regs isn't modified in the loop. Nevertheless, that could be more
"clear" if put outside indeed.
> In any case, for a misaligned read why not just read (addr & ~7)[0]
> and (if needed) (addr & ~7)[1] and then ahift and or together?
Makes sense, the original code was like that but probably copy/pasted
from openSBI I guess. (?)
Let's keep that for the moment (this patch is about fixing wrong __user
address space). I will try to submit another series using what you proposed.
Regards,
Clément
>
> clang will do it for misaligned structure members with known
> misalignment, but it is almost certainly also better for reads
> with unknown misalignment.
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
From: Clément Léger
> Sent: 27 November 2023 10:24
>
> On 25/11/2023 16:37, David Laight wrote:
> > ...
> >> @@ -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;
> >> }
> >
> > I'd really have thought that you'd want to pull the kernel/user
> > check way outside the loop?
>
> Hi David,
>
> I hope the compiler is able to extract that 'if' out of the loop since
> regs isn't modified in the loop. Nevertheless, that could be more
> "clear" if put outside indeed.
If has access regs->xxx then the compiler can't do so because it
will must assume that the assignment might alias into 'regs'.
That is even true for byte writes if 'strict-aliasing' is enabled
- which it isn't for linux kernel builds.
It might do so if 'regs' were 'const'; it tends to assume that if
it can't change something nothing can - although that isn't true.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On 27/11/2023 11:35, David Laight wrote:
> From: Clément Léger
>> Sent: 27 November 2023 10:24
>>
>> On 25/11/2023 16:37, David Laight wrote:
>>> ...
>>>> @@ -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;
>>>> }
>>>
>>> I'd really have thought that you'd want to pull the kernel/user
>>> check way outside the loop?
>>
>> Hi David,
>>
>> I hope the compiler is able to extract that 'if' out of the loop since
>> regs isn't modified in the loop. Nevertheless, that could be more
>> "clear" if put outside indeed.
>
> If has access regs->xxx then the compiler can't do so because it
> will must assume that the assignment might alias into 'regs'.
> That is even true for byte writes if 'strict-aliasing' is enabled
> - which it isn't for linux kernel builds.
>
> It might do so if 'regs' were 'const'; it tends to assume that if
> it can't change something nothing can - although that isn't true.
Ok, good to know ! As I said, I'll modify that in a subsequent patch.
Thanks,
Clément
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
From: Clément Léger
> Sent: 27 November 2023 10:37
>
> On 27/11/2023 11:35, David Laight wrote:
> > From: Clément Léger
> >> Sent: 27 November 2023 10:24
> >>
> >> On 25/11/2023 16:37, David Laight wrote:
> >>> ...
> >>>> @@ -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;
> >>>> }
> >>>
> >>> I'd really have thought that you'd want to pull the kernel/user
> >>> check way outside the loop?
> >>
> >> Hi David,
> >>
> >> I hope the compiler is able to extract that 'if' out of the loop since
> >> regs isn't modified in the loop. Nevertheless, that could be more
> >> "clear" if put outside indeed.
> >
> > If has access regs->xxx then the compiler can't do so because it
> > will must assume that the assignment might alias into 'regs'.
> > That is even true for byte writes if 'strict-aliasing' is enabled
> > - which it isn't for linux kernel builds.
> >
> > It might do so if 'regs' were 'const'; it tends to assume that if
> > it can't change something nothing can - although that isn't true.
>
> Ok, good to know ! As I said, I'll modify that in a subsequent patch.
Actually the following loops will (probably) generate much better code:
// Read kernel
val = 0;
for (i = 0; i < len; i++)
val |= addr[i] << (i * 8);
// write kernel
for (i = 0; i < len; i++, val >>= 8)
addr[i] = val;
For user using __get/put_user() as appropriate.
I think there is a 'goto' variant of the user access functions
that probably make the code clearer.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
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 <[email protected]>
>> Closes:
>> https://lore.kernel.org/oe-kbuild-all/[email protected]/
>> Signed-off-by: Clément Léger <[email protected]>
>> ---
>> 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;
>> }
>>
>
On 27/11/2023 13:46, Clément Léger wrote:
>
>
>>> @@ -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.
While memcpy/copy_from_user will be slower than David Laight proposed
solution (read two 4/8 bytes long once and shifting the content) it is
more maintainable and this path of code will not suffer a few lost cycle
(emulating misaligned access is already horribly slow...).
BTW, need to check but maybe we can get rid of all the specific M_MODE
code entirely (__read_insn) also.
Clément
>
> Thanks,
>
> Clément
>
>>
>> return -1;
>>> }
>>>
>>