From b9f0bc3ef83b40c29e093acda1d0741b8f5610e5 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 1 Mar 2024 09:11:48 +0100 Subject: [PATCH] MdeModulePkg: fix stack overflow issue due to nested interrupts Content-Type: text/plain; charset=UTF-8 This is a heavily simplified version of the fix in the OvmfPkg NestedInterruptTplLib. Putting it in DXE core allows CoreRestoreTpl() to lower the TPL while keeping interrupts disabled, removing the need for either DisableInterruptsOnIret() or the complex deferred execution mechanism. Instead, CoreRaiseTpl() uses the current state of the interrupt flag to second guess whether it's being called from an interrupt handler; when restoring the outer TPL at the end of the handler, interrupts remain disabled until IRET. This eliminates the possibility that a nested invocation of the interrupt handler has the same TPL as the outer one. Signed-off-by: Michael D Kinney Signed-off-by: Ray Ni [Rewrote the CoreRestoreTpl part and the commit message. - Paolo] Signed-off-by: Paolo Bonzini --- MdeModulePkg/Core/Dxe/Event/Tpl.c | 85 ++++++++++++++++++++++++++----- 1 file changed, 72 insertions(+), 13 deletions(-) diff --git a/MdeModulePkg/Core/Dxe/Event/Tpl.c b/MdeModulePkg/Core/Dxe/Event/Tpl.c index b33f80573c..0a4f99521c 100644 --- a/MdeModulePkg/Core/Dxe/Event/Tpl.c +++ b/MdeModulePkg/Core/Dxe/Event/Tpl.c @@ -9,6 +9,16 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #include "DxeMain.h" #include "Event.h" +/// +/// Bit mask of TPLs that were interrupted (typically during RestoreTPL's +/// event dispatching, though there are reports that the Windows boot loader +/// executes stray STIs at TPL_HIGH_LEVEL). CoreRaiseTpl() sets the +/// OldTpl-th bit when it detects it was called from and interrupt handler, +/// because the corresponding CoreRestoreTpl() needs different semantics for +/// the CPU interrupt state. See CoreRaiseTpl() and CoreRestoreTpl() below. +/// +static UINTN mInterruptedTplMask = 0; + /** Set Interrupt State. @@ -59,6 +69,7 @@ CoreRaiseTpl ( ) { EFI_TPL OldTpl; + BOOLEAN InterruptState; OldTpl = gEfiCurrentTpl; if (OldTpl > NewTpl) { @@ -72,7 +83,31 @@ CoreRaiseTpl ( // If raising to high level, disable interrupts // if ((NewTpl >= TPL_HIGH_LEVEL) && (OldTpl < TPL_HIGH_LEVEL)) { - CoreSetInterruptState (FALSE); + // + // When gCpu is NULL, assume we're not called from an interrupt handler. + // Calling CoreSetInterruptState() with TRUE is safe as CoreSetInterruptState() will directly return + // when gCpu is NULL. + // + InterruptState = TRUE; + if (gCpu != NULL) { + gCpu->GetInterruptState (gCpu, &InterruptState); + } + + if (InterruptState) { + // + // Interrupts are currently enabled. + // Keep them disabled while at TPL_HIGH_LEVEL. + // + CoreSetInterruptState (FALSE); + } else { + // + // Within an interrupt handler. Save the TPL that was interrupted; + // It must be higher than the previously interrupted TPL, since + // CoreRestoreTpl reset all bits up to and including the requested TPL. + // + ASSERT ((INTN)OldTpl > HighBitSet64 (mInterruptedTplMask)); + mInterruptedTplMask |= (UINTN)(1 << OldTpl); + } } // @@ -107,11 +142,6 @@ CoreRestoreTpl ( ASSERT (VALID_TPL (NewTpl)); - // - // If lowering below HIGH_LEVEL, make sure - // interrupts are enabled - // - if ((OldTpl >= TPL_HIGH_LEVEL) && (NewTpl < TPL_HIGH_LEVEL)) { gEfiCurrentTpl = TPL_HIGH_LEVEL; } @@ -126,6 +156,13 @@ CoreRestoreTpl ( } gEfiCurrentTpl = PendingTpl; + + // + // If lowering below TPL_HIGH_LEVEL, make sure interrupts are + // enabled to avoid priority inversions. Note however that + // the TPL remains higher than the caller's. This limits the + // number of nested interrupts that can happen. + // if (gEfiCurrentTpl < TPL_HIGH_LEVEL) { CoreSetInterruptState (TRUE); } @@ -134,16 +171,43 @@ CoreRestoreTpl ( } // - // Set the new value + // The CPU disables interrupts while handlers run, therefore the + // interrupt handler wants to set TPL_HIGH_LEVEL while it runs, + // for consistency. However, when the handler calls RestoreTPL + // before returning, we want to keep interrupts disabled. This + // restores the exact state at the beginning of the handler, + // before the call to RaiseTPL(): low TPL and interrupts disabled. // - + // Disabling interrupts below TPL_HIGH_LEVEL is temporarily + // inconsistent but, if we did not do so, another interrupt + // could trigger in the small window between + // CoreSetInterruptState (TRUE) and the IRET instruction. + // The nested interrupt would start with the same TPL as the + // outer one, and nothing would prevents infinite recursion and + // a stack overflow. + // + // Instead, disable interrupts so that nested interrupt handlers + // will only fire before gEfiCurrentTpl gets its final value. + // This ensures that nested handlers see a TPL higher than + // the outer handler, thus bounding the overall stack depth. + // + CoreSetInterruptState (FALSE); gEfiCurrentTpl = NewTpl; - // - // If lowering below HIGH_LEVEL, make sure - // interrupts are enabled - // - if (gEfiCurrentTpl < TPL_HIGH_LEVEL) { + if ((INTN)NewTpl <= HighBitSet64 (mInterruptedTplMask)) { + // + // We were called from an interrupt handler. Return with + // interrupts disabled to ensure that the stack does + // not blow up. + // + ASSERT (mInterruptedTplMask & (1 << NewTpl)); + ASSERT (GetInterruptState () == FALSE); + mInterruptedTplMask &= (UINTN)(1 << NewTpl) - 1; + } else if (NewTpl < TPL_HIGH_LEVEL) { + // + // Lowering below TPL_HIGH_LEVEL, make sure + // interrupts are enabled + // CoreSetInterruptState (TRUE); } } -- 2.43.2