Skip to contentSkip to conversation

arm64: traps.c fix + cleanup

PATCH New Mark Rutland 4 participants 7 messages Jun 13, 2016 – Jun 21, 2016 arm64

lore
Share & tools
lore

Mark Rutland

author reviewer
#0 lore raw

cover arm64: traps.c fix + cleanup

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(-)
Signature · 1 line
1.9.1
author reviewer
#1 lore raw

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

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 <[email protected]>
Reported-by Vladimir Murzin <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc James Morse <[email protected]>
Cc Robin Murphy <[email protected]>
Cc Will Deacon <[email protected]>
Fixes 57f4959bad0a154a ("arm64: kernel: Add support for User Access Override")
---
 arch/arm64/kernel/traps.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)
arch/arm64/kernel/traps.c +13 -13
index f7cf463..2a43012 100644
@@ -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)
Signature · 1 line
1.9.1
#2 lore raw

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

Quoted 17 lines · 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.

Quoted 7 lines
Signed-off-by Mark Rutland <[email protected]>
Reported-by Vladimir Murzin <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc James Morse <[email protected]>
Cc Robin Murphy <[email protected]>
Cc Will Deacon <[email protected]>
Fixes 57f4959bad0a154a ("arm64: kernel: Add support for User Access Override")

FWIW:

Tested-by Vladimir Murzin <[email protected]>

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

#3 lore raw

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

In reply to Mark Rutland
Quoted 24 lines · 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 <[email protected]>
Reported-by Vladimir Murzin <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc James Morse <[email protected]>
Cc Robin Murphy <[email protected]>
Cc Will Deacon <[email protected]>
Fixes 57f4959bad0a154a ("arm64: kernel: Add support for User Access Override")
Quoted 3 lines
---
 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

author reviewer
#4 lore raw

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

Quoted 29 lines · Will Deacon
Quoted 24 lines · 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 <[email protected]>
Reported-by Vladimir Murzin <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc James Morse <[email protected]>
Cc Robin Murphy <[email protected]>
Cc Will Deacon <[email protected]>
Fixes 57f4959bad0a154a ("arm64: kernel: Add support for User Access Override")
Quoted 3 lines
---
 arch/arm64/kernel/traps.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)
Quoted 2 lines

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.

author reviewer
#5 lore raw

PATCH 2/2 arm64: simplify dump_mem

In reply to 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 <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
---
 arch/arm64/kernel/traps.c | 31 +++++++++++--------------------
 1 file changed, 11 insertions(+), 20 deletions(-)
arch/arm64/kernel/traps.c +11 -20
index 2a43012..d9da2c5 100644
@@ -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);
 	}
Signature · 1 line
1.9.1
maintainer Applied
#6 lore raw

PATCH 2/2 arm64: simplify dump_mem

Quoted 19 lines · 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.

Quoted 3 lines
Signed-off-by Mark Rutland <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>

Queued for 4.8. Thanks.

Signature · 1 line
Catalin