Messages in this thread Patch in this message | | | Subject | Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse | From | "Leizhen (ThunderTown)" <> | Date | Mon, 28 Nov 2022 16:39:20 +0800 |
| |
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
| |