arm64: mm: print out correct page table entries

When we take a fault from EL0 that can't be handled, we print out the page table entries associated with the faulting address. This allows userspace to print out any current page table entries, including kernel (TTBR1) entries. Exposing kernel mappings like this could pose a security risk, so don't print out page table information on EL0 faults. (But still print it out for EL1 faults.) This also follows the same behaviour as x86, printing out page table entries on kernel mode faults but not user mode faults.

Signed-off-by Kristina Martsenko <[email protected]>

---

v2:
- mention x86 in commit message

 arch/arm64/mm/fault.c | 1 -
 1 file changed, 1 deletion(-)

arch/arm64/mm/fault.c +0 -0

index a9dfb37c87a2..b070dcd50ed0 100644

Print out the name of the file associated with the vma that faulted. This is usually the executable or shared library name. We already print out the task name, but also printing the library name is useful for pinpointing bugs to libraries.

Also print the base address and size of the vma, which together with the PC (printed by __show_regs) gives the offset into the library.

Fault prints now look like:
test[2361]: unhandled level 2 translation fault (11) at 0x00000012, esr 0x92000006, in libfoo.so[ffffa0145000+1000]

This is already done on x86, for more details see commit 03252919b798 ("x86: print which shared library/executable faulted in segfault etc. messages v3").

Signed-off-by Kristina Martsenko <[email protected]>

---

v2:
- add this patch

 arch/arm64/mm/fault.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

arch/arm64/mm/fault.c +3 -1

