From 9839d74e468ea3fa0dd7e528839bce3a2d75f21c Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Thu, 11 May 2023 07:30:24 +0200 Subject: [PATCH] update NestedInterruptTplLib patches --- ...invariants-for-NestedInterruptTplLib.patch | 66 ++++++++++++++++ ...terruptTplLib-replace-ASSERT-with-a-.patch | 40 ---------- ...sertion-that-interrupts-do-not-occur.patch | 75 +++++++++++++++++++ edk2.spec | 3 +- 4 files changed, 143 insertions(+), 41 deletions(-) create mode 100644 0016-OvmfPkg-Clarify-invariants-for-NestedInterruptTplLib.patch delete mode 100644 0016-OvmfPkg-NestedInterruptTplLib-replace-ASSERT-with-a-.patch create mode 100644 0017-OvmfPkg-Relax-assertion-that-interrupts-do-not-occur.patch diff --git a/0016-OvmfPkg-Clarify-invariants-for-NestedInterruptTplLib.patch b/0016-OvmfPkg-Clarify-invariants-for-NestedInterruptTplLib.patch new file mode 100644 index 0000000..318149a --- /dev/null +++ b/0016-OvmfPkg-Clarify-invariants-for-NestedInterruptTplLib.patch @@ -0,0 +1,66 @@ +From 51846ff74e3352151f99cfcfbe091c09f3ec8097 Mon Sep 17 00:00:00 2001 +From: Michael Brown +Date: Tue, 9 May 2023 12:09:30 +0000 +Subject: [PATCH 16/18] OvmfPkg: Clarify invariants for NestedInterruptTplLib + +NestedInterruptTplLib relies on CPU interrupts being disabled to +guarantee exclusive (and hence atomic) access to the shared state in +IsrState. Nothing in the calling interrupt handler should have +re-enabled interrupts before calling NestedInterruptRestoreTPL(), and +the loop in NestedInterruptRestoreTPL() itself maintains the invariant +that interrupts are disabled at the start of each iteration. + +Add assertions to clarify this invariant, and expand the comments +around the calls to RestoreTPL() and DisableInterrupts() to clarify +the expectations around enabling and disabling interrupts. + +Signed-off-by: Michael Brown +Acked-by: Laszlo Ersek +(cherry picked from commit ae0be176a83efebe9a8c13d2124151f7dd13443a) +--- + OvmfPkg/Library/NestedInterruptTplLib/Tpl.c | 10 ++++++++-- + 1 file changed, 8 insertions(+), 2 deletions(-) + +diff --git a/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c b/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c +index e19d98878eb7..e921a09c5599 100644 +--- a/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c ++++ b/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c +@@ -104,6 +104,7 @@ NestedInterruptRestoreTPL ( + // defer our call to RestoreTPL() to the in-progress outer instance + // of the same interrupt handler. + // ++ ASSERT (GetInterruptState () == FALSE); + if (InterruptedTPL == IsrState->InProgressRestoreTPL) { + // + // Trigger outer instance of this interrupt handler to perform the +@@ -153,6 +154,7 @@ NestedInterruptRestoreTPL ( + // + // Check shared state loop invariants. + // ++ ASSERT (GetInterruptState () == FALSE); + ASSERT (IsrState->InProgressRestoreTPL < InterruptedTPL); + ASSERT (IsrState->DeferredRestoreTPL == FALSE); + +@@ -167,13 +169,17 @@ NestedInterruptRestoreTPL ( + + // + // Call RestoreTPL() to allow event notifications to be +- // dispatched. This will implicitly re-enable interrupts. ++ // dispatched. This will implicitly re-enable interrupts (if ++ // InterruptedTPL is below TPL_HIGH_LEVEL), even though we are ++ // still inside the interrupt handler. + // + gBS->RestoreTPL (InterruptedTPL); + + // + // Re-disable interrupts after the call to RestoreTPL() to ensure +- // that we have exclusive access to the shared state. ++ // that we have exclusive access to the shared state. Interrupts ++ // will be re-enabled by the IRET or equivalent instruction when ++ // we subsequently return from the interrupt handler. + // + DisableInterrupts (); + +-- +2.40.1 + diff --git a/0016-OvmfPkg-NestedInterruptTplLib-replace-ASSERT-with-a-.patch b/0016-OvmfPkg-NestedInterruptTplLib-replace-ASSERT-with-a-.patch deleted file mode 100644 index 62d0e21..0000000 --- a/0016-OvmfPkg-NestedInterruptTplLib-replace-ASSERT-with-a-.patch +++ /dev/null @@ -1,40 +0,0 @@ -From 7c65395d2b45e388c6c106cb619ab93c2359f5e4 Mon Sep 17 00:00:00 2001 -From: Gerd Hoffmann -Date: Thu, 27 Apr 2023 12:14:16 +0200 -Subject: [PATCH 16/17] OvmfPkg/NestedInterruptTplLib: replace ASSERT() with a - warning logged. - -OVMF can't guarantee that the ASSERT() doesn't happen. Misbehaving -EFI applications can trigger this. So log a warning instead and try -to continue. - -Reproducer: Fetch windows 11 22H2 iso image, boot it in qemu with OVMF. - -Traced to BootServices->Stall() being called with IPL=TPL_HIGH_LEVEL -and Interrupts /enabled/ while windows is booting. - -Cc: Michael Brown -Cc: Laszlo Ersek -Signed-off-by: Gerd Hoffmann ---- - OvmfPkg/Library/NestedInterruptTplLib/Tpl.c | 4 +++- - 1 file changed, 3 insertions(+), 1 deletion(-) - -diff --git a/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c b/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c -index e19d98878eb7..fdd7d15c4ba8 100644 ---- a/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c -+++ b/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c -@@ -39,7 +39,9 @@ NestedInterruptRaiseTPL ( - // - ASSERT (GetInterruptState () == FALSE); - InterruptedTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL); -- ASSERT (InterruptedTPL < TPL_HIGH_LEVEL); -+ if (InterruptedTPL >= TPL_HIGH_LEVEL) { -+ DEBUG ((DEBUG_WARN, "%a: Called at IPL %d\n", __func__, InterruptedTPL)); -+ } - - return InterruptedTPL; - } --- -2.40.1 - diff --git a/0017-OvmfPkg-Relax-assertion-that-interrupts-do-not-occur.patch b/0017-OvmfPkg-Relax-assertion-that-interrupts-do-not-occur.patch new file mode 100644 index 0000000..a9323b9 --- /dev/null +++ b/0017-OvmfPkg-Relax-assertion-that-interrupts-do-not-occur.patch @@ -0,0 +1,75 @@ +From 7272c2fbe66941f0785be7ec437ed79ab9e35b80 Mon Sep 17 00:00:00 2001 +From: Michael Brown +Date: Tue, 9 May 2023 12:09:33 +0000 +Subject: [PATCH 17/18] OvmfPkg: Relax assertion that interrupts do not occur + at TPL_HIGH_LEVEL + +At TPL_HIGH_LEVEL, CPU interrupts are disabled (as per the UEFI +specification) and so we should never encounter a situation in which +an interrupt occurs at TPL_HIGH_LEVEL. The specification also +restricts usage of TPL_HIGH_LEVEL to the firmware itself. + +However, nothing actually prevents a UEFI application from calling +gBS->RaiseTPL(TPL_HIGH_LEVEL) and then violating the invariant by +enabling interrupts via the STI or equivalent instruction. Some +versions of the Microsoft Windows bootloader are known to do this. + +NestedInterruptTplLib maintains the invariant that interrupts are +disabled at TPL_HIGH_LEVEL (even when performing the dark art of +deliberately manipulating the stack so that IRET will return with +interrupts still disabled), but does not itself rely on external code +maintaining this invariant. + +Relax the assertion that the interrupted TPL is below TPL_HIGH_LEVEL +to an error message, to allow UEFI applications such as these versions +of the Microsoft Windows bootloader to continue to function. + +Debugged-by: Gerd Hoffmann +Debugged-by: Laszlo Ersek +Ref: https://bugzilla.redhat.com/show_bug.cgi?id=2189136 +Signed-off-by: Michael Brown +Acked-by: Laszlo Ersek +Reviewed-by: Gerd Hoffmann +(cherry picked from commit bee67e0c142af6599a85aa7640094816b8a24c4f) +--- + OvmfPkg/Library/NestedInterruptTplLib/Tpl.c | 21 ++++++++++++++++++--- + 1 file changed, 18 insertions(+), 3 deletions(-) + +diff --git a/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c b/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c +index e921a09c5599..d56c12a44529 100644 +--- a/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c ++++ b/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c +@@ -34,12 +34,27 @@ NestedInterruptRaiseTPL ( + + // + // Raise TPL and assert that we were called from within an interrupt +- // handler (i.e. with TPL below TPL_HIGH_LEVEL but with interrupts +- // disabled). ++ // handler (i.e. with interrupts already disabled before raising the ++ // TPL). + // + ASSERT (GetInterruptState () == FALSE); + InterruptedTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL); +- ASSERT (InterruptedTPL < TPL_HIGH_LEVEL); ++ ++ // ++ // At TPL_HIGH_LEVEL, CPU interrupts are disabled (as per the UEFI ++ // specification) and so we should never encounter a situation in ++ // which InterruptedTPL==TPL_HIGH_LEVEL. The specification also ++ // restricts usage of TPL_HIGH_LEVEL to the firmware itself. ++ // ++ // However, nothing actually prevents a UEFI application from ++ // invalidly calling gBS->RaiseTPL(TPL_HIGH_LEVEL) and then ++ // violating the invariant by enabling interrupts via the STI or ++ // equivalent instruction. Some versions of the Microsoft Windows ++ // bootloader are known to do this. ++ // ++ if (InterruptedTPL >= TPL_HIGH_LEVEL) { ++ DEBUG ((DEBUG_ERROR, "ERROR: Interrupts enabled at TPL_HIGH_LEVEL!\n")); ++ } + + return InterruptedTPL; + } +-- +2.40.1 + diff --git a/edk2.spec b/edk2.spec index 66cabfc..80ec131 100644 --- a/edk2.spec +++ b/edk2.spec @@ -98,7 +98,8 @@ Patch0012: 0012-OvmfPkg-QemuKernelLoaderFsDxe-suppress-error-on-no-k.patch Patch0013: 0013-SecurityPkg-Tcg2Dxe-suppress-error-on-no-swtpm-in-si.patch Patch0014: 0014-SecurityPkg-add-TIS-sanity-check-tpm2.patch Patch0015: 0015-SecurityPkg-add-TIS-sanity-check-tpm12.patch -Patch0016: 0016-OvmfPkg-NestedInterruptTplLib-replace-ASSERT-with-a-.patch +Patch0016: 0016-OvmfPkg-Clarify-invariants-for-NestedInterruptTplLib.patch +Patch0017: 0017-OvmfPkg-Relax-assertion-that-interrupts-do-not-occur.patch # python3-devel and libuuid-devel are required for building tools.