Fix stack overflow issue due to nested interrupts #11276

xuweiintel · 2025-07-10T08:25:04Z

Description

<Include a description of the change and why this change was made.>

<For each item, place an "x" in between [ and ] if true. Example: [x] (you can also check items in GitHub UI)>

<Create the PR as a Draft PR if it is only created to run CI checks.>

<Delete lines in <> tags before creating the PR.>

Breaking change?
- Breaking change - Does this PR cause a break in build or boot behavior?
- Examples: Does it add a new library class or move a module to a different repo.
Impacts security?
- Security - Does this PR have a direct security impact?
- Examples: Crypto algorithm change or buffer overflow fix.
Includes tests?
- Tests - Does this PR include any explicit test code?
- Examples: Unit tests or integration tests.

How This Was Tested

<Describe the test(s) that were run to verify the changes.>

Integration Instructions

<Describe how these changes should be integrated. Use N/A if nothing is required.>

CpuDxe driver uses a global C variable to record the interrupt state. The state variable is updated every time CpuArch.EnableInterrupt() or CpuArch.DisableInterrupt() is called. CpuArch.GetInterruptState() simply returns the state variable. But when CpuArch.GetInterruptState() is called in the interrupt context, even the interrupt state is enabled before interrupt happens, because the interrupt is not disabled through CpuArch.DisableInterrupts(), CpuArch.GetInterruptState() still returns that the interrupt state is enabled. It's not correct. The commit removes the C global variable and always reads the interrupt state from CPU register. Signed-off-by: Ray Ni <[email protected]>

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 <[email protected]> Signed-off-by: Ray Ni <[email protected]> Signed-off-by: Paolo Bonzini <[email protected]>

…ectly Because gBS.Raise/RestoreTPL have been fixed to handle the nested timer interrupts. Signed-off-by: Ray Ni <[email protected]> Signed-off-by: Wei6 Xu <[email protected]>

Because gBS.Raise/RestoreTPL have been fixed to handle the nested timer interrupts, NestedInterruptTplLib is now redundant, remove it entirely from OvmfPkg. Signed-off-by: Wei6 Xu <[email protected]>

kraxel · 2025-07-11T13:07:13Z

Filling the description with some actual content would be very nice, especially on testing.

The patches look good to me.

@mcb30 FYI

mcb30 · 2025-07-11T13:20:56Z

Looks plausible to me. Has it been tested in the two use cases that originally caused problems?

Using a USB network device.
Executing an EFI application that illegally enables interrupts (via a direct sti instruction) while executing at TPL_HIGH_LEVEL.

xuweiintel · 2025-07-13T03:59:41Z

Looks plausible to me. Has it been tested in the two use cases that originally caused problems?

Using a USB network device.

Executing an EFI application that illegally enables interrupts (via a direct sti instruction) while executing at TPL_HIGH_LEVEL.

I will try the above two cases and update the result here later.

ardbiesheuvel · 2025-07-16T01:19:53Z

@leiflindholm @samimujawar

The generic changes in this PR affect all architectures, not just X86.

ardbiesheuvel · 2025-07-16T01:21:07Z

MdeModulePkg/Core/Dxe/Event/Tpl.c

+//
+// 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


Could we please avoid X86-specific lingo here? This code is shared by all architectures, and terms like STI and IRET may not be familiar to everyone.

agree. we could just say "Windows boot loader disables CPU interrupt at TPL_HIGH_LEVEL"

agree. we could just say "Windows boot loader disables CPU interrupt at TPL_HIGH_LEVEL"

Enables, not disables. Interrupts are supposed to be disabled at TPL_HIGH_LEVEL, and EDK2 assumes that this invariant will always hold. The Windows bug is that it first raises to TPL_HIGH_LEVEL (which is already illegal: that level is specified to be "for use exclusively by the firmware") and then enables interrupts (which breaks the firmware's invariant assumption).

MdeModulePkg/Core/Dxe/Event/Tpl.c

ardbiesheuvel · 2025-07-16T01:28:57Z

MdeModulePkg/Core/Dxe/Event/Tpl.c

+      // 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.
+      //


We are conflating interrupts are disabled with running within an interrupt handler here. This needs justification, because not all CPU arch protocol implementations may behave the same in this regard.

The code assumes that the CPU interrupt is disabled as soon as the interrupt handler runs.
So, when interrupts are disabled, the code assumes running in the interrupt context.
Can you explain what else the implementation could be? Or suggest a better change?

niruiyu · 2025-07-28T03:37:55Z

UefiCpuPkg/CpuDxe/CpuDxe.c

When I reread the commit message, I found it could be a bit easier to understand if reworded as follows:

X86 CpuDxe driver uses a global C variable to record the interrupt state.
The global C variable is updated every time CpuArch.EnableInterrupt() or
CpuArch.DisableInterrupt() is called.
CpuArch.GetInterruptState() returns the global C variable value.

But CpuArch.GetInterruptState() incorrectly returns TRUE when it is called
in the interrupt context.
The reason is that the C global variable indicating the interrupt state
remains TRUE after the interrupt is disabled by CLI from the CpuExceptionHandlerLib.

The commit removes the C global variable and always reads the
interrupt state from the CPU register (EFLAGS).

niruiyu · 2025-07-28T03:38:07Z

MdeModulePkg/Core/Dxe/Event/Tpl.c

+//
+// 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


agree. we could just say "Windows boot loader disables CPU interrupt at TPL_HIGH_LEVEL"

MdeModulePkg/Core/Dxe/Event/Tpl.c

niruiyu · 2025-07-28T03:58:14Z

MdeModulePkg/Core/Dxe/Event/Tpl.c

+      // 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.
+      //


The code assumes that the CPU interrupt is disabled as soon as the interrupt handler runs.
So, when interrupts are disabled, the code assumes running in the interrupt context.
Can you explain what else the implementation could be? Or suggest a better change?

ardbiesheuvel · 2025-08-09T23:23:32Z

@eblipp please stop pasting LLM generated drivel into random PRs

sarah-walker-arm · 2025-09-02T09:38:21Z

Looks okay for Arm, besides the already raised issue around x86isms in comments.

mdkinney · 2025-09-23T18:50:22Z

@xuweiintel Please fix CI issues.

@niruiyu are any more changes required to merge?

xunfengd · 2025-10-09T08:10:57Z

Looks plausible to me. Has it been tested in the two use cases that originally caused problems?

Using a USB network device.

Executing an EFI application that illegally enables interrupts (via a direct sti instruction) while executing at TPL_HIGH_LEVEL.

I will try the above two cases and update the result here later.

For the test2,
we have validated pass,

  OldTpl = gBS->RaiseTPL (TPL_HIGH_LEVEL);

  EFlags.UintN = AsmReadEflags ();
  DEBUG((DEBUG_ERROR,"[PXE][Function:%a]\n",__FUNCTION__));
  DEBUG((DEBUG_ERROR,"[PXE] EFlags.UintN = %x\n",EFlags.UintN));
  DEBUG((DEBUG_ERROR,"[PXE] EFlags.Bits.IF = %x Tpl: %x\n",EFlags.Bits.IF, OldTpl));

 
  EnableInterrupts ();

  EFlags.UintN = 0x0;
  EFlags.UintN = AsmReadEflags ();
  DEBUG((DEBUG_ERROR,"[PXE][After] [Function:%a] Enable IF by STI \n",__FUNCTION__));
  DEBUG((DEBUG_ERROR,"[PXE][After] EFlags.UintN = %x\n",EFlags.UintN));
  DEBUG((DEBUG_ERROR,"[PXE][After] EFlags.Bits.IF = %x Tpl: %x\n",EFlags.Bits.IF, OldTpl));
  
  gBS->RestoreTPL (OldTpl);

For test1, we're still reproducing it.

mcb30 · 2025-10-09T12:22:47Z

For the test2, we have validated pass,

  OldTpl = gBS->RaiseTPL (TPL_HIGH_LEVEL);

  EFlags.UintN = AsmReadEflags ();
  DEBUG((DEBUG_ERROR,"[PXE][Function:%a]\n",__FUNCTION__));
  DEBUG((DEBUG_ERROR,"[PXE] EFlags.UintN = %x\n",EFlags.UintN));
  DEBUG((DEBUG_ERROR,"[PXE] EFlags.Bits.IF = %x Tpl: %x\n",EFlags.Bits.IF, OldTpl));

 
  EnableInterrupts ();

  EFlags.UintN = 0x0;
  EFlags.UintN = AsmReadEflags ();
  DEBUG((DEBUG_ERROR,"[PXE][After] [Function:%a] Enable IF by STI \n",__FUNCTION__));
  DEBUG((DEBUG_ERROR,"[PXE][After] EFlags.UintN = %x\n",EFlags.UintN));
  DEBUG((DEBUG_ERROR,"[PXE][After] EFlags.Bits.IF = %x Tpl: %x\n",EFlags.Bits.IF, OldTpl));
  
  gBS->RestoreTPL (OldTpl);

Thank you. How do we tell in this test that an interrupt actually occurred between EnableInterrupts() and RestoreTPL()?

gumingyun · 2025-10-13T03:24:39Z

Looks plausible to me. Has it been tested in the two use cases that originally caused problems?

Using a USB network device.

Executing an EFI application that illegally enables interrupts (via a direct sti instruction) while executing at TPL_HIGH_LEVEL.

I will try the above two cases and update the result here later.

For the test2, we have validated pass,
  OldTpl = gBS->RaiseTPL (TPL_HIGH_LEVEL);

  EFlags.UintN = AsmReadEflags ();
  DEBUG((DEBUG_ERROR,"[PXE][Function:%a]\n",__FUNCTION__));
  DEBUG((DEBUG_ERROR,"[PXE] EFlags.UintN = %x\n",EFlags.UintN));
  DEBUG((DEBUG_ERROR,"[PXE] EFlags.Bits.IF = %x Tpl: %x\n",EFlags.Bits.IF, OldTpl));

 
  EnableInterrupts ();

  EFlags.UintN = 0x0;
  EFlags.UintN = AsmReadEflags ();
  DEBUG((DEBUG_ERROR,"[PXE][After] [Function:%a] Enable IF by STI \n",__FUNCTION__));
  DEBUG((DEBUG_ERROR,"[PXE][After] EFlags.UintN = %x\n",EFlags.UintN));
  DEBUG((DEBUG_ERROR,"[PXE][After] EFlags.Bits.IF = %x Tpl: %x\n",EFlags.Bits.IF, OldTpl));
  
  gBS->RestoreTPL (OldTpl);
For test1, we're still reproducing it.

For the test1, we have tested default BIOS and BIOS with patch, from the result test1 validated pass too.
USB network device MAC: 0C3796CC6DDA

The result as below:

mcb30 · 2025-10-21T15:32:19Z

The testing seems quite sketchy to me, compared to the testing that was done for NestedInterruptTplLib. I would like to have some confidence that:

The testers have understood the original two problem cases that I mentioned.
The testers have managed to reproduce the original problem cases, with 100% repeatability. (Note that reproducing the problem will necessarily require a patch to disable the use of NestedInterruptTplLib, and the details of this patch should ideally be provided as part of the test results.)
The testers have verified that the precise same tests that fail when deliberately reproducing the problem will pass when this PR is applied, with absolutely no other changes.

Otherwise, the testing documented so far is just at the level of "it seems to work ok", which is really not sufficient for something with this level of subtlety and complexity.

mdkinney · 2025-10-21T17:06:56Z

@mcb30 Do you have a link to the testing that was done for the NestedInterruptTplLib so we can rerun the same tests?

I think we should also consider some unit tests to make sure the scenarios in Case 2 are always covered so we never introduce a regression.

For Case 1, USB Network Stack, is there a link to the root cause analysis for that scenario, so we can potentially implement unit tests for that general case as well?

mcb30 · 2025-10-23T09:43:35Z

@mcb30 Do you have a link to the testing that was done for the NestedInterruptTplLib so we can rerun the same tests?

It's unfortunately all handwritten on paper. I'm happy to share copies of the notebooks, if it would help.

I think we should also consider some unit tests to make sure the scenarios in Case 2 are always covered so we never introduce a regression.

For Case 1, USB Network Stack, is there a link to the root cause analysis for that scenario, so we can potentially implement unit tests for that general case as well?

There's a very long conversation about it in the original bugzilla, which is now at #9256

gumingyun · 2025-11-12T02:38:22Z

For the test2, trigger software GPE by setting register SWGPE_CTRL bit17 to 1. And read register GPE0_STS_127_96, check bit2 SWGPE_STS is 1.

niruiyu and others added 4 commits July 10, 2025 16:23

OvmfPkg/LocalApicTimer: Raise/RestoreTPL in TimerInterruptHandler dir…

d28038e

…ectly Because gBS.Raise/RestoreTPL have been fixed to handle the nested timer interrupts. Signed-off-by: Ray Ni <[email protected]> Signed-off-by: Wei6 Xu <[email protected]>

OvmfPkg: Remove NestedInterruptTplLib

6bacccf

Because gBS.Raise/RestoreTPL have been fixed to handle the nested timer interrupts, NestedInterruptTplLib is now redundant, remove it entirely from OvmfPkg. Signed-off-by: Wei6 Xu <[email protected]>

tianocore-assign-reviewers bot requested review from ChaselChiu, LiuZhiguang001, ardbiesheuvel, benjamindoron, gdong1, gguo11837463, jameslu8, jiaxinwu, jongwu, jyao1, kraxel, lgao4, lzeng14, mdroth and mxu9 July 10, 2025 08:25

ardbiesheuvel reviewed Jul 16, 2025

View reviewed changes

MdeModulePkg/Core/Dxe/Event/Tpl.c Show resolved Hide resolved

ardbiesheuvel reviewed Jul 16, 2025

View reviewed changes

MdeModulePkg/Core/Dxe/Event/Tpl.c Show resolved Hide resolved

ardbiesheuvel reviewed Jul 16, 2025

View reviewed changes

niruiyu reviewed Jul 28, 2025

View reviewed changes

os-d mentioned this pull request Jul 30, 2025

[Bug]: Consider Patina Nested Interrupt Functionality OpenDevicePartnership/patina#632

Open

1 task

tianocore deleted a comment from eblipp Aug 31, 2025

Fix stack overflow issue due to nested interrupts #11276

Are you sure you want to change the base?

Fix stack overflow issue due to nested interrupts #11276

Conversation

xuweiintel commented Jul 10, 2025

Description

How This Was Tested

Integration Instructions

Uh oh!

kraxel commented Jul 11, 2025

Uh oh!

mcb30 commented Jul 11, 2025

Uh oh!

xuweiintel commented Jul 13, 2025

Uh oh!

ardbiesheuvel commented Jul 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ardbiesheuvel commented Aug 9, 2025

Uh oh!

sarah-walker-arm commented Sep 2, 2025

Uh oh!

mdkinney commented Sep 23, 2025

Uh oh!

xunfengd commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcb30 commented Oct 9, 2025

Uh oh!

gumingyun commented Oct 13, 2025

Uh oh!

mcb30 commented Oct 21, 2025

Uh oh!

mdkinney commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcb30 commented Oct 23, 2025

Uh oh!

gumingyun commented Nov 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

xunfengd commented Oct 9, 2025 •

edited

Loading

mdkinney commented Oct 21, 2025 •

edited

Loading