arm: ensure dump_instr() checks addr_limit

Message ID 20171102163452.7652-1-mark.rutland@arm.com
State New
Headers show
Series
  • arm: ensure dump_instr() checks addr_limit
Related show

Commit Message

Mark Rutland Nov. 2, 2017, 4:34 p.m.
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.

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
---
 arch/arm/kernel/traps.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

-- 
2.11.0

Comments

Greg Kroah-Hartman Nov. 2, 2017, 4:47 p.m. | #1
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
Russell King - ARM Linux Nov. 2, 2017, 4:57 p.m. | #2
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
Mark Rutland Nov. 2, 2017, 5:30 p.m. | #3
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.
Greg Kroah-Hartman Nov. 2, 2017, 5:46 p.m. | #4
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
Russell King - ARM Linux Nov. 2, 2017, 5:50 p.m. | #5
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

Patch

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