* [PATCH v2 0/2] ARM: Make the dumped instructions are consistent with the disassembled ones
@ 2022-10-10 9:53 ` Zhen Lei
0 siblings, 0 replies; 40+ messages in thread
From: Zhen Lei @ 2022-10-10 9:53 UTC (permalink / raw)
To: Russell King, linux-arm-kernel, linux-kernel; +Cc: Zhen Lei
v1 --> v2:
1. Cleanup sparse warnings.
Zhen Lei (2):
ARM: Fix some check warnings of tool sparse
ARM: Make the dumped instructions are consistent with the disassembled
ones
arch/arm/kernel/traps.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH v2 0/2] ARM: Make the dumped instructions are consistent with the disassembled ones @ 2022-10-10 9:53 ` Zhen Lei 0 siblings, 0 replies; 40+ messages in thread From: Zhen Lei @ 2022-10-10 9:53 UTC (permalink / raw) To: Russell King, linux-arm-kernel, linux-kernel; +Cc: Zhen Lei v1 --> v2: 1. Cleanup sparse warnings. Zhen Lei (2): ARM: Fix some check warnings of tool sparse ARM: Make the dumped instructions are consistent with the disassembled ones arch/arm/kernel/traps.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse 2022-10-10 9:53 ` Zhen Lei @ 2022-10-10 9:53 ` Zhen Lei -1 siblings, 0 replies; 40+ messages in thread From: Zhen Lei @ 2022-10-10 9:53 UTC (permalink / raw) To: Russell King, linux-arm-kernel, linux-kernel; +Cc: Zhen Lei Fix the following warnings: warning: incorrect type in initializer (different address spaces) expected unsigned short [noderef] __user *register __p got unsigned short [usertype] * warning: cast removes address space '__user' of expression Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> --- arch/arm/kernel/traps.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c index 20b2db6dcd1ced7..34aa80c09c508c1 100644 --- a/arch/arm/kernel/traps.c +++ b/arch/arm/kernel/traps.c @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) } } else { if (thumb) - bad = get_user(val, &((u16 *)addr)[i]); + bad = get_user(val, &((u16 __user *)addr)[i]); else - bad = get_user(val, &((u32 *)addr)[i]); + bad = get_user(val, &((u32 __user *)addr)[i]); } if (!bad) @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) if (processor_mode(regs) == SVC_MODE) { #ifdef CONFIG_THUMB2_KERNEL if (thumb_mode(regs)) { - instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); + instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]); if (is_wide_instruction(instr)) { u16 inst2; - inst2 = __mem_to_opcode_thumb16(((u16 *)pc)[1]); + inst2 = __mem_to_opcode_thumb16(((__force u16 *)pc)[1]); instr = __opcode_thumb32_compose(instr, inst2); } } else #endif - instr = __mem_to_opcode_arm(*(u32 *) pc); + instr = __mem_to_opcode_arm(*(__force u32 *) pc); } else if (thumb_mode(regs)) { if (get_user(instr, (u16 __user *)pc)) goto die_sig; -- 2.25.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse @ 2022-10-10 9:53 ` Zhen Lei 0 siblings, 0 replies; 40+ messages in thread From: Zhen Lei @ 2022-10-10 9:53 UTC (permalink / raw) To: Russell King, linux-arm-kernel, linux-kernel; +Cc: Zhen Lei Fix the following warnings: warning: incorrect type in initializer (different address spaces) expected unsigned short [noderef] __user *register __p got unsigned short [usertype] * warning: cast removes address space '__user' of expression Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> --- arch/arm/kernel/traps.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c index 20b2db6dcd1ced7..34aa80c09c508c1 100644 --- a/arch/arm/kernel/traps.c +++ b/arch/arm/kernel/traps.c @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) } } else { if (thumb) - bad = get_user(val, &((u16 *)addr)[i]); + bad = get_user(val, &((u16 __user *)addr)[i]); else - bad = get_user(val, &((u32 *)addr)[i]); + bad = get_user(val, &((u32 __user *)addr)[i]); } if (!bad) @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) if (processor_mode(regs) == SVC_MODE) { #ifdef CONFIG_THUMB2_KERNEL if (thumb_mode(regs)) { - instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); + instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]); if (is_wide_instruction(instr)) { u16 inst2; - inst2 = __mem_to_opcode_thumb16(((u16 *)pc)[1]); + inst2 = __mem_to_opcode_thumb16(((__force u16 *)pc)[1]); instr = __opcode_thumb32_compose(instr, inst2); } } else #endif - instr = __mem_to_opcode_arm(*(u32 *) pc); + instr = __mem_to_opcode_arm(*(__force u32 *) pc); } else if (thumb_mode(regs)) { if (get_user(instr, (u16 __user *)pc)) goto die_sig; -- 2.25.1 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse 2022-10-10 9:53 ` Zhen Lei @ 2022-10-10 10:20 ` Ard Biesheuvel -1 siblings, 0 replies; 40+ messages in thread From: Ard Biesheuvel @ 2022-10-10 10:20 UTC (permalink / raw) To: Zhen Lei; +Cc: Russell King, linux-arm-kernel, linux-kernel On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote: > > Fix the following warnings: > warning: incorrect type in initializer (different address spaces) > expected unsigned short [noderef] __user *register __p > got unsigned short [usertype] * > warning: cast removes address space '__user' of expression > > Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > --- > arch/arm/kernel/traps.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > index 20b2db6dcd1ced7..34aa80c09c508c1 100644 > --- a/arch/arm/kernel/traps.c > +++ b/arch/arm/kernel/traps.c > @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) > } > } else { > if (thumb) > - bad = get_user(val, &((u16 *)addr)[i]); > + bad = get_user(val, &((u16 __user *)addr)[i]); > else > - bad = get_user(val, &((u32 *)addr)[i]); > + bad = get_user(val, &((u32 __user *)addr)[i]); > } > > if (!bad) > @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) > if (processor_mode(regs) == SVC_MODE) { > #ifdef CONFIG_THUMB2_KERNEL > if (thumb_mode(regs)) { > - instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); > + instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]); Shouldn't this be __user as well? (and below) > if (is_wide_instruction(instr)) { > u16 inst2; > - inst2 = __mem_to_opcode_thumb16(((u16 *)pc)[1]); > + inst2 = __mem_to_opcode_thumb16(((__force u16 *)pc)[1]); > instr = __opcode_thumb32_compose(instr, inst2); > } > } else > #endif > - instr = __mem_to_opcode_arm(*(u32 *) pc); > + instr = __mem_to_opcode_arm(*(__force u32 *) pc); > } else if (thumb_mode(regs)) { > if (get_user(instr, (u16 __user *)pc)) > goto die_sig; > -- > 2.25.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse @ 2022-10-10 10:20 ` Ard Biesheuvel 0 siblings, 0 replies; 40+ messages in thread From: Ard Biesheuvel @ 2022-10-10 10:20 UTC (permalink / raw) To: Zhen Lei; +Cc: Russell King, linux-arm-kernel, linux-kernel On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote: > > Fix the following warnings: > warning: incorrect type in initializer (different address spaces) > expected unsigned short [noderef] __user *register __p > got unsigned short [usertype] * > warning: cast removes address space '__user' of expression > > Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > --- > arch/arm/kernel/traps.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > index 20b2db6dcd1ced7..34aa80c09c508c1 100644 > --- a/arch/arm/kernel/traps.c > +++ b/arch/arm/kernel/traps.c > @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) > } > } else { > if (thumb) > - bad = get_user(val, &((u16 *)addr)[i]); > + bad = get_user(val, &((u16 __user *)addr)[i]); > else > - bad = get_user(val, &((u32 *)addr)[i]); > + bad = get_user(val, &((u32 __user *)addr)[i]); > } > > if (!bad) > @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) > if (processor_mode(regs) == SVC_MODE) { > #ifdef CONFIG_THUMB2_KERNEL > if (thumb_mode(regs)) { > - instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); > + instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]); Shouldn't this be __user as well? (and below) > if (is_wide_instruction(instr)) { > u16 inst2; > - inst2 = __mem_to_opcode_thumb16(((u16 *)pc)[1]); > + inst2 = __mem_to_opcode_thumb16(((__force u16 *)pc)[1]); > instr = __opcode_thumb32_compose(instr, inst2); > } > } else > #endif > - instr = __mem_to_opcode_arm(*(u32 *) pc); > + instr = __mem_to_opcode_arm(*(__force u32 *) pc); > } else if (thumb_mode(regs)) { > if (get_user(instr, (u16 __user *)pc)) > goto die_sig; > -- > 2.25.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse 2022-10-10 10:20 ` Ard Biesheuvel @ 2022-10-10 10:58 ` Leizhen (ThunderTown) -1 siblings, 0 replies; 40+ messages in thread From: Leizhen (ThunderTown) @ 2022-10-10 10:58 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: Russell King, linux-arm-kernel, linux-kernel On 2022/10/10 18:20, Ard Biesheuvel wrote: > On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote: >> >> Fix the following warnings: >> warning: incorrect type in initializer (different address spaces) >> expected unsigned short [noderef] __user *register __p >> got unsigned short [usertype] * >> warning: cast removes address space '__user' of expression >> >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >> --- >> arch/arm/kernel/traps.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c >> index 20b2db6dcd1ced7..34aa80c09c508c1 100644 >> --- a/arch/arm/kernel/traps.c >> +++ b/arch/arm/kernel/traps.c >> @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) >> } >> } else { >> if (thumb) >> - bad = get_user(val, &((u16 *)addr)[i]); >> + bad = get_user(val, &((u16 __user *)addr)[i]); >> else >> - bad = get_user(val, &((u32 *)addr)[i]); >> + bad = get_user(val, &((u32 __user *)addr)[i]); >> } >> >> if (!bad) >> @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) >> if (processor_mode(regs) == SVC_MODE) { >> #ifdef CONFIG_THUMB2_KERNEL >> if (thumb_mode(regs)) { >> - instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); >> + instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]); > > Shouldn't this be __user as well? (and below) unsigned int instr; void __user *pc; The __user can clear the warning, but a new warning will be generated. instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); ^new ^old arch/arm/kernel/traps.c:473:33: warning: dereference of noderef expression > >> if (is_wide_instruction(instr)) { >> u16 inst2; >> - inst2 = __mem_to_opcode_thumb16(((u16 *)pc)[1]); >> + inst2 = __mem_to_opcode_thumb16(((__force u16 *)pc)[1]); >> instr = __opcode_thumb32_compose(instr, inst2); >> } >> } else >> #endif >> - instr = __mem_to_opcode_arm(*(u32 *) pc); >> + instr = __mem_to_opcode_arm(*(__force u32 *) pc); >> } else if (thumb_mode(regs)) { >> if (get_user(instr, (u16 __user *)pc)) >> goto die_sig; >> -- >> 2.25.1 >> >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > . > -- Regards, Zhen Lei _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse @ 2022-10-10 10:58 ` Leizhen (ThunderTown) 0 siblings, 0 replies; 40+ messages in thread From: Leizhen (ThunderTown) @ 2022-10-10 10:58 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: Russell King, linux-arm-kernel, linux-kernel On 2022/10/10 18:20, Ard Biesheuvel wrote: > On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote: >> >> Fix the following warnings: >> warning: incorrect type in initializer (different address spaces) >> expected unsigned short [noderef] __user *register __p >> got unsigned short [usertype] * >> warning: cast removes address space '__user' of expression >> >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >> --- >> arch/arm/kernel/traps.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c >> index 20b2db6dcd1ced7..34aa80c09c508c1 100644 >> --- a/arch/arm/kernel/traps.c >> +++ b/arch/arm/kernel/traps.c >> @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) >> } >> } else { >> if (thumb) >> - bad = get_user(val, &((u16 *)addr)[i]); >> + bad = get_user(val, &((u16 __user *)addr)[i]); >> else >> - bad = get_user(val, &((u32 *)addr)[i]); >> + bad = get_user(val, &((u32 __user *)addr)[i]); >> } >> >> if (!bad) >> @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) >> if (processor_mode(regs) == SVC_MODE) { >> #ifdef CONFIG_THUMB2_KERNEL >> if (thumb_mode(regs)) { >> - instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); >> + instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]); > > Shouldn't this be __user as well? (and below) unsigned int instr; void __user *pc; The __user can clear the warning, but a new warning will be generated. instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); ^new ^old arch/arm/kernel/traps.c:473:33: warning: dereference of noderef expression > >> if (is_wide_instruction(instr)) { >> u16 inst2; >> - inst2 = __mem_to_opcode_thumb16(((u16 *)pc)[1]); >> + inst2 = __mem_to_opcode_thumb16(((__force u16 *)pc)[1]); >> instr = __opcode_thumb32_compose(instr, inst2); >> } >> } else >> #endif >> - instr = __mem_to_opcode_arm(*(u32 *) pc); >> + instr = __mem_to_opcode_arm(*(__force u32 *) pc); >> } else if (thumb_mode(regs)) { >> if (get_user(instr, (u16 __user *)pc)) >> goto die_sig; >> -- >> 2.25.1 >> >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > . > -- Regards, Zhen Lei ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse 2022-10-10 10:58 ` Leizhen (ThunderTown) @ 2022-10-10 11:06 ` Ard Biesheuvel -1 siblings, 0 replies; 40+ messages in thread From: Ard Biesheuvel @ 2022-10-10 11:06 UTC (permalink / raw) To: Leizhen (ThunderTown); +Cc: Russell King, linux-arm-kernel, linux-kernel On Mon, 10 Oct 2022 at 12:58, Leizhen (ThunderTown) <thunder.leizhen@huawei.com> wrote: > > > > On 2022/10/10 18:20, Ard Biesheuvel wrote: > > On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote: > >> > >> Fix the following warnings: > >> warning: incorrect type in initializer (different address spaces) > >> expected unsigned short [noderef] __user *register __p > >> got unsigned short [usertype] * > >> warning: cast removes address space '__user' of expression > >> > >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > >> --- > >> arch/arm/kernel/traps.c | 10 +++++----- > >> 1 file changed, 5 insertions(+), 5 deletions(-) > >> > >> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > >> index 20b2db6dcd1ced7..34aa80c09c508c1 100644 > >> --- a/arch/arm/kernel/traps.c > >> +++ b/arch/arm/kernel/traps.c > >> @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) > >> } > >> } else { > >> if (thumb) > >> - bad = get_user(val, &((u16 *)addr)[i]); > >> + bad = get_user(val, &((u16 __user *)addr)[i]); > >> else > >> - bad = get_user(val, &((u32 *)addr)[i]); > >> + bad = get_user(val, &((u32 __user *)addr)[i]); > >> } > >> > >> if (!bad) > >> @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) > >> if (processor_mode(regs) == SVC_MODE) { > >> #ifdef CONFIG_THUMB2_KERNEL > >> if (thumb_mode(regs)) { > >> - instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); > >> + instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]); > > > > Shouldn't this be __user as well? (and below) > > unsigned int instr; > void __user *pc; > > The __user can clear the warning, but a new warning will be generated. > > instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); > ^new ^old > > arch/arm/kernel/traps.c:473:33: warning: dereference of noderef expression > This is because dereferencing a __user pointer is not permitted. So this code should be using get_kernel_nofault() here not a plain dereference of PC. So better to fix that properly instead of papering over it with a __force cast just to make sparse happy. > > > >> if (is_wide_instruction(instr)) { > >> u16 inst2; > >> - inst2 = __mem_to_opcode_thumb16(((u16 *)pc)[1]); > >> + inst2 = __mem_to_opcode_thumb16(((__force u16 *)pc)[1]); > >> instr = __opcode_thumb32_compose(instr, inst2); > >> } > >> } else > >> #endif > >> - instr = __mem_to_opcode_arm(*(u32 *) pc); > >> + instr = __mem_to_opcode_arm(*(__force u32 *) pc); > >> } else if (thumb_mode(regs)) { > >> if (get_user(instr, (u16 __user *)pc)) > >> goto die_sig; > >> -- > >> 2.25.1 > >> > >> > >> _______________________________________________ > >> linux-arm-kernel mailing list > >> linux-arm-kernel@lists.infradead.org > >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > . > > > > -- > Regards, > Zhen Lei _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse @ 2022-10-10 11:06 ` Ard Biesheuvel 0 siblings, 0 replies; 40+ messages in thread From: Ard Biesheuvel @ 2022-10-10 11:06 UTC (permalink / raw) To: Leizhen (ThunderTown); +Cc: Russell King, linux-arm-kernel, linux-kernel On Mon, 10 Oct 2022 at 12:58, Leizhen (ThunderTown) <thunder.leizhen@huawei.com> wrote: > > > > On 2022/10/10 18:20, Ard Biesheuvel wrote: > > On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote: > >> > >> Fix the following warnings: > >> warning: incorrect type in initializer (different address spaces) > >> expected unsigned short [noderef] __user *register __p > >> got unsigned short [usertype] * > >> warning: cast removes address space '__user' of expression > >> > >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > >> --- > >> arch/arm/kernel/traps.c | 10 +++++----- > >> 1 file changed, 5 insertions(+), 5 deletions(-) > >> > >> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > >> index 20b2db6dcd1ced7..34aa80c09c508c1 100644 > >> --- a/arch/arm/kernel/traps.c > >> +++ b/arch/arm/kernel/traps.c > >> @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) > >> } > >> } else { > >> if (thumb) > >> - bad = get_user(val, &((u16 *)addr)[i]); > >> + bad = get_user(val, &((u16 __user *)addr)[i]); > >> else > >> - bad = get_user(val, &((u32 *)addr)[i]); > >> + bad = get_user(val, &((u32 __user *)addr)[i]); > >> } > >> > >> if (!bad) > >> @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) > >> if (processor_mode(regs) == SVC_MODE) { > >> #ifdef CONFIG_THUMB2_KERNEL > >> if (thumb_mode(regs)) { > >> - instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); > >> + instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]); > > > > Shouldn't this be __user as well? (and below) > > unsigned int instr; > void __user *pc; > > The __user can clear the warning, but a new warning will be generated. > > instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); > ^new ^old > > arch/arm/kernel/traps.c:473:33: warning: dereference of noderef expression > This is because dereferencing a __user pointer is not permitted. So this code should be using get_kernel_nofault() here not a plain dereference of PC. So better to fix that properly instead of papering over it with a __force cast just to make sparse happy. > > > >> if (is_wide_instruction(instr)) { > >> u16 inst2; > >> - inst2 = __mem_to_opcode_thumb16(((u16 *)pc)[1]); > >> + inst2 = __mem_to_opcode_thumb16(((__force u16 *)pc)[1]); > >> instr = __opcode_thumb32_compose(instr, inst2); > >> } > >> } else > >> #endif > >> - instr = __mem_to_opcode_arm(*(u32 *) pc); > >> + instr = __mem_to_opcode_arm(*(__force u32 *) pc); > >> } else if (thumb_mode(regs)) { > >> if (get_user(instr, (u16 __user *)pc)) > >> goto die_sig; > >> -- > >> 2.25.1 > >> > >> > >> _______________________________________________ > >> linux-arm-kernel mailing list > >> linux-arm-kernel@lists.infradead.org > >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > . > > > > -- > Regards, > Zhen Lei ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse 2022-10-10 11:06 ` Ard Biesheuvel @ 2022-10-10 16:08 ` Russell King (Oracle) -1 siblings, 0 replies; 40+ messages in thread From: Russell King (Oracle) @ 2022-10-10 16:08 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: Leizhen (ThunderTown), linux-arm-kernel, linux-kernel On Mon, Oct 10, 2022 at 01:06:19PM +0200, Ard Biesheuvel wrote: > On Mon, 10 Oct 2022 at 12:58, Leizhen (ThunderTown) > <thunder.leizhen@huawei.com> wrote: > > > > > > > > On 2022/10/10 18:20, Ard Biesheuvel wrote: > > > On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote: > > >> > > >> Fix the following warnings: > > >> warning: incorrect type in initializer (different address spaces) > > >> expected unsigned short [noderef] __user *register __p > > >> got unsigned short [usertype] * > > >> warning: cast removes address space '__user' of expression > > >> > > >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > > >> --- > > >> arch/arm/kernel/traps.c | 10 +++++----- > > >> 1 file changed, 5 insertions(+), 5 deletions(-) > > >> > > >> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > > >> index 20b2db6dcd1ced7..34aa80c09c508c1 100644 > > >> --- a/arch/arm/kernel/traps.c > > >> +++ b/arch/arm/kernel/traps.c > > >> @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) > > >> } > > >> } else { > > >> if (thumb) > > >> - bad = get_user(val, &((u16 *)addr)[i]); > > >> + bad = get_user(val, &((u16 __user *)addr)[i]); > > >> else > > >> - bad = get_user(val, &((u32 *)addr)[i]); > > >> + bad = get_user(val, &((u32 __user *)addr)[i]); > > >> } > > >> > > >> if (!bad) > > >> @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) > > >> if (processor_mode(regs) == SVC_MODE) { > > >> #ifdef CONFIG_THUMB2_KERNEL > > >> if (thumb_mode(regs)) { > > >> - instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); > > >> + instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]); > > > > > > Shouldn't this be __user as well? (and below) > > > > unsigned int instr; > > void __user *pc; > > > > The __user can clear the warning, but a new warning will be generated. > > > > instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); > > ^new ^old > > > > arch/arm/kernel/traps.c:473:33: warning: dereference of noderef expression > > > > This is because dereferencing a __user pointer is not permitted. > > So this code should be using get_kernel_nofault() here not a plain > dereference of PC. So better to fix that properly instead of papering > over it with a __force cast just to make sparse happy. Why? We won't get here unless the PC can be dereferenced. If it's not able to be dereferenced, then we'd be dealing with a prefetch abort. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse @ 2022-10-10 16:08 ` Russell King (Oracle) 0 siblings, 0 replies; 40+ messages in thread From: Russell King (Oracle) @ 2022-10-10 16:08 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: Leizhen (ThunderTown), linux-arm-kernel, linux-kernel On Mon, Oct 10, 2022 at 01:06:19PM +0200, Ard Biesheuvel wrote: > On Mon, 10 Oct 2022 at 12:58, Leizhen (ThunderTown) > <thunder.leizhen@huawei.com> wrote: > > > > > > > > On 2022/10/10 18:20, Ard Biesheuvel wrote: > > > On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote: > > >> > > >> Fix the following warnings: > > >> warning: incorrect type in initializer (different address spaces) > > >> expected unsigned short [noderef] __user *register __p > > >> got unsigned short [usertype] * > > >> warning: cast removes address space '__user' of expression > > >> > > >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > > >> --- > > >> arch/arm/kernel/traps.c | 10 +++++----- > > >> 1 file changed, 5 insertions(+), 5 deletions(-) > > >> > > >> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > > >> index 20b2db6dcd1ced7..34aa80c09c508c1 100644 > > >> --- a/arch/arm/kernel/traps.c > > >> +++ b/arch/arm/kernel/traps.c > > >> @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) > > >> } > > >> } else { > > >> if (thumb) > > >> - bad = get_user(val, &((u16 *)addr)[i]); > > >> + bad = get_user(val, &((u16 __user *)addr)[i]); > > >> else > > >> - bad = get_user(val, &((u32 *)addr)[i]); > > >> + bad = get_user(val, &((u32 __user *)addr)[i]); > > >> } > > >> > > >> if (!bad) > > >> @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) > > >> if (processor_mode(regs) == SVC_MODE) { > > >> #ifdef CONFIG_THUMB2_KERNEL > > >> if (thumb_mode(regs)) { > > >> - instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); > > >> + instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]); > > > > > > Shouldn't this be __user as well? (and below) > > > > unsigned int instr; > > void __user *pc; > > > > The __user can clear the warning, but a new warning will be generated. > > > > instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); > > ^new ^old > > > > arch/arm/kernel/traps.c:473:33: warning: dereference of noderef expression > > > > This is because dereferencing a __user pointer is not permitted. > > So this code should be using get_kernel_nofault() here not a plain > dereference of PC. So better to fix that properly instead of papering > over it with a __force cast just to make sparse happy. Why? We won't get here unless the PC can be dereferenced. If it's not able to be dereferenced, then we'd be dealing with a prefetch abort. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse 2022-10-10 16:08 ` Russell King (Oracle) @ 2022-10-10 16:14 ` Ard Biesheuvel -1 siblings, 0 replies; 40+ messages in thread From: Ard Biesheuvel @ 2022-10-10 16:14 UTC (permalink / raw) To: Russell King (Oracle) Cc: Leizhen (ThunderTown), linux-arm-kernel, linux-kernel On Mon, 10 Oct 2022 at 18:08, Russell King (Oracle) <linux@armlinux.org.uk> wrote: > > On Mon, Oct 10, 2022 at 01:06:19PM +0200, Ard Biesheuvel wrote: > > On Mon, 10 Oct 2022 at 12:58, Leizhen (ThunderTown) > > <thunder.leizhen@huawei.com> wrote: > > > > > > > > > > > > On 2022/10/10 18:20, Ard Biesheuvel wrote: > > > > On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote: > > > >> > > > >> Fix the following warnings: > > > >> warning: incorrect type in initializer (different address spaces) > > > >> expected unsigned short [noderef] __user *register __p > > > >> got unsigned short [usertype] * > > > >> warning: cast removes address space '__user' of expression > > > >> > > > >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > > > >> --- > > > >> arch/arm/kernel/traps.c | 10 +++++----- > > > >> 1 file changed, 5 insertions(+), 5 deletions(-) > > > >> > > > >> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > > > >> index 20b2db6dcd1ced7..34aa80c09c508c1 100644 > > > >> --- a/arch/arm/kernel/traps.c > > > >> +++ b/arch/arm/kernel/traps.c > > > >> @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) > > > >> } > > > >> } else { > > > >> if (thumb) > > > >> - bad = get_user(val, &((u16 *)addr)[i]); > > > >> + bad = get_user(val, &((u16 __user *)addr)[i]); > > > >> else > > > >> - bad = get_user(val, &((u32 *)addr)[i]); > > > >> + bad = get_user(val, &((u32 __user *)addr)[i]); > > > >> } > > > >> > > > >> if (!bad) > > > >> @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) > > > >> if (processor_mode(regs) == SVC_MODE) { > > > >> #ifdef CONFIG_THUMB2_KERNEL > > > >> if (thumb_mode(regs)) { > > > >> - instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); > > > >> + instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]); > > > > > > > > Shouldn't this be __user as well? (and below) > > > > > > unsigned int instr; > > > void __user *pc; > > > > > > The __user can clear the warning, but a new warning will be generated. > > > > > > instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); > > > ^new ^old > > > > > > arch/arm/kernel/traps.c:473:33: warning: dereference of noderef expression > > > > > > > This is because dereferencing a __user pointer is not permitted. > > > > So this code should be using get_kernel_nofault() here not a plain > > dereference of PC. So better to fix that properly instead of papering > > over it with a __force cast just to make sparse happy. > > Why? We won't get here unless the PC can be dereferenced. If it's not > able to be dereferenced, then we'd be dealing with a prefetch abort. > If that is guaranteed (i.e., there is no way we might be racing with a module unload on another CPU or something like that), then I agree that dereferencing PC is fine. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse @ 2022-10-10 16:14 ` Ard Biesheuvel 0 siblings, 0 replies; 40+ messages in thread From: Ard Biesheuvel @ 2022-10-10 16:14 UTC (permalink / raw) To: Russell King (Oracle) Cc: Leizhen (ThunderTown), linux-arm-kernel, linux-kernel On Mon, 10 Oct 2022 at 18:08, Russell King (Oracle) <linux@armlinux.org.uk> wrote: > > On Mon, Oct 10, 2022 at 01:06:19PM +0200, Ard Biesheuvel wrote: > > On Mon, 10 Oct 2022 at 12:58, Leizhen (ThunderTown) > > <thunder.leizhen@huawei.com> wrote: > > > > > > > > > > > > On 2022/10/10 18:20, Ard Biesheuvel wrote: > > > > On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote: > > > >> > > > >> Fix the following warnings: > > > >> warning: incorrect type in initializer (different address spaces) > > > >> expected unsigned short [noderef] __user *register __p > > > >> got unsigned short [usertype] * > > > >> warning: cast removes address space '__user' of expression > > > >> > > > >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > > > >> --- > > > >> arch/arm/kernel/traps.c | 10 +++++----- > > > >> 1 file changed, 5 insertions(+), 5 deletions(-) > > > >> > > > >> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > > > >> index 20b2db6dcd1ced7..34aa80c09c508c1 100644 > > > >> --- a/arch/arm/kernel/traps.c > > > >> +++ b/arch/arm/kernel/traps.c > > > >> @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) > > > >> } > > > >> } else { > > > >> if (thumb) > > > >> - bad = get_user(val, &((u16 *)addr)[i]); > > > >> + bad = get_user(val, &((u16 __user *)addr)[i]); > > > >> else > > > >> - bad = get_user(val, &((u32 *)addr)[i]); > > > >> + bad = get_user(val, &((u32 __user *)addr)[i]); > > > >> } > > > >> > > > >> if (!bad) > > > >> @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) > > > >> if (processor_mode(regs) == SVC_MODE) { > > > >> #ifdef CONFIG_THUMB2_KERNEL > > > >> if (thumb_mode(regs)) { > > > >> - instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); > > > >> + instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]); > > > > > > > > Shouldn't this be __user as well? (and below) > > > > > > unsigned int instr; > > > void __user *pc; > > > > > > The __user can clear the warning, but a new warning will be generated. > > > > > > instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); > > > ^new ^old > > > > > > arch/arm/kernel/traps.c:473:33: warning: dereference of noderef expression > > > > > > > This is because dereferencing a __user pointer is not permitted. > > > > So this code should be using get_kernel_nofault() here not a plain > > dereference of PC. So better to fix that properly instead of papering > > over it with a __force cast just to make sparse happy. > > Why? We won't get here unless the PC can be dereferenced. If it's not > able to be dereferenced, then we'd be dealing with a prefetch abort. > If that is guaranteed (i.e., there is no way we might be racing with a module unload on another CPU or something like that), then I agree that dereferencing PC is fine. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse 2022-10-10 16:14 ` Ard Biesheuvel @ 2022-10-10 16:17 ` Russell King (Oracle) -1 siblings, 0 replies; 40+ messages in thread From: Russell King (Oracle) @ 2022-10-10 16:17 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: Leizhen (ThunderTown), linux-arm-kernel, linux-kernel On Mon, Oct 10, 2022 at 06:14:56PM +0200, Ard Biesheuvel wrote: > On Mon, 10 Oct 2022 at 18:08, Russell King (Oracle) > <linux@armlinux.org.uk> wrote: > > > > On Mon, Oct 10, 2022 at 01:06:19PM +0200, Ard Biesheuvel wrote: > > > On Mon, 10 Oct 2022 at 12:58, Leizhen (ThunderTown) > > > <thunder.leizhen@huawei.com> wrote: > > > > > > > > > > > > > > > > On 2022/10/10 18:20, Ard Biesheuvel wrote: > > > > > On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote: > > > > >> > > > > >> Fix the following warnings: > > > > >> warning: incorrect type in initializer (different address spaces) > > > > >> expected unsigned short [noderef] __user *register __p > > > > >> got unsigned short [usertype] * > > > > >> warning: cast removes address space '__user' of expression > > > > >> > > > > >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > > > > >> --- > > > > >> arch/arm/kernel/traps.c | 10 +++++----- > > > > >> 1 file changed, 5 insertions(+), 5 deletions(-) > > > > >> > > > > >> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > > > > >> index 20b2db6dcd1ced7..34aa80c09c508c1 100644 > > > > >> --- a/arch/arm/kernel/traps.c > > > > >> +++ b/arch/arm/kernel/traps.c > > > > >> @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) > > > > >> } > > > > >> } else { > > > > >> if (thumb) > > > > >> - bad = get_user(val, &((u16 *)addr)[i]); > > > > >> + bad = get_user(val, &((u16 __user *)addr)[i]); > > > > >> else > > > > >> - bad = get_user(val, &((u32 *)addr)[i]); > > > > >> + bad = get_user(val, &((u32 __user *)addr)[i]); > > > > >> } > > > > >> > > > > >> if (!bad) > > > > >> @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) > > > > >> if (processor_mode(regs) == SVC_MODE) { > > > > >> #ifdef CONFIG_THUMB2_KERNEL > > > > >> if (thumb_mode(regs)) { > > > > >> - instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); > > > > >> + instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]); > > > > > > > > > > Shouldn't this be __user as well? (and below) > > > > > > > > unsigned int instr; > > > > void __user *pc; > > > > > > > > The __user can clear the warning, but a new warning will be generated. > > > > > > > > instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); > > > > ^new ^old > > > > > > > > arch/arm/kernel/traps.c:473:33: warning: dereference of noderef expression > > > > > > > > > > This is because dereferencing a __user pointer is not permitted. > > > > > > So this code should be using get_kernel_nofault() here not a plain > > > dereference of PC. So better to fix that properly instead of papering > > > over it with a __force cast just to make sparse happy. > > > > Why? We won't get here unless the PC can be dereferenced. If it's not > > able to be dereferenced, then we'd be dealing with a prefetch abort. > > > > If that is guaranteed (i.e., there is no way we might be racing with a > module unload on another CPU or something like that), then I agree > that dereferencing PC is fine. If we get here for an instruction in a module that's being unloaded, we have way bigger problems. We shouldn't be executing code in a module being unloaded in the first place. That becomes a case of "deserves to oops". The more likely case would be a prefetch abort when the page is unmapped. You'd have to try pretty hard to get an undef to race with a module unload. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse @ 2022-10-10 16:17 ` Russell King (Oracle) 0 siblings, 0 replies; 40+ messages in thread From: Russell King (Oracle) @ 2022-10-10 16:17 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: Leizhen (ThunderTown), linux-arm-kernel, linux-kernel On Mon, Oct 10, 2022 at 06:14:56PM +0200, Ard Biesheuvel wrote: > On Mon, 10 Oct 2022 at 18:08, Russell King (Oracle) > <linux@armlinux.org.uk> wrote: > > > > On Mon, Oct 10, 2022 at 01:06:19PM +0200, Ard Biesheuvel wrote: > > > On Mon, 10 Oct 2022 at 12:58, Leizhen (ThunderTown) > > > <thunder.leizhen@huawei.com> wrote: > > > > > > > > > > > > > > > > On 2022/10/10 18:20, Ard Biesheuvel wrote: > > > > > On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote: > > > > >> > > > > >> Fix the following warnings: > > > > >> warning: incorrect type in initializer (different address spaces) > > > > >> expected unsigned short [noderef] __user *register __p > > > > >> got unsigned short [usertype] * > > > > >> warning: cast removes address space '__user' of expression > > > > >> > > > > >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > > > > >> --- > > > > >> arch/arm/kernel/traps.c | 10 +++++----- > > > > >> 1 file changed, 5 insertions(+), 5 deletions(-) > > > > >> > > > > >> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > > > > >> index 20b2db6dcd1ced7..34aa80c09c508c1 100644 > > > > >> --- a/arch/arm/kernel/traps.c > > > > >> +++ b/arch/arm/kernel/traps.c > > > > >> @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) > > > > >> } > > > > >> } else { > > > > >> if (thumb) > > > > >> - bad = get_user(val, &((u16 *)addr)[i]); > > > > >> + bad = get_user(val, &((u16 __user *)addr)[i]); > > > > >> else > > > > >> - bad = get_user(val, &((u32 *)addr)[i]); > > > > >> + bad = get_user(val, &((u32 __user *)addr)[i]); > > > > >> } > > > > >> > > > > >> if (!bad) > > > > >> @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) > > > > >> if (processor_mode(regs) == SVC_MODE) { > > > > >> #ifdef CONFIG_THUMB2_KERNEL > > > > >> if (thumb_mode(regs)) { > > > > >> - instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); > > > > >> + instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]); > > > > > > > > > > Shouldn't this be __user as well? (and below) > > > > > > > > unsigned int instr; > > > > void __user *pc; > > > > > > > > The __user can clear the warning, but a new warning will be generated. > > > > > > > > instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); > > > > ^new ^old > > > > > > > > arch/arm/kernel/traps.c:473:33: warning: dereference of noderef expression > > > > > > > > > > This is because dereferencing a __user pointer is not permitted. > > > > > > So this code should be using get_kernel_nofault() here not a plain > > > dereference of PC. So better to fix that properly instead of papering > > > over it with a __force cast just to make sparse happy. > > > > Why? We won't get here unless the PC can be dereferenced. If it's not > > able to be dereferenced, then we'd be dealing with a prefetch abort. > > > > If that is guaranteed (i.e., there is no way we might be racing with a > module unload on another CPU or something like that), then I agree > that dereferencing PC is fine. If we get here for an instruction in a module that's being unloaded, we have way bigger problems. We shouldn't be executing code in a module being unloaded in the first place. That becomes a case of "deserves to oops". The more likely case would be a prefetch abort when the page is unmapped. You'd have to try pretty hard to get an undef to race with a module unload. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse 2022-10-10 11:06 ` Ard Biesheuvel @ 2022-10-11 2:29 ` Leizhen (ThunderTown) -1 siblings, 0 replies; 40+ messages in thread From: Leizhen (ThunderTown) @ 2022-10-11 2:29 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: Russell King, linux-arm-kernel, linux-kernel On 2022/10/10 19:06, Ard Biesheuvel wrote: > On Mon, 10 Oct 2022 at 12:58, Leizhen (ThunderTown) > <thunder.leizhen@huawei.com> wrote: >> >> >> >> On 2022/10/10 18:20, Ard Biesheuvel wrote: >>> On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote: >>>> >>>> Fix the following warnings: >>>> warning: incorrect type in initializer (different address spaces) >>>> expected unsigned short [noderef] __user *register __p >>>> got unsigned short [usertype] * >>>> warning: cast removes address space '__user' of expression >>>> >>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >>>> --- >>>> arch/arm/kernel/traps.c | 10 +++++----- >>>> 1 file changed, 5 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c >>>> index 20b2db6dcd1ced7..34aa80c09c508c1 100644 >>>> --- a/arch/arm/kernel/traps.c >>>> +++ b/arch/arm/kernel/traps.c >>>> @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) >>>> } >>>> } else { >>>> if (thumb) >>>> - bad = get_user(val, &((u16 *)addr)[i]); >>>> + bad = get_user(val, &((u16 __user *)addr)[i]); >>>> else >>>> - bad = get_user(val, &((u32 *)addr)[i]); >>>> + bad = get_user(val, &((u32 __user *)addr)[i]); >>>> } >>>> >>>> if (!bad) >>>> @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) >>>> if (processor_mode(regs) == SVC_MODE) { >>>> #ifdef CONFIG_THUMB2_KERNEL >>>> if (thumb_mode(regs)) { >>>> - instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); >>>> + instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]); >>> >>> Shouldn't this be __user as well? (and below) >> >> unsigned int instr; >> void __user *pc; >> >> The __user can clear the warning, but a new warning will be generated. >> >> instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); >> ^new ^old >> >> arch/arm/kernel/traps.c:473:33: warning: dereference of noderef expression >> > > This is because dereferencing a __user pointer is not permitted. > > So this code should be using get_kernel_nofault() here not a plain > dereference of PC. So better to fix that properly instead of papering > over it with a __force cast just to make sparse happy. How about: @@ -451,9 +451,9 @@ int call_undef_hook(struct pt_regs *regs, unsigned int instr) asmlinkage void do_undefinstr(struct pt_regs *regs) { unsigned int instr; - void __user *pc; + void *pc; - pc = (void __user *)instruction_pointer(regs); + pc = (void *)instruction_pointer(regs); if (processor_mode(regs) == SVC_MODE) { #ifdef CONFIG_THUMB2_KERNEL @@ -497,7 +497,7 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) } #endif arm_notify_die("Oops - undefined instruction", regs, - SIGILL, ILL_ILLOPC, pc, 0, 6); + SIGILL, ILL_ILLOPC, (void __user *)pc, 0, 6); } NOKPROBE_SYMBOL(do_undefinstr) The 'pc' may come from kernel or user. And I found that all the get_user() calls have already done type casts to the pc, except arm_notify_die(). I think the above changes are reasonable. > > >>> >>>> if (is_wide_instruction(instr)) { >>>> u16 inst2; >>>> - inst2 = __mem_to_opcode_thumb16(((u16 *)pc)[1]); >>>> + inst2 = __mem_to_opcode_thumb16(((__force u16 *)pc)[1]); >>>> instr = __opcode_thumb32_compose(instr, inst2); >>>> } >>>> } else >>>> #endif >>>> - instr = __mem_to_opcode_arm(*(u32 *) pc); >>>> + instr = __mem_to_opcode_arm(*(__force u32 *) pc); >>>> } else if (thumb_mode(regs)) { >>>> if (get_user(instr, (u16 __user *)pc)) >>>> goto die_sig; >>>> -- >>>> 2.25.1 >>>> >>>> >>>> _______________________________________________ >>>> linux-arm-kernel mailing list >>>> linux-arm-kernel@lists.infradead.org >>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >>> . >>> >> >> -- >> Regards, >> Zhen Lei > . > -- Regards, Zhen Lei _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse @ 2022-10-11 2:29 ` Leizhen (ThunderTown) 0 siblings, 0 replies; 40+ messages in thread From: Leizhen (ThunderTown) @ 2022-10-11 2:29 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: Russell King, linux-arm-kernel, linux-kernel On 2022/10/10 19:06, Ard Biesheuvel wrote: > On Mon, 10 Oct 2022 at 12:58, Leizhen (ThunderTown) > <thunder.leizhen@huawei.com> wrote: >> >> >> >> On 2022/10/10 18:20, Ard Biesheuvel wrote: >>> On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote: >>>> >>>> Fix the following warnings: >>>> warning: incorrect type in initializer (different address spaces) >>>> expected unsigned short [noderef] __user *register __p >>>> got unsigned short [usertype] * >>>> warning: cast removes address space '__user' of expression >>>> >>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >>>> --- >>>> arch/arm/kernel/traps.c | 10 +++++----- >>>> 1 file changed, 5 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c >>>> index 20b2db6dcd1ced7..34aa80c09c508c1 100644 >>>> --- a/arch/arm/kernel/traps.c >>>> +++ b/arch/arm/kernel/traps.c >>>> @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) >>>> } >>>> } else { >>>> if (thumb) >>>> - bad = get_user(val, &((u16 *)addr)[i]); >>>> + bad = get_user(val, &((u16 __user *)addr)[i]); >>>> else >>>> - bad = get_user(val, &((u32 *)addr)[i]); >>>> + bad = get_user(val, &((u32 __user *)addr)[i]); >>>> } >>>> >>>> if (!bad) >>>> @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) >>>> if (processor_mode(regs) == SVC_MODE) { >>>> #ifdef CONFIG_THUMB2_KERNEL >>>> if (thumb_mode(regs)) { >>>> - instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); >>>> + instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]); >>> >>> Shouldn't this be __user as well? (and below) >> >> unsigned int instr; >> void __user *pc; >> >> The __user can clear the warning, but a new warning will be generated. >> >> instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); >> ^new ^old >> >> arch/arm/kernel/traps.c:473:33: warning: dereference of noderef expression >> > > This is because dereferencing a __user pointer is not permitted. > > So this code should be using get_kernel_nofault() here not a plain > dereference of PC. So better to fix that properly instead of papering > over it with a __force cast just to make sparse happy. How about: @@ -451,9 +451,9 @@ int call_undef_hook(struct pt_regs *regs, unsigned int instr) asmlinkage void do_undefinstr(struct pt_regs *regs) { unsigned int instr; - void __user *pc; + void *pc; - pc = (void __user *)instruction_pointer(regs); + pc = (void *)instruction_pointer(regs); if (processor_mode(regs) == SVC_MODE) { #ifdef CONFIG_THUMB2_KERNEL @@ -497,7 +497,7 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) } #endif arm_notify_die("Oops - undefined instruction", regs, - SIGILL, ILL_ILLOPC, pc, 0, 6); + SIGILL, ILL_ILLOPC, (void __user *)pc, 0, 6); } NOKPROBE_SYMBOL(do_undefinstr) The 'pc' may come from kernel or user. And I found that all the get_user() calls have already done type casts to the pc, except arm_notify_die(). I think the above changes are reasonable. > > >>> >>>> if (is_wide_instruction(instr)) { >>>> u16 inst2; >>>> - inst2 = __mem_to_opcode_thumb16(((u16 *)pc)[1]); >>>> + inst2 = __mem_to_opcode_thumb16(((__force u16 *)pc)[1]); >>>> instr = __opcode_thumb32_compose(instr, inst2); >>>> } >>>> } else >>>> #endif >>>> - instr = __mem_to_opcode_arm(*(u32 *) pc); >>>> + instr = __mem_to_opcode_arm(*(__force u32 *) pc); >>>> } else if (thumb_mode(regs)) { >>>> if (get_user(instr, (u16 __user *)pc)) >>>> goto die_sig; >>>> -- >>>> 2.25.1 >>>> >>>> >>>> _______________________________________________ >>>> linux-arm-kernel mailing list >>>> linux-arm-kernel@lists.infradead.org >>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >>> . >>> >> >> -- >> Regards, >> Zhen Lei > . > -- Regards, Zhen Lei ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse 2022-10-11 2:29 ` Leizhen (ThunderTown) @ 2022-10-13 10:51 ` Russell King (Oracle) -1 siblings, 0 replies; 40+ messages in thread From: Russell King (Oracle) @ 2022-10-13 10:51 UTC (permalink / raw) To: Leizhen (ThunderTown); +Cc: Ard Biesheuvel, linux-arm-kernel, linux-kernel On Tue, Oct 11, 2022 at 10:29:58AM +0800, Leizhen (ThunderTown) wrote: > On 2022/10/10 19:06, Ard Biesheuvel wrote: > > On Mon, 10 Oct 2022 at 12:58, Leizhen (ThunderTown) > > <thunder.leizhen@huawei.com> wrote: > >> On 2022/10/10 18:20, Ard Biesheuvel wrote: > >>> On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote: > >>>> > >>>> Fix the following warnings: > >>>> warning: incorrect type in initializer (different address spaces) > >>>> expected unsigned short [noderef] __user *register __p > >>>> got unsigned short [usertype] * > >>>> warning: cast removes address space '__user' of expression > >>>> > >>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > >>>> --- > >>>> arch/arm/kernel/traps.c | 10 +++++----- > >>>> 1 file changed, 5 insertions(+), 5 deletions(-) > >>>> > >>>> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > >>>> index 20b2db6dcd1ced7..34aa80c09c508c1 100644 > >>>> --- a/arch/arm/kernel/traps.c > >>>> +++ b/arch/arm/kernel/traps.c > >>>> @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) > >>>> } > >>>> } else { > >>>> if (thumb) > >>>> - bad = get_user(val, &((u16 *)addr)[i]); > >>>> + bad = get_user(val, &((u16 __user *)addr)[i]); > >>>> else > >>>> - bad = get_user(val, &((u32 *)addr)[i]); > >>>> + bad = get_user(val, &((u32 __user *)addr)[i]); > >>>> } > >>>> > >>>> if (!bad) > >>>> @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) > >>>> if (processor_mode(regs) == SVC_MODE) { > >>>> #ifdef CONFIG_THUMB2_KERNEL > >>>> if (thumb_mode(regs)) { > >>>> - instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); > >>>> + instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]); > >>> > >>> Shouldn't this be __user as well? (and below) > >> > >> unsigned int instr; > >> void __user *pc; > >> > >> The __user can clear the warning, but a new warning will be generated. > >> > >> instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); > >> ^new ^old > >> > >> arch/arm/kernel/traps.c:473:33: warning: dereference of noderef expression > >> > > > > This is because dereferencing a __user pointer is not permitted. > > > > So this code should be using get_kernel_nofault() here not a plain > > dereference of PC. So better to fix that properly instead of papering > > over it with a __force cast just to make sparse happy. > > How about: > @@ -451,9 +451,9 @@ int call_undef_hook(struct pt_regs *regs, unsigned int instr) > asmlinkage void do_undefinstr(struct pt_regs *regs) > { > unsigned int instr; > - void __user *pc; > + void *pc; > > - pc = (void __user *)instruction_pointer(regs); > + pc = (void *)instruction_pointer(regs); > > if (processor_mode(regs) == SVC_MODE) { > #ifdef CONFIG_THUMB2_KERNEL > @@ -497,7 +497,7 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) > } > #endif > arm_notify_die("Oops - undefined instruction", regs, > - SIGILL, ILL_ILLOPC, pc, 0, 6); > + SIGILL, ILL_ILLOPC, (void __user *)pc, 0, 6); > } > NOKPROBE_SYMBOL(do_undefinstr) > > > The 'pc' may come from kernel or user. And I found that all the get_user() > calls have already done type casts to the pc, except arm_notify_die(). > I think the above changes are reasonable. If we're going to do that, lets do it properly - I think the above would need some __force usage to stop sparse complaining, whereas I don't think this will (untested): diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c index 3f468ac98592..827cbc022900 100644 --- a/arch/arm/kernel/traps.c +++ b/arch/arm/kernel/traps.c @@ -449,36 +449,45 @@ int call_undef_hook(struct pt_regs *regs, unsigned int instr) asmlinkage void do_undefinstr(struct pt_regs *regs) { unsigned int instr; - void __user *pc; + unsigned long pc; - pc = (void __user *)instruction_pointer(regs); + pc = instruction_pointer(regs); if (processor_mode(regs) == SVC_MODE) { -#ifdef CONFIG_THUMB2_KERNEL - if (thumb_mode(regs)) { - instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); + if (IS_ENABLED(CONFIG_THUMB2_KERNEL) && thumb_mode(regs)) { + u16 *tpc = (u16 *)pc; + + instr = __mem_to_opcode_thumb16(tpc[0]); if (is_wide_instruction(instr)) { u16 inst2; - inst2 = __mem_to_opcode_thumb16(((u16 *)pc)[1]); + + inst2 = __mem_to_opcode_thumb16(tpc[1]); instr = __opcode_thumb32_compose(instr, inst2); } - } else -#endif - instr = __mem_to_opcode_arm(*(u32 *) pc); + } else { + u32 *apc = (u32 *)pc; + + instr = __mem_to_opcode_arm(*apc); + } } else if (thumb_mode(regs)) { - if (get_user(instr, (u16 __user *)pc)) + u16 __user *tpc = (u16 __user *)pc; + + if (get_user(instr, tpc)) goto die_sig; instr = __mem_to_opcode_thumb16(instr); if (is_wide_instruction(instr)) { unsigned int instr2; - if (get_user(instr2, (u16 __user *)pc+1)) + if (get_user(instr2, tpc + 1)) goto die_sig; instr2 = __mem_to_opcode_thumb16(instr2); instr = __opcode_thumb32_compose(instr, instr2); } } else { - if (get_user(instr, (u32 __user *)pc)) + u32 __user *apc = (u32 __user *)pc; + + if (get_user(instr, apc)) goto die_sig; + instr = __mem_to_opcode_arm(instr); } @@ -495,7 +504,7 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) } #endif arm_notify_die("Oops - undefined instruction", regs, - SIGILL, ILL_ILLOPC, pc, 0, 6); + SIGILL, ILL_ILLOPC, (void __user *)pc, 0, 6); } NOKPROBE_SYMBOL(do_undefinstr) -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse @ 2022-10-13 10:51 ` Russell King (Oracle) 0 siblings, 0 replies; 40+ messages in thread From: Russell King (Oracle) @ 2022-10-13 10:51 UTC (permalink / raw) To: Leizhen (ThunderTown); +Cc: Ard Biesheuvel, linux-arm-kernel, linux-kernel On Tue, Oct 11, 2022 at 10:29:58AM +0800, Leizhen (ThunderTown) wrote: > On 2022/10/10 19:06, Ard Biesheuvel wrote: > > On Mon, 10 Oct 2022 at 12:58, Leizhen (ThunderTown) > > <thunder.leizhen@huawei.com> wrote: > >> On 2022/10/10 18:20, Ard Biesheuvel wrote: > >>> On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote: > >>>> > >>>> Fix the following warnings: > >>>> warning: incorrect type in initializer (different address spaces) > >>>> expected unsigned short [noderef] __user *register __p > >>>> got unsigned short [usertype] * > >>>> warning: cast removes address space '__user' of expression > >>>> > >>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > >>>> --- > >>>> arch/arm/kernel/traps.c | 10 +++++----- > >>>> 1 file changed, 5 insertions(+), 5 deletions(-) > >>>> > >>>> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > >>>> index 20b2db6dcd1ced7..34aa80c09c508c1 100644 > >>>> --- a/arch/arm/kernel/traps.c > >>>> +++ b/arch/arm/kernel/traps.c > >>>> @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) > >>>> } > >>>> } else { > >>>> if (thumb) > >>>> - bad = get_user(val, &((u16 *)addr)[i]); > >>>> + bad = get_user(val, &((u16 __user *)addr)[i]); > >>>> else > >>>> - bad = get_user(val, &((u32 *)addr)[i]); > >>>> + bad = get_user(val, &((u32 __user *)addr)[i]); > >>>> } > >>>> > >>>> if (!bad) > >>>> @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) > >>>> if (processor_mode(regs) == SVC_MODE) { > >>>> #ifdef CONFIG_THUMB2_KERNEL > >>>> if (thumb_mode(regs)) { > >>>> - instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); > >>>> + instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]); > >>> > >>> Shouldn't this be __user as well? (and below) > >> > >> unsigned int instr; > >> void __user *pc; > >> > >> The __user can clear the warning, but a new warning will be generated. > >> > >> instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); > >> ^new ^old > >> > >> arch/arm/kernel/traps.c:473:33: warning: dereference of noderef expression > >> > > > > This is because dereferencing a __user pointer is not permitted. > > > > So this code should be using get_kernel_nofault() here not a plain > > dereference of PC. So better to fix that properly instead of papering > > over it with a __force cast just to make sparse happy. > > How about: > @@ -451,9 +451,9 @@ int call_undef_hook(struct pt_regs *regs, unsigned int instr) > asmlinkage void do_undefinstr(struct pt_regs *regs) > { > unsigned int instr; > - void __user *pc; > + void *pc; > > - pc = (void __user *)instruction_pointer(regs); > + pc = (void *)instruction_pointer(regs); > > if (processor_mode(regs) == SVC_MODE) { > #ifdef CONFIG_THUMB2_KERNEL > @@ -497,7 +497,7 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) > } > #endif > arm_notify_die("Oops - undefined instruction", regs, > - SIGILL, ILL_ILLOPC, pc, 0, 6); > + SIGILL, ILL_ILLOPC, (void __user *)pc, 0, 6); > } > NOKPROBE_SYMBOL(do_undefinstr) > > > The 'pc' may come from kernel or user. And I found that all the get_user() > calls have already done type casts to the pc, except arm_notify_die(). > I think the above changes are reasonable. If we're going to do that, lets do it properly - I think the above would need some __force usage to stop sparse complaining, whereas I don't think this will (untested): diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c index 3f468ac98592..827cbc022900 100644 --- a/arch/arm/kernel/traps.c +++ b/arch/arm/kernel/traps.c @@ -449,36 +449,45 @@ int call_undef_hook(struct pt_regs *regs, unsigned int instr) asmlinkage void do_undefinstr(struct pt_regs *regs) { unsigned int instr; - void __user *pc; + unsigned long pc; - pc = (void __user *)instruction_pointer(regs); + pc = instruction_pointer(regs); if (processor_mode(regs) == SVC_MODE) { -#ifdef CONFIG_THUMB2_KERNEL - if (thumb_mode(regs)) { - instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); + if (IS_ENABLED(CONFIG_THUMB2_KERNEL) && thumb_mode(regs)) { + u16 *tpc = (u16 *)pc; + + instr = __mem_to_opcode_thumb16(tpc[0]); if (is_wide_instruction(instr)) { u16 inst2; - inst2 = __mem_to_opcode_thumb16(((u16 *)pc)[1]); + + inst2 = __mem_to_opcode_thumb16(tpc[1]); instr = __opcode_thumb32_compose(instr, inst2); } - } else -#endif - instr = __mem_to_opcode_arm(*(u32 *) pc); + } else { + u32 *apc = (u32 *)pc; + + instr = __mem_to_opcode_arm(*apc); + } } else if (thumb_mode(regs)) { - if (get_user(instr, (u16 __user *)pc)) + u16 __user *tpc = (u16 __user *)pc; + + if (get_user(instr, tpc)) goto die_sig; instr = __mem_to_opcode_thumb16(instr); if (is_wide_instruction(instr)) { unsigned int instr2; - if (get_user(instr2, (u16 __user *)pc+1)) + if (get_user(instr2, tpc + 1)) goto die_sig; instr2 = __mem_to_opcode_thumb16(instr2); instr = __opcode_thumb32_compose(instr, instr2); } } else { - if (get_user(instr, (u32 __user *)pc)) + u32 __user *apc = (u32 __user *)pc; + + if (get_user(instr, apc)) goto die_sig; + instr = __mem_to_opcode_arm(instr); } @@ -495,7 +504,7 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) } #endif arm_notify_die("Oops - undefined instruction", regs, - SIGILL, ILL_ILLOPC, pc, 0, 6); + SIGILL, ILL_ILLOPC, (void __user *)pc, 0, 6); } NOKPROBE_SYMBOL(do_undefinstr) -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse 2022-10-13 10:51 ` Russell King (Oracle) @ 2022-10-13 11:34 ` Leizhen (ThunderTown) -1 siblings, 0 replies; 40+ messages in thread From: Leizhen (ThunderTown) @ 2022-10-13 11:34 UTC (permalink / raw) To: Russell King (Oracle); +Cc: Ard Biesheuvel, linux-arm-kernel, linux-kernel On 2022/10/13 18:51, Russell King (Oracle) wrote: > On Tue, Oct 11, 2022 at 10:29:58AM +0800, Leizhen (ThunderTown) wrote: >> On 2022/10/10 19:06, Ard Biesheuvel wrote: >>> On Mon, 10 Oct 2022 at 12:58, Leizhen (ThunderTown) >>> <thunder.leizhen@huawei.com> wrote: >>>> On 2022/10/10 18:20, Ard Biesheuvel wrote: >>>>> On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote: >>>>>> >>>>>> Fix the following warnings: >>>>>> warning: incorrect type in initializer (different address spaces) >>>>>> expected unsigned short [noderef] __user *register __p >>>>>> got unsigned short [usertype] * >>>>>> warning: cast removes address space '__user' of expression >>>>>> >>>>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >>>>>> --- >>>>>> arch/arm/kernel/traps.c | 10 +++++----- >>>>>> 1 file changed, 5 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c >>>>>> index 20b2db6dcd1ced7..34aa80c09c508c1 100644 >>>>>> --- a/arch/arm/kernel/traps.c >>>>>> +++ b/arch/arm/kernel/traps.c >>>>>> @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) >>>>>> } >>>>>> } else { >>>>>> if (thumb) >>>>>> - bad = get_user(val, &((u16 *)addr)[i]); >>>>>> + bad = get_user(val, &((u16 __user *)addr)[i]); >>>>>> else >>>>>> - bad = get_user(val, &((u32 *)addr)[i]); >>>>>> + bad = get_user(val, &((u32 __user *)addr)[i]); >>>>>> } >>>>>> >>>>>> if (!bad) >>>>>> @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) >>>>>> if (processor_mode(regs) == SVC_MODE) { >>>>>> #ifdef CONFIG_THUMB2_KERNEL >>>>>> if (thumb_mode(regs)) { >>>>>> - instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); >>>>>> + instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]); >>>>> >>>>> Shouldn't this be __user as well? (and below) >>>> >>>> unsigned int instr; >>>> void __user *pc; >>>> >>>> The __user can clear the warning, but a new warning will be generated. >>>> >>>> instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); >>>> ^new ^old >>>> >>>> arch/arm/kernel/traps.c:473:33: warning: dereference of noderef expression >>>> >>> >>> This is because dereferencing a __user pointer is not permitted. >>> >>> So this code should be using get_kernel_nofault() here not a plain >>> dereference of PC. So better to fix that properly instead of papering >>> over it with a __force cast just to make sparse happy. >> >> How about: >> @@ -451,9 +451,9 @@ int call_undef_hook(struct pt_regs *regs, unsigned int instr) >> asmlinkage void do_undefinstr(struct pt_regs *regs) >> { >> unsigned int instr; >> - void __user *pc; >> + void *pc; >> >> - pc = (void __user *)instruction_pointer(regs); >> + pc = (void *)instruction_pointer(regs); >> >> if (processor_mode(regs) == SVC_MODE) { >> #ifdef CONFIG_THUMB2_KERNEL >> @@ -497,7 +497,7 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) >> } >> #endif >> arm_notify_die("Oops - undefined instruction", regs, >> - SIGILL, ILL_ILLOPC, pc, 0, 6); >> + SIGILL, ILL_ILLOPC, (void __user *)pc, 0, 6); >> } >> NOKPROBE_SYMBOL(do_undefinstr) >> >> >> The 'pc' may come from kernel or user. And I found that all the get_user() >> calls have already done type casts to the pc, except arm_notify_die(). >> I think the above changes are reasonable. > > If we're going to do that, lets do it properly - I think the above would > need some __force usage to stop sparse complaining, whereas I don't No need to add __force. I sent it after testing. $ cat .config | grep THUMB2 CONFIG_THUMB2_KERNEL=y $ make C=2 ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- arch/arm/kernel/traps.o CHECK scripts/mod/empty.c CALL scripts/checksyscalls.sh CHECK arch/arm/kernel/traps.c arch/arm/kernel/traps.c:97:6: warning: symbol 'dump_backtrace_stm' was not declared. Should it be static? arch/arm/kernel/traps.c:451:17: warning: symbol 'do_undefinstr' was not declared. Should it be static? arch/arm/kernel/traps.c:516:39: warning: symbol 'handle_fiq_as_nmi' was not declared. Should it be static? arch/arm/kernel/traps.c:535:17: warning: symbol 'bad_mode' was not declared. Should it be static? arch/arm/kernel/traps.c:608:16: warning: symbol 'arm_syscall' was not declared. Should it be static? arch/arm/kernel/traps.c:734:1: warning: symbol 'baddataabort' was not declared. Should it be static? arch/arm/kernel/traps.c:774:17: warning: symbol '__div0' was not declared. Should it be static? arch/arm/kernel/traps.c:781:6: warning: symbol 'abort' was not declared. Should it be static? arch/arm/kernel/traps.c:905:12: warning: symbol 'overflow_stack_ptr' was not declared. Should it be static? arch/arm/kernel/traps.c:922:17: warning: symbol 'handle_bad_stack' was not declared. Should it be static? > think this will (untested): > > diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > index 3f468ac98592..827cbc022900 100644 > --- a/arch/arm/kernel/traps.c > +++ b/arch/arm/kernel/traps.c > @@ -449,36 +449,45 @@ int call_undef_hook(struct pt_regs *regs, unsigned int instr) > asmlinkage void do_undefinstr(struct pt_regs *regs) > { > unsigned int instr; > - void __user *pc; > + unsigned long pc; > > - pc = (void __user *)instruction_pointer(regs); > + pc = instruction_pointer(regs); > > if (processor_mode(regs) == SVC_MODE) { > -#ifdef CONFIG_THUMB2_KERNEL > - if (thumb_mode(regs)) { > - instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); > + if (IS_ENABLED(CONFIG_THUMB2_KERNEL) && thumb_mode(regs)) { > + u16 *tpc = (u16 *)pc; > + > + instr = __mem_to_opcode_thumb16(tpc[0]); > if (is_wide_instruction(instr)) { > u16 inst2; > - inst2 = __mem_to_opcode_thumb16(((u16 *)pc)[1]); > + > + inst2 = __mem_to_opcode_thumb16(tpc[1]); > instr = __opcode_thumb32_compose(instr, inst2); > } > - } else > -#endif > - instr = __mem_to_opcode_arm(*(u32 *) pc); > + } else { > + u32 *apc = (u32 *)pc; > + > + instr = __mem_to_opcode_arm(*apc); > + } > } else if (thumb_mode(regs)) { > - if (get_user(instr, (u16 __user *)pc)) > + u16 __user *tpc = (u16 __user *)pc; > + > + if (get_user(instr, tpc)) > goto die_sig; > instr = __mem_to_opcode_thumb16(instr); > if (is_wide_instruction(instr)) { > unsigned int instr2; > - if (get_user(instr2, (u16 __user *)pc+1)) > + if (get_user(instr2, tpc + 1)) > goto die_sig; > instr2 = __mem_to_opcode_thumb16(instr2); > instr = __opcode_thumb32_compose(instr, instr2); > } > } else { > - if (get_user(instr, (u32 __user *)pc)) > + u32 __user *apc = (u32 __user *)pc; > + > + if (get_user(instr, apc)) > goto die_sig; > + > instr = __mem_to_opcode_arm(instr); > } > > @@ -495,7 +504,7 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) > } > #endif > arm_notify_die("Oops - undefined instruction", regs, > - SIGILL, ILL_ILLOPC, pc, 0, 6); > + SIGILL, ILL_ILLOPC, (void __user *)pc, 0, 6); > } > NOKPROBE_SYMBOL(do_undefinstr) > > -- Regards, Zhen Lei _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse @ 2022-10-13 11:34 ` Leizhen (ThunderTown) 0 siblings, 0 replies; 40+ messages in thread From: Leizhen (ThunderTown) @ 2022-10-13 11:34 UTC (permalink / raw) To: Russell King (Oracle); +Cc: Ard Biesheuvel, linux-arm-kernel, linux-kernel On 2022/10/13 18:51, Russell King (Oracle) wrote: > On Tue, Oct 11, 2022 at 10:29:58AM +0800, Leizhen (ThunderTown) wrote: >> On 2022/10/10 19:06, Ard Biesheuvel wrote: >>> On Mon, 10 Oct 2022 at 12:58, Leizhen (ThunderTown) >>> <thunder.leizhen@huawei.com> wrote: >>>> On 2022/10/10 18:20, Ard Biesheuvel wrote: >>>>> On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote: >>>>>> >>>>>> Fix the following warnings: >>>>>> warning: incorrect type in initializer (different address spaces) >>>>>> expected unsigned short [noderef] __user *register __p >>>>>> got unsigned short [usertype] * >>>>>> warning: cast removes address space '__user' of expression >>>>>> >>>>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >>>>>> --- >>>>>> arch/arm/kernel/traps.c | 10 +++++----- >>>>>> 1 file changed, 5 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c >>>>>> index 20b2db6dcd1ced7..34aa80c09c508c1 100644 >>>>>> --- a/arch/arm/kernel/traps.c >>>>>> +++ b/arch/arm/kernel/traps.c >>>>>> @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) >>>>>> } >>>>>> } else { >>>>>> if (thumb) >>>>>> - bad = get_user(val, &((u16 *)addr)[i]); >>>>>> + bad = get_user(val, &((u16 __user *)addr)[i]); >>>>>> else >>>>>> - bad = get_user(val, &((u32 *)addr)[i]); >>>>>> + bad = get_user(val, &((u32 __user *)addr)[i]); >>>>>> } >>>>>> >>>>>> if (!bad) >>>>>> @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) >>>>>> if (processor_mode(regs) == SVC_MODE) { >>>>>> #ifdef CONFIG_THUMB2_KERNEL >>>>>> if (thumb_mode(regs)) { >>>>>> - instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); >>>>>> + instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]); >>>>> >>>>> Shouldn't this be __user as well? (and below) >>>> >>>> unsigned int instr; >>>> void __user *pc; >>>> >>>> The __user can clear the warning, but a new warning will be generated. >>>> >>>> instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); >>>> ^new ^old >>>> >>>> arch/arm/kernel/traps.c:473:33: warning: dereference of noderef expression >>>> >>> >>> This is because dereferencing a __user pointer is not permitted. >>> >>> So this code should be using get_kernel_nofault() here not a plain >>> dereference of PC. So better to fix that properly instead of papering >>> over it with a __force cast just to make sparse happy. >> >> How about: >> @@ -451,9 +451,9 @@ int call_undef_hook(struct pt_regs *regs, unsigned int instr) >> asmlinkage void do_undefinstr(struct pt_regs *regs) >> { >> unsigned int instr; >> - void __user *pc; >> + void *pc; >> >> - pc = (void __user *)instruction_pointer(regs); >> + pc = (void *)instruction_pointer(regs); >> >> if (processor_mode(regs) == SVC_MODE) { >> #ifdef CONFIG_THUMB2_KERNEL >> @@ -497,7 +497,7 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) >> } >> #endif >> arm_notify_die("Oops - undefined instruction", regs, >> - SIGILL, ILL_ILLOPC, pc, 0, 6); >> + SIGILL, ILL_ILLOPC, (void __user *)pc, 0, 6); >> } >> NOKPROBE_SYMBOL(do_undefinstr) >> >> >> The 'pc' may come from kernel or user. And I found that all the get_user() >> calls have already done type casts to the pc, except arm_notify_die(). >> I think the above changes are reasonable. > > If we're going to do that, lets do it properly - I think the above would > need some __force usage to stop sparse complaining, whereas I don't No need to add __force. I sent it after testing. $ cat .config | grep THUMB2 CONFIG_THUMB2_KERNEL=y $ make C=2 ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- arch/arm/kernel/traps.o CHECK scripts/mod/empty.c CALL scripts/checksyscalls.sh CHECK arch/arm/kernel/traps.c arch/arm/kernel/traps.c:97:6: warning: symbol 'dump_backtrace_stm' was not declared. Should it be static? arch/arm/kernel/traps.c:451:17: warning: symbol 'do_undefinstr' was not declared. Should it be static? arch/arm/kernel/traps.c:516:39: warning: symbol 'handle_fiq_as_nmi' was not declared. Should it be static? arch/arm/kernel/traps.c:535:17: warning: symbol 'bad_mode' was not declared. Should it be static? arch/arm/kernel/traps.c:608:16: warning: symbol 'arm_syscall' was not declared. Should it be static? arch/arm/kernel/traps.c:734:1: warning: symbol 'baddataabort' was not declared. Should it be static? arch/arm/kernel/traps.c:774:17: warning: symbol '__div0' was not declared. Should it be static? arch/arm/kernel/traps.c:781:6: warning: symbol 'abort' was not declared. Should it be static? arch/arm/kernel/traps.c:905:12: warning: symbol 'overflow_stack_ptr' was not declared. Should it be static? arch/arm/kernel/traps.c:922:17: warning: symbol 'handle_bad_stack' was not declared. Should it be static? > think this will (untested): > > diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > index 3f468ac98592..827cbc022900 100644 > --- a/arch/arm/kernel/traps.c > +++ b/arch/arm/kernel/traps.c > @@ -449,36 +449,45 @@ int call_undef_hook(struct pt_regs *regs, unsigned int instr) > asmlinkage void do_undefinstr(struct pt_regs *regs) > { > unsigned int instr; > - void __user *pc; > + unsigned long pc; > > - pc = (void __user *)instruction_pointer(regs); > + pc = instruction_pointer(regs); > > if (processor_mode(regs) == SVC_MODE) { > -#ifdef CONFIG_THUMB2_KERNEL > - if (thumb_mode(regs)) { > - instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); > + if (IS_ENABLED(CONFIG_THUMB2_KERNEL) && thumb_mode(regs)) { > + u16 *tpc = (u16 *)pc; > + > + instr = __mem_to_opcode_thumb16(tpc[0]); > if (is_wide_instruction(instr)) { > u16 inst2; > - inst2 = __mem_to_opcode_thumb16(((u16 *)pc)[1]); > + > + inst2 = __mem_to_opcode_thumb16(tpc[1]); > instr = __opcode_thumb32_compose(instr, inst2); > } > - } else > -#endif > - instr = __mem_to_opcode_arm(*(u32 *) pc); > + } else { > + u32 *apc = (u32 *)pc; > + > + instr = __mem_to_opcode_arm(*apc); > + } > } else if (thumb_mode(regs)) { > - if (get_user(instr, (u16 __user *)pc)) > + u16 __user *tpc = (u16 __user *)pc; > + > + if (get_user(instr, tpc)) > goto die_sig; > instr = __mem_to_opcode_thumb16(instr); > if (is_wide_instruction(instr)) { > unsigned int instr2; > - if (get_user(instr2, (u16 __user *)pc+1)) > + if (get_user(instr2, tpc + 1)) > goto die_sig; > instr2 = __mem_to_opcode_thumb16(instr2); > instr = __opcode_thumb32_compose(instr, instr2); > } > } else { > - if (get_user(instr, (u32 __user *)pc)) > + u32 __user *apc = (u32 __user *)pc; > + > + if (get_user(instr, apc)) > goto die_sig; > + > instr = __mem_to_opcode_arm(instr); > } > > @@ -495,7 +504,7 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) > } > #endif > arm_notify_die("Oops - undefined instruction", regs, > - SIGILL, ILL_ILLOPC, pc, 0, 6); > + SIGILL, ILL_ILLOPC, (void __user *)pc, 0, 6); > } > NOKPROBE_SYMBOL(do_undefinstr) > > -- Regards, Zhen Lei ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse 2022-10-13 10:51 ` Russell King (Oracle) @ 2022-11-28 8:39 ` Leizhen (ThunderTown) -1 siblings, 0 replies; 40+ messages in thread From: Leizhen (ThunderTown) @ 2022-11-28 8:39 UTC (permalink / raw) To: Russell King (Oracle); +Cc: Ard Biesheuvel, linux-arm-kernel, linux-kernel On 2022/10/13 18:51, Russell King (Oracle) wrote: > On Tue, Oct 11, 2022 at 10:29:58AM +0800, Leizhen (ThunderTown) wrote: >> On 2022/10/10 19:06, Ard Biesheuvel wrote: >>> On Mon, 10 Oct 2022 at 12:58, Leizhen (ThunderTown) >>> <thunder.leizhen@huawei.com> wrote: >>>> On 2022/10/10 18:20, Ard Biesheuvel wrote: >>>>> On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote: >>>>>> >>>>>> Fix the following warnings: >>>>>> warning: incorrect type in initializer (different address spaces) >>>>>> expected unsigned short [noderef] __user *register __p >>>>>> got unsigned short [usertype] * >>>>>> warning: cast removes address space '__user' of expression >>>>>> >>>>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >>>>>> --- >>>>>> arch/arm/kernel/traps.c | 10 +++++----- >>>>>> 1 file changed, 5 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c >>>>>> index 20b2db6dcd1ced7..34aa80c09c508c1 100644 >>>>>> --- a/arch/arm/kernel/traps.c >>>>>> +++ b/arch/arm/kernel/traps.c >>>>>> @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) >>>>>> } >>>>>> } else { >>>>>> if (thumb) >>>>>> - bad = get_user(val, &((u16 *)addr)[i]); >>>>>> + bad = get_user(val, &((u16 __user *)addr)[i]); >>>>>> else >>>>>> - bad = get_user(val, &((u32 *)addr)[i]); >>>>>> + bad = get_user(val, &((u32 __user *)addr)[i]); >>>>>> } >>>>>> >>>>>> if (!bad) >>>>>> @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) >>>>>> if (processor_mode(regs) == SVC_MODE) { >>>>>> #ifdef CONFIG_THUMB2_KERNEL >>>>>> if (thumb_mode(regs)) { >>>>>> - instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); >>>>>> + instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]); >>>>> >>>>> Shouldn't this be __user as well? (and below) >>>> >>>> unsigned int instr; >>>> void __user *pc; >>>> >>>> The __user can clear the warning, but a new warning will be generated. >>>> >>>> instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); >>>> ^new ^old >>>> >>>> arch/arm/kernel/traps.c:473:33: warning: dereference of noderef expression >>>> >>> >>> This is because dereferencing a __user pointer is not permitted. >>> >>> So this code should be using get_kernel_nofault() here not a plain >>> dereference of PC. So better to fix that properly instead of papering >>> over it with a __force cast just to make sparse happy. >> >> How about: >> @@ -451,9 +451,9 @@ int call_undef_hook(struct pt_regs *regs, unsigned int instr) >> asmlinkage void do_undefinstr(struct pt_regs *regs) >> { >> unsigned int instr; >> - void __user *pc; >> + void *pc; >> >> - pc = (void __user *)instruction_pointer(regs); >> + pc = (void *)instruction_pointer(regs); >> >> if (processor_mode(regs) == SVC_MODE) { >> #ifdef CONFIG_THUMB2_KERNEL >> @@ -497,7 +497,7 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) >> } >> #endif >> arm_notify_die("Oops - undefined instruction", regs, >> - SIGILL, ILL_ILLOPC, pc, 0, 6); >> + SIGILL, ILL_ILLOPC, (void __user *)pc, 0, 6); >> } >> NOKPROBE_SYMBOL(do_undefinstr) >> >> >> The 'pc' may come from kernel or user. And I found that all the get_user() >> calls have already done type casts to the pc, except arm_notify_die(). >> I think the above changes are reasonable. > > If we're going to do that, lets do it properly - I think the above would > need some __force usage to stop sparse complaining, whereas I don't > think this will (untested): The following changes are a little too big, and the above changes have solved the problem completely. In order to focus on the main target, I removed this part of the modification in v4. I hope you'll take the time to review v4. > > diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > index 3f468ac98592..827cbc022900 100644 > --- a/arch/arm/kernel/traps.c > +++ b/arch/arm/kernel/traps.c > @@ -449,36 +449,45 @@ int call_undef_hook(struct pt_regs *regs, unsigned int instr) > asmlinkage void do_undefinstr(struct pt_regs *regs) > { > unsigned int instr; > - void __user *pc; > + unsigned long pc; The following changes need to be added: @@ -487,7 +497,7 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) die_sig: #ifdef CONFIG_DEBUG_USER if (user_debug & UDBG_UNDEFINED) { - pr_info("%s (%d): undefined instruction: pc=%px\n", + pr_info("%s (%d): undefined instruction: pc=%lx\n", current->comm, task_pid_nr(current), pc); __show_regs(regs); dump_instr(KERN_INFO, regs); > > - pc = (void __user *)instruction_pointer(regs); > + pc = instruction_pointer(regs); > > if (processor_mode(regs) == SVC_MODE) { > -#ifdef CONFIG_THUMB2_KERNEL > - if (thumb_mode(regs)) { > - instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); > + if (IS_ENABLED(CONFIG_THUMB2_KERNEL) && thumb_mode(regs)) { > + u16 *tpc = (u16 *)pc; > + > + instr = __mem_to_opcode_thumb16(tpc[0]); > if (is_wide_instruction(instr)) { > u16 inst2; > - inst2 = __mem_to_opcode_thumb16(((u16 *)pc)[1]); > + > + inst2 = __mem_to_opcode_thumb16(tpc[1]); > instr = __opcode_thumb32_compose(instr, inst2); > } > - } else > -#endif > - instr = __mem_to_opcode_arm(*(u32 *) pc); > + } else { > + u32 *apc = (u32 *)pc; > + > + instr = __mem_to_opcode_arm(*apc); > + } > } else if (thumb_mode(regs)) { > - if (get_user(instr, (u16 __user *)pc)) > + u16 __user *tpc = (u16 __user *)pc; > + > + if (get_user(instr, tpc)) > goto die_sig; > instr = __mem_to_opcode_thumb16(instr); > if (is_wide_instruction(instr)) { > unsigned int instr2; > - if (get_user(instr2, (u16 __user *)pc+1)) > + if (get_user(instr2, tpc + 1)) > goto die_sig; > instr2 = __mem_to_opcode_thumb16(instr2); > instr = __opcode_thumb32_compose(instr, instr2); > } > } else { > - if (get_user(instr, (u32 __user *)pc)) > + u32 __user *apc = (u32 __user *)pc; > + > + if (get_user(instr, apc)) > goto die_sig; > + > instr = __mem_to_opcode_arm(instr); > } > > @@ -495,7 +504,7 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) > } > #endif > arm_notify_die("Oops - undefined instruction", regs, > - SIGILL, ILL_ILLOPC, pc, 0, 6); > + SIGILL, ILL_ILLOPC, (void __user *)pc, 0, 6); > } > NOKPROBE_SYMBOL(do_undefinstr) > > -- Regards, Zhen Lei _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse @ 2022-11-28 8:39 ` Leizhen (ThunderTown) 0 siblings, 0 replies; 40+ messages in thread From: Leizhen (ThunderTown) @ 2022-11-28 8:39 UTC (permalink / raw) To: Russell King (Oracle); +Cc: Ard Biesheuvel, linux-arm-kernel, linux-kernel On 2022/10/13 18:51, Russell King (Oracle) wrote: > On Tue, Oct 11, 2022 at 10:29:58AM +0800, Leizhen (ThunderTown) wrote: >> On 2022/10/10 19:06, Ard Biesheuvel wrote: >>> On Mon, 10 Oct 2022 at 12:58, Leizhen (ThunderTown) >>> <thunder.leizhen@huawei.com> wrote: >>>> On 2022/10/10 18:20, Ard Biesheuvel wrote: >>>>> On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote: >>>>>> >>>>>> Fix the following warnings: >>>>>> warning: incorrect type in initializer (different address spaces) >>>>>> expected unsigned short [noderef] __user *register __p >>>>>> got unsigned short [usertype] * >>>>>> warning: cast removes address space '__user' of expression >>>>>> >>>>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >>>>>> --- >>>>>> arch/arm/kernel/traps.c | 10 +++++----- >>>>>> 1 file changed, 5 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c >>>>>> index 20b2db6dcd1ced7..34aa80c09c508c1 100644 >>>>>> --- a/arch/arm/kernel/traps.c >>>>>> +++ b/arch/arm/kernel/traps.c >>>>>> @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) >>>>>> } >>>>>> } else { >>>>>> if (thumb) >>>>>> - bad = get_user(val, &((u16 *)addr)[i]); >>>>>> + bad = get_user(val, &((u16 __user *)addr)[i]); >>>>>> else >>>>>> - bad = get_user(val, &((u32 *)addr)[i]); >>>>>> + bad = get_user(val, &((u32 __user *)addr)[i]); >>>>>> } >>>>>> >>>>>> if (!bad) >>>>>> @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) >>>>>> if (processor_mode(regs) == SVC_MODE) { >>>>>> #ifdef CONFIG_THUMB2_KERNEL >>>>>> if (thumb_mode(regs)) { >>>>>> - instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); >>>>>> + instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]); >>>>> >>>>> Shouldn't this be __user as well? (and below) >>>> >>>> unsigned int instr; >>>> void __user *pc; >>>> >>>> The __user can clear the warning, but a new warning will be generated. >>>> >>>> instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); >>>> ^new ^old >>>> >>>> arch/arm/kernel/traps.c:473:33: warning: dereference of noderef expression >>>> >>> >>> This is because dereferencing a __user pointer is not permitted. >>> >>> So this code should be using get_kernel_nofault() here not a plain >>> dereference of PC. So better to fix that properly instead of papering >>> over it with a __force cast just to make sparse happy. >> >> How about: >> @@ -451,9 +451,9 @@ int call_undef_hook(struct pt_regs *regs, unsigned int instr) >> asmlinkage void do_undefinstr(struct pt_regs *regs) >> { >> unsigned int instr; >> - void __user *pc; >> + void *pc; >> >> - pc = (void __user *)instruction_pointer(regs); >> + pc = (void *)instruction_pointer(regs); >> >> if (processor_mode(regs) == SVC_MODE) { >> #ifdef CONFIG_THUMB2_KERNEL >> @@ -497,7 +497,7 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) >> } >> #endif >> arm_notify_die("Oops - undefined instruction", regs, >> - SIGILL, ILL_ILLOPC, pc, 0, 6); >> + SIGILL, ILL_ILLOPC, (void __user *)pc, 0, 6); >> } >> NOKPROBE_SYMBOL(do_undefinstr) >> >> >> The 'pc' may come from kernel or user. And I found that all the get_user() >> calls have already done type casts to the pc, except arm_notify_die(). >> I think the above changes are reasonable. > > If we're going to do that, lets do it properly - I think the above would > need some __force usage to stop sparse complaining, whereas I don't > think this will (untested): The following changes are a little too big, and the above changes have solved the problem completely. In order to focus on the main target, I removed this part of the modification in v4. I hope you'll take the time to review v4. > > diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > index 3f468ac98592..827cbc022900 100644 > --- a/arch/arm/kernel/traps.c > +++ b/arch/arm/kernel/traps.c > @@ -449,36 +449,45 @@ int call_undef_hook(struct pt_regs *regs, unsigned int instr) > asmlinkage void do_undefinstr(struct pt_regs *regs) > { > unsigned int instr; > - void __user *pc; > + unsigned long pc; The following changes need to be added: @@ -487,7 +497,7 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) die_sig: #ifdef CONFIG_DEBUG_USER if (user_debug & UDBG_UNDEFINED) { - pr_info("%s (%d): undefined instruction: pc=%px\n", + pr_info("%s (%d): undefined instruction: pc=%lx\n", current->comm, task_pid_nr(current), pc); __show_regs(regs); dump_instr(KERN_INFO, regs); > > - pc = (void __user *)instruction_pointer(regs); > + pc = instruction_pointer(regs); > > if (processor_mode(regs) == SVC_MODE) { > -#ifdef CONFIG_THUMB2_KERNEL > - if (thumb_mode(regs)) { > - instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]); > + if (IS_ENABLED(CONFIG_THUMB2_KERNEL) && thumb_mode(regs)) { > + u16 *tpc = (u16 *)pc; > + > + instr = __mem_to_opcode_thumb16(tpc[0]); > if (is_wide_instruction(instr)) { > u16 inst2; > - inst2 = __mem_to_opcode_thumb16(((u16 *)pc)[1]); > + > + inst2 = __mem_to_opcode_thumb16(tpc[1]); > instr = __opcode_thumb32_compose(instr, inst2); > } > - } else > -#endif > - instr = __mem_to_opcode_arm(*(u32 *) pc); > + } else { > + u32 *apc = (u32 *)pc; > + > + instr = __mem_to_opcode_arm(*apc); > + } > } else if (thumb_mode(regs)) { > - if (get_user(instr, (u16 __user *)pc)) > + u16 __user *tpc = (u16 __user *)pc; > + > + if (get_user(instr, tpc)) > goto die_sig; > instr = __mem_to_opcode_thumb16(instr); > if (is_wide_instruction(instr)) { > unsigned int instr2; > - if (get_user(instr2, (u16 __user *)pc+1)) > + if (get_user(instr2, tpc + 1)) > goto die_sig; > instr2 = __mem_to_opcode_thumb16(instr2); > instr = __opcode_thumb32_compose(instr, instr2); > } > } else { > - if (get_user(instr, (u32 __user *)pc)) > + u32 __user *apc = (u32 __user *)pc; > + > + if (get_user(instr, apc)) > goto die_sig; > + > instr = __mem_to_opcode_arm(instr); > } > > @@ -495,7 +504,7 @@ asmlinkage void do_undefinstr(struct pt_regs *regs) > } > #endif > arm_notify_die("Oops - undefined instruction", regs, > - SIGILL, ILL_ILLOPC, pc, 0, 6); > + SIGILL, ILL_ILLOPC, (void __user *)pc, 0, 6); > } > NOKPROBE_SYMBOL(do_undefinstr) > > -- Regards, Zhen Lei ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse 2022-10-10 9:53 ` Zhen Lei @ 2022-10-10 16:05 ` Russell King (Oracle) -1 siblings, 0 replies; 40+ messages in thread From: Russell King (Oracle) @ 2022-10-10 16:05 UTC (permalink / raw) To: Zhen Lei; +Cc: linux-arm-kernel, linux-kernel On Mon, Oct 10, 2022 at 05:53:45PM +0800, Zhen Lei wrote: > Fix the following warnings: > warning: incorrect type in initializer (different address spaces) > expected unsigned short [noderef] __user *register __p > got unsigned short [usertype] * > warning: cast removes address space '__user' of expression I have a general principle that not all warnings should be fixed, especially when it comes to sparse. The aim is not to get to zero warnings - it's to get to a point where the code is correct, and plastering the code with __force casts means it isn't correct - you're just masking the warning. So no, I really don't like this. And I really don't like seeing __force being used in open code in casts. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse @ 2022-10-10 16:05 ` Russell King (Oracle) 0 siblings, 0 replies; 40+ messages in thread From: Russell King (Oracle) @ 2022-10-10 16:05 UTC (permalink / raw) To: Zhen Lei; +Cc: linux-arm-kernel, linux-kernel On Mon, Oct 10, 2022 at 05:53:45PM +0800, Zhen Lei wrote: > Fix the following warnings: > warning: incorrect type in initializer (different address spaces) > expected unsigned short [noderef] __user *register __p > got unsigned short [usertype] * > warning: cast removes address space '__user' of expression I have a general principle that not all warnings should be fixed, especially when it comes to sparse. The aim is not to get to zero warnings - it's to get to a point where the code is correct, and plastering the code with __force casts means it isn't correct - you're just masking the warning. So no, I really don't like this. And I really don't like seeing __force being used in open code in casts. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse 2022-10-10 16:05 ` Russell King (Oracle) @ 2022-10-11 2:13 ` Leizhen (ThunderTown) -1 siblings, 0 replies; 40+ messages in thread From: Leizhen (ThunderTown) @ 2022-10-11 2:13 UTC (permalink / raw) To: Russell King (Oracle); +Cc: linux-arm-kernel, linux-kernel On 2022/10/11 0:05, Russell King (Oracle) wrote: > On Mon, Oct 10, 2022 at 05:53:45PM +0800, Zhen Lei wrote: >> Fix the following warnings: >> warning: incorrect type in initializer (different address spaces) >> expected unsigned short [noderef] __user *register __p >> got unsigned short [usertype] * >> warning: cast removes address space '__user' of expression > > I have a general principle that not all warnings should be fixed, > especially when it comes to sparse. OK, I got it. > > The aim is not to get to zero warnings - it's to get to a point where > the code is correct, and plastering the code with __force casts means > it isn't correct - you're just masking the warning. > > So no, I really don't like this. And I really don't like seeing > __force being used in open code in casts. OK, I will clear only the first warning and leave the second warning. > -- Regards, Zhen Lei _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse @ 2022-10-11 2:13 ` Leizhen (ThunderTown) 0 siblings, 0 replies; 40+ messages in thread From: Leizhen (ThunderTown) @ 2022-10-11 2:13 UTC (permalink / raw) To: Russell King (Oracle); +Cc: linux-arm-kernel, linux-kernel On 2022/10/11 0:05, Russell King (Oracle) wrote: > On Mon, Oct 10, 2022 at 05:53:45PM +0800, Zhen Lei wrote: >> Fix the following warnings: >> warning: incorrect type in initializer (different address spaces) >> expected unsigned short [noderef] __user *register __p >> got unsigned short [usertype] * >> warning: cast removes address space '__user' of expression > > I have a general principle that not all warnings should be fixed, > especially when it comes to sparse. OK, I got it. > > The aim is not to get to zero warnings - it's to get to a point where > the code is correct, and plastering the code with __force casts means > it isn't correct - you're just masking the warning. > > So no, I really don't like this. And I really don't like seeing > __force being used in open code in casts. OK, I will clear only the first warning and leave the second warning. > -- Regards, Zhen Lei ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse 2022-10-11 2:13 ` Leizhen (ThunderTown) @ 2022-10-13 1:28 ` Leizhen (ThunderTown) -1 siblings, 0 replies; 40+ messages in thread From: Leizhen (ThunderTown) @ 2022-10-13 1:28 UTC (permalink / raw) To: Russell King (Oracle); +Cc: linux-arm-kernel, linux-kernel On 2022/10/11 10:13, Leizhen (ThunderTown) wrote: > > > On 2022/10/11 0:05, Russell King (Oracle) wrote: >> On Mon, Oct 10, 2022 at 05:53:45PM +0800, Zhen Lei wrote: >>> Fix the following warnings: >>> warning: incorrect type in initializer (different address spaces) >>> expected unsigned short [noderef] __user *register __p >>> got unsigned short [usertype] * >>> warning: cast removes address space '__user' of expression >> >> I have a general principle that not all warnings should be fixed, >> especially when it comes to sparse. > > OK, I got it. > >> >> The aim is not to get to zero warnings - it's to get to a point where >> the code is correct, and plastering the code with __force casts means >> it isn't correct - you're just masking the warning. >> >> So no, I really don't like this. And I really don't like seeing >> __force being used in open code in casts. > > OK, I will clear only the first warning and leave the second warning. Sorry, Maybe I misunderstood. '__force casts' mentioned here may include not only '(__force u16 *)', but also '(u16 __user *)'. > >> > -- Regards, Zhen Lei _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse @ 2022-10-13 1:28 ` Leizhen (ThunderTown) 0 siblings, 0 replies; 40+ messages in thread From: Leizhen (ThunderTown) @ 2022-10-13 1:28 UTC (permalink / raw) To: Russell King (Oracle); +Cc: linux-arm-kernel, linux-kernel On 2022/10/11 10:13, Leizhen (ThunderTown) wrote: > > > On 2022/10/11 0:05, Russell King (Oracle) wrote: >> On Mon, Oct 10, 2022 at 05:53:45PM +0800, Zhen Lei wrote: >>> Fix the following warnings: >>> warning: incorrect type in initializer (different address spaces) >>> expected unsigned short [noderef] __user *register __p >>> got unsigned short [usertype] * >>> warning: cast removes address space '__user' of expression >> >> I have a general principle that not all warnings should be fixed, >> especially when it comes to sparse. > > OK, I got it. > >> >> The aim is not to get to zero warnings - it's to get to a point where >> the code is correct, and plastering the code with __force casts means >> it isn't correct - you're just masking the warning. >> >> So no, I really don't like this. And I really don't like seeing >> __force being used in open code in casts. > > OK, I will clear only the first warning and leave the second warning. Sorry, Maybe I misunderstood. '__force casts' mentioned here may include not only '(__force u16 *)', but also '(u16 __user *)'. > >> > -- Regards, Zhen Lei ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 2/2] ARM: Make the dumped instructions are consistent with the disassembled ones 2022-10-10 9:53 ` Zhen Lei @ 2022-10-10 9:53 ` Zhen Lei -1 siblings, 0 replies; 40+ messages in thread From: Zhen Lei @ 2022-10-10 9:53 UTC (permalink / raw) To: Russell King, linux-arm-kernel, linux-kernel; +Cc: Zhen Lei In ARM, the mapping of instruction memory is always little-endian, except some BE-32 supported ARM architectures. Such as ARMv7-R, its instruction endianness may be BE-32. Of course, its data endianness will also be BE-32 mode. Due to two negatives make a positive, the instruction stored in the register after reading is in little-endian format. But for the case of BE-8, the instruction endianness is LE, the instruction stored in the register after reading is in big-endian format, which is inconsistent with the disassembled one. For example: The content of disassembly: c0429ee8: e3500000 cmp r0, #0 c0429eec: 159f2044 ldrne r2, [pc, #68] c0429ef0: 108f2002 addne r2, pc, r2 c0429ef4: 1882000a stmne r2, {r1, r3} c0429ef8: e7f000f0 udf #0 The output of undefined instruction exception: Internal error: Oops - undefined instruction: 0 [#1] SMP ARM ... ... Code: 000050e3 44209f15 02208f10 0a008218 (f000f0e7) This inconveniences the checking of instructions. What's worse is that, for somebody who don't know about this, might think the instructions are all broken. So, when CONFIG_CPU_ENDIAN_BE8=y, let's convert the instructions to little-endian format before they are printed. The conversion result is as follows: Code: e3500000 159f2044 108f2002 1882000a (e7f000f0) Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> --- arch/arm/kernel/traps.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c index 34aa80c09c508c1..50b00c9091f079d 100644 --- a/arch/arm/kernel/traps.c +++ b/arch/arm/kernel/traps.c @@ -193,6 +193,13 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) bad = get_user(val, &((u32 __user *)addr)[i]); } + if (IS_ENABLED(CONFIG_CPU_ENDIAN_BE8)) { + if (thumb) + val = (__force unsigned int)cpu_to_le16(val); + else + val = (__force unsigned int)cpu_to_le32(val); + } + if (!bad) p += sprintf(p, i == 0 ? "(%0*x) " : "%0*x ", width, val); -- 2.25.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v2 2/2] ARM: Make the dumped instructions are consistent with the disassembled ones @ 2022-10-10 9:53 ` Zhen Lei 0 siblings, 0 replies; 40+ messages in thread From: Zhen Lei @ 2022-10-10 9:53 UTC (permalink / raw) To: Russell King, linux-arm-kernel, linux-kernel; +Cc: Zhen Lei In ARM, the mapping of instruction memory is always little-endian, except some BE-32 supported ARM architectures. Such as ARMv7-R, its instruction endianness may be BE-32. Of course, its data endianness will also be BE-32 mode. Due to two negatives make a positive, the instruction stored in the register after reading is in little-endian format. But for the case of BE-8, the instruction endianness is LE, the instruction stored in the register after reading is in big-endian format, which is inconsistent with the disassembled one. For example: The content of disassembly: c0429ee8: e3500000 cmp r0, #0 c0429eec: 159f2044 ldrne r2, [pc, #68] c0429ef0: 108f2002 addne r2, pc, r2 c0429ef4: 1882000a stmne r2, {r1, r3} c0429ef8: e7f000f0 udf #0 The output of undefined instruction exception: Internal error: Oops - undefined instruction: 0 [#1] SMP ARM ... ... Code: 000050e3 44209f15 02208f10 0a008218 (f000f0e7) This inconveniences the checking of instructions. What's worse is that, for somebody who don't know about this, might think the instructions are all broken. So, when CONFIG_CPU_ENDIAN_BE8=y, let's convert the instructions to little-endian format before they are printed. The conversion result is as follows: Code: e3500000 159f2044 108f2002 1882000a (e7f000f0) Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> --- arch/arm/kernel/traps.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c index 34aa80c09c508c1..50b00c9091f079d 100644 --- a/arch/arm/kernel/traps.c +++ b/arch/arm/kernel/traps.c @@ -193,6 +193,13 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) bad = get_user(val, &((u32 __user *)addr)[i]); } + if (IS_ENABLED(CONFIG_CPU_ENDIAN_BE8)) { + if (thumb) + val = (__force unsigned int)cpu_to_le16(val); + else + val = (__force unsigned int)cpu_to_le32(val); + } + if (!bad) p += sprintf(p, i == 0 ? "(%0*x) " : "%0*x ", width, val); -- 2.25.1 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v2 2/2] ARM: Make the dumped instructions are consistent with the disassembled ones 2022-10-10 9:53 ` Zhen Lei @ 2022-10-10 10:10 ` Ard Biesheuvel -1 siblings, 0 replies; 40+ messages in thread From: Ard Biesheuvel @ 2022-10-10 10:10 UTC (permalink / raw) To: Zhen Lei; +Cc: Russell King, linux-arm-kernel, linux-kernel On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote: > > In ARM, the mapping of instruction memory is always little-endian, except > some BE-32 supported ARM architectures. Such as ARMv7-R, its instruction > endianness may be BE-32. Of course, its data endianness will also be BE-32 > mode. Due to two negatives make a positive, the instruction stored in the > register after reading is in little-endian format. But for the case of > BE-8, the instruction endianness is LE, the instruction stored in the > register after reading is in big-endian format, which is inconsistent > with the disassembled one. > > For example: > The content of disassembly: > c0429ee8: e3500000 cmp r0, #0 > c0429eec: 159f2044 ldrne r2, [pc, #68] > c0429ef0: 108f2002 addne r2, pc, r2 > c0429ef4: 1882000a stmne r2, {r1, r3} > c0429ef8: e7f000f0 udf #0 > > The output of undefined instruction exception: > Internal error: Oops - undefined instruction: 0 [#1] SMP ARM > ... ... > Code: 000050e3 44209f15 02208f10 0a008218 (f000f0e7) > > This inconveniences the checking of instructions. What's worse is that, > for somebody who don't know about this, might think the instructions are > all broken. > > So, when CONFIG_CPU_ENDIAN_BE8=y, let's convert the instructions to > little-endian format before they are printed. The conversion result is > as follows: > Code: e3500000 159f2044 108f2002 1882000a (e7f000f0) > > Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > --- > arch/arm/kernel/traps.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > index 34aa80c09c508c1..50b00c9091f079d 100644 > --- a/arch/arm/kernel/traps.c > +++ b/arch/arm/kernel/traps.c > @@ -193,6 +193,13 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) > bad = get_user(val, &((u32 __user *)addr)[i]); > } > > + if (IS_ENABLED(CONFIG_CPU_ENDIAN_BE8)) { > + if (thumb) > + val = (__force unsigned int)cpu_to_le16(val); Better use swab16() here instead of the ugly __force cast, given that the swab is going to occur unconditionally here. > + else > + val = (__force unsigned int)cpu_to_le32(val); and swab32() here > + } > + > if (!bad) > p += sprintf(p, i == 0 ? "(%0*x) " : "%0*x ", > width, val); > -- > 2.25.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 2/2] ARM: Make the dumped instructions are consistent with the disassembled ones @ 2022-10-10 10:10 ` Ard Biesheuvel 0 siblings, 0 replies; 40+ messages in thread From: Ard Biesheuvel @ 2022-10-10 10:10 UTC (permalink / raw) To: Zhen Lei; +Cc: Russell King, linux-arm-kernel, linux-kernel On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote: > > In ARM, the mapping of instruction memory is always little-endian, except > some BE-32 supported ARM architectures. Such as ARMv7-R, its instruction > endianness may be BE-32. Of course, its data endianness will also be BE-32 > mode. Due to two negatives make a positive, the instruction stored in the > register after reading is in little-endian format. But for the case of > BE-8, the instruction endianness is LE, the instruction stored in the > register after reading is in big-endian format, which is inconsistent > with the disassembled one. > > For example: > The content of disassembly: > c0429ee8: e3500000 cmp r0, #0 > c0429eec: 159f2044 ldrne r2, [pc, #68] > c0429ef0: 108f2002 addne r2, pc, r2 > c0429ef4: 1882000a stmne r2, {r1, r3} > c0429ef8: e7f000f0 udf #0 > > The output of undefined instruction exception: > Internal error: Oops - undefined instruction: 0 [#1] SMP ARM > ... ... > Code: 000050e3 44209f15 02208f10 0a008218 (f000f0e7) > > This inconveniences the checking of instructions. What's worse is that, > for somebody who don't know about this, might think the instructions are > all broken. > > So, when CONFIG_CPU_ENDIAN_BE8=y, let's convert the instructions to > little-endian format before they are printed. The conversion result is > as follows: > Code: e3500000 159f2044 108f2002 1882000a (e7f000f0) > > Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > --- > arch/arm/kernel/traps.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > index 34aa80c09c508c1..50b00c9091f079d 100644 > --- a/arch/arm/kernel/traps.c > +++ b/arch/arm/kernel/traps.c > @@ -193,6 +193,13 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) > bad = get_user(val, &((u32 __user *)addr)[i]); > } > > + if (IS_ENABLED(CONFIG_CPU_ENDIAN_BE8)) { > + if (thumb) > + val = (__force unsigned int)cpu_to_le16(val); Better use swab16() here instead of the ugly __force cast, given that the swab is going to occur unconditionally here. > + else > + val = (__force unsigned int)cpu_to_le32(val); and swab32() here > + } > + > if (!bad) > p += sprintf(p, i == 0 ? "(%0*x) " : "%0*x ", > width, val); > -- > 2.25.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 2/2] ARM: Make the dumped instructions are consistent with the disassembled ones 2022-10-10 10:10 ` Ard Biesheuvel @ 2022-10-10 10:46 ` Leizhen (ThunderTown) -1 siblings, 0 replies; 40+ messages in thread From: Leizhen (ThunderTown) @ 2022-10-10 10:46 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: Russell King, linux-arm-kernel, linux-kernel On 2022/10/10 18:10, Ard Biesheuvel wrote: > On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote: >> >> In ARM, the mapping of instruction memory is always little-endian, except >> some BE-32 supported ARM architectures. Such as ARMv7-R, its instruction >> endianness may be BE-32. Of course, its data endianness will also be BE-32 >> mode. Due to two negatives make a positive, the instruction stored in the >> register after reading is in little-endian format. But for the case of >> BE-8, the instruction endianness is LE, the instruction stored in the >> register after reading is in big-endian format, which is inconsistent >> with the disassembled one. >> >> For example: >> The content of disassembly: >> c0429ee8: e3500000 cmp r0, #0 >> c0429eec: 159f2044 ldrne r2, [pc, #68] >> c0429ef0: 108f2002 addne r2, pc, r2 >> c0429ef4: 1882000a stmne r2, {r1, r3} >> c0429ef8: e7f000f0 udf #0 >> >> The output of undefined instruction exception: >> Internal error: Oops - undefined instruction: 0 [#1] SMP ARM >> ... ... >> Code: 000050e3 44209f15 02208f10 0a008218 (f000f0e7) >> >> This inconveniences the checking of instructions. What's worse is that, >> for somebody who don't know about this, might think the instructions are >> all broken. >> >> So, when CONFIG_CPU_ENDIAN_BE8=y, let's convert the instructions to >> little-endian format before they are printed. The conversion result is >> as follows: >> Code: e3500000 159f2044 108f2002 1882000a (e7f000f0) >> >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >> --- >> arch/arm/kernel/traps.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c >> index 34aa80c09c508c1..50b00c9091f079d 100644 >> --- a/arch/arm/kernel/traps.c >> +++ b/arch/arm/kernel/traps.c >> @@ -193,6 +193,13 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) >> bad = get_user(val, &((u32 __user *)addr)[i]); >> } >> >> + if (IS_ENABLED(CONFIG_CPU_ENDIAN_BE8)) { >> + if (thumb) >> + val = (__force unsigned int)cpu_to_le16(val); > > Better use swab16() here instead of the ugly __force cast, given that > the swab is going to occur unconditionally here. Good idea. > > >> + else >> + val = (__force unsigned int)cpu_to_le32(val); > > and swab32() here OK > > >> + } >> + >> if (!bad) >> p += sprintf(p, i == 0 ? "(%0*x) " : "%0*x ", >> width, val); >> -- >> 2.25.1 >> >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > . > -- Regards, Zhen Lei _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 2/2] ARM: Make the dumped instructions are consistent with the disassembled ones @ 2022-10-10 10:46 ` Leizhen (ThunderTown) 0 siblings, 0 replies; 40+ messages in thread From: Leizhen (ThunderTown) @ 2022-10-10 10:46 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: Russell King, linux-arm-kernel, linux-kernel On 2022/10/10 18:10, Ard Biesheuvel wrote: > On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote: >> >> In ARM, the mapping of instruction memory is always little-endian, except >> some BE-32 supported ARM architectures. Such as ARMv7-R, its instruction >> endianness may be BE-32. Of course, its data endianness will also be BE-32 >> mode. Due to two negatives make a positive, the instruction stored in the >> register after reading is in little-endian format. But for the case of >> BE-8, the instruction endianness is LE, the instruction stored in the >> register after reading is in big-endian format, which is inconsistent >> with the disassembled one. >> >> For example: >> The content of disassembly: >> c0429ee8: e3500000 cmp r0, #0 >> c0429eec: 159f2044 ldrne r2, [pc, #68] >> c0429ef0: 108f2002 addne r2, pc, r2 >> c0429ef4: 1882000a stmne r2, {r1, r3} >> c0429ef8: e7f000f0 udf #0 >> >> The output of undefined instruction exception: >> Internal error: Oops - undefined instruction: 0 [#1] SMP ARM >> ... ... >> Code: 000050e3 44209f15 02208f10 0a008218 (f000f0e7) >> >> This inconveniences the checking of instructions. What's worse is that, >> for somebody who don't know about this, might think the instructions are >> all broken. >> >> So, when CONFIG_CPU_ENDIAN_BE8=y, let's convert the instructions to >> little-endian format before they are printed. The conversion result is >> as follows: >> Code: e3500000 159f2044 108f2002 1882000a (e7f000f0) >> >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >> --- >> arch/arm/kernel/traps.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c >> index 34aa80c09c508c1..50b00c9091f079d 100644 >> --- a/arch/arm/kernel/traps.c >> +++ b/arch/arm/kernel/traps.c >> @@ -193,6 +193,13 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) >> bad = get_user(val, &((u32 __user *)addr)[i]); >> } >> >> + if (IS_ENABLED(CONFIG_CPU_ENDIAN_BE8)) { >> + if (thumb) >> + val = (__force unsigned int)cpu_to_le16(val); > > Better use swab16() here instead of the ugly __force cast, given that > the swab is going to occur unconditionally here. Good idea. > > >> + else >> + val = (__force unsigned int)cpu_to_le32(val); > > and swab32() here OK > > >> + } >> + >> if (!bad) >> p += sprintf(p, i == 0 ? "(%0*x) " : "%0*x ", >> width, val); >> -- >> 2.25.1 >> >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > . > -- Regards, Zhen Lei ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 2/2] ARM: Make the dumped instructions are consistent with the disassembled ones 2022-10-10 10:46 ` Leizhen (ThunderTown) @ 2022-10-10 11:07 ` Ard Biesheuvel -1 siblings, 0 replies; 40+ messages in thread From: Ard Biesheuvel @ 2022-10-10 11:07 UTC (permalink / raw) To: Leizhen (ThunderTown); +Cc: Russell King, linux-arm-kernel, linux-kernel On Mon, 10 Oct 2022 at 12:46, Leizhen (ThunderTown) <thunder.leizhen@huawei.com> wrote: > > > > On 2022/10/10 18:10, Ard Biesheuvel wrote: > > On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote: > >> > >> In ARM, the mapping of instruction memory is always little-endian, except > >> some BE-32 supported ARM architectures. Such as ARMv7-R, its instruction > >> endianness may be BE-32. Of course, its data endianness will also be BE-32 > >> mode. Due to two negatives make a positive, the instruction stored in the > >> register after reading is in little-endian format. But for the case of > >> BE-8, the instruction endianness is LE, the instruction stored in the > >> register after reading is in big-endian format, which is inconsistent > >> with the disassembled one. > >> > >> For example: > >> The content of disassembly: > >> c0429ee8: e3500000 cmp r0, #0 > >> c0429eec: 159f2044 ldrne r2, [pc, #68] > >> c0429ef0: 108f2002 addne r2, pc, r2 > >> c0429ef4: 1882000a stmne r2, {r1, r3} > >> c0429ef8: e7f000f0 udf #0 > >> > >> The output of undefined instruction exception: > >> Internal error: Oops - undefined instruction: 0 [#1] SMP ARM > >> ... ... > >> Code: 000050e3 44209f15 02208f10 0a008218 (f000f0e7) > >> > >> This inconveniences the checking of instructions. What's worse is that, > >> for somebody who don't know about this, might think the instructions are > >> all broken. > >> > >> So, when CONFIG_CPU_ENDIAN_BE8=y, let's convert the instructions to > >> little-endian format before they are printed. The conversion result is > >> as follows: > >> Code: e3500000 159f2044 108f2002 1882000a (e7f000f0) > >> > >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > >> --- > >> arch/arm/kernel/traps.c | 7 +++++++ > >> 1 file changed, 7 insertions(+) > >> > >> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > >> index 34aa80c09c508c1..50b00c9091f079d 100644 > >> --- a/arch/arm/kernel/traps.c > >> +++ b/arch/arm/kernel/traps.c > >> @@ -193,6 +193,13 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) > >> bad = get_user(val, &((u32 __user *)addr)[i]); > >> } > >> > >> + if (IS_ENABLED(CONFIG_CPU_ENDIAN_BE8)) { > >> + if (thumb) > >> + val = (__force unsigned int)cpu_to_le16(val); > > > > Better use swab16() here instead of the ugly __force cast, given that > > the swab is going to occur unconditionally here. > > Good idea. > > > > > > >> + else > >> + val = (__force unsigned int)cpu_to_le32(val); > > > > and swab32() here > > OK > Actually, come to think of it, should this code perhaps be using the mem_to_opcode helpers that are being used elsewhere in the file? _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 2/2] ARM: Make the dumped instructions are consistent with the disassembled ones @ 2022-10-10 11:07 ` Ard Biesheuvel 0 siblings, 0 replies; 40+ messages in thread From: Ard Biesheuvel @ 2022-10-10 11:07 UTC (permalink / raw) To: Leizhen (ThunderTown); +Cc: Russell King, linux-arm-kernel, linux-kernel On Mon, 10 Oct 2022 at 12:46, Leizhen (ThunderTown) <thunder.leizhen@huawei.com> wrote: > > > > On 2022/10/10 18:10, Ard Biesheuvel wrote: > > On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote: > >> > >> In ARM, the mapping of instruction memory is always little-endian, except > >> some BE-32 supported ARM architectures. Such as ARMv7-R, its instruction > >> endianness may be BE-32. Of course, its data endianness will also be BE-32 > >> mode. Due to two negatives make a positive, the instruction stored in the > >> register after reading is in little-endian format. But for the case of > >> BE-8, the instruction endianness is LE, the instruction stored in the > >> register after reading is in big-endian format, which is inconsistent > >> with the disassembled one. > >> > >> For example: > >> The content of disassembly: > >> c0429ee8: e3500000 cmp r0, #0 > >> c0429eec: 159f2044 ldrne r2, [pc, #68] > >> c0429ef0: 108f2002 addne r2, pc, r2 > >> c0429ef4: 1882000a stmne r2, {r1, r3} > >> c0429ef8: e7f000f0 udf #0 > >> > >> The output of undefined instruction exception: > >> Internal error: Oops - undefined instruction: 0 [#1] SMP ARM > >> ... ... > >> Code: 000050e3 44209f15 02208f10 0a008218 (f000f0e7) > >> > >> This inconveniences the checking of instructions. What's worse is that, > >> for somebody who don't know about this, might think the instructions are > >> all broken. > >> > >> So, when CONFIG_CPU_ENDIAN_BE8=y, let's convert the instructions to > >> little-endian format before they are printed. The conversion result is > >> as follows: > >> Code: e3500000 159f2044 108f2002 1882000a (e7f000f0) > >> > >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > >> --- > >> arch/arm/kernel/traps.c | 7 +++++++ > >> 1 file changed, 7 insertions(+) > >> > >> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > >> index 34aa80c09c508c1..50b00c9091f079d 100644 > >> --- a/arch/arm/kernel/traps.c > >> +++ b/arch/arm/kernel/traps.c > >> @@ -193,6 +193,13 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) > >> bad = get_user(val, &((u32 __user *)addr)[i]); > >> } > >> > >> + if (IS_ENABLED(CONFIG_CPU_ENDIAN_BE8)) { > >> + if (thumb) > >> + val = (__force unsigned int)cpu_to_le16(val); > > > > Better use swab16() here instead of the ugly __force cast, given that > > the swab is going to occur unconditionally here. > > Good idea. > > > > > > >> + else > >> + val = (__force unsigned int)cpu_to_le32(val); > > > > and swab32() here > > OK > Actually, come to think of it, should this code perhaps be using the mem_to_opcode helpers that are being used elsewhere in the file? ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 2/2] ARM: Make the dumped instructions are consistent with the disassembled ones 2022-10-10 11:07 ` Ard Biesheuvel @ 2022-10-10 11:29 ` Leizhen (ThunderTown) -1 siblings, 0 replies; 40+ messages in thread From: Leizhen (ThunderTown) @ 2022-10-10 11:29 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: Russell King, linux-arm-kernel, linux-kernel On 2022/10/10 19:07, Ard Biesheuvel wrote: > On Mon, 10 Oct 2022 at 12:46, Leizhen (ThunderTown) > <thunder.leizhen@huawei.com> wrote: >> >> >> >> On 2022/10/10 18:10, Ard Biesheuvel wrote: >>> On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote: >>>> >>>> In ARM, the mapping of instruction memory is always little-endian, except >>>> some BE-32 supported ARM architectures. Such as ARMv7-R, its instruction >>>> endianness may be BE-32. Of course, its data endianness will also be BE-32 >>>> mode. Due to two negatives make a positive, the instruction stored in the >>>> register after reading is in little-endian format. But for the case of >>>> BE-8, the instruction endianness is LE, the instruction stored in the >>>> register after reading is in big-endian format, which is inconsistent >>>> with the disassembled one. >>>> >>>> For example: >>>> The content of disassembly: >>>> c0429ee8: e3500000 cmp r0, #0 >>>> c0429eec: 159f2044 ldrne r2, [pc, #68] >>>> c0429ef0: 108f2002 addne r2, pc, r2 >>>> c0429ef4: 1882000a stmne r2, {r1, r3} >>>> c0429ef8: e7f000f0 udf #0 >>>> >>>> The output of undefined instruction exception: >>>> Internal error: Oops - undefined instruction: 0 [#1] SMP ARM >>>> ... ... >>>> Code: 000050e3 44209f15 02208f10 0a008218 (f000f0e7) >>>> >>>> This inconveniences the checking of instructions. What's worse is that, >>>> for somebody who don't know about this, might think the instructions are >>>> all broken. >>>> >>>> So, when CONFIG_CPU_ENDIAN_BE8=y, let's convert the instructions to >>>> little-endian format before they are printed. The conversion result is >>>> as follows: >>>> Code: e3500000 159f2044 108f2002 1882000a (e7f000f0) >>>> >>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >>>> --- >>>> arch/arm/kernel/traps.c | 7 +++++++ >>>> 1 file changed, 7 insertions(+) >>>> >>>> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c >>>> index 34aa80c09c508c1..50b00c9091f079d 100644 >>>> --- a/arch/arm/kernel/traps.c >>>> +++ b/arch/arm/kernel/traps.c >>>> @@ -193,6 +193,13 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) >>>> bad = get_user(val, &((u32 __user *)addr)[i]); >>>> } >>>> >>>> + if (IS_ENABLED(CONFIG_CPU_ENDIAN_BE8)) { >>>> + if (thumb) >>>> + val = (__force unsigned int)cpu_to_le16(val); >>> >>> Better use swab16() here instead of the ugly __force cast, given that >>> the swab is going to occur unconditionally here. >> >> Good idea. >> >>> >>> >>>> + else >>>> + val = (__force unsigned int)cpu_to_le32(val); >>> >>> and swab32() here >> >> OK >> > > Actually, come to think of it, should this code perhaps be using the > mem_to_opcode helpers that are being used elsewhere in the file? Right, __mem_to_opcode_xxx is the correct solution. I need to test it. > . > -- Regards, Zhen Lei _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 2/2] ARM: Make the dumped instructions are consistent with the disassembled ones @ 2022-10-10 11:29 ` Leizhen (ThunderTown) 0 siblings, 0 replies; 40+ messages in thread From: Leizhen (ThunderTown) @ 2022-10-10 11:29 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: Russell King, linux-arm-kernel, linux-kernel On 2022/10/10 19:07, Ard Biesheuvel wrote: > On Mon, 10 Oct 2022 at 12:46, Leizhen (ThunderTown) > <thunder.leizhen@huawei.com> wrote: >> >> >> >> On 2022/10/10 18:10, Ard Biesheuvel wrote: >>> On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote: >>>> >>>> In ARM, the mapping of instruction memory is always little-endian, except >>>> some BE-32 supported ARM architectures. Such as ARMv7-R, its instruction >>>> endianness may be BE-32. Of course, its data endianness will also be BE-32 >>>> mode. Due to two negatives make a positive, the instruction stored in the >>>> register after reading is in little-endian format. But for the case of >>>> BE-8, the instruction endianness is LE, the instruction stored in the >>>> register after reading is in big-endian format, which is inconsistent >>>> with the disassembled one. >>>> >>>> For example: >>>> The content of disassembly: >>>> c0429ee8: e3500000 cmp r0, #0 >>>> c0429eec: 159f2044 ldrne r2, [pc, #68] >>>> c0429ef0: 108f2002 addne r2, pc, r2 >>>> c0429ef4: 1882000a stmne r2, {r1, r3} >>>> c0429ef8: e7f000f0 udf #0 >>>> >>>> The output of undefined instruction exception: >>>> Internal error: Oops - undefined instruction: 0 [#1] SMP ARM >>>> ... ... >>>> Code: 000050e3 44209f15 02208f10 0a008218 (f000f0e7) >>>> >>>> This inconveniences the checking of instructions. What's worse is that, >>>> for somebody who don't know about this, might think the instructions are >>>> all broken. >>>> >>>> So, when CONFIG_CPU_ENDIAN_BE8=y, let's convert the instructions to >>>> little-endian format before they are printed. The conversion result is >>>> as follows: >>>> Code: e3500000 159f2044 108f2002 1882000a (e7f000f0) >>>> >>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >>>> --- >>>> arch/arm/kernel/traps.c | 7 +++++++ >>>> 1 file changed, 7 insertions(+) >>>> >>>> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c >>>> index 34aa80c09c508c1..50b00c9091f079d 100644 >>>> --- a/arch/arm/kernel/traps.c >>>> +++ b/arch/arm/kernel/traps.c >>>> @@ -193,6 +193,13 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) >>>> bad = get_user(val, &((u32 __user *)addr)[i]); >>>> } >>>> >>>> + if (IS_ENABLED(CONFIG_CPU_ENDIAN_BE8)) { >>>> + if (thumb) >>>> + val = (__force unsigned int)cpu_to_le16(val); >>> >>> Better use swab16() here instead of the ugly __force cast, given that >>> the swab is going to occur unconditionally here. >> >> Good idea. >> >>> >>> >>>> + else >>>> + val = (__force unsigned int)cpu_to_le32(val); >>> >>> and swab32() here >> >> OK >> > > Actually, come to think of it, should this code perhaps be using the > mem_to_opcode helpers that are being used elsewhere in the file? Right, __mem_to_opcode_xxx is the correct solution. I need to test it. > . > -- Regards, Zhen Lei ^ permalink raw reply [flat|nested] 40+ messages in thread
end of thread, other threads:[~2022-11-28 8:40 UTC | newest] Thread overview: 40+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-10-10 9:53 [PATCH v2 0/2] ARM: Make the dumped instructions are consistent with the disassembled ones Zhen Lei 2022-10-10 9:53 ` Zhen Lei 2022-10-10 9:53 ` [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse Zhen Lei 2022-10-10 9:53 ` Zhen Lei 2022-10-10 10:20 ` Ard Biesheuvel 2022-10-10 10:20 ` Ard Biesheuvel 2022-10-10 10:58 ` Leizhen (ThunderTown) 2022-10-10 10:58 ` Leizhen (ThunderTown) 2022-10-10 11:06 ` Ard Biesheuvel 2022-10-10 11:06 ` Ard Biesheuvel 2022-10-10 16:08 ` Russell King (Oracle) 2022-10-10 16:08 ` Russell King (Oracle) 2022-10-10 16:14 ` Ard Biesheuvel 2022-10-10 16:14 ` Ard Biesheuvel 2022-10-10 16:17 ` Russell King (Oracle) 2022-10-10 16:17 ` Russell King (Oracle) 2022-10-11 2:29 ` Leizhen (ThunderTown) 2022-10-11 2:29 ` Leizhen (ThunderTown) 2022-10-13 10:51 ` Russell King (Oracle) 2022-10-13 10:51 ` Russell King (Oracle) 2022-10-13 11:34 ` Leizhen (ThunderTown) 2022-10-13 11:34 ` Leizhen (ThunderTown) 2022-11-28 8:39 ` Leizhen (ThunderTown) 2022-11-28 8:39 ` Leizhen (ThunderTown) 2022-10-10 16:05 ` Russell King (Oracle) 2022-10-10 16:05 ` Russell King (Oracle) 2022-10-11 2:13 ` Leizhen (ThunderTown) 2022-10-11 2:13 ` Leizhen (ThunderTown) 2022-10-13 1:28 ` Leizhen (ThunderTown) 2022-10-13 1:28 ` Leizhen (ThunderTown) 2022-10-10 9:53 ` [PATCH v2 2/2] ARM: Make the dumped instructions are consistent with the disassembled ones Zhen Lei 2022-10-10 9:53 ` Zhen Lei 2022-10-10 10:10 ` Ard Biesheuvel 2022-10-10 10:10 ` Ard Biesheuvel 2022-10-10 10:46 ` Leizhen (ThunderTown) 2022-10-10 10:46 ` Leizhen (ThunderTown) 2022-10-10 11:07 ` Ard Biesheuvel 2022-10-10 11:07 ` Ard Biesheuvel 2022-10-10 11:29 ` Leizhen (ThunderTown) 2022-10-10 11:29 ` Leizhen (ThunderTown)
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.