[PATCH v2 RESEND 1/2] ARM: spectre-v2: Fix potential missing mitigations

[PATCH v2 RESEND 1/2] ARM: spectre-v2: Fix potential missing mitigations

[Xie Yuanbin]

For the latest kernel, with arm's multi_v7_defconfig, and set
CONFIG_PREEMPT=y, CONFIG_DEBUG_PREEMPT=y, CONFIG_ARM_LPAE=y,
if a user program try to accesses any valid kernel address, for example:
```c
static void han(int x)
{
	while (1);
}

int main(void)
{
	signal(SIGSEGV, han);
	/* 0xc0331fd4 is just a kernel address in kernel .text section */
	__asm__ volatile (""::"r"(*(int *)(uintptr_t)0xc0331fd4):"memory");
	while (1);
	return 0;
}
```
, the following WARN will be triggered:

[    1.089103] BUG: using smp_processor_id() in preemptible [00000000] code: init/1
[    1.093367] caller is __do_user_fault+0x20/0x6c
[    1.094355] CPU: 0 UID: 0 PID: 1 Comm: init Not tainted 6.14.3 #7
[    1.094585] Hardware name: Generic DT based system
[    1.094706] Call trace:
[    1.095211]  unwind_backtrace from show_stack+0x10/0x14
[    1.095329]  show_stack from dump_stack_lvl+0x50/0x5c
[    1.095352]  dump_stack_lvl from check_preemption_disabled+0x104/0x108
[    1.095448]  check_preemption_disabled from __do_user_fault+0x20/0x6c
[    1.095459]  __do_user_fault from do_page_fault+0x334/0x3dc
[    1.095505]  do_page_fault from do_DataAbort+0x30/0xa8
[    1.095528]  do_DataAbort from __dabt_usr+0x54/0x60
[    1.095570] Exception stack(0xf0825fb0 to 0xf0825ff8)

This WARN indicates that the current CPU is not stable, which means that
current can be migrated to other CPUs.
Therefore, in some scenarios, mitigation measures may be missed, such as:
1. Thread A attacks on cpu0 and triggers do_page_fault
2. Thread A migrates to cpu1 before bp_hardening
3. Thread A do bp_hardening on cpu1
4. Thread A migrates to cpu0
5. Thread A ret_to_user on cpu0

Assuming that all of the context_stwitch() mentioned above does not
trigger switch_mm(), therefore all of the context_stwitch() does not
trigger mitigation. Thread A successfully bypassed the mitigation on cpu0.

Over the past six years, there have been continuous reports of this bug:
2025.4.24 https://lore.kernel.org/all/20250424100437.27477-1-xieyuanbin1@huawei.com/
2022.6.22 https://lore.kernel.org/all/795c9463-452e-bf64-1cc0-c318ccecb1da@I-love.SAKURA.ne.jp/
2021.3.25 https://lore.kernel.org/all/20210325095049.6948-1-liu.xiang@zlingsmart.com/
2021.3.12 https://lore.kernel.org/all/20210312041246.15113-1-qiang.zhang@windriver.com/
2021.3.11 https://lore.kernel.org/all/0000000000007604cb05bd3e6968@google.com/
2019.5.27 https://lore.kernel.org/all/1558949979-129251-1-git-send-email-gaoyongliang@huawei.com/
2019.3.19 https://lore.kernel.org/all/20190319203239.gl46fxnfz6gzeeic@linutronix.de/

To fix it, we must check whether mitigation are needed before enabling
interrupt(with PREEMPT) or before calling mm_read_lock()(without PREEMPT).

Fixes: f5fe12b1eaee ("ARM: spectre-v2: harden user aborts in kernel space")

Signed-off-by: Xie Yuanbin <xieyuanbin1@huawei.com>
Cc: Russell King (Oracle) <linux@armlinux.org.uk>
---
 arch/arm/mm/fault.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
index 2bc828a1940c..e4dc7c2cfe75 100644
--- a/arch/arm/mm/fault.c
+++ b/arch/arm/mm/fault.c
@@ -265,20 +265,27 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 	struct mm_struct *mm = current->mm;
 	struct vm_area_struct *vma;
 	int sig, code;
 	vm_fault_t fault;
 	unsigned int flags = FAULT_FLAG_DEFAULT;
 	vm_flags_t vm_flags = VM_ACCESS_FLAGS;
 
 	if (kprobe_page_fault(regs, fsr))
 		return 0;
 
