[PATCH 0/2] arm64: traps.c fix + cleanup

[PATCH 0/2] arm64: traps.c fix + cleanup

[Mark Rutland]

Vladimir pointed out that we have an issue with handling undefined
instruction when both PAN & UAO are in use, due to dump_instr always
using KERNEL_DS, regardles of the context of the undefined instruction.
Patch 1 addresses this.

While investigating, I noticed that our dump_mem usage looked a bit
suspicious. It turns out that we try to handle a case that doesn't
happen in practice, and do so inconsistently, but as the code is never
called this is not harmful in practice. Patch 2 removes the redundant
code.

Thanks,
Mark.

Mark Rutland (2):
  arm64: fix dump_instr when PAN and UAO are in use
  arm64: simplify dump_mem

 arch/arm64/kernel/traps.c | 57 ++++++++++++++++++++---------------------------
 1 file changed, 24 insertions(+), 33 deletions(-)

-- 
1.9.1

[PATCH 1/2] arm64: fix dump_instr when PAN and UAO are in use

[Mark Rutland]

If the kernel is set to show unhandled signals, and a user task does not
handle a SIGILL as a result of an instruction abort, we will attempt to
log the offending instruction with dump_instr before killing the task.

We use dump_instr to log the encoding of the offending userspace
instruction. However, dump_instr is also used to dump instructions from
kernel space, and internally always switches to KERNEL_DS before dumping
the instruction with get_user. When both PAN and UAO are in use, reading
a user instruction via get_user while in KERNEL_DS will result in a
permission fault, which leads to an Oops.

As we have regs corresponding to the context of the original instruction
abort, we can inspect this and only flip to KERNEL_DS if the original
abort was taken from the kernel, avoiding this issue. At the same time,
remove the redundant (and incorrect) comments regarding the order
dump_mem and dump_instr are called in.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Reported-by: Vladimir Murzin <vladimir.murzin@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Fixes: 57f4959bad0a154a ("arm64: kernel: Add support for User Access Override")
---
 arch/arm64/kernel/traps.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index f7cf463..2a43012 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -64,8 +64,7 @@ static void dump_mem(const char *lvl, const char *str, unsigned long bottom,
 
 	/*
 	 * 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.
+	 * to safely read from kernel space.
 	 */
 	fs = get_fs();
 	set_fs(KERNEL_DS);
@@ -111,21 +110,12 @@ static void dump_backtrace_entry(unsigned long where)
 	print_ip_sym(where);
 }
 
-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);
-	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.
-	 */
-	fs = get_fs();
-	set_fs(KERNEL_DS);
-
 	for (i = -4; i < 1; i++) {
 		unsigned int val, bad;
 
@@ -139,8 +129,18 @@ 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)
+{
+	if (!user_mode(regs)) {
+		mm_segment_t fs = get_fs();
+		set_fs(KERNEL_DS);
+		__dump_instr(lvl, regs);
+		set_fs(fs);
+	} else {
+		__dump_instr(lvl, regs);
+	}
 }
 
 static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
-- 
1.9.1

[PATCH 1/2] arm64: fix dump_instr when PAN and UAO are in use

[Vladimir Murzin]

On 13/06/16 11:15, Mark Rutland wrote:
> If the kernel is set to show unhandled signals, and a user task does not
> handle a SIGILL as a result of an instruction abort, we will attempt to
> log the offending instruction with dump_instr before killing the task.
> 
> We use dump_instr to log the encoding of the offending userspace
> instruction. However, dump_instr is also used to dump instructions from
> kernel space, and internally always switches to KERNEL_DS before dumping
> the instruction with get_user. When both PAN and UAO are in use, reading
> a user instruction via get_user while in KERNEL_DS will result in a
> permission fault, which leads to an Oops.
> 
> As we have regs corresponding to the context of the original instruction
> abort, we can inspect this and only flip to KERNEL_DS if the original
> abort was taken from the kernel, avoiding this issue. At the same time,
> remove the redundant (and incorrect) comments regarding the order
> dump_mem and dump_instr are called in.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Reported-by: Vladimir Murzin <vladimir.murzin@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: James Morse <james.morse@arm.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Fixes: 57f4959bad0a154a ("arm64: kernel: Add support for User Access Override")

FWIW:

Tested-by: Vladimir Murzin <vladimir.murzin@arm.com>

with

$ echo 0 > /proc/sys/abi/swp

Removed swp emulation handler

$. /swp_test

