diff mbox series

powerpc/64s/interrupt: Fix interrupt exit race with security mitigation switch

Message ID 20230206042240.92103-1-npiggin@gmail.com (mailing list archive)
State Accepted
Commit 2ea31e2e62bbc4d11c411eeb36f1b02841dbcab1
Headers show
Series powerpc/64s/interrupt: Fix interrupt exit race with security mitigation switch | expand

Checks

Context Check Description
snowpatch_ozlabs/github-powerpc_ppctests success Successfully ran 8 jobs.
snowpatch_ozlabs/github-powerpc_selftests success Successfully ran 8 jobs.
snowpatch_ozlabs/github-powerpc_kernel_qemu success Successfully ran 24 jobs.
snowpatch_ozlabs/github-powerpc_sparse success Successfully ran 4 jobs.
snowpatch_ozlabs/github-powerpc_clang success Successfully ran 6 jobs.

Commit Message

Nicholas Piggin Feb. 6, 2023, 4:22 a.m. UTC 
The RFI and STF security mitigation options can flip the
interrupt_exit_not_reentrant static branch condition concurrently with
the interrupt exit code which tests that branch.

Interrupt exit tests this condition to set MSR[EE|RI] for exit, then
again in the case a soft-masked interrupt is found pending, to recover
the MSR so the interrupt can be replayed before attempting to exit
again. If the condition changes between these two tests, the MSR and irq
soft-mask state will become corrupted, leading to warnings and possible
crashes. For example, if the branch is initially true then false,
MSR[EE] will be 0 but PACA_IRQ_HARD_DIS clear and EE may not get
enabled, leading to warnings in irq_64.c.

Reported-by: Sachin Sant <sachinp@linux.ibm.com>
Tested-by: Sachin Sant <sachinp@linux.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
The static_branch condition should not be evaluated multiple times by
the caller when coded like this AFAIKS because if code patching is
disabled then it becomes an atomic_read of the key, and if it is enabled
then it should be done with asm volatile so the compiler should be
unable to expand it again.

Thanks,
Nick

 arch/powerpc/kernel/interrupt.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Michael Ellerman Feb. 15, 2023, 12:16 p.m. UTC | #1 
On Mon, 6 Feb 2023 14:22:40 +1000, Nicholas Piggin wrote:
> The RFI and STF security mitigation options can flip the
> interrupt_exit_not_reentrant static branch condition concurrently with
> the interrupt exit code which tests that branch.
> 
> Interrupt exit tests this condition to set MSR[EE|RI] for exit, then
> again in the case a soft-masked interrupt is found pending, to recover
> the MSR so the interrupt can be replayed before attempting to exit
> again. If the condition changes between these two tests, the MSR and irq
> soft-mask state will become corrupted, leading to warnings and possible
> crashes. For example, if the branch is initially true then false,
> MSR[EE] will be 0 but PACA_IRQ_HARD_DIS clear and EE may not get
> enabled, leading to warnings in irq_64.c.
> 
> [...]

Applied to powerpc/fixes.

[1/1] powerpc/64s/interrupt: Fix interrupt exit race with security mitigation switch
      https://git.kernel.org/powerpc/c/2ea31e2e62bbc4d11c411eeb36f1b02841dbcab1

cheers
diff mbox series

Patch

diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
index fc6631a80527..0ec1581619db 100644
--- a/arch/powerpc/kernel/interrupt.c
+++ b/arch/powerpc/kernel/interrupt.c
@@ -50,16 +50,18 @@  static inline bool exit_must_hard_disable(void)
  */
 static notrace __always_inline bool prep_irq_for_enabled_exit(bool restartable)
 {
+	bool must_hard_disable = (exit_must_hard_disable() || !restartable);
+
 	/* This must be done with RI=1 because tracing may touch vmaps */
 	trace_hardirqs_on();
 
-	if (exit_must_hard_disable() || !restartable)
+	if (must_hard_disable)
 		__hard_EE_RI_disable();
 
 #ifdef CONFIG_PPC64
 	/* This pattern matches prep_irq_for_idle */
 	if (unlikely(lazy_irq_pending_nocheck())) {
-		if (exit_must_hard_disable() || !restartable) {
+		if (must_hard_disable) {
 			local_paca->irq_happened |= PACA_IRQ_HARD_DIS;
 			__hard_RI_enable();
 		}