arm64: fatal kernel fault reporting improvements

PATCH New Mark Rutland 3 participants 6 messages May 21, 2018 – May 23, 2018 arm64

lore

Share & tools

lore

Mark Rutland

author reviewer

2018-05-21 13:14 UTC #0 lore raw

cover arm64: fatal kernel fault reporting improvements

Hi,

Catalin noticed that do_page_fault() can die() for some faults without reporting the faulting address, which is unfortunate. We also don't decode the ESR, or attempt to dump the page tables.

These patches unify the handling of those faults with the ones we handle in __do_kernel_fault(), making things a little more consistent.

Thanks,
Mark.

Mark Rutland (2):
arm64: make is_permission_fault() name clearer
arm64: Unify kernel fault reporting

 arch/arm64/mm/fault.c | 46 ++++++++++++++++++++++++++++------------------
 1 file changed, 28 insertions(+), 18 deletions(-)

Signature · 1 line

2.11.0

Mark Rutland

author reviewer

2018-05-21 13:14 UTC #1 lore raw

PATCH 1/2 arm64: make is_permission_fault() name clearer

The naming of is_permission_fault() makes it sound like it should return true for permission faults from EL0, but by design, it only does so for faults from EL1.

Let's make this clear by dropping el1 in the name, as we do for is_el1_instruction_abort().

Signed-off-by Mark Rutland <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

---
 arch/arm64/mm/fault.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

arch/arm64/mm/fault.c +5 -4

index 4165485e8b6e..59990491e72a 100644
@@ -235,8 +235,9 @@ static bool is_el1_instruction_abort(unsigned int esr)
 	return ESR_ELx_EC(esr) == ESR_ELx_EC_IABT_CUR;
 }
 