+#ifdef CONFIG_HARDEN_BRANCH_PREDICTOR
+	if (unlikely(addr > TASK_SIZE) && user_mode(regs)) {
+		fault = 0;
+		code = SEGV_MAPERR;
+		goto bad_area;
+	}
+#endif
 
 	/* Enable interrupts if they were enabled in the parent context. */
 	if (interrupts_enabled(regs))
 		local_irq_enable();
 
 	/*
 	 * If we're in an interrupt or have no user
 	 * context, we must not take the fault..
 	 */
 	if (faulthandler_disabled() || !mm)
-- 
2.48.1

[PATCH v2 RESEND 2/2] ARM: mm: Optimize page_fault to reduce the impact of spectre-v2 bugfix

[Xie Yuanbin]

Submitting this optimized performance commit is because of
last spectre-v2 bugfix, which adds a branch to the hot code path
and may cause performance degradation.

It mainly does the following things:
1. Extract the judgment of user_mode and reduce some redundant branch.
For example, for user mode, interrupts must be enabled, and
`(faulthandler_disabled() || !mm)` is impossible. I think some other
branches can also be optimized, such as `(fsr & FSR_LNX_PF)` and
`ttbr0_usermode_access_allowed()`, but I'm not sure. Among them,
`ttbr0_usermode_access_allowed()` is after `perf_sw_event()`, I think
these two are not dependent. If `ttbr0_usermode_access_allowed()` can be
placed front, it can also be optimized into the kernel mode branch.

2. Add some like/unlikely.

3. `__do_user_fault` is cold code path, inlining may lead to negative
optimization, so add noinline.
I also want to do this for `__do_kernel_fault`, but it seems that
`fixup_exception()` in `__do_kernel_fault` is not cold code, which may be
triggered by `copy_from_user_nofault`.

Signed-off-by: Xie Yuanbin <xieyuanbin1@huawei.com>
---
 arch/arm/mm/fault.c | 54 +++++++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 26 deletions(-)

diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
index e4dc7c2cfe75..09dde89a88ed 100644
--- a/arch/arm/mm/fault.c
+++ b/arch/arm/mm/fault.c
@@ -147,21 +147,21 @@ static void die_kernel_fault(const char *msg, struct mm_struct *mm,
  * Oops.  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 fsr,
 		  struct pt_regs *regs)
 {
 	const char *msg;
 	/*
 	 * Are we prepared to handle this kernel fault?
 	 */
-	if (fixup_exception(regs))
+	if (likely(fixup_exception(regs)))
 		return;
 
 	/*
 	 * No handler, we'll have to terminate things with extreme prejudice.
 	 */
 	if (addr < PAGE_SIZE) {
 		msg = "NULL pointer dereference";
 	} else {
 		if (is_translation_fault(fsr) &&
 		    kfence_handle_page_fault(addr, is_write_fault(fsr), regs))
@@ -170,21 +170,21 @@ __do_kernel_fault(struct mm_struct *mm, unsigned long addr, unsigned int fsr,
 		msg = "paging request";
 	}
 
 	die_kernel_fault(msg, mm, addr, fsr, regs);
 }
 
 /*
  * Something tried to access memory that isn't in our memory map..
  * User mode accesses just cause a SIGSEGV
  */
-static void
+static noinline void
 __do_user_fault(unsigned long addr, unsigned int fsr, unsigned int sig,
 		int code, struct pt_regs *regs)
 {
 	struct task_struct *tsk = current;
 
 	if (addr > TASK_SIZE)
 		harden_branch_predictor();
 
 #ifdef CONFIG_DEBUG_USER
 	if (((user_debug & UDBG_SEGV) && (sig == SIGSEGV)) ||
@@ -265,135 +265,137 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 	struct mm_struct *mm = current->mm;
 	struct vm_area_struct *vma;
 	int sig, code;
 	vm_fault_t fault;
 	unsigned int flags = FAULT_FLAG_DEFAULT;
 	vm_flags_t vm_flags = VM_ACCESS_FLAGS;
 
 	if (kprobe_page_fault(regs, fsr))
 		return 0;
 
-#ifdef CONFIG_HARDEN_BRANCH_PREDICTOR
-	if (unlikely(addr > TASK_SIZE) && user_mode(regs)) {
-		fault = 0;
-		code = SEGV_MAPERR;
-		goto bad_area;
-	}
-#endif
+	if (user_mode(regs)) {
+		if (IS_ENABLED(CONFIG_HARDEN_BRANCH_PREDICTOR) && unlikely(addr > TASK_SIZE)) {
+			fault = 0;
+			code = SEGV_MAPERR;
+			goto bad_area;
+		}
 
-	/* Enable interrupts if they were enabled in the parent context. */
-	if (interrupts_enabled(regs))
+		/* Enable interrupts if they were enabled in the parent context. */
 		local_irq_enable();
 
-	/*
-	 * If we're in an interrupt or have no user
-	 * context, we must not take the fault..
-	 */
-	if (faulthandler_disabled() || !mm)
-		goto no_context;
-
-	if (user_mode(regs))
 		flags |= FAULT_FLAG_USER;
+	} else {
+		/* Enable interrupts if they were enabled in the parent context. */
+		if (interrupts_enabled(regs))
+			local_irq_enable();
+
+		/*
+		 * If we're in an interrupt or have no user
+		 * context, we must not take the fault..
+		 */
+		if (faulthandler_disabled() || unlikely(!mm))
+			goto no_context;
+	}
 
 	if (is_write_fault(fsr)) {
 		flags |= FAULT_FLAG_WRITE;
 		vm_flags = VM_WRITE;
 	}
 
 	if (fsr & FSR_LNX_PF) {
 		vm_flags = VM_EXEC;
 
-		if (is_permission_fault(fsr) && !user_mode(regs))
+		if (unlikely(is_permission_fault(fsr)) && !user_mode(regs))
 			die_kernel_fault("execution of memory",
 					 mm, addr, fsr, regs);
 	}
 
 	perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, addr);
 
 	/*
 	 * Privileged access aborts with CONFIG_CPU_TTBR0_PAN enabled are
 	 * routed via the translation fault mechanism. Check whether uaccess
 	 * is disabled while in kernel mode.
 	 */
-	if (!ttbr0_usermode_access_allowed(regs))
+	if (unlikely(!ttbr0_usermode_access_allowed(regs)))
 		goto no_context;
 
 	if (!(flags & FAULT_FLAG_USER))
 		goto lock_mmap;
 
 	vma = lock_vma_under_rcu(mm, addr);
 	if (!vma)
 		goto lock_mmap;
 
-	if (!(vma->vm_flags & vm_flags)) {
+	if (unlikely(!(vma->vm_flags & vm_flags))) {
 		vma_end_read(vma);
 		count_vm_vma_lock_event(VMA_LOCK_SUCCESS);
 		fault = 0;
 		code = SEGV_ACCERR;
 		goto bad_area;
 	}
 	fault = handle_mm_fault(vma, addr, flags | FAULT_FLAG_VMA_LOCK, regs);
 	if (!(fault & (VM_FAULT_RETRY | VM_FAULT_COMPLETED)))
 		vma_end_read(vma);
 
 	if (!(fault & VM_FAULT_RETRY)) {
 		count_vm_vma_lock_event(VMA_LOCK_SUCCESS);
 		goto done;
 	}
 	count_vm_vma_lock_event(VMA_LOCK_RETRY);
 	if (fault & VM_FAULT_MAJOR)
 		flags |= FAULT_FLAG_TRIED;
 
 	/* Quick path to respond to signals */
-	if (fault_signal_pending(fault, regs)) {
+	if (unlikely(fault_signal_pending(fault, regs))) {
 		if (!user_mode(regs))
 			goto no_context;
 		return 0;
 	}
 lock_mmap:
 
 retry:
 	vma = lock_mm_and_find_vma(mm, addr, regs);
 	if (unlikely(!vma)) {
 		fault = 0;
 		code = SEGV_MAPERR;
 		goto bad_area;
 	}
 
 	/*
 	 * ok, we have a good vm_area for this memory access, check the
 	 * permissions on the VMA allow for the fault which occurred.
 	 */
-	if (!(vma->vm_flags & vm_flags)) {
+	if (unlikely(!(vma->vm_flags & vm_flags))) {
 		mmap_read_unlock(mm);
 		fault = 0;
 		code = SEGV_ACCERR;
 		goto bad_area;
 	}
 
 	fault = handle_mm_fault(vma, addr & PAGE_MASK, flags, regs);
 
 	/* If we need to retry but a fatal signal is pending, handle the
 	 * signal first. We do not need to release the mmap_lock because
 	 * it would already be released in __lock_page_or_retry in
 	 * mm/filemap.c. */
-	if (fault_signal_pending(fault, regs)) {
+	if (unlikely(fault_signal_pending(fault, regs))) {
 		if (!user_mode(regs))
 			goto no_context;
 		return 0;
 	}
 
 	/* The fault is fully completed (including releasing mmap lock) */
 	if (fault & VM_FAULT_COMPLETED)
 		return 0;
 
-	if (!(fault & VM_FAULT_ERROR)) {
+	if (likely(!(fault & VM_FAULT_ERROR))) {
 		if (fault & VM_FAULT_RETRY) {
 			flags |= FAULT_FLAG_TRIED;
 			goto retry;
 		}
 	}
 
 	mmap_read_unlock(mm);
 done:
 
 	/* Handle the "normal" case first */
-- 
2.48.1

Re: [PATCH v2 RESEND 1/2] ARM: spectre-v2: Fix potential missing mitigations

[Sebastian Andrzej Siewior]

On 2025-10-16 20:16:21 [+0800], Xie Yuanbin wrote:
> Over the past six years, there have been continuous reports of this bug:
…
> 2019.3.19 https://lore.kernel.org/all/20190319203239.gl46fxnfz6gzeeic@linutronix.de/
> 
> To fix it, we must check whether mitigation are needed before enabling
> interrupt(with PREEMPT) or before calling mm_read_lock()(without PREEMPT).
> 
> Fixes: f5fe12b1eaee ("ARM: spectre-v2: harden user aborts in kernel space")

Hmm.
I was moving things back in 2019 but things shifted and this is no
longer required. If I apply both patches (of yours) then it sends a
signal with disabled interrupts which breaks my PREEMPT_RT case.

The requirement is to invoke the mitigation callback of the right CPU.
What about disabling preemption before getting the callback and doing
the invocation?

Sebastian

Re: [PATCH v2 RESEND 1/2] ARM: spectre-v2: Fix potential missing mitigations

[Sebastian Andrzej Siewior]

On 2025-10-28 17:20:06 [+0100], To Xie Yuanbin wrote:
> The requirement is to invoke the mitigation callback of the right CPU.
> What about disabling preemption before getting the callback and doing
> the invocation?

Something like this:

diff --git a/arch/arm/include/asm/system_misc.h b/arch/arm/include/asm/system_misc.h
--- a/arch/arm/include/asm/system_misc.h
+++ b/arch/arm/include/asm/system_misc.h
@@ -20,8 +20,10 @@ typedef void (*harden_branch_predictor_fn_t)(void);
 DECLARE_PER_CPU(harden_branch_predictor_fn_t, harden_branch_predictor_fn);
 static inline void harden_branch_predictor(void)
 {
-	harden_branch_predictor_fn_t fn = per_cpu(harden_branch_predictor_fn,
-						  smp_processor_id());
+	harden_branch_predictor_fn_t fn;
+
+	guard(preempt)();
+	fn = per_cpu(harden_branch_predictor_fn, smp_processor_id());
 	if (fn)
 		fn();
 }

Sebastian

Re: [PATCH v2 RESEND 1/2] ARM: spectre-v2: Fix potential missing mitigations

[Sebastian Andrzej Siewior]

On 2025-10-28 17:20:06 [+0100], To Xie Yuanbin wrote:
> On 2025-10-16 20:16:21 [+0800], Xie Yuanbin wrote:
> > Over the past six years, there have been continuous reports of this bug:
> …
> > 2019.3.19 https://lore.kernel.org/all/20190319203239.gl46fxnfz6gzeeic@linutronix.de/
> > 
> > To fix it, we must check whether mitigation are needed before enabling
> > interrupt(with PREEMPT) or before calling mm_read_lock()(without PREEMPT).
> > 
> > Fixes: f5fe12b1eaee ("ARM: spectre-v2: harden user aborts in kernel space")
> 
> Hmm.
> I was moving things back in 2019 but things shifted and this is no
> longer required. If I apply both patches (of yours) then it sends a
> signal with disabled interrupts which breaks my PREEMPT_RT case.

Now I got my things together.
LPAE enables interrupts early in do_page_fault(), therefore accessing a
kernel address from userland triggers the warning in
harden_branch_predictor() before sending the signal.

!LPAE does do_bad_area() -> __do_user_fault() and does not trigger the
warning in harden_branch_predictor() because the interrupts are off. 
On PREEMPT_RT this leads to an error due to accessing spinlock_t from
force_sig_fault() with disabled interrupts. Therefore I did enable
interrupts early and would need end up with the same warning as in the
LPAE case.

Now Russell wants to keep interrupts/ preemption disabled for the
address > TASK_SIZE for the entire page fault path to so that
harden_branch_predictor() works properly.

If we need that, then it won't work with the preempt-disable suggestion
I had… We don't send SIGKILL because userland might want emulate paging
for the kernel regions. Okay.

I guess the requirement is to invoke harden_branch_predictor() on the
same CPU that triggered the page_fault, right? Couldn't we then move
harden_branch_predictor() a little bit earlier, invoke it in the >=
TASK_SIZE case and then enable interrupts if they were enabled?

That would make me happy ;)

Sebastian

Re: [PATCH v2 RESEND 1/2] ARM: spectre-v2: Fix potential missing mitigations

[Xie Yuanbin]

On Tue, 28 Oct 2025 17:20:05 +0100, Sebastian Andrzej Siewior wrote:
> If I apply both patches (of yours) then it sends a
> signal with disabled interrupts which breaks my PREEMPT_RT case.

I am not familiar with PREEMPT_RT yet and do not know that signals cannot
be sent with disabled interrupts and PREEMPT_RT=y.
I apologize for this.

On Tue, 28 Oct 2025 19:20:52 +0100, Sebastian Andrzej Siewior wrote:
> !LPAE does do_bad_area() -> __do_user_fault() and does not trigger the
> warning in harden_branch_predictor() because the interrupts are off.
> On PREEMPT_RT this leads to an error due to accessing spinlock_t from
> force_sig_fault() with disabled interrupts.

This seems to be a more serious bug, and may require another patch to
fix it. Not only !LPAE is affected, but LAPE=y is also affected:
do_translation_fault() -> do_bad_area() -> __do_user_fault()
This code path seems very easy to trigger.

> I guess the requirement is to invoke harden_branch_predictor() on the
> same CPU that triggered the page_fault, right? Couldn't we then move
> harden_branch_predictor() a little bit earlier, invoke it in the >=
> TASK_SIZE case and then enable interrupts if they were enabled?
>
> That would make me happy ;)

This seems to only fix the warning in harden_branch_predictor, but cannot
fix the issue of sending signals with disabled interrupts mentioned above.

What about adding:

diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
index 09dde89a88ed..b9c9c80db109 100644
--- a/arch/arm/mm/fault.c
+++ b/arch/arm/mm/fault.c
@@ -182,6 +182,12 @@ __do_user_fault(unsigned long addr, unsigned int fsr, unsigned int sig,
 		int code, struct pt_regs *regs)
 {
 	struct task_struct *tsk = current;
+	const bool save_irqs_disabled = irqs_disabled();
+
+	if (save_irqs_disabled) {
+		preempt_disable();
+		local_irq_enable();
+	}

 	if (addr > TASK_SIZE)
 		harden_branch_predictor();
@@ -207,6 +213,11 @@ __do_user_fault(unsigned long addr, unsigned int fsr, unsigned int sig,
 	tsk->thread.error_code = fsr;
 	tsk->thread.trap_no = 14;
 	force_sig_fault(sig, code, (void __user *)addr);
+
+	if (save_irqs_disabled) {
+		local_irq_disable();
+		preempt_enable_no_resched();
+	}
 }

and the modification of patch 1 is still retained.

Xie Yuanbin

Re: [PATCH v2 RESEND 1/2] ARM: spectre-v2: Fix potential missing mitigations

[Sebastian Andrzej Siewior]

On 2025-10-29 10:41:51 [+0800], Xie Yuanbin wrote:
> On Tue, 28 Oct 2025 17:20:05 +0100, Sebastian Andrzej Siewior wrote:
> > If I apply both patches (of yours) then it sends a
> > signal with disabled interrupts which breaks my PREEMPT_RT case.
> 
> I am not familiar with PREEMPT_RT yet and do not know that signals cannot
> be sent with disabled interrupts and PREEMPT_RT=y.
> I apologize for this.

no worries.

> On Tue, 28 Oct 2025 19:20:52 +0100, Sebastian Andrzej Siewior wrote:
> > !LPAE does do_bad_area() -> __do_user_fault() and does not trigger the
> > warning in harden_branch_predictor() because the interrupts are off.
> > On PREEMPT_RT this leads to an error due to accessing spinlock_t from
> > force_sig_fault() with disabled interrupts.
> 
> This seems to be a more serious bug, and may require another patch to
> fix it. Not only !LPAE is affected, but LAPE=y is also affected:
> do_translation_fault() -> do_bad_area() -> __do_user_fault()
> This code path seems very easy to trigger.

correct.

> > I guess the requirement is to invoke harden_branch_predictor() on the
> > same CPU that triggered the page_fault, right? Couldn't we then move
> > harden_branch_predictor() a little bit earlier, invoke it in the >=
> > TASK_SIZE case and then enable interrupts if they were enabled?
> >
> > That would make me happy ;)
> 
> This seems to only fix the warning in harden_branch_predictor, but cannot
> fix the issue of sending signals with disabled interrupts mentioned above.
> 
> What about adding:

I was planning to just move it up. Let me try to form something in a
bit.

> Xie Yuanbin

Sebastian