[PATCH v2] ARM: mm: Provide better fault message for permission fault

[PATCH v2] ARM: mm: Provide better fault message for permission fault

[Kefeng Wang]

If there is a permission fault in __do_kernel_fault(), we only
print the generic "paging request" message which don't show
read, write or excute information, let's provide better fault
message for them.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
v2: fix !CONFIG_MMU built

 arch/arm/mm/fault.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
index 46cccd6bf705..387c87112d80 100644
--- a/arch/arm/mm/fault.c
+++ b/arch/arm/mm/fault.c
@@ -105,6 +105,19 @@ static inline bool is_write_fault(unsigned int fsr)
 	return (fsr & FSR_WRITE) && !(fsr & FSR_CM);
 }
 
+static inline bool is_permission_fault(unsigned int fsr)
+{
+	int fs = fsr_fs(fsr);
+#ifdef CONFIG_ARM_LPAE
+	if ((fs & FS_PERM_NOLL_MASK) == FS_PERM_NOLL)
+		return true;
+#else
+	if (fs == FS_L1_PERM || fs == FS_L2_PERM)
+		return true;
+#endif
+	return false;
+}
+
 static void die_kernel_fault(const char *msg, struct mm_struct *mm,
 			     unsigned long addr, unsigned int fsr,
 			     struct pt_regs *regs)
@@ -137,7 +150,14 @@ __do_kernel_fault(struct mm_struct *mm, unsigned long addr, unsigned int fsr,
 	/*
 	 * No handler, we'll have to terminate things with extreme prejudice.
 	 */
-	if (addr < PAGE_SIZE) {
+	if (is_permission_fault(fsr)) {
+		if (fsr & FSR_WRITE)
+			msg = "write to read-only memory";
+		else if (fsr & FSR_LNX_PF)
+			msg = "execute from non-executable memory";
+		else
+			msg = "read from unreadable memory";
+	} else if (addr < PAGE_SIZE) {
 		msg = "NULL pointer dereference";
 	} else {
 		if (kfence_handle_page_fault(addr, is_write_fault(fsr), regs))
@@ -204,19 +224,6 @@ void do_bad_area(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 #define VM_FAULT_BADMAP		((__force vm_fault_t)0x010000)
 #define VM_FAULT_BADACCESS	((__force vm_fault_t)0x020000)
 
-static inline bool is_permission_fault(unsigned int fsr)
-{
-	int fs = fsr_fs(fsr);
-#ifdef CONFIG_ARM_LPAE
-	if ((fs & FS_PERM_NOLL_MASK) == FS_PERM_NOLL)
-		return true;
-#else
-	if (fs == FS_L1_PERM || fs == FS_L2_PERM)
-		return true;
-#endif
-	return false;
-}
-
 static vm_fault_t __kprobes
 __do_page_fault(struct mm_struct *mm, unsigned long addr, unsigned int flags,
 		unsigned long vma_flags, struct pt_regs *regs)
-- 
2.35.3

Re: [PATCH v2] ARM: mm: Provide better fault message for permission fault

[Russell King (Oracle)]

On Mon, Sep 19, 2022 at 06:38:45PM +0800, Kefeng Wang wrote:
> If there is a permission fault in __do_kernel_fault(), we only
> print the generic "paging request" message which don't show
> read, write or excute information, let's provide better fault
> message for them.

I don't like this change. With CPUs that do not have the ability to
relocate the vectors to 0xffff0000, the vectors live at address 0,
so NULL pointer dereferences can produce permission faults.

I would much rather we did something similar to what x86 does:

        pr_alert("#PF: %s %s in %s mode\n",
                 (error_code & X86_PF_USER)  ? "user" : "supervisor",
                 (error_code & X86_PF_INSTR) ? "instruction fetch" :
                 (error_code & X86_PF_WRITE) ? "write access" :
                                               "read access",
                             user_mode(regs) ? "user" : "kernel");

As we already print whether we're in user or kernel mode in the
register dump, there's no need to repeat that. I think we just
need an extra line to decode the FSR PF and write bits.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2] ARM: mm: Provide better fault message for permission fault

[Kefeng Wang]

On 2022/9/26 18:13, Russell King (Oracle) wrote:
> On Mon, Sep 19, 2022 at 06:38:45PM +0800, Kefeng Wang wrote:
>> If there is a permission fault in __do_kernel_fault(), we only
>> print the generic "paging request" message which don't show
>> read, write or excute information, let's provide better fault
>> message for them.
> I don't like this change. With CPUs that do not have the ability to
> relocate the vectors to 0xffff0000, the vectors live at address 0,
> so NULL pointer dereferences can produce permission faults.
The __do_user_fault(), do_DataAbort() and do_PrefetchAbort() shows
the FSR when printing, we could do it in die_kernel_fault(), and
which will be easy for us to check whether the page fault is permision
fault,


--- a/arch/arm/mm/fault.c
+++ b/arch/arm/mm/fault.c
@@ -111,8 +111,8 @@ static void die_kernel_fault(const char *msg, struct 
mm_struct *mm,
  {
         bust_spinlocks(1);
         pr_alert("8<--- cut here ---\n");
-       pr_alert("Unable to handle kernel %s at virtual address %08lx\n",
-                msg, addr);
+       pr_alert("Unable to handle kernel %s (0x%08x) at virtual address 
%08lx\n",
+                msg, fsr, addr);

         show_pte(KERN_ALERT, mm, addr);
         die("Oops", regs, fsr);


or,

> I would much rather we did something similar to what x86 does:
>
>          pr_alert("#PF: %s %s in %s mode\n",
>                   (error_code & X86_PF_USER)  ? "user" : "supervisor",
>                   (error_code & X86_PF_INSTR) ? "instruction fetch" :
>                   (error_code & X86_PF_WRITE) ? "write access" :
>                                                 "read access",
>                               user_mode(regs) ? "user" : "kernel");
>
> As we already print whether we're in user or kernel mode in the
> register dump, there's no need to repeat that. I think we just
> need an extra line to decode the FSR PF and write bits.

We could decode the FSR register,

diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c

index 46cccd6bf705..406e0210c3c5 100644
--- a/arch/arm/mm/fault.c
+++ b/arch/arm/mm/fault.c
@@ -113,6 +113,10 @@ static void die_kernel_fault(const char *msg, 
struct mm_struct *mm,
         pr_alert("8<--- cut here ---\n");
         pr_alert("Unable to handle kernel %s at virtual address %08lx\n",
                  msg, addr);
+       pr_alert("FSR: 0x%08x, LNX_PF = %u, CM = %u, WnR = %u\n", fsr,
+                (fsr & FSR_LNX_PF) >> FSR_LNX_PF_SHIFT,
+                (fsr & FSR_CM) >> FSR_CM_SHIFT,
+                (fsr & FSR_WRITE) >> FSR_WRITE_SHIFT);

         show_pte(KERN_ALERT, mm, addr);
         die("Oops", regs, fsr);
diff --git a/arch/arm/mm/fault.h b/arch/arm/mm/fault.h
index 83b5ab32d7a4..18f882aa2b32 100644
--- a/arch/arm/mm/fault.h
+++ b/arch/arm/mm/fault.h
@@ -5,9 +5,12 @@
  /*
   * Fault status register encodings.  We steal bit 31 for our own purposes.
   */
-#define FSR_LNX_PF             (1 << 31)
-#define FSR_CM                 (1 << 13)
-#define FSR_WRITE              (1 << 11)
+#define FSR_LNX_PF_SHIFT       (31)
+#define FSR_LNX_PF             (1 << FSR_LNX_PF_SHIFT)
+#define FSR_CM_SHIFT           (13)
+#define FSR_CM                 (1 << FSR_CM_SHIFT)
+#define FSR_WRITE_SHIFT                (11)
+#define FSR_WRITE              (1 << FSR_WRITE_SHIFT)
  #define FSR_FS4                        (1 << 10)
  #define FSR_FS3_0              (15)
  #define FSR_FS5_0              (0x3f)


What's your option ?

>

Re: [PATCH v2] ARM: mm: Provide better fault message for permission fault

[Russell King (Oracle)]

On Mon, Sep 26, 2022 at 09:26:50PM +0800, Kefeng Wang wrote:
> 
> On 2022/9/26 18:13, Russell King (Oracle) wrote:
> > On Mon, Sep 19, 2022 at 06:38:45PM +0800, Kefeng Wang wrote:
> > > If there is a permission fault in __do_kernel_fault(), we only
> > > print the generic "paging request" message which don't show
> > > read, write or excute information, let's provide better fault
> > > message for them.
> > I don't like this change. With CPUs that do not have the ability to
> > relocate the vectors to 0xffff0000, the vectors live at address 0,
> > so NULL pointer dereferences can produce permission faults.
> The __do_user_fault(), do_DataAbort() and do_PrefetchAbort() shows
> the FSR when printing, we could do it in die_kernel_fault(), and
> which will be easy for us to check whether the page fault is permision
> fault,

We print the hex value already in the oops:

Internal error: Oops: (fsr hex value) [#num] ...

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

[PATCH v3] ARM: mm: Provide better message when kernel fault

[Kefeng Wang]

If there is a kernel fault, see do_kernel_fault(), we only print
the generic "paging request" or "NULL pointer dereference" message
which don't show read, write or excute information, let's provide
better fault message for them.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
v3: show the infos in die_kernel_fault()
 arch/arm/mm/fault.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
index 46cccd6bf705..f8fe0ec64c23 100644
--- a/arch/arm/mm/fault.c
+++ b/arch/arm/mm/fault.c
@@ -111,8 +111,9 @@ static void die_kernel_fault(const char *msg, struct mm_struct *mm,
 {
 	bust_spinlocks(1);
 	pr_alert("8<--- cut here ---\n");
-	pr_alert("Unable to handle kernel %s at virtual address %08lx\n",
-		 msg, addr);
+	pr_alert("Unable to handle kernel %s at virtual address %08lx when %s\n",
+		 msg, addr, fsr & FSR_LNX_PF ? "execute" :
+		 fsr & FSR_WRITE ? "write" : "read");
 
 	show_pte(KERN_ALERT, mm, addr);
 	die("Oops", regs, fsr);
-- 
2.35.3

Re: [PATCH v3] ARM: mm: Provide better message when kernel fault

[Kefeng Wang]

On 2022/9/27 14:21, Kefeng Wang wrote:
> If there is a kernel fault, see do_kernel_fault(), we only print
> the generic "paging request" or "NULL pointer dereference" message
> which don't show read, write or excute information, let's provide
> better fault message for them.

Hi Russell, what's your option about this one, if no object,

I will send to ARM patch system, thanks,

> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
> v3: show the infos in die_kernel_fault()
>   arch/arm/mm/fault.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
> index 46cccd6bf705..f8fe0ec64c23 100644
> --- a/arch/arm/mm/fault.c
> +++ b/arch/arm/mm/fault.c
> @@ -111,8 +111,9 @@ static void die_kernel_fault(const char *msg, struct mm_struct *mm,
>   {
>   	bust_spinlocks(1);
>   	pr_alert("8<--- cut here ---\n");
> -	pr_alert("Unable to handle kernel %s at virtual address %08lx\n",
> -		 msg, addr);
> +	pr_alert("Unable to handle kernel %s at virtual address %08lx when %s\n",
> +		 msg, addr, fsr & FSR_LNX_PF ? "execute" :
> +		 fsr & FSR_WRITE ? "write" : "read");
>   
>   	show_pte(KERN_ALERT, mm, addr);
>   	die("Oops", regs, fsr);

Re: [PATCH v3] ARM: mm: Provide better message when kernel fault

[Russell King (Oracle)]

On Mon, Oct 10, 2022 at 07:24:15PM +0800, Kefeng Wang wrote:
> 
> On 2022/9/27 14:21, Kefeng Wang wrote:
> > If there is a kernel fault, see do_kernel_fault(), we only print
> > the generic "paging request" or "NULL pointer dereference" message
> > which don't show read, write or excute information, let's provide
> > better fault message for them.
> 
> Hi Russell, what's your option about this one, if no object,

LGTM.

> I will send to ARM patch system, thanks,

Yes please, if not already done.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!