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: 2489 bytes --]

On Thu, Feb 29, 2024 at 2:04 PM Ray Ni <ray.ni@intel.com> wrote:
> @@ -134,9 +262,9 @@ CoreRestoreTpl (
>    }
>
>    //
> -  // Set the new value
> +  // Set the new TPL with interrupt disabled.
>    //
> -
> +  CoreSetInterruptState (FALSE);
>    gEfiCurrentTpl = NewTpl;
>
>    //
> @@ -144,7 +272,22 @@ CoreRestoreTpl (
>    // interrupts are enabled
>    //
>    if (gEfiCurrentTpl < TPL_HIGH_LEVEL) {
> -    CoreSetInterruptState (TRUE);
> +    if ((INTN)gEfiCurrentTpl > HighBitSet64 (mInterruptedTplMask)) {
> +      //
> +      // Only enable interrupts if restoring to a level above the highest
> +      // interrupted TPL level.  This allows interrupt nesting, but only for
> +      // events at higher TPL level than the current TPL level.
> +      //
> +      CoreSetInterruptState (TRUE);
> +    } else {
> +      //
> +      // Clear interrupted TPL level mask, but do not re-enable interrupts here
> +      // This will return to CoreTimerTick() and interrupts will be re-enabled
> +      // when the timer interrupt handlers returns from interrupt context.
> +      //
> +      ASSERT ((INTN)gEfiCurrentTpl == HighBitSet64 (mInterruptedTplMask));
> +      mInterruptedTplMask &= ~(UINTN)(1 << gEfiCurrentTpl);
> +    }
>    }

Ok, now I understand what's going on and it's indeed the same logic as
NestedInterruptTplLib, with DisableInterruptsOnIret() replaced by
skipping CoreSetInterruptState(TRUE). It's similar to what I proposed
elsewhere in the thread, just written differenty.

I agree with Michael Brown that the spec is unclear on the state of
the interrupt flag on exit from gBS->RestoreTPL(), but perhaps this
change is feasible if the interrupt handlers just raise the TPL first
and restore it last.

Just as an exercise for me to understand the code better, I tried
rewriting the code in terms of the CoreRestoreTplInternal() function
that I proposed. I find it easier to read, but I guess that's a bit in
the eye of the beholder, and it is a little more defensively coded. I
attach it (untested beyond compilation) for reference.

Paolo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116192): https://edk2.groups.io/g/devel/message/116192
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: 7391 bytes --]

From 23c4f60cf79f29ab5eff55a02c72bb504804d02a Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Thu, 29 Feb 2024 23:54:50 +0100
Subject: [PATCH 1/2] MdeModulePkg: introduce CoreRestoreTplInternal()
Content-Type: text/plain; charset=UTF-8

Introduce a function that restores the TPL just like gBS->RestoreTPL(),
but can optionally return with interrupts disabled.  This can be used
from interrupt handlers, so that any nested interrupt handler will only
see an elevated TPL and not the value on entry to the interrupt handler.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 MdeModulePkg/Core/Dxe/Event/Tpl.c | 46 +++++++++++++++++++++----------
 1 file changed, 31 insertions(+), 15 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Event/Tpl.c b/MdeModulePkg/Core/Dxe/Event/Tpl.c