-static inline bool is_permission_fault(unsigned int esr, struct pt_regs *regs,
-				       unsigned long addr)
+static inline bool is_el1_permission_fault(unsigned int esr,
+					   struct pt_regs *regs,
+					   unsigned long addr)
 {
 	unsigned int ec       = ESR_ELx_EC(esr);
 	unsigned int fsc_type = esr & ESR_ELx_FSC_TYPE;

@@ -268,7 +269,7 @@ static void __do_kernel_fault(unsigned long addr, unsigned int esr,
 
 	bust_spinlocks(1);
 
-	if (is_permission_fault(esr, regs, addr)) {
+	if (is_el1_permission_fault(esr, regs, addr)) {
 		if (esr & ESR_ELx_WNR)
 			msg = "write to read-only memory";
 		else

@@ -395,7 +396,7 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
 		mm_flags |= FAULT_FLAG_WRITE;
 	}
 
-	if (addr < TASK_SIZE && is_permission_fault(esr, regs, addr)) {
+	if (addr < TASK_SIZE && is_el1_permission_fault(esr, regs, addr)) {
 		/* regs->orig_addr_limit may be 0 if we entered from EL0 */
 		if (regs->orig_addr_limit == KERNEL_DS)
 			die("Accessing user space memory with fs=KERNEL_DS", regs, esr);

Signature · 1 line

2.11.0

Will Deacon

2018-05-22 17:06 UTC #2 lore raw

PATCH 1/2 arm64: make is_permission_fault() name clearer

Quoted 7 lines · Mark Rutland

The naming of is_permission_fault() makes it sound like it should return true for permission faults from EL0, but by design, it only does so for faults from EL1.
Let's make this clear by dropping el1 in the name, as we do for is_el1_instruction_abort().

Quoted 6 lines

Signed-off-by Mark Rutland <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
---
 arch/arm64/mm/fault.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Acked-by Will Deacon <[email protected]>

Will

Mark Rutland

author reviewer

2018-05-21 13:14 UTC #3 lore raw

PATCH 2/2 arm64: Unify kernel fault reporting

In reply to Mark Rutland

In do_page_fault(), we handle some kernel faults early, and simply die() with a message. For faults handled later, we dump the faulting address, decode the ESR, walk the page tables, and perform a number of steps to ensure that this data is reported.

Let's unify the handling of fatal kernel faults with a new die_kernel_fault() helper, handling all of these details. This is largely the same as the existing logic in __do_kernel_fault(), except that addresses are consistently padded to 16 hex characters, as would be expected for a 64-bit address.

The messages currently logged in do_page_fault are adjusted to fit into the die_kernel_fault() message template.

Signed-off-by Mark Rutland <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

---
 arch/arm64/mm/fault.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

arch/arm64/mm/fault.c +23 ~~-14~~

index 59990491e72a..27cbe0b38960 100644
@@ -255,6 +255,22 @@ static inline bool is_el1_permission_fault(unsigned int esr,
 	return false;
 }
 
+static void die_kernel_fault(const char *msg, unsigned long addr,
+			     unsigned int esr, struct pt_regs *regs)
+{
+	bust_spinlocks(1);
+
+	pr_alert("Unable to handle kernel %s at virtual address %016lx\n", msg,
+		 addr);
+
+	mem_abort_decode(esr);
+
+	show_pte(addr);
+	die("Oops", regs, esr);
+	bust_spinlocks(0);
+	do_exit(SIGKILL);
+}
+
 static void __do_kernel_fault(unsigned long addr, unsigned int esr,
 			      struct pt_regs *regs)
 {

@@ -267,8 +283,6 @@ static void __do_kernel_fault(unsigned long addr, unsigned int esr,
 	if (!is_el1_instruction_abort(esr) && fixup_exception(regs))
 		return;
 
-	bust_spinlocks(1);
-
 	if (is_el1_permission_fault(esr, regs, addr)) {
 		if (esr & ESR_ELx_WNR)
 			msg = "write to read-only memory";

@@ -280,15 +294,7 @@ static void __do_kernel_fault(unsigned long addr, unsigned int esr,
 		msg = "paging request";
 	}
 
-	pr_alert("Unable to handle kernel %s at virtual address %08lx\n", msg,
-		 addr);
-
-	mem_abort_decode(esr);
-
-	show_pte(addr);
-	die("Oops", regs, esr);
-	bust_spinlocks(0);
-	do_exit(SIGKILL);
+	die_kernel_fault(msg, addr, esr, regs);
 }
 
 static void __do_user_fault(struct siginfo *info, unsigned int esr)

@@ -399,13 +405,16 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
 	if (addr < TASK_SIZE && is_el1_permission_fault(esr, regs, addr)) {
 		/* regs->orig_addr_limit may be 0 if we entered from EL0 */
 		if (regs->orig_addr_limit == KERNEL_DS)
-			die("Accessing user space memory with fs=KERNEL_DS", regs, esr);
+			die_kernel_fault("access to user memory with fs=KERNEL_DS",
+					 addr, esr, regs);
 
 		if (is_el1_instruction_abort(esr))
-			die("Attempting to execute userspace memory", regs, esr);
+			die_kernel_fault("execution of user memory",
+					 addr, esr, regs);
 
 		if (!search_exception_tables(regs->pc))
-			die("Accessing user space memory outside uaccess.h routines", regs, esr);
+			die_kernel_fault("access to user memory outside uaccess routines",
+					 addr, esr, regs);
 	}
 
 	perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, addr);

Signature · 1 line

2.11.0

Will Deacon

2018-05-22 17:17 UTC #4 lore raw

PATCH 2/2 arm64: Unify kernel fault reporting

Quoted 14 lines · Mark Rutland

In do_page_fault(), we handle some kernel faults early, and simply die() with a message. For faults handled later, we dump the faulting address, decode the ESR, walk the page tables, and perform a number of steps to ensure that this data is reported.
Let's unify the handling of fatal kernel faults with a new die_kernel_fault() helper, handling all of these details. This is largely the same as the existing logic in __do_kernel_fault(), except that addresses are consistently padded to 16 hex characters, as would be expected for a 64-bit address.
The messages currently logged in do_page_fault are adjusted to fit into the die_kernel_fault() message template.

Quoted 6 lines

Signed-off-by Mark Rutland <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
---
 arch/arm64/mm/fault.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

Acked-by Will Deacon <[email protected]>

Will

Catalin Marinas

maintainer Applied

2018-05-23 10:47 UTC #5 lore raw

cover arm64: fatal kernel fault reporting improvements

In reply to Mark Rutland

Quoted 7 lines · Mark Rutland

Catalin noticed that do_page_fault() can die() for some faults without reporting the faulting address, which is unfortunate. We also don't decode the ESR, or attempt to dump the page tables.
These patches unify the handling of those faults with the ones we handle in __do_kernel_fault(), making things a little more consistent.

Quoted 6 lines

Thanks,
Mark.
Mark Rutland (2):
arm64: make is_permission_fault() name clearer
arm64: Unify kernel fault reporting

Queued for 4.18. Thanks.

Signature · 1 line

Catalin