Messages in this thread Patch in this message | | | Date | Thu, 13 Oct 2022 11:51:40 +0100 | From | "Russell King (Oracle)" <> | Subject | Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse |
| |
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!
| |