[PATCH v2] arm64: print a fault message when attempting to write RO memory

[PATCH v2] arm64: print a fault message when attempting to write RO memory

[Stephen Boyd]

If a page is marked read only we should print out that fact,
instead of printing out that there was a page fault. Right now we
get a cryptic error message that something went wrong with an
unhandled fault, but we don't evaluate the esr to figure out that
it was a read/write permission fault.

Instead of seeing:

  Unable to handle kernel paging request at virtual address ffff000008e460d8
  pgd = ffff800003504000
  [ffff000008e460d8] *pgd=0000000083473003, *pud=0000000083503003, *pmd=0000000000000000
  Internal error: Oops: 9600004f [#1] PREEMPT SMP

we'll see:

  Unable to handle kernel write to read-only memory at virtual address ffff000008e760d8
  pgd = ffff80003d3de000
  [ffff000008e760d8] *pgd=0000000083472003, *pud=0000000083435003, *pmd=0000000000000000
  Internal error: Oops: 9600004f [#1] PREEMPT SMP

Cc: James Morse <james.morse@arm.com>
Cc: Laura Abbott <labbott@redhat.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
---

Changes from v1:
 * Move into __do_kernel_fault() (Mark Rutland)

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

diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 156169c6981b..8bd4e7f11c70 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -160,12 +160,28 @@ 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 int ec       = ESR_ELx_EC(esr);
+	unsigned int fsc_type = esr & ESR_ELx_FSC_TYPE;
+
+	if (ec != ESR_ELx_EC_DABT_CUR && ec != ESR_ELx_EC_IABT_CUR)
+		return false;
+
+	if (system_uses_ttbr0_pan())
+		return fsc_type == ESR_ELx_FSC_FAULT &&
+			(regs->pstate & PSR_PAN_BIT);
+	else
+		return fsc_type == ESR_ELx_FSC_PERM;
+}
+
 /*
  * 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)
 {
+	const char *msg;
 	/*
 	 * Are we prepared to handle this kernel fault?
 	 * We are almost certainly not prepared to handle instruction faults.
@@ -177,9 +193,19 @@ static void __do_kernel_fault(struct mm_struct *mm, unsigned long addr,
 	 * No handler, we'll have to terminate things with extreme prejudice.
 	 */
 	bust_spinlocks(1);
-	pr_alert("Unable to handle kernel %s at virtual address %08lx\n",
-		 (addr < PAGE_SIZE) ? "NULL pointer dereference" :
-		 "paging request", addr);
+
+	if (is_permission_fault(esr, regs)) {
+		if (esr & ESR_ELx_WNR)
+			msg = "write to read-only memory";
+		else
+			msg = "read from unreadable memory";
+	} else if (addr < PAGE_SIZE)
+		msg = "NULL pointer dereference";
+	else
+		msg = "paging request";
+
+	pr_alert("Unable to handle kernel %s@virtual address %08lx\n", msg,
+		 addr);
 
 	show_pte(mm, addr);
 	die("Oops", regs, esr);
@@ -269,21 +295,6 @@ static int __do_page_fault(struct mm_struct *mm, unsigned long addr,
 	return fault;
 }
 
-static inline bool is_permission_fault(unsigned int esr, struct pt_regs *regs)
-{
-	unsigned int ec       = ESR_ELx_EC(esr);
-	unsigned int fsc_type = esr & ESR_ELx_FSC_TYPE;
-
-	if (ec != ESR_ELx_EC_DABT_CUR && ec != ESR_ELx_EC_IABT_CUR)
-		return false;
-
-	if (system_uses_ttbr0_pan())
-		return fsc_type == ESR_ELx_FSC_FAULT &&
-			(regs->pstate & PSR_PAN_BIT);
-	else
-		return fsc_type == ESR_ELx_FSC_PERM;
-}
-
 static bool is_el0_instruction_abort(unsigned int esr)
 {
 	return ESR_ELx_EC(esr) == ESR_ELx_EC_IABT_LOW;
-- 
2.10.0.297.gf6727b0

[PATCH v2] arm64: print a fault message when attempting to write RO memory

[James Morse]

Hi Stephen,

On 17/02/17 01:19, Stephen Boyd wrote:
> If a page is marked read only we should print out that fact,
> instead of printing out that there was a page fault. Right now we
> get a cryptic error message that something went wrong with an
> unhandled fault, but we don't evaluate the esr to figure out that
> it was a read/write permission fault.
> 
> Instead of seeing:
> 
>   Unable to handle kernel paging request at virtual address ffff000008e460d8
>   pgd = ffff800003504000
>   [ffff000008e460d8] *pgd=0000000083473003, *pud=0000000083503003, *pmd=0000000000000000
>   Internal error: Oops: 9600004f [#1] PREEMPT SMP
> 
> we'll see:
> 
>   Unable to handle kernel write to read-only memory at virtual address ffff000008e760d8
>   pgd = ffff80003d3de000
>   [ffff000008e760d8] *pgd=0000000083472003, *pud=0000000083435003, *pmd=0000000000000000
>   Internal error: Oops: 9600004f [#1] PREEMPT SMP

This looks like a good idea..



> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index 156169c6981b..8bd4e7f11c70 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c

>  /*
>   * 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)
>  {
> +	const char *msg;
>  	/*
>  	 * Are we prepared to handle this kernel fault?
>  	 * We are almost certainly not prepared to handle instruction faults.
> @@ -177,9 +193,19 @@ static void __do_kernel_fault(struct mm_struct *mm, unsigned long addr,
>  	 * No handler, we'll have to terminate things with extreme prejudice.
>  	 */
>  	bust_spinlocks(1);
> -	pr_alert("Unable to handle kernel %s at virtual address %08lx\n",
> -		 (addr < PAGE_SIZE) ? "NULL pointer dereference" :
> -		 "paging request", addr);
> +
> +	if (is_permission_fault(esr, regs)) {

is_permission_fault() was previously guarded with a 'addr<USER_DS' check, this
is because it assumes software-PAN is relevant.

The corner case is when the kernel accesses TTBR1-mapped memory while
software-PAN happens to have swivelled TTBR0. Translation faults will be matched
by is_permission_fault(), but permission faults won't.

Juggling is_permission_fault() to look something like:
---%<---
	if (fsc_type == ESR_ELx_FSC_PERM)
		return true;

	if (addr < USER_DS && system_uses_ttbr0_pan())
		return fsc_type == ESR_ELx_FSC_FAULT &&
			(regs->pstate & PSR_PAN_BIT);

	return false;
---%<---
... should fix this.



> +		if (esr & ESR_ELx_WNR)
> +			msg = "write to read-only memory";
> +		else
> +			msg = "read from unreadable memory";
> +	} else if (addr < PAGE_SIZE)
> +		msg = "NULL pointer dereference";
> +	else
> +		msg = "paging request";

Nit: {} all the way down!


> +
> +	pr_alert("Unable to handle kernel %s at virtual address %08lx\n", msg,
> +		 addr);
>  
>  	show_pte(mm, addr);
>  	die("Oops", regs, esr);
> @@ -269,21 +295,6 @@ static int __do_page_fault(struct mm_struct *mm, unsigned long addr,
>  	return fault;
>  }

> -static inline bool is_permission_fault(unsigned int esr, struct pt_regs *regs)
> -{
> -	unsigned int ec       = ESR_ELx_EC(esr);
> -	unsigned int fsc_type = esr & ESR_ELx_FSC_TYPE;
> -
> -	if (ec != ESR_ELx_EC_DABT_CUR && ec != ESR_ELx_EC_IABT_CUR)
> -		return false;
> -
> -	if (system_uses_ttbr0_pan())
> -		return fsc_type == ESR_ELx_FSC_FAULT &&
> -			(regs->pstate & PSR_PAN_BIT);
> -	else
> -		return fsc_type == ESR_ELx_FSC_PERM;
> -}


Thanks!

James

[PATCH v2] arm64: print a fault message when attempting to write RO memory

[Stephen Boyd]

Quoting James Morse (2017-02-17 03:00:39)
> On 17/02/17 01:19, Stephen Boyd wrote:
> > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> > index 156169c6981b..8bd4e7f11c70 100644
> > --- a/arch/arm64/mm/fault.c
> > +++ b/arch/arm64/mm/fault.c
> 
> >  /*
> >   * 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)
> >  {
> > +     const char *msg;
> >       /*
> >        * Are we prepared to handle this kernel fault?
> >        * We are almost certainly not prepared to handle instruction faults.
> > @@ -177,9 +193,19 @@ static void __do_kernel_fault(struct mm_struct *mm, unsigned long addr,
> >        * No handler, we'll have to terminate things with extreme prejudice.
> >        */
> >       bust_spinlocks(1);
> > -     pr_alert("Unable to handle kernel %s at virtual address %08lx\n",
> > -              (addr < PAGE_SIZE) ? "NULL pointer dereference" :
> > -              "paging request", addr);
> > +
> > +     if (is_permission_fault(esr, regs)) {
> 
> is_permission_fault() was previously guarded with a 'addr<USER_DS' check, this
> is because it assumes software-PAN is relevant.
> 
> The corner case is when the kernel accesses TTBR1-mapped memory while
> software-PAN happens to have swivelled TTBR0. Translation faults will be matched
> by is_permission_fault(), but permission faults won't.

If I understand correctly, and I most definitely don't because there are
quite a few combinations, you're saying that __do_kernel_fault() could
be called if the kernel attempts to access some userspace address with
software PAN? That won't be caught in do_page_fault() with the previous
is_permission_fault() check?

> 
> Juggling is_permission_fault() to look something like:
> ---%<---
>         if (fsc_type == ESR_ELx_FSC_PERM)
>                 return true;
> 
>         if (addr < USER_DS && system_uses_ttbr0_pan())
>                 return fsc_type == ESR_ELx_FSC_FAULT &&
>                         (regs->pstate & PSR_PAN_BIT);
> 
>         return false;
> ---%<---
> ... should fix this.

But we don't need to check ec anymore?

	if (ec != ESR_ELx_EC_DABT_CUR && ec != ESR_ELx_EC_IABT_CUR)
		return false;

> 
> 
> > +             if (esr & ESR_ELx_WNR)
> > +                     msg = "write to read-only memory";
> > +             else
> > +                     msg = "read from unreadable memory";
> > +     } else if (addr < PAGE_SIZE)
> > +             msg = "NULL pointer dereference";
> > +     else
> > +             msg = "paging request";
> 
> Nit: {} all the way down!

Ok.

[PATCH v2] arm64: print a fault message when attempting to write RO memory

[James Morse]

Hi Stephen,

On 17/02/17 15:53, Stephen Boyd wrote:
> Quoting James Morse (2017-02-17 03:00:39)
>> On 17/02/17 01:19, Stephen Boyd wrote:
>>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
>>> index 156169c6981b..8bd4e7f11c70 100644
>>> --- a/arch/arm64/mm/fault.c
>>> +++ b/arch/arm64/mm/fault.c
>>> @@ -177,9 +193,19 @@ static void __do_kernel_fault(struct mm_struct *mm, unsigned long addr,
>>>        * No handler, we'll have to terminate things with extreme prejudice.
>>>        */
>>>       bust_spinlocks(1);
>>> -     pr_alert("Unable to handle kernel %s at virtual address %08lx\n",
>>> -              (addr < PAGE_SIZE) ? "NULL pointer dereference" :
>>> -              "paging request", addr);
>>> +
>>> +     if (is_permission_fault(esr, regs)) {
>>
>> is_permission_fault() was previously guarded with a 'addr<USER_DS' check, this
>> is because it assumes software-PAN is relevant.
>>
>> The corner case is when the kernel accesses TTBR1-mapped memory while
>> software-PAN happens to have swivelled TTBR0. Translation faults will be matched
>> by is_permission_fault(), but permission faults won't.
> 
> If I understand correctly, and I most definitely don't because there are
> quite a few combinations, you're saying that __do_kernel_fault() could
> be called if the kernel attempts to access some userspace address with
> software PAN? That won't be caught in do_page_fault() with the previous
> is_permission_fault() check?

You're right the user-address side of things will get caught in do_page_fault().
I was trying to badly-explain 'is_permission_fault(esr)' isn't as general
purpose as its name and prototype suggest, it only gives the results that the
PAN checks expect when called with a user address.


>> Juggling is_permission_fault() to look something like:
>> ---%<---
>>         if (fsc_type == ESR_ELx_FSC_PERM)
>>                 return true;
>>
>>         if (addr < USER_DS && system_uses_ttbr0_pan())
>>                 return fsc_type == ESR_ELx_FSC_FAULT &&
>>                         (regs->pstate & PSR_PAN_BIT);
>>
>>         return false;
>> ---%<---
>> ... should fix this.
> 
> But we don't need to check ec anymore?

Sorry, I was being sloppy, something like the above could replace the if/else
block at the end of is_permission_fault(). You're right we still need the ec check!


Thanks,

James

[PATCH v2] arm64: print a fault message when attempting to write RO memory

[Stephen Boyd]

Quoting James Morse (2017-02-20 03:10:10)
> Hi Stephen,
> 
> On 17/02/17 15:53, Stephen Boyd wrote:
> > Quoting James Morse (2017-02-17 03:00:39)
> >> On 17/02/17 01:19, Stephen Boyd wrote:
> >>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> >>> index 156169c6981b..8bd4e7f11c70 100644
> >>> --- a/arch/arm64/mm/fault.c
> >>> +++ b/arch/arm64/mm/fault.c
> >>> @@ -177,9 +193,19 @@ static void __do_kernel_fault(struct mm_struct *mm, unsigned long addr,
> >>>        * No handler, we'll have to terminate things with extreme prejudice.
> >>>        */
> >>>       bust_spinlocks(1);
> >>> -     pr_alert("Unable to handle kernel %s at virtual address %08lx\n",
> >>> -              (addr < PAGE_SIZE) ? "NULL pointer dereference" :
> >>> -              "paging request", addr);
> >>> +
> >>> +     if (is_permission_fault(esr, regs)) {
> >>
> >> is_permission_fault() was previously guarded with a 'addr<USER_DS' check, this
> >> is because it assumes software-PAN is relevant.
> >>
> >> The corner case is when the kernel accesses TTBR1-mapped memory while
> >> software-PAN happens to have swivelled TTBR0. Translation faults will be matched
> >> by is_permission_fault(), but permission faults won't.
> > 
> > If I understand correctly, and I most definitely don't because there are
> > quite a few combinations, you're saying that __do_kernel_fault() could
> > be called if the kernel attempts to access some userspace address with
> > software PAN? That won't be caught in do_page_fault() with the previous
> > is_permission_fault() check?
> 
> You're right the user-address side of things will get caught in do_page_fault().
> I was trying to badly-explain 'is_permission_fault(esr)' isn't as general
> purpose as its name and prototype suggest, it only gives the results that the
> PAN checks expect when called with a user address.

Ok. I'd rather not change the function in this patch because I'm only
moving the code around to use it higher up in the file. But if you
prefer I can combine the code movement with the addition of a new 'addr'
argument to this function and rework things based on that.

[PATCH v2] arm64: print a fault message when attempting to write RO memory

[Will Deacon]

Hi Stephen,

On Fri, Feb 24, 2017 at 05:39:08PM -0800, Stephen Boyd wrote:
> Quoting James Morse (2017-02-20 03:10:10)
> > On 17/02/17 15:53, Stephen Boyd wrote:
> > > Quoting James Morse (2017-02-17 03:00:39)
> > >> On 17/02/17 01:19, Stephen Boyd wrote:
> > >>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> > >>> index 156169c6981b..8bd4e7f11c70 100644
> > >>> --- a/arch/arm64/mm/fault.c
> > >>> +++ b/arch/arm64/mm/fault.c
> > >>> @@ -177,9 +193,19 @@ static void __do_kernel_fault(struct mm_struct *mm, unsigned long addr,
> > >>>        * No handler, we'll have to terminate things with extreme prejudice.
> > >>>        */
> > >>>       bust_spinlocks(1);
> > >>> -     pr_alert("Unable to handle kernel %s at virtual address %08lx\n",
> > >>> -              (addr < PAGE_SIZE) ? "NULL pointer dereference" :
> > >>> -              "paging request", addr);
> > >>> +
> > >>> +     if (is_permission_fault(esr, regs)) {
> > >>
> > >> is_permission_fault() was previously guarded with a 'addr<USER_DS' check, this
> > >> is because it assumes software-PAN is relevant.
> > >>
> > >> The corner case is when the kernel accesses TTBR1-mapped memory while
> > >> software-PAN happens to have swivelled TTBR0. Translation faults will be matched
> > >> by is_permission_fault(), but permission faults won't.
> > > 
> > > If I understand correctly, and I most definitely don't because there are
> > > quite a few combinations, you're saying that __do_kernel_fault() could
> > > be called if the kernel attempts to access some userspace address with
> > > software PAN? That won't be caught in do_page_fault() with the previous
> > > is_permission_fault() check?
> > 
> > You're right the user-address side of things will get caught in do_page_fault().
> > I was trying to badly-explain 'is_permission_fault(esr)' isn't as general
> > purpose as its name and prototype suggest, it only gives the results that the
> > PAN checks expect when called with a user address.
> 
> Ok. I'd rather not change the function in this patch because I'm only
> moving the code around to use it higher up in the file. But if you
> prefer I can combine the code movement with the addition of a new 'addr'
> argument to this function and rework things based on that.

Are you planning to send a v3 of this?

Will

[PATCH v2] arm64: print a fault message when attempting to write RO memory

[Stephen Boyd]

Quoting Will Deacon (2017-03-23 07:22:39)
> On Fri, Feb 24, 2017 at 05:39:08PM -0800, Stephen Boyd wrote:
> > Quoting James Morse (2017-02-20 03:10:10)
> > > 
> > > You're right the user-address side of things will get caught in do_page_fault().
> > > I was trying to badly-explain 'is_permission_fault(esr)' isn't as general
> > > purpose as its name and prototype suggest, it only gives the results that the
> > > PAN checks expect when called with a user address.
> > 
> > Ok. I'd rather not change the function in this patch because I'm only
> > moving the code around to use it higher up in the file. But if you
> > prefer I can combine the code movement with the addition of a new 'addr'
> > argument to this function and rework things based on that.
> 
> Are you planning to send a v3 of this?
> 

Sorry for the late reply. I was hoping that James would indicate
preference one way or the other. I suppose no reply means "yes" here, so
I'll go ahead and fold everything together into one patch and resend.