[PATCH 1/2] arm64: mm: print out correct page table entries

[PATCH 1/2] arm64: mm: print out correct page table entries

[Kristina Martsenko]

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 <kristina.martsenko@arm.com>
---
 arch/arm64/include/asm/system_misc.h |  2 +-
 arch/arm64/mm/fault.c                | 30 ++++++++++++++++++++----------
 2 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/include/asm/system_misc.h b/arch/arm64/include/asm/system_misc.h
index bc812435bc76..d0beefeb6d25 100644
--- a/arch/arm64/include/asm/system_misc.h
+++ b/arch/arm64/include/asm/system_misc.h
@@ -40,7 +40,7 @@ void hook_debug_fault_code(int nr, int (*fn)(unsigned long, unsigned int,
 			   int sig, int code, const char *name);
 
 struct mm_struct;
-extern void show_pte(struct mm_struct *mm, unsigned long addr);
+extern void show_pte(unsigned long addr);
 extern void __show_regs(struct pt_regs *);
 
 extern void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 37b95dff0b07..0c32f34fb4af 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -80,14 +80,25 @@ 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)
 {
+	unsigned long ttbr_mask = ~0UL << VA_BITS;
+	struct mm_struct *mm;
 	pgd_t *pgd;
 
-	if (!mm)
+	if (!(addr & ttbr_mask)) {
+		/* TTBR0 */
+		mm = current->active_mm;
+	} else if ((addr & ttbr_mask) == ttbr_mask) {
+		/* TTBR1 */
 		mm = &init_mm;
+	} else {
+		/* Between TTBR0 and TTBR1, no table to walk */
+		pr_alert("[%08lx] address between TTBR ranges\n", addr);
+		return;
+	}
 
 	pr_alert("pgd = %p\n", mm->pgd);
 	pgd = pgd_offset(mm, addr);
@@ -196,8 +207,8 @@ static inline bool is_permission_fault(unsigned int esr, struct pt_regs *regs,
 /*
  * The kernel tried to access some page that wasn't present.
  */
-static void __do_kernel_fault(struct mm_struct *mm, unsigned long addr,
-			      unsigned int esr, struct pt_regs *regs)
+static void __do_kernel_fault(unsigned long addr, unsigned int esr,
+			      struct pt_regs *regs)
 {
 	const char *msg;
 
@@ -227,7 +238,7 @@ static void __do_kernel_fault(struct mm_struct *mm, unsigned long addr,
 	pr_alert("Unable to handle kernel %s at virtual address %08lx\n", msg,
 		 addr);
 
-	show_pte(mm, addr);
+	show_pte(addr);
 	die("Oops", regs, esr);
 	bust_spinlocks(0);
 	do_exit(SIGKILL);
@@ -249,7 +260,7 @@ static void __do_user_fault(struct task_struct *tsk, unsigned long addr,
 		pr_info("%s[%d]: unhandled %s (%d) at 0x%08lx, esr 0x%03x\n",
 			tsk->comm, task_pid_nr(tsk), inf->name, sig,
 			addr, esr);
-		show_pte(tsk->mm, addr);
+		show_pte(addr);
 		show_regs(regs);
 	}
 
@@ -265,7 +276,6 @@ static void __do_user_fault(struct task_struct *tsk, unsigned long addr,
 static void do_bad_area(unsigned long addr, unsigned int esr, struct pt_regs *regs)
 {
 	struct task_struct *tsk = current;
-	struct mm_struct *mm = tsk->active_mm;
 	const struct fault_info *inf;
 
 	/*
@@ -276,7 +286,7 @@ static void do_bad_area(unsigned long addr, unsigned int esr, struct pt_regs *re
 		inf = esr_to_fault_info(esr);
 		__do_user_fault(tsk, addr, esr, inf->sig, inf->code, regs);
 	} else
-		__do_kernel_fault(mm, addr, esr, regs);
+		__do_kernel_fault(addr, esr, regs);
 }
 
 #define VM_FAULT_BADMAP		0x010000
@@ -475,7 +485,7 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
 	return 0;
 
 no_context:
-	__do_kernel_fault(mm, addr, esr, regs);
+	__do_kernel_fault(addr, esr, regs);
 	return 0;
 }
 
-- 
2.1.4

[PATCH 2/2] arm64: mm: don't print out page table entries on EL0 faults

[Kristina Martsenko]

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.)

Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com>
---
 arch/arm64/mm/fault.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 0c32f34fb4af..798841f4b00a 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -260,7 +260,6 @@ static void __do_user_fault(struct task_struct *tsk, unsigned long addr,
 		pr_info("%s[%d]: unhandled %s (%d) at 0x%08lx, esr 0x%03x\n",
 			tsk->comm, task_pid_nr(tsk), inf->name, sig,
 			addr, esr);
-		show_pte(addr);
 		show_regs(regs);
 	}
 
-- 
2.1.4

[PATCH 1/2] arm64: mm: print out correct page table entries

[Will Deacon]

Hi Kristina,

On Mon, May 22, 2017 at 12:45:53PM +0100, Kristina Martsenko wrote:
> 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 <kristina.martsenko@arm.com>
> ---
>  arch/arm64/include/asm/system_misc.h |  2 +-
>  arch/arm64/mm/fault.c                | 30 ++++++++++++++++++++----------
>  2 files changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/system_misc.h b/arch/arm64/include/asm/system_misc.h
> index bc812435bc76..d0beefeb6d25 100644
> --- a/arch/arm64/include/asm/system_misc.h
> +++ b/arch/arm64/include/asm/system_misc.h
> @@ -40,7 +40,7 @@ void hook_debug_fault_code(int nr, int (*fn)(unsigned long, unsigned int,
>  			   int sig, int code, const char *name);
>  
>  struct mm_struct;
> -extern void show_pte(struct mm_struct *mm, unsigned long addr);
> +extern void show_pte(unsigned long addr);
>  extern void __show_regs(struct pt_regs *);
>  
>  extern void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index 37b95dff0b07..0c32f34fb4af 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -80,14 +80,25 @@ 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)
>  {
> +	unsigned long ttbr_mask = ~0UL << VA_BITS;
> +	struct mm_struct *mm;
>  	pgd_t *pgd;
>  
> -	if (!mm)
> +	if (!(addr & ttbr_mask)) {
> +		/* TTBR0 */
> +		mm = current->active_mm;
> +	} else if ((addr & ttbr_mask) == ttbr_mask) {
> +		/* TTBR1 */
>  		mm = &init_mm;

I wonder if this would be better structured as:

	if (addr < TASK_SIZE)
		/* TTBR0 */
	else if (addr >= VA_START)
		/* TTBR1 */
	else
		/* no man's land */

what do you think?

> @@ -196,8 +207,8 @@ static inline bool is_permission_fault(unsigned int esr, struct pt_regs *regs,
>  /*
>   * The kernel tried to access some page that wasn't present.
>   */
> -static void __do_kernel_fault(struct mm_struct *mm, unsigned long addr,
> -			      unsigned int esr, struct pt_regs *regs)
> +static void __do_kernel_fault(unsigned long addr, unsigned int esr,
> +			      struct pt_regs *regs)
>  {
>  	const char *msg;
>  
> @@ -227,7 +238,7 @@ static void __do_kernel_fault(struct mm_struct *mm, unsigned long addr,
>  	pr_alert("Unable to handle kernel %s at virtual address %08lx\n", msg,
>  		 addr);
>  
> -	show_pte(mm, addr);
> +	show_pte(addr);

Now that you're not passing the mm, __do_kernel_fault doesn't need it
for anything so there's further cleanup you can do here.

>  	die("Oops", regs, esr);
>  	bust_spinlocks(0);
>  	do_exit(SIGKILL);
> @@ -249,7 +260,7 @@ static void __do_user_fault(struct task_struct *tsk, unsigned long addr,
>  		pr_info("%s[%d]: unhandled %s (%d) at 0x%08lx, esr 0x%03x\n",
>  			tsk->comm, task_pid_nr(tsk), inf->name, sig,
>  			addr, esr);
> -		show_pte(tsk->mm, addr);
> +		show_pte(addr);
>  		show_regs(regs);
>  	}
>  
> @@ -265,7 +276,6 @@ static void __do_user_fault(struct task_struct *tsk, unsigned long addr,
>  static void do_bad_area(unsigned long addr, unsigned int esr, struct pt_regs *regs)
>  {
>  	struct task_struct *tsk = current;
> -	struct mm_struct *mm = tsk->active_mm;
>  	const struct fault_info *inf;
>  
>  	/*
> @@ -276,7 +286,7 @@ static void do_bad_area(unsigned long addr, unsigned int esr, struct pt_regs *re
>  		inf = esr_to_fault_info(esr);
>  		__do_user_fault(tsk, addr, esr, inf->sig, inf->code, regs);
>  	} else
> -		__do_kernel_fault(mm, addr, esr, regs);
> +		__do_kernel_fault(addr, esr, regs);

Similar thing here: I don't think the mm local variable is used anymore.
Does GCC not warn about this?

Will

[PATCH 2/2] arm64: mm: don't print out page table entries on EL0 faults

[Will Deacon]

On Mon, May 22, 2017 at 12:45:54PM +0100, Kristina Martsenko wrote:
> 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.)
> 
> Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com>

It might be worth mentioning that this follows the same behaviour as x86.
In fact, they print the name of the faulting VMA using print_vma_addr,
which might be useful too.

Will

> ---
>  arch/arm64/mm/fault.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index 0c32f34fb4af..798841f4b00a 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -260,7 +260,6 @@ static void __do_user_fault(struct task_struct *tsk, unsigned long addr,
>  		pr_info("%s[%d]: unhandled %s (%d) at 0x%08lx, esr 0x%03x\n",
>  			tsk->comm, task_pid_nr(tsk), inf->name, sig,
>  			addr, esr);
> -		show_pte(addr);
>  		show_regs(regs);
>  	}
>  
> -- 
> 2.1.4
> 

[PATCH 1/2] arm64: mm: print out correct page table entries

[Kristina Martsenko]

Hi Will,

On 22/05/17 13:18, Will Deacon wrote:
> On Mon, May 22, 2017 at 12:45:53PM +0100, Kristina Martsenko wrote:
>> 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 <kristina.martsenko@arm.com>
>> ---
>>  arch/arm64/include/asm/system_misc.h |  2 +-
>>  arch/arm64/mm/fault.c                | 30 ++++++++++++++++++++----------
>>  2 files changed, 21 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/system_misc.h b/arch/arm64/include/asm/system_misc.h
>> index bc812435bc76..d0beefeb6d25 100644
>> --- a/arch/arm64/include/asm/system_misc.h
>> +++ b/arch/arm64/include/asm/system_misc.h
>> @@ -40,7 +40,7 @@ void hook_debug_fault_code(int nr, int (*fn)(unsigned long, unsigned int,
>>  			   int sig, int code, const char *name);
>>  
>>  struct mm_struct;
>> -extern void show_pte(struct mm_struct *mm, unsigned long addr);
>> +extern void show_pte(unsigned long addr);
>>  extern void __show_regs(struct pt_regs *);
>>  
>>  extern void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
>> index 37b95dff0b07..0c32f34fb4af 100644
>> --- a/arch/arm64/mm/fault.c
>> +++ b/arch/arm64/mm/fault.c
>> @@ -80,14 +80,25 @@ 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)
>>  {
>> +	unsigned long ttbr_mask = ~0UL << VA_BITS;
>> +	struct mm_struct *mm;
>>  	pgd_t *pgd;
>>  
>> -	if (!mm)
>> +	if (!(addr & ttbr_mask)) {
>> +		/* TTBR0 */
>> +		mm = current->active_mm;
>> +	} else if ((addr & ttbr_mask) == ttbr_mask) {
>> +		/* TTBR1 */
>>  		mm = &init_mm;
> 
> I wonder if this would be better structured as:
> 
> 	if (addr < TASK_SIZE)
> 		/* TTBR0 */
> 	else if (addr >= VA_START)
> 		/* TTBR1 */
> 	else
> 		/* no man's land */
> 
> what do you think?

I agree this is better, I'll change it in v2.

>> @@ -196,8 +207,8 @@ static inline bool is_permission_fault(unsigned int esr, struct pt_regs *regs,
>>  /*
>>   * The kernel tried to access some page that wasn't present.
>>   */
>> -static void __do_kernel_fault(struct mm_struct *mm, unsigned long addr,
>> -			      unsigned int esr, struct pt_regs *regs)
>> +static void __do_kernel_fault(unsigned long addr, unsigned int esr,
>> +			      struct pt_regs *regs)
>>  {
>>  	const char *msg;
>>  
>> @@ -227,7 +238,7 @@ static void __do_kernel_fault(struct mm_struct *mm, unsigned long addr,
>>  	pr_alert("Unable to handle kernel %s at virtual address %08lx\n", msg,
>>  		 addr);
>>  
>> -	show_pte(mm, addr);
>> +	show_pte(addr);
> 
> Now that you're not passing the mm, __do_kernel_fault doesn't need it
> for anything so there's further cleanup you can do here.

This patch already removes the mm from __do_kernel_fault.

>>  	die("Oops", regs, esr);
>>  	bust_spinlocks(0);
>>  	do_exit(SIGKILL);
>> @@ -249,7 +260,7 @@ static void __do_user_fault(struct task_struct *tsk, unsigned long addr,
>>  		pr_info("%s[%d]: unhandled %s (%d) at 0x%08lx, esr 0x%03x\n",
>>  			tsk->comm, task_pid_nr(tsk), inf->name, sig,
>>  			addr, esr);
>> -		show_pte(tsk->mm, addr);
>> +		show_pte(addr);
>>  		show_regs(regs);
>>  	}
>>  
>> @@ -265,7 +276,6 @@ static void __do_user_fault(struct task_struct *tsk, unsigned long addr,
>>  static void do_bad_area(unsigned long addr, unsigned int esr, struct pt_regs *regs)
>>  {
>>  	struct task_struct *tsk = current;
>> -	struct mm_struct *mm = tsk->active_mm;
>>  	const struct fault_info *inf;
>>  
>>  	/*
>> @@ -276,7 +286,7 @@ static void do_bad_area(unsigned long addr, unsigned int esr, struct pt_regs *re
>>  		inf = esr_to_fault_info(esr);
>>  		__do_user_fault(tsk, addr, esr, inf->sig, inf->code, regs);
>>  	} else
>> -		__do_kernel_fault(mm, addr, esr, regs);
>> +		__do_kernel_fault(addr, esr, regs);
> 
> Similar thing here: I don't think the mm local variable is used anymore.
> Does GCC not warn about this?

Same here - this patch already removes the mm variable.

Thanks for the review,
Kristina

[PATCH 2/2] arm64: mm: don't print out page table entries on EL0 faults

[Kristina Martsenko]

On 22/05/17 13:21, Will Deacon wrote:
> On Mon, May 22, 2017 at 12:45:54PM +0100, Kristina Martsenko wrote:
>> 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.)
>>
>> Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com>
> 
> It might be worth mentioning that this follows the same behaviour as x86.

Yep.

> In fact, they print the name of the faulting VMA using print_vma_addr,
> which might be useful too.

Looks useful, should I add that in a separate patch in v2?

Thanks,
Kristina

>> ---
>>  arch/arm64/mm/fault.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
>> index 0c32f34fb4af..798841f4b00a 100644
>> --- a/arch/arm64/mm/fault.c
>> +++ b/arch/arm64/mm/fault.c
>> @@ -260,7 +260,6 @@ static void __do_user_fault(struct task_struct *tsk, unsigned long addr,
>>  		pr_info("%s[%d]: unhandled %s (%d) at 0x%08lx, esr 0x%03x\n",
>>  			tsk->comm, task_pid_nr(tsk), inf->name, sig,
>>  			addr, esr);
>> -		show_pte(addr);
>>  		show_regs(regs);
>>  	}
>>  
>> -- 
>> 2.1.4
>>

[PATCH 2/2] arm64: mm: don't print out page table entries on EL0 faults

[Will Deacon]

On Wed, May 31, 2017 at 08:38:00PM +0100, Kristina Martsenko wrote:
> On 22/05/17 13:21, Will Deacon wrote:
> > On Mon, May 22, 2017 at 12:45:54PM +0100, Kristina Martsenko wrote:
> >> 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.)
> >>
> >> Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com>
> > 
> > It might be worth mentioning that this follows the same behaviour as x86.
> 
> Yep.
> 
> > In fact, they print the name of the faulting VMA using print_vma_addr,
> > which might be useful too.
> 
> Looks useful, should I add that in a separate patch in v2?

Sure, works for me.

Will