| Message ID | 20171102163452.7652-1-mark.rutland@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | arm: ensure dump_instr() checks addr_limit | expand |
On Thu, Nov 02, 2017 at 04:34:52PM +0000, Mark Rutland wrote: > Signed-off-by: Mark Rutland <mark.rutland@arm.com> Huh? What's that doing up here? > When CONFIG_DEBUG_USER is enabled, it's possible for a user to > deliberately trigger dump_instr() with a chosen kernel address. > > Let's avoid problems resulting from this by using get_user() rather than > __get_user(), ensuring that we don't erroneously access kernel memory. > > So that we can use the same code to dump user instructions and kernel > instructions, the common dumping code is factored out to __dump_instr(), > with the fs manipulated appropriately in dump_instr() around calls to > this. > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Cc: Russell King <rmk+kernel@armlinux.org.uk> > Cc: stable@vger.kernel.org It's right here... confused. greg k-h
On Thu, Nov 02, 2017 at 04:34:52PM +0000, Mark Rutland wrote: > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > When CONFIG_DEBUG_USER is enabled, it's possible for a user to > deliberately trigger dump_instr() with a chosen kernel address. Hi Mark, Please show how userspace could trigger that, as I don't think it's possible. Firstly, you need to set PC to a kernel address, which will trigger an abort. When that happens, we'll get a section permission fault, and head to do_sect_fault(). This will call do_bad_area(), which will detect that it's a userspace fault (because of the CPSR value). This calls show_pte() and show_regs(). show_pte() shows the page table values only. show_regs() shows the register values, and then dumps the kernel stack via show_stack(). Nothing in this path calls dump_instr(). The places where dump_instr() is called from are: die() do_undefinstr() bad_syscall() arm_syscall() baddataabort() (only for late v4 faulting architectures) The last four all need the CPU to have read and attempted to execute the instruction, so the permission checks must have passed. Userspace can't achieve that by setting the PC to a kernel address. die() is called when we enter an exception in an invalid mode, or we have a kernel mode fault (CPSR in kernel mode) that we can't handle, we have fault handling disabled (never the case when running user code), or we have no mm (kernel thread). So, I don't see how the kernel could be provoked to dump instructions from kernel space by the user. Please show a working example. Thanks. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up
On Thu, Nov 02, 2017 at 05:47:06PM +0100, Greg KH wrote: > On Thu, Nov 02, 2017 at 04:34:52PM +0000, Mark Rutland wrote: > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > Huh? What's that doing up here? [...] > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > Cc: Russell King <rmk+kernel@armlinux.org.uk> > > Cc: stable@vger.kernel.org > > It's right here... Sorry; I'd generated this commit message from another. I must've accidentally passed -s to git commit before copying the text I wanted. I will be more careful in future, and I'll remove the first instance if/when submitting this to Russell's patch system. Thanks, Mark.
On Thu, Nov 02, 2017 at 05:30:04PM +0000, Mark Rutland wrote: > On Thu, Nov 02, 2017 at 05:47:06PM +0100, Greg KH wrote: > > On Thu, Nov 02, 2017 at 04:34:52PM +0000, Mark Rutland wrote: > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > > > Huh? What's that doing up here? > > [...] > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > > Cc: Russell King <rmk+kernel@armlinux.org.uk> > > > Cc: stable@vger.kernel.org > > > > It's right here... > > Sorry; I'd generated this commit message from another. I must've > accidentally passed -s to git commit before copying the text I wanted. > > I will be more careful in future, and I'll remove the first instance > if/when submitting this to Russell's patch system. Russell's patch system? That's still in use and required? Ugh, that's crazy... Anyway, thanks for fixing it up. greg k-h
On Thu, Nov 02, 2017 at 06:46:28PM +0100, Greg KH wrote: > On Thu, Nov 02, 2017 at 05:30:04PM +0000, Mark Rutland wrote: > > On Thu, Nov 02, 2017 at 05:47:06PM +0100, Greg KH wrote: > > > On Thu, Nov 02, 2017 at 04:34:52PM +0000, Mark Rutland wrote: > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > > > > > Huh? What's that doing up here? > > > > [...] > > > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > > > Cc: Russell King <rmk+kernel@armlinux.org.uk> > > > > Cc: stable@vger.kernel.org > > > > > > It's right here... > > > > Sorry; I'd generated this commit message from another. I must've > > accidentally passed -s to git commit before copying the text I wanted. > > > > I will be more careful in future, and I'll remove the first instance > > if/when submitting this to Russell's patch system. > > Russell's patch system? That's still in use and required? Ugh, that's > crazy... Better than me missing patches because they've been buried under tonnes of email. It also does a fair number of checks, and there's been some discussion today about adding to the checks it does to catch stupidity in what the binutils assembler now accepts (to stop patches that appear to be tested but are broken from getting in, as what happened last night.) -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up
diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c index 948c648fea00..0fcd82f01388 100644 --- a/arch/arm/kernel/traps.c +++ b/arch/arm/kernel/traps.c @@ -154,30 +154,26 @@ static void dump_mem(const char *lvl, const char *str, unsigned long bottom, set_fs(fs); } -static void dump_instr(const char *lvl, struct pt_regs *regs) +static void __dump_instr(const char *lvl, struct pt_regs *regs) { unsigned long addr = instruction_pointer(regs); const int thumb = thumb_mode(regs); const int width = thumb ? 4 : 8; - mm_segment_t fs; char str[sizeof("00000000 ") * 5 + 2 + 1], *p = str; int i; /* - * We need to switch to kernel mode so that we can use __get_user - * to safely read from kernel space. Note that we now dump the - * code first, just in case the backtrace kills us. + * Note that we now dump the code first, just in case the backtrace + * kills us. */ - fs = get_fs(); - set_fs(KERNEL_DS); for (i = -4; i < 1 + !!thumb; i++) { unsigned int val, bad; if (thumb) - bad = __get_user(val, &((u16 *)addr)[i]); + bad = get_user(val, &((u16 *)addr)[i]); else - bad = __get_user(val, &((u32 *)addr)[i]); + bad = get_user(val, &((u32 *)addr)[i]); if (!bad) p += sprintf(p, i == 0 ? "(%0*x) " : "%0*x ", @@ -188,8 +184,20 @@ static void dump_instr(const char *lvl, struct pt_regs *regs) } } printk("%sCode: %s\n", lvl, str); +} - set_fs(fs); +static void dump_instr(const char *lvl, struct pt_regs *regs) +{ + mm_segment_t fs; + + if (!user_mode(regs)) { + fs = get_fs(); + set_fs(KERNEL_DS); + __dump_instr(lvl, regs); + set_fs(fs); + } else { + __dump_instr(lvl, regs); + } } #ifdef CONFIG_ARM_UNWIND