$ cat swp_test.c

int main(void)
{
        unsigned long ret, x = 42, y = 24;

        asm volatile("swp     %0, %1, [%2]"
                     : "=&r" (ret)
                     : "r" (x), "r" (&y)
                     : "memory");

        return ret;
}

Thanks
Vladimir

[PATCH 1/2] arm64: fix dump_instr when PAN and UAO are in use

[Will Deacon]

On Mon, Jun 13, 2016 at 11:15:14AM +0100, Mark Rutland wrote:
> If the kernel is set to show unhandled signals, and a user task does not
> handle a SIGILL as a result of an instruction abort, we will attempt to
> log the offending instruction with dump_instr before killing the task.
> 
> We use dump_instr to log the encoding of the offending userspace
> instruction. However, dump_instr is also used to dump instructions from
> kernel space, and internally always switches to KERNEL_DS before dumping
> the instruction with get_user. When both PAN and UAO are in use, reading
> a user instruction via get_user while in KERNEL_DS will result in a
> permission fault, which leads to an Oops.
> 
> As we have regs corresponding to the context of the original instruction
> abort, we can inspect this and only flip to KERNEL_DS if the original
> abort was taken from the kernel, avoiding this issue. At the same time,
> remove the redundant (and incorrect) comments regarding the order
> dump_mem and dump_instr are called in.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Reported-by: Vladimir Murzin <vladimir.murzin@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: James Morse <james.morse@arm.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Fixes: 57f4959bad0a154a ("arm64: kernel: Add support for User Access Override")
> ---
>  arch/arm64/kernel/traps.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)

Queued as a fix for 4.8 w/ Vladimir's Tested-by. Please try to keep fixes
and cleanups/features separate in future series.

Will

[PATCH 1/2] arm64: fix dump_instr when PAN and UAO are in use

[Mark Rutland]

On Mon, Jun 13, 2016 at 01:48:24PM +0100, Will Deacon wrote:
> On Mon, Jun 13, 2016 at 11:15:14AM +0100, Mark Rutland wrote:
> > If the kernel is set to show unhandled signals, and a user task does not
> > handle a SIGILL as a result of an instruction abort, we will attempt to
> > log the offending instruction with dump_instr before killing the task.
> > 
> > We use dump_instr to log the encoding of the offending userspace
> > instruction. However, dump_instr is also used to dump instructions from
> > kernel space, and internally always switches to KERNEL_DS before dumping
> > the instruction with get_user. When both PAN and UAO are in use, reading
> > a user instruction via get_user while in KERNEL_DS will result in a
> > permission fault, which leads to an Oops.
> > 
> > As we have regs corresponding to the context of the original instruction
> > abort, we can inspect this and only flip to KERNEL_DS if the original
> > abort was taken from the kernel, avoiding this issue. At the same time,
> > remove the redundant (and incorrect) comments regarding the order
> > dump_mem and dump_instr are called in.
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Reported-by: Vladimir Murzin <vladimir.murzin@arm.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: James Morse <james.morse@arm.com>
> > Cc: Robin Murphy <robin.murphy@arm.com>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Fixes: 57f4959bad0a154a ("arm64: kernel: Add support for User Access Override")
> > ---
> >  arch/arm64/kernel/traps.c | 26 +++++++++++++-------------
> >  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> Queued as a fix for 4.8 w/ Vladimir's Tested-by. Please try to keep fixes
> and cleanups/features separate in future series.

Cheers, will do.

I assume you've only taken patch 1, and it's up to Catalin to take patch
2 for v4.8. I'll poke as necessary for that to happen.

Thanks,
Mark.

[PATCH 2/2] arm64: simplify dump_mem

[Mark Rutland]

