| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 13 Oct 2022 11:51:40 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: "Leizhen (ThunderTown)" <thunder.leizhen@...wei.com>
Cc: Ard Biesheuvel <ardb@...nel.org>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
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@...wei.com> wrote:
> >> On 2022/10/10 18:20, Ard Biesheuvel wrote:
> >>> On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@...wei.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@...wei.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!
Powered by blists - more mailing lists