index b070dcd50ed0..6a2a5dd7cf36 100644
@@ -256,9 +256,11 @@ static void __do_user_fault(struct task_struct *tsk, unsigned long addr,
 
 	if (unhandled_signal(tsk, sig) && show_unhandled_signals_ratelimited()) {
 		inf = esr_to_fault_info(esr);
-		pr_info("%s[%d]: unhandled %s (%d) at 0x%08lx, esr 0x%03x\n",
+		pr_info("%s[%d]: unhandled %s (%d) at 0x%08lx, esr 0x%03x",
 			tsk->comm, task_pid_nr(tsk), inf->name, sig,
 			addr, esr);
+		print_vma_addr(KERN_CONT ", in ", regs->pc);
+		pr_cont("\n");
 		__show_regs(regs);
 	}
 

Signature · 1 line

2.1.4

/*
- * Dump out the page tables associated with 'addr' in mm 'mm'.
+ * Dump out the page tables associated with 'addr' in the currently active mm. */

Quoted 4 lines · Kristina Martsenko

/*
- * Dump out the page tables associated with 'addr' in mm 'mm'.
+ * Dump out the page tables associated with 'addr' in the currently active mm. */

arch/arm64/mm/fault.c static inline int notify_page_fault(struct pt_regs *regs, unsigned int esr) · line 92

Quoted 10 lines

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

-void show_pte(struct mm_struct *mm, unsigned long addr)
+void show_pte(unsigned long addr)
 {
+	struct mm_struct *mm;
 	pgd_t *pgd;
 
-	if (!mm)
+	if (addr < TASK_SIZE) {
+		/* TTBR0 */
+		mm = current->active_mm;

Can we log something for the active_mm == &init_mm case?

e.g.

	if (addr < TASK_SIZE) {
		if (current->active_mm == &init_mm)
			pr_alert("[%016lx] address in unknown TTBR0 range\n",
				addr);

Quoted 20 lines · Mark Rutland

Quoted 4 lines · Kristina Martsenko

/*
- * Dump out the page tables associated with 'addr' in mm 'mm'.
+ * Dump out the page tables associated with 'addr' in the currently active mm. */

arch/arm64/mm/fault.c static inline int notify_page_fault(struct pt_regs *regs, unsigned int esr) · line 92

Quoted 10 lines

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

-void show_pte(struct mm_struct *mm, unsigned long addr)
+void show_pte(unsigned long addr)
 {
+	struct mm_struct *mm;
 	pgd_t *pgd;
 
-	if (!mm)
+	if (addr < TASK_SIZE) {
+		/* TTBR0 */
+		mm = current->active_mm;

Can we log something for the active_mm == &init_mm case?

e.g.

Quoted 4 lines

	if (addr < TASK_SIZE) {
		if (current->active_mm == &init_mm)
			pr_alert("[%016lx] address in unknown TTBR0 range\n",
				addr);

I don't understand the case you're trying to highlight here, nor which table you want to walk.

When we take a fault that can't be handled, we print out the page table entries associated with the faulting address. In some cases we currently print out the wrong entries. For a faulting TTBR1 address, we sometimes print out TTBR0 table entries instead, and for a faulting TTBR0 address we sometimes print out TTBR1 table entries. Fix this by choosing the tables based on the faulting address.

Signed-off-by Kristina Martsenko <[email protected]>

---

v2:
- use TASK_SIZE and VA_START, change printed message

 arch/arm64/include/asm/system_misc.h |  2 +-
 arch/arm64/mm/fault.c                | 29 +++++++++++++++++++----------
 2 files changed, 20 insertions(+), 11 deletions(-)

arch/arm64/mm/fault.c +7 -3

index c3f2b1048f83..a9dfb37c87a2 100644
@@ -80,14 +80,24 @@ static inline int notify_page_fault(struct pt_regs *regs, unsigned int esr)
 #endif
 
 /*
- * Dump out the page tables associated with 'addr' in mm 'mm'.
+ * Dump out the page tables associated with 'addr' in the currently active mm.
  */
-void show_pte(struct mm_struct *mm, unsigned long addr)
+void show_pte(unsigned long addr)
 {
+	struct mm_struct *mm;
 	pgd_t *pgd;
 
-	if (!mm)
+	if (addr < TASK_SIZE) {
+		/* TTBR0 */
+		mm = current->active_mm;
+	} else if (addr >= VA_START) {

Quoted 22 lines · Kristina Martsenko

arch/arm64/mm/fault.c +7 -3

index c3f2b1048f83..a9dfb37c87a2 100644
@@ -80,14 +80,24 @@ static inline int notify_page_fault(struct pt_regs *regs, unsigned int esr)
 #endif
 
 /*
- * Dump out the page tables associated with 'addr' in mm 'mm'.
+ * Dump out the page tables associated with 'addr' in the currently active mm.
  */
-void show_pte(struct mm_struct *mm, unsigned long addr)
+void show_pte(unsigned long addr)
 {
+	struct mm_struct *mm;
 	pgd_t *pgd;
 
-	if (!mm)
+	if (addr < TASK_SIZE) {
+		/* TTBR0 */
+		mm = current->active_mm;
+	} else if (addr >= VA_START) {

arch/arm64/mm/fault.c static inline int notify_page_fault(struct pt_regs *regs, unsigned int esr) · line 103

Quoted 10 lines

arch/arm64/mm/fault.c +6 -0

+		/* TTBR1 */
 		mm = &init_mm;
+	} else {
+		pr_alert("[%08lx] address between user and kernel address ranges\n",
+			 addr);
+		return;
+	}
 
 	pr_alert("pgd = %p\n", mm->pgd);
 	pgd = pgd_offset(mm, addr);

Is there any reason to change the prototype of the function? You say nothing about it in patch description. The show_pte() is implemented in several arches, and everywhere it takes mm_struct as 1st argument. For me, if you don't need to change the prototype, you'd better leave things as is. The patch will get much shorter and expressive with it.

If you really need to change the prototype, it would be better to do it in separated patch and give clear explanations - what for?

Quoted 41 lines · Yury Norov

Quoted 22 lines · Kristina Martsenko

arch/arm64/mm/fault.c +7 -3

index c3f2b1048f83..a9dfb37c87a2 100644
@@ -80,14 +80,24 @@ static inline int notify_page_fault(struct pt_regs *regs, unsigned int esr)
 #endif
 
 /*
- * Dump out the page tables associated with 'addr' in mm 'mm'.
+ * Dump out the page tables associated with 'addr' in the currently active mm.
  */
-void show_pte(struct mm_struct *mm, unsigned long addr)
+void show_pte(unsigned long addr)
 {
+	struct mm_struct *mm;
 	pgd_t *pgd;
 
-	if (!mm)
+	if (addr < TASK_SIZE) {
+		/* TTBR0 */
+		mm = current->active_mm;
+	} else if (addr >= VA_START) {

arch/arm64/mm/fault.c static inline int notify_page_fault(struct pt_regs *regs, unsigned int esr) · line 103

Quoted 10 lines

arch/arm64/mm/fault.c +6 -0

+		/* TTBR1 */
 		mm = &init_mm;
+	} else {
+		pr_alert("[%08lx] address between user and kernel address ranges\n",
+			 addr);
+		return;
+	}
 
 	pr_alert("pgd = %p\n", mm->pgd);
 	pgd = pgd_offset(mm, addr);

Is there any reason to change the prototype of the function? You say nothing about it in patch description. The show_pte() is implemented in several arches, and everywhere it takes mm_struct as 1st argument. For me, if you don't need to change the prototype, you'd better leave things as is. The patch will get much shorter and expressive with it.

Quoted 2 lines

If you really need to change the prototype, it would be better to do it in separated patch and give clear explanations - what for?

The mm is unused and this isn't a core interface. In fact, it's only available on architectures that started off by copying from arch/arm/.

We can always add the mm back if we need it in future.

Quoted 48 lines · Will Deacon

Quoted 41 lines · Yury Norov

Quoted 22 lines · Kristina Martsenko

arch/arm64/mm/fault.c +7 -3

index c3f2b1048f83..a9dfb37c87a2 100644
@@ -80,14 +80,24 @@ static inline int notify_page_fault(struct pt_regs *regs, unsigned int esr)
 #endif
 
 /*
- * Dump out the page tables associated with 'addr' in mm 'mm'.
+ * Dump out the page tables associated with 'addr' in the currently active mm.
  */
-void show_pte(struct mm_struct *mm, unsigned long addr)
+void show_pte(unsigned long addr)
 {
+	struct mm_struct *mm;
 	pgd_t *pgd;
 
-	if (!mm)
+	if (addr < TASK_SIZE) {
+		/* TTBR0 */
+		mm = current->active_mm;
+	} else if (addr >= VA_START) {

arch/arm64/mm/fault.c static inline int notify_page_fault(struct pt_regs *regs, unsigned int esr) · line 103

Quoted 10 lines

arch/arm64/mm/fault.c +6 -0

+		/* TTBR1 */
 		mm = &init_mm;
+	} else {
+		pr_alert("[%08lx] address between user and kernel address ranges\n",
+			 addr);
+		return;
+	}
 
 	pr_alert("pgd = %p\n", mm->pgd);
 	pgd = pgd_offset(mm, addr);

Is there any reason to change the prototype of the function? You say nothing about it in patch description. The show_pte() is implemented in several arches, and everywhere it takes mm_struct as 1st argument. For me, if you don't need to change the prototype, you'd better leave things as is. The patch will get much shorter and expressive with it.

Quoted 2 lines

If you really need to change the prototype, it would be better to do it in separated patch and give clear explanations - what for?

The mm is unused and this isn't a core interface. In fact, it's only available on architectures that started off by copying from arch/arm/.

Quoted 1 line

We can always add the mm back if we need it in future.

OK, understood that. But I still think it should be done in separeted patch, and it would be also good to revise sh and unicore32. It seems that sh may be easily switched to the proposed interface, and the unicore32 most probably too.