index b33f80573c..fe95ea3896 100644
--- a/MdeModulePkg/Core/Dxe/Event/Tpl.c
+++ b/MdeModulePkg/Core/Dxe/Event/Tpl.c
@@ -90,10 +90,11 @@ CoreRaiseTpl (
   @param  NewTpl  New, lower, task priority
 
 **/
-VOID
+static VOID
 EFIAPI
-CoreRestoreTpl (
-  IN EFI_TPL  NewTpl
+CoreRestoreTplInternal (
+  IN EFI_TPL  NewTpl,
+  IN BOOLEAN  DesiredInterruptState
   )
 {
   EFI_TPL  OldTpl;
@@ -107,11 +108,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 +122,11 @@ CoreRestoreTpl (
     }
 
     gEfiCurrentTpl = PendingTpl;
+
+    //
+    // If lowering below HIGH_LEVEL, make sure
+    // interrupts are enabled
+    //
     if (gEfiCurrentTpl < TPL_HIGH_LEVEL) {
       CoreSetInterruptState (TRUE);
     }
@@ -134,16 +135,31 @@ CoreRestoreTpl (
   }
 
   //
-  // Set the new value
+  // Set the new TPL with interrupt disabled.  If DesiredInterruptState
+  // is FALSE, this ensures that any nested interrupt handler will only
+  // see an elevated TPL and not NewTpl.
   //
-
+  CoreSetInterruptState (FALSE);
   gEfiCurrentTpl = NewTpl;
 
-  //
-  // If lowering below HIGH_LEVEL, make sure
-  // interrupts are enabled
-  //
-  if (gEfiCurrentTpl < TPL_HIGH_LEVEL) {
+  if (DesiredInterruptState) {
+    ASSERT(gEfiCurrentTpl < TPL_HIGH_LEVEL);
     CoreSetInterruptState (TRUE);
   }
 }
+
+/**
+  Lowers the task priority to the previous value.   If the new
+  priority unmasks events at a higher priority, they are dispatched.
+
+  @param  NewTpl  New, lower, task priority
+
+**/
+VOID
+EFIAPI
+CoreRestoreTpl (
+  IN EFI_TPL NewTpl
+  )
+{
+  CoreRestoreTplInternal (NewTpl, NewTpl < TPL_HIGH_LEVEL);
+}
-- 
2.43.2


From 97b07f53f9a0711eb55e02e8af591eee09971668 Mon Sep 17 00:00:00 2001
From: Michael D Kinney <michael.d.kinney@intel.com>
Date: Fri, 1 Mar 2024 00:40:53 +0100
Subject: [PATCH 2/2] MdeModulePkg: fix stack overflow issue due to nested
 interrupts
Content-Type: text/plain; charset=UTF-8

This is a heavily simplified version of the logic in the OvmfPkg
NestedInterruptTplLib.  Because the new CoreRestoreTplInternal()
function allows to lower the TPL while interrupts are disabled,
there is no need for 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; and when restoring the outer TPL at the end of the handler,
interrupts remain disabled until IRET and there cannot be nested
invocation of the interrupt handler at the same TPL.

Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
Signed-off-by: Ray Ni <ray.ni@intel.com>
[Rewritten the CoreRestoreTpl part. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 MdeModulePkg/Core/Dxe/Event/Tpl.c | 73 ++++++++++++++++++++++++++++++-
 1 file changed, 72 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/Dxe/Event/Tpl.c b/MdeModulePkg/Core/Dxe/Event/Tpl.c
index fe95ea3896..715b0626b8 100644
--- a/MdeModulePkg/Core/Dxe/Event/Tpl.c
+++ b/MdeModulePkg/Core/Dxe/Event/Tpl.c
@@ -9,6 +9,11 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include "DxeMain.h"
 #include "Event.h"
 
+///
+/// Bit mask of TPLs that were interrupted during RestoreTPL().
+///
+static UINTN   mInterruptedTplMask = 0;
+
 /**
   Set Interrupt State.
 
@@ -59,6 +64,7 @@ CoreRaiseTpl (
   )
 {
   EFI_TPL  OldTpl;
+  BOOLEAN  InterruptState;
 
   OldTpl = gEfiCurrentTpl;
   if (OldTpl > NewTpl) {
@@ -72,7 +78,31 @@ CoreRaiseTpl (
   // If raising to high level, disable interrupts
   //
   if ((NewTpl >= TPL_HIGH_LEVEL) &&  (OldTpl < TPL_HIGH_LEVEL)) {
-    CoreSetInterruptState (FALSE);
+    //
+    // When gCpu is NULL, InterruptState is TRUE.
+    // 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.
+      // Disable them for going to 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 unwinds all handlers up to the requested TPL.
+      //
+      ASSERT ((INTN)gEfiCurrentTpl > HighBitSet64 (mInterruptedTplMask));
+      mInterruptedTplMask |= (UINTN)(1 << gEfiCurrentTpl);
+    }
   }
 
   //
@@ -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)) {
+    UINTN InterruptedTpl = HighBitSet64 (mInterruptedTplMask);
+    mInterruptedTplMask &= ~(UINTN)(1 << InterruptedTpl);
+
+    ASSERT (GetInterruptState () == FALSE);
+    InInterruptHandler = TRUE;
+
+    //
+    // Take the TPL down a notch to allow event notifications to be
+    // dispatched.  This will implicitly re-enable interrupts (if
+    // InterruptedTPL is below TPL_HIGH_LEVEL), even though we are
+    // still inside the interrupt handler, but the new TPL will
+    // be set while they are disabled.
+    //
+    // DesiredInterruptState must be FALSE to ensure that the
+    // stack does not blow up.  If we used, as in the final call
+    // below, "InterruptedTpl < TPL_HIGH_LEVEL", the timer interrupt
+    // handler could be invoked repeatedly in the small window between
+    // CoreSetInterruptState (TRUE) and the IRET instruction.
+    //
+    CoreRestoreTplInternal (InterruptedTpl, FALSE);
+
+    if (InterruptedTpl == NewTpl) {
+      break;
+    }
+  }
+
+  //
+  // If we get here with InInterruptHandler == TRUE, an interrupt
+  // handler forgot to restore the TPL.
+  //
+  ASSERT (!InInterruptHandler);
   CoreRestoreTplInternal (NewTpl, NewTpl < TPL_HIGH_LEVEL);
 }
-- 
2.43.2