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, 17 Sep 2020 19:29:37 +0200 From: Arnd Bergmann <arnd@...db.de> To: Christoph Hellwig <hch@....de> Cc: Russell King <rmk@....linux.org.uk>, Alexander Viro <viro@...iv.linux.org.uk>, linux- <kernel@...r.kernel.org>, linux-arch <linux-arch@...r.kernel.org>, Linux ARM <linux-arm-kernel@...ts.infradead.org>, Linus Walleij <linus.walleij@...aro.org>, Russell King <linux@...linux.org.uk>, Andrew Morton <akpm@...ux-foundation.org>, Dmitry Safonov <0x7f454c46@...il.com>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org> Subject: Re: [PATCH 2/9] ARM: traps: use get_kernel_nofault instead of set_fs() On Tue, Sep 8, 2020 at 8:15 AM Christoph Hellwig <hch@....de> wrote: > > > +static void dump_mem(const char *, const char *, unsigned long, unsigned long, bool kernel_mode); > > This adds a pointlessly long line. Fixed. > And looking at the code I don't see why the argument is even needed. > > dump_mem() currently does an unconditional set_fs(KERNEL_DS), so it > should always use get_kernel_nofault. I had looked at if (!user_mode(regs) || in_interrupt()) { dump_mem(KERN_EMERG, "Stack: ", regs->ARM_sp, THREAD_SIZE + (unsigned long)task_stack_page(tsk), kernel_mode); which told me that there should be at least some code path ending up in __die() that has in_interrupt() set but comes from user mode. In this case, the memory to print would be a user pointer and cannot be accessed by get_kernel_nofault() (but could be accessed with "set_fs(KERNEL_DS); __get_user()"). I looked through the history now and the only code path I could find that would arrive here this way is from bad_mode(), indicating that there is probably a hardware bug or the contents of *regs are corrupted. Russell might have a better explanation for this, but I would assume now that you are right in that we don't ever need to care about dumping user space addresses here. > > +static void dump_instr(const char *lvl, struct pt_regs *regs) > > { > > unsigned long addr = instruction_pointer(regs); > > const int thumb = thumb_mode(regs); > > @@ -173,10 +169,20 @@ static void __dump_instr(const char *lvl, struct pt_regs *regs) > > for (i = -4; i < 1 + !!thumb; i++) { > > unsigned int val, bad; > > > > - if (thumb) > > - bad = get_user(val, &((u16 *)addr)[i]); > > - else > > - bad = get_user(val, &((u32 *)addr)[i]); > > + if (!user_mode(regs)) { > > + if (thumb) { > > + u16 val16; > > + bad = get_kernel_nofault(val16, &((u16 *)addr)[i]); > > + val = val16; > > + } else { > > + bad = get_kernel_nofault(val, &((u32 *)addr)[i]); > > + } > > + } else { > > + if (thumb) > > + bad = get_user(val, &((u16 *)addr)[i]); > > + else > > + bad = get_user(val, &((u32 *)addr)[i]); > > + } > > When I looked at this earlier I just added a little helper to make > this a little easier to read. Here is my patch from an old tree: > > http://git.infradead.org/users/hch/misc.git/commitdiff/67413030ccb7a64a7eb828e13ff0795f4eadfeb7 I think your version was broken for the in-kernel thumb version because get_kernel_nofault does not widen the result from 16 to 32 bits. I tried fixing this in your version, but the result ended up more complex than my version here, so I decided to keep what I had. Arnd
Powered by blists - more mailing lists