Currently dump_mem attempts to dump memory in 64-bit chunks when
reporting a failure in 64-bit code, or 32-bit chunks when reporting a
failure in 32-bit code. We added code to handle these two cases
separately in commit e147ae6d7f908412 ("arm64: modify the dump mem for
64 bit addresses").

However, in all cases dump_mem is called, the failing context is a
kernel rather than user context. Additionally dump_mem is assumed to
only be used for kernel contexts, as internally it switches to
KERNEL_DS, and its callers pass kernel stack bounds.

This patch removes the redundant 32-bit chunk logic and associated
compat parameter, largely reverting the aforementioned commit. For the
call in __die(), the check of in_interrupt() is removed also, as __die()
is only called in response to faults from the kernel's exception level,
and thus the !user_mode(regs) check is sufficient. Were this not the
case, the used of task_stack_page(tsk) to generate the stack bounds
would be erroneous.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/kernel/traps.c | 31 +++++++++++--------------------
 1 file changed, 11 insertions(+), 20 deletions(-)

diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 2a43012..d9da2c5 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -52,15 +52,14 @@ static const char *handler[]= {
 int show_unhandled_signals = 1;
 
 /*
- * Dump out the contents of some memory nicely...
+ * Dump out the contents of some kernel memory nicely...
  */
 static void dump_mem(const char *lvl, const char *str, unsigned long bottom,
-		     unsigned long top, bool compat)
+		     unsigned long top)
 {
 	unsigned long first;
 	mm_segment_t fs;
 	int i;
-	unsigned int width = compat ? 4 : 8;
 
 	/*
 	 * We need to switch to kernel mode so that we can use __get_user
@@ -78,22 +77,15 @@ static void dump_mem(const char *lvl, const char *str, unsigned long bottom,
 		memset(str, ' ', sizeof(str));
 		str[sizeof(str) - 1] = '\0';
 
-		for (p = first, i = 0; i < (32 / width)
-					&& p < top; i++, p += width) {
+		for (p = first, i = 0; i < (32 / 8)
+					&& p < top; i++, p += 8) {
 			if (p >= bottom && p < top) {
 				unsigned long val;
 
-				if (width == 8) {
-					if (__get_user(val, (unsigned long *)p) == 0)
-						sprintf(str + i * 17, " %016lx", val);
-					else
-						sprintf(str + i * 17, " ????????????????");
-				} else {
-					if (__get_user(val, (unsigned int *)p) == 0)
-						sprintf(str + i * 9, " %08lx", val);
-					else
-						sprintf(str + i * 9, " ????????");
-				}
+				if (__get_user(val, (unsigned long *)p) == 0)
+					sprintf(str + i * 17, " %016lx", val);
+				else
+					sprintf(str + i * 17, " ????????????????");
 			}
 		}
 		printk("%s%04lx:%s\n", lvl, first & 0xffff, str);
@@ -216,7 +208,7 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
 				stack = IRQ_STACK_TO_TASK_STACK(irq_stack_ptr);
 
 			dump_mem("", "Exception stack", stack,
-				 stack + sizeof(struct pt_regs), false);
+				 stack + sizeof(struct pt_regs));
 		}
 	}
 }
@@ -254,10 +246,9 @@ static int __die(const char *str, int err, struct thread_info *thread,
 	pr_emerg("Process %.*s (pid: %d, stack limit = 0x%p)\n",
 		 TASK_COMM_LEN, tsk->comm, task_pid_nr(tsk), thread + 1);
 
-	if (!user_mode(regs) || in_interrupt()) {
+	if (!user_mode(regs)) {
 		dump_mem(KERN_EMERG, "Stack: ", regs->sp,
-			 THREAD_SIZE + (unsigned long)task_stack_page(tsk),
-			 compat_user_mode(regs));
+			 THREAD_SIZE + (unsigned long)task_stack_page(tsk));
 		dump_backtrace(regs, tsk);
 		dump_instr(KERN_EMERG, regs);
 	}
-- 
1.9.1

[PATCH 2/2] arm64: simplify dump_mem

[Catalin Marinas]

On Mon, Jun 13, 2016 at 11:15:15AM +0100, Mark Rutland wrote:
> Currently dump_mem attempts to dump memory in 64-bit chunks when
> reporting a failure in 64-bit code, or 32-bit chunks when reporting a
> failure in 32-bit code. We added code to handle these two cases
> separately in commit e147ae6d7f908412 ("arm64: modify the dump mem for
> 64 bit addresses").
> 
> However, in all cases dump_mem is called, the failing context is a
> kernel rather than user context. Additionally dump_mem is assumed to
> only be used for kernel contexts, as internally it switches to
> KERNEL_DS, and its callers pass kernel stack bounds.
> 
> This patch removes the redundant 32-bit chunk logic and associated
> compat parameter, largely reverting the aforementioned commit. For the
> call in __die(), the check of in_interrupt() is removed also, as __die()
> is only called in response to faults from the kernel's exception level,
> and thus the !user_mode(regs) check is sufficient. Were this not the
> case, the used of task_stack_page(tsk) to generate the stack bounds
> would be erroneous.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>

Queued for 4.8. Thanks.

-- 
Catalin