ARM: fix hash_name() and branch predictor issues

This series, which I'm intending to be part of my upcoming pull request for this merge window (note - will be sent before Thursday, otherwise it won't be until maybe the middle of January) fixes the recently reported hash_name() issue, and also the long standing branch predictor hardening smp_processor_id() warning which occurs due to interrupts being enabled.

Fixing this isn't trivial because there are many paths through do_page_fault() which are non-obvious.

For example, we detect page faults by the kernel attempting to execute memory that it shouldn't and print such a warning. If we introduce an early address check, we avoid that detection, making it become a plain kernel oops without the informative error message.

The first patch adds an additional check in __do_kernel_fault() to detect this condition. This patch is a non-obvious dependency for the next patch.

The second patch handles faults above the top of userspace entirely separately, meaning we have a simpler and more obvious fault path, which avoids any possibility of taking any MM mutexes, which is the cause of the hash_name() problem.

The third patch moves harden_branch_predictor() out of __do_user_fault() and to appropriate places in the parent functions. The reason this has been avoided thus far is because do_page_fault() can be a hot path (since it's used for page aging as well) but with kernel address faults being handled by an entirely separate path, this avoids adding to that overhead.

I would like to get some attributations for this - specifically tested-by.

Thanks.

 arch/arm/mm/alignment.c |  6 +++-
 arch/arm/mm/fault.c     | 74 +++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 67 insertions(+), 13 deletions(-)

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

Allow __do_kernel_fault() to detect the execution of memory, so we can provide the same fault message as do_page_fault() would do. This is required when we split the kernel address fault handling from the main do_page_fault() code path.

---
 arch/arm/mm/fault.c | 2 ++
 1 file changed, 2 insertions(+)

index 2bc828a1940c..f0884bf91dfa 100644
@@ -162,6 +162,8 @@ __do_kernel_fault(struct mm_struct *mm, unsigned long addr, unsigned int fsr,
 	 */
 	if (addr < PAGE_SIZE) {
 		msg = "NULL pointer dereference";
+	} else if (is_permission_fault(fsr) && fsr & FSR_LNX_PF) {
+		msg = "execution of memory";
 	} else {
 		if (is_translation_fault(fsr) &&
 		    kfence_handle_page_fault(addr, is_write_fault(fsr), regs))

2.47.3

Thanks - as you've guessed I have some other patches on top of this which move the is_xxx_fault() into fault.h.

However, for the sake of this series, my preferred fix is to group all the is_xxx_fault() inline functions together as the first patch of this series:

8<===
From: "Russell King (Oracle)" <[email protected]>
Subject: [PATCH] ARM: group is_permission_fault() with is_translation_fault()

Group is_permission_fault() with is_translation_fault(), which is needed to use is_permission_fault() in __do_kernel_fault(). As this is static inline, there is no need for this to be under CONFIG_MMU.

---
 arch/arm/mm/fault.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

index 2bc828a1940c..f87f353e5a8b 100644
@@ -128,6 +128,19 @@ static inline bool is_translation_fault(unsigned int fsr)
 	return false;
 }
 
+static inline bool is_permission_fault(unsigned int fsr)
+{
+	int fs = fsr_fs(fsr);
+#ifdef CONFIG_ARM_LPAE
+	if ((fs & FS_MMU_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)

@@ -225,19 +238,6 @@ void do_bad_area(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 }
 
 #ifdef CONFIG_MMU
-static inline bool is_permission_fault(unsigned int fsr)
-{
-	int fs = fsr_fs(fsr);
-#ifdef CONFIG_ARM_LPAE
-	if ((fs & FS_MMU_NOLL_MASK) == FS_PERM_NOLL)
-		return true;
-#else
-	if (fs == FS_L1_PERM || fs == FS_L2_PERM)
-		return true;
-#endif
-	return false;
-}
-
 #ifdef CONFIG_CPU_TTBR0_PAN
 static inline bool ttbr0_usermode_access_allowed(struct pt_regs *regs)
 {

2.47.3

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

Zizhi Wo reports:

"During the execution of hash_name()->load_unaligned_zeropad(), a
potential memory access beyond the PAGE boundary may occur. For example, when the filename length is near the PAGE_SIZE boundary. This triggers a page fault, which leads to a call to do_page_fault()->mmap_read_trylock(). If we can't acquire the lock, we have to fall back to the mmap_read_lock() path, which calls might_sleep(). This breaks RCU semantics because path lookup occurs under an RCU read-side critical section."

This is seen with CONFIG_DEBUG_ATOMIC_SLEEP=y and CONFIG_KFENCE=y.

Kernel addresses (with the exception of the vectors/kuser helper page) do not have VMAs associated with them. If the vectors/kuser helper page faults, then there are two possibilities:

1. if the fault happened while in kernel mode, then we're basically dead, because the CPU won't be able to vector through this page to handle the fault.
2. if the fault happened while in user mode, that means the page was protected from user access, and we want to fault anyway.

Thus, we can handle kernel addresses from any context entirely separately without going anywhere near the mmap lock. This gives us an entirely non-sleeping path for all kernel mode kernel address faults.

As we handle the kernel address faults before interrupts are enabled, this change has the side effect of improving the branch predictor hardening, but does not completely solve the issue.

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

index f0884bf91dfa..92fc70b56d72 100644
@@ -261,6 +261,35 @@ static inline bool ttbr0_usermode_access_allowed(struct pt_regs *regs)
 }
 #endif
 
+static int __kprobes
+do_kernel_address_page_fault(struct mm_struct *mm, unsigned long addr,
+			     unsigned int fsr, struct pt_regs *regs)
+{
+	if (user_mode(regs)) {
+		/*
+		 * Fault from user mode for a kernel space address. User mode
+		 * should not be faulting in kernel space, which includes the
+		 * vector/khelper page. Send a SIGSEGV.
+		 */
+		__do_user_fault(addr, fsr, SIGSEGV, SEGV_MAPERR, regs);
+	} else {
+		/*
+		 * Fault from kernel mode. Enable interrupts if they were
+		 * enabled in the parent context. Section (upper page table)
+		 * translation faults are handled via do_translation_fault(),
+		 * so we will only get here for a non-present kernel space
+		 * PTE or PTE permission fault. This may happen in exceptional
+		 * circumstances and need the fixup tables to be walked.
+		 */
+		if (interrupts_enabled(regs))
+			local_irq_enable();
+
+		__do_kernel_fault(mm, addr, fsr, regs);
+	}
+
+	return 0;
+}
+
 static int __kprobes
 do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 {

@@ -274,6 +303,12 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 	if (kprobe_page_fault(regs, fsr))
 		return 0;
 
+	/*
+	 * Handle kernel addresses faults separately, which avoids touching
+	 * the mmap lock from contexts that are not able to sleep.
+	 */
+	if (addr >= TASK_SIZE)
+		return do_kernel_address_page_fault(mm, addr, fsr, regs);
 
 	/* Enable interrupts if they were enabled in the parent context. */
 	if (interrupts_enabled(regs))

2.47.3

__do_user_fault() may be called with indeterminent interrupt enable state, which means we may be preemptive at this point. This causes problems when calling harden_branch_predictor(). For example, when called from a data abort, do_alignment_fault()->do_bad_area().

Move harden_branch_predictor() out of __do_user_fault() and into the calling contexts.

Moving it into do_kernel_address_page_fault(), we can be sure that interrupts will be disabled here.

Converting do_translation_fault() to use do_kernel_address_page_fault() rather than do_bad_area() means that we keep branch predictor handling for translation faults. Interrupts will also be disabled at this call site.

do_sect_fault() needs special handling, so detect user mode accesses to kernel-addresses, and add an explicit call to branch predictor hardening.

Finally, add branch predictor hardening to do_alignment() for the faulting case (user mode accessing kernel addresses) before interrupts are enabled.

This should cover all cases where harden_branch_predictor() is called, ensuring that it is always has interrupts disabled, also ensuring that it is called early in each call path.

---
 arch/arm/mm/alignment.c |  6 +++++-
 arch/arm/mm/fault.c     | 39 ++++++++++++++++++++++++++-------------
 2 files changed, 31 insertions(+), 14 deletions(-)

index 3c6ddb1afdc4..812380f30ae3 100644
@@ -19,10 +19,11 @@
 #include <linux/init.h>
 #include <linux/sched/signal.h>
 #include <linux/uaccess.h>
+#include <linux/unaligned.h>
 
 #include <asm/cp15.h>
 #include <asm/system_info.h>
-#include <linux/unaligned.h>
+#include <asm/system_misc.h>
 #include <asm/opcodes.h>
 
 #include "fault.h"

@@ -809,6 +810,9 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 	int thumb2_32b = 0;
 	int fault;
 
+	if (addr >= TASK_SIZE && user_mode(regs))
+		harden_branch_predictor();
+
 	if (interrupts_enabled(regs))
 		local_irq_enable();
 

index 92fc70b56d72..6d7a7e639321 100644
@@ -185,9 +185,6 @@ __do_user_fault(unsigned long addr, unsigned int fsr, unsigned int sig,
 {
 	struct task_struct *tsk = current;
 
-	if (addr > TASK_SIZE)
-		harden_branch_predictor();
-
 #ifdef CONFIG_DEBUG_USER
 	if (((user_debug & UDBG_SEGV) && (sig == SIGSEGV)) ||
 	    ((user_debug & UDBG_BUS)  && (sig == SIGBUS))) {

@@ -269,8 +266,10 @@ do_kernel_address_page_fault(struct mm_struct *mm, unsigned long addr,
 		/*
 		 * Fault from user mode for a kernel space address. User mode
 		 * should not be faulting in kernel space, which includes the
-		 * vector/khelper page. Send a SIGSEGV.
+		 * vector/khelper page. Handle the branch predictor hardening
+		 * while interrupts are still disabled, then send a SIGSEGV.
 		 */
+		harden_branch_predictor();
 		__do_user_fault(addr, fsr, SIGSEGV, SEGV_MAPERR, regs);
 	} else {
 		/*

@@ -485,16 +484,20 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
  * We enter here because the first level page table doesn't contain
  * a valid entry for the address.
  *
- * If the address is in kernel space (>= TASK_SIZE), then we are
- * probably faulting in the vmalloc() area.
+ * If this is a user address (addr < TASK_SIZE), we handle this as a
+ * normal page fault. This leaves the remainder of the function to handle
+ * kernel address translation faults.
  *
- * If the init_task's first level page tables contains the relevant
- * entry, we copy the it to this task.  If not, we send the process
- * a signal, fixup the exception, or oops the kernel.
+ * Since user mode is not permitted to access kernel addresses, pass these
+ * directly to do_kernel_address_page_fault() to handle.
  *
- * NOTE! We MUST NOT take any locks for this case. We may be in an
- * interrupt or a critical region, and should only copy the information
- * from the master page table, nothing more.
+ * Otherwise, we're probably faulting in the vmalloc() area, so try to fix
+ * that up. Note that we must not take any locks or enable interrupts in
+ * this case.
+ *
+ * If vmalloc() fixup fails, that means the non-leaf page tables did not
+ * contain an entry for this address, so handle this via
+ * do_kernel_address_page_fault().
  */
 #ifdef CONFIG_MMU
 static int __kprobes

@@ -560,7 +563,8 @@ do_translation_fault(unsigned long addr, unsigned int fsr,
 	return 0;
 
 bad_area:
-	do_bad_area(addr, fsr, regs);
+	do_kernel_address_page_fault(current->mm, addr, fsr, regs);
+
 	return 0;
 }
 #else					/* CONFIG_MMU */

@@ -580,7 +584,16 @@ do_translation_fault(unsigned long addr, unsigned int fsr,
 static int
 do_sect_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 {
+	/*
+	 * If this is a kernel address, but from user mode, then userspace
+	 * is trying bad stuff. Invoke the branch predictor handling.
+	 * Interrupts are disabled here.
+	 */
+	if (addr >= TASK_SIZE && user_mode(regs))
+		harden_branch_predictor();
+
 	do_bad_area(addr, fsr, regs);
+
 	return 0;
 }
 #endif /* CONFIG_ARM_LPAE */

2.47.3

I don't have any objections to the first and second patches. However, for the third patch, I feel it complicates some things. It removes harden_branch_predictor() from __do_user_fault() and adds it to various previous calling points.

This seems to be solely for adapting to the scenario of do_alignment() && user_mode(regs) && addr >= TASK_SIZE. Does this scenario really exist? Again, in your last email, you described it as follows:

In other words, if there is a BUG_ON():

```patch
diff --git a/arch/arm/mm/alignment.c b/arch/arm/mm/alignment.c
index 3c6ddb1afdc4..d61c5adc7f96 100644
--- a/arch/arm/mm/alignment.c
+++ b/arch/arm/mm/alignment.c
@@ -833,10 +833,12 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 		}
 	} else {
 		fault = alignment_get_arm(regs, (void *)instrptr, &instr);
 	}
 
+	BUG_ON(user_mode(regs) && !fault && addr >= TASK_SIZE);
+
 	if (fault) {
 		type = TYPE_FAULT;
 		goto bad_or_fault;
 	}
```

Could this BUG_ON() be triggered?

If the answer is "no," then I think that something this may be enough:

```patch
diff --git a/arch/arm/mm/alignment.c b/arch/arm/mm/alignment.c
index 3c6ddb1afdc4..03aa2cff8c16 100644
--- a/arch/arm/mm/alignment.c
+++ b/arch/arm/mm/alignment.c
@@ -809,6 +809,9 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 	int thumb2_32b = 0;
 	int fault;

+	if (unlikely(addr >= TASK_SIZE && user_mode(regs)))
+		return do_kernel_address_page_fault(current->mm, addr, fsr, regs);
+
 	if (interrupts_enabled(regs))
 		local_irq_enable();

```

And just keep harden_branch_predictor() stay in __do_user_fault().

If I have misunderstood anything, please point it out. Thanks very much.

As explained in the other thread, alignment faults are the first thing that are checked when enabled, before permission fauls, so we can end up entering do_alignment() from userspace with an address in the kernel address space.

However, there is a user accessible page above TASK_SIZE, that is the vectors page which can have userspace helpers in. Userspace can read from this page, and thus can perform unaligned loads to this page.

The system behaviour of unaligned loads has been configurable, ARMv4, for example, had the ability to check for alignment faults and raise this exception. If that was disabled, then unaligned loads would load the equivalent 32-bit aligned address and rotate the data in the register. Compilers knew about this, and would code for it, which made it ABI. Later compilers stopped using that which cut down on the unaligned loads that userspace issued. Modern CPUs (ARMv7) tend to behave "sanely" in that unaligned loads are generally supported in hardware.

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

Also, would you mind giving ASAP some tested-by/reviewed-by for these patches so they can be pushed out to linux-next for a bit of testing there pelase? I'm on vacation from Thursday, so time is very short to get these out - if we're not ready by Wednesday, then it'll be the new year, possibly after the 11th January, before I can do anything further (medical stuff.)

Thanks.

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

As mentioned in the previous email, this(these patches) should be the best solution, I think. And I have carefully read the three patches, so:

As for testing, I can perform some simple tests myself, and I am trying to let the testing team to help with some further testing, like syzkaller.

Hi, Russell!

TL; DR:
patch1 and patch2 with compilation error fixed:

patch3:
The simple test case passed, but the syzkaller test (the original scenario that triggered the branch predictor issue) is still running. The syzkaller test is better to run for a day.

All patches with compilation error fixed:

Detailed Description:

Your first patch caused a compilation error: is_permission_fault() is not defined. I fixed it with this patch:

```patch
diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
index 2bc828a1940c..7082ba87e886 100644
--- a/arch/arm/mm/fault.c
+++ b/arch/arm/mm/fault.c
@@ -143,6 +143,21 @@ static void die_kernel_fault(const char *msg, struct mm_struct *mm,
 	make_task_dead(SIGKILL);
 }

+static inline bool is_permission_fault(unsigned int fsr)
+{
+#ifdef CONFIG_MMU
+	int fs = fsr_fs(fsr);
+#ifdef CONFIG_ARM_LPAE
+	if ((fs & FS_MMU_NOLL_MASK) == FS_PERM_NOLL)
+		return true;
+#else
+	if (fs == FS_L1_PERM || fs == FS_L2_PERM)
+		return true;
+#endif
+#endif
+	return false;
+}
+
 /*
  * Oops.  The kernel tried to access some page that wasn't present.
  */
@@ -225,19 +240,6 @@ void do_bad_area(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 }

 #ifdef CONFIG_MMU
-static inline bool is_permission_fault(unsigned int fsr)
-{
-	int fs = fsr_fs(fsr);
-#ifdef CONFIG_ARM_LPAE
-	if ((fs & FS_MMU_NOLL_MASK) == FS_PERM_NOLL)
-		return true;
-#else
-	if (fs == FS_L1_PERM || fs == FS_L2_PERM)
-		return true;
-#endif
-	return false;
-}
-
 #ifdef CONFIG_CPU_TTBR0_PAN
 static inline bool ttbr0_usermode_access_allowed(struct pt_regs *regs)
 {
```

With the compilation error fixing patch first, and then your three patches, and then Liyuan's patch:

The current testing situation is as follows:

1. About the hash_name() fault issue:
Simple test case:

that can trigger this issue, as well as the original scenario which trigger this issue, have passed the tests.

So patch1 and patch2:

2. About the branch predictor issue:
Simple test cases (Link same as above) that can trigger this issue, has passed the tests. However, the original scenario which trigger the issue is the syzkaller test, which might take some time, possibly around a day.

To avoid any confusion, does this include my preferred fix that I sent or your change?

Thanks.

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

I am using my change, which is detailed in the last email, because you send your preferred fix just 10 mins ago.

Also, I do the test with Liyuan's patch as "suffix" patch, as mentioned in the previous email.

Hi, Russell!

The syzkaller test has been running for 19+ hours now, and no issues have been found so far. So to patch3:

This test will continue to run. If any issues are found, I will email you. If I don't send any emails, then there are no issues. :)

Thanks. I've updated the three patches, and pushed out the updated tree. See the top four commits:

https://git.kernel.org/pub/scm/linux/kernel/git/rmk/linux.git/log/?h=fixes

Given the timings, I doubt that linux-next will pick this up before I need to send Linus a pull request before going on vacation for the remainder of this year:

https://git.kernel.org/pub/scm/linux/kernel/git/rmk/linux.git/log/?h=for-linus

However, I also can't guarantee that I'll be able to look at mainline at the start of next year due to other pressures - and I don't yet know for how long that's likely to be at the moment.

So, it's either a pull request for the for-linus tag later today, at some point next year likely not before mid-January and splitting it up into fixes (for -rc) and the two PREEMPT_RT non-fix patches (for the next merge window.)

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

Thanks! These patches seem to have been merged into the linux-next branch.

By the way, before the PREEMPT_RT enabled patch are finally merged, something like this should be applied first:

I believe you won't forget this, but I currently don't see it in any patch queue/list/system. Therefore, out of caution, I still sent this email.

Thanks for your reply.

This is a key point, I think. This means that triggering an kernel address's do_alignment() from user-mode is possible, and it's a normal situation, not even a fault. And I also found that alignment_get_thumb() and alignment_get_arm() might cause a fault, which will make things even more complicated if we still keep harden_branch_predictor() inside __do_user_fault().

In conclusion, this(these patches) should be the best solution; I can't think of a better way.

Yes, I have a general understanding of this. Anyway, thanks for your reply.