Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore: Fix stack overflow issue due to nested interrupts

["Paolo Bonzini" <pbonzini@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3141 bytes --]

On Fri, Mar 1, 2024 at 4:08 AM Ni, Ray <ray.ni@intel.com> wrote:
> @@ -161,5 +191,46 @@ CoreRestoreTpl (
>    IN EFI_TPL NewTpl
>    )
>  {
> +  BOOLEAN InInterruptHandler = FALSE;
> +
> +  //
> +  // Unwind the nested interrupt handlers up to the required
> +  // TPL, paying attention not to overflow the stack.  While
> +  // not strictly necessary according to the specification,
> +  // accept the possibility that multiple RaiseTPL calls are
> +  // undone by a single RestoreTPL
> +  //
> +  while ((INTN)NewTpl <= HighBitSet64 (mInterruptedTplMask)) {
> 1. why "<="? I thought when RestoreTPL() is called there are only two cases:
>    a. NewTpl == HighBitSet64 (...)
>    b. NewTpl > HighBitSet64 (...)
>   1.a is the case when TimerInterruptHandler() or CoreTimerTick() restores
>   TPL from HIGH to non-HIGH.
>   1.b is the case when the pending event backs call RaiseTPL/RestoreTPL().
>   Because only pending events whose TPL > "Interrupted TPL" can run, the
>   RestoreTPL() call from the event callbacks cannot change the TPL to a value
>   less than or equal to "Interrupted TPL".
>   So, I think "<=" can be "==".
>
> 2. can you explain a bit more about the reason of "while"?

Both are just for extra safety. The required invariant is that all
bits at or below current TPL are cleared, and using "while (... <=
...)" makes it more robust to incorrect usage of gBS->RestoreTPL().

Indeed, the patch at the top of thread also uses "(INTN)gEfiCurrentTpl
> HighBitSet64 (mInterruptedTplMask)", which is <= when you reverse
the condition.  It then asserts inside the conditional that "==" would
be enough.

So I am starting to see more and more similarities between the two
approaches.  I went a step further with fresh mind, removing the while
loop... and basically reinvented your and Michael's patch. :) The only
difference in the logic is a slightly different handling of
mInterruptedTplMask in CoreRestoreTpl(), which is a bit safer in my
case.

However, my roundabout way of getting to the same patch resulted in
very different comments. Personally, I found the large text at the
head of mInterruptedTplMask a bit too much, and the ones inside the
function too focused on "how" and not "why". Maybe it's my exposure to
NestedInterruptTplLib, but I find that a much smaller text can achieve
the same purpose, by explaining the logic instead of the individual
steps.

My version is attached, feel free to reuse it (either entirely or
partially) for a hypothetical v2. Apologies to you and Mike K for the
confusion!

> +
> +    if (InterruptedTpl == NewTpl) {
> +      break;
> 3. "break" or "return"? I think we should exit from this function.

Indeed, this should have been a return.

Paolo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116224): https://edk2.groups.io/g/devel/message/116224
Mute This Topic: https://groups.io/mt/104642317/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: tpl.patch --]
[-- Type: text/x-patch, Size: 6111 bytes --]

From b9f0bc3ef83b40c29e093acda1d0741b8f5610e5 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
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 <michael.d.kinney@intel.com>
Signed-off-by: Ray Ni <ray.ni@intel.com>
[Rewrote the CoreRestoreTpl part and the commit message. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 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