From f53835584aaef0be7a92f88a0028239a504aec04 Mon Sep 17 00:00:00 2001 From: Miroslav Rezanina Date: Tue, 16 May 2023 02:23:24 -0400 Subject: [PATCH] * Tue May 16 2023 Miroslav Rezanina - 20230301gitf80f052277c8-4 - edk2-OvmfPkg-Clarify-invariants-for-NestedInterruptTplLib.patch [bz#2189136] - edk2-OvmfPkg-Relax-assertion-that-interrupts-do-not-occur.patch [bz#2189136] - Resolves: bz#2189136 (windows 11 installation broken with edk2-20230301gitf80f052277c8-1.el9) --- ...invariants-for-NestedInterruptTplLib.patch | 72 +++++++++++++++++ ...sertion-that-interrupts-do-not-occur.patch | 81 +++++++++++++++++++ edk2.spec | 12 ++- 3 files changed, 164 insertions(+), 1 deletion(-) create mode 100644 edk2-OvmfPkg-Clarify-invariants-for-NestedInterruptTplLib.patch create mode 100644 edk2-OvmfPkg-Relax-assertion-that-interrupts-do-not-occur.patch diff --git a/edk2-OvmfPkg-Clarify-invariants-for-NestedInterruptTplLib.patch b/edk2-OvmfPkg-Clarify-invariants-for-NestedInterruptTplLib.patch new file mode 100644 index 0000000..34e7b47 --- /dev/null +++ b/edk2-OvmfPkg-Clarify-invariants-for-NestedInterruptTplLib.patch @@ -0,0 +1,72 @@ +From d259d959bab00b76e7187e79731201c9a3628d7c Mon Sep 17 00:00:00 2001 +From: Michael Brown +Date: Tue, 9 May 2023 12:09:30 +0000 +Subject: [PATCH 1/2] OvmfPkg: Clarify invariants for NestedInterruptTplLib + +RH-Author: Gerd Hoffmann +RH-MergeRequest: 35: OvmfPkg: backport NestedInterruptTplLib updates +RH-Bugzilla: 2189136 +RH-Acked-by: Laszlo Ersek +RH-Commit: [1/2] 3f4c655bd43f38f43769c01eadbf96ce578818d0 (kraxel/centos-edk2) + +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 e19d98878e..e921a09c55 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.39.1 + diff --git a/edk2-OvmfPkg-Relax-assertion-that-interrupts-do-not-occur.patch b/edk2-OvmfPkg-Relax-assertion-that-interrupts-do-not-occur.patch new file mode 100644 index 0000000..eb0f673 --- /dev/null +++ b/edk2-OvmfPkg-Relax-assertion-that-interrupts-do-not-occur.patch @@ -0,0 +1,81 @@ +From 28c2cf5f8fa5021ff3a6b83fe07dc519084143c0 Mon Sep 17 00:00:00 2001 +From: Michael Brown +Date: Tue, 9 May 2023 12:09:33 +0000 +Subject: [PATCH 2/2] OvmfPkg: Relax assertion that interrupts do not occur at + TPL_HIGH_LEVEL + +RH-Author: Gerd Hoffmann +RH-MergeRequest: 35: OvmfPkg: backport NestedInterruptTplLib updates +RH-Bugzilla: 2189136 +RH-Acked-by: Laszlo Ersek +RH-Commit: [2/2] ffb3cf6132b774295a49d16d4ef35164b61c0de3 (kraxel/centos-edk2) + +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 e921a09c55..d56c12a445 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.39.1 + diff --git a/edk2.spec b/edk2.spec index 083bfbf..3d9f5f8 100644 --- a/edk2.spec +++ b/edk2.spec @@ -18,7 +18,7 @@ ExclusiveArch: x86_64 aarch64 Name: edk2 Version: %{GITDATE}git%{GITCOMMIT} -Release: 3%{?dist} +Release: 4%{?dist} Summary: UEFI firmware for 64-bit virtual machines License: BSD-2-Clause-Patent and OpenSSL and MIT URL: http://www.tianocore.org @@ -76,6 +76,10 @@ Patch0025: 0025-MdePkg-Remove-Itanium-leftover-data-structure-RH-onl.patch Patch0027: 0027-CryptoPkg-OpensslLib-list-RHEL8-specific-OpenSSL-fil.patch Patch0028: 0028-Revert-ArmVirtPkg-ArmVirtQemu-enable-initial-ID-map-.patch Patch0029: 0029-OvmfPkg-disable-dynamic-mmio-window-rhel-only.patch +# For bz#2189136 - windows 11 installation broken with edk2-20230301gitf80f052277c8-1.el9 +Patch30: edk2-OvmfPkg-Clarify-invariants-for-NestedInterruptTplLib.patch +# For bz#2189136 - windows 11 installation broken with edk2-20230301gitf80f052277c8-1.el9 +Patch31: edk2-OvmfPkg-Relax-assertion-that-interrupts-do-not-occur.patch # python3-devel and libuuid-devel are required for building tools. @@ -395,6 +399,12 @@ install -m 0644 \ %changelog +* Tue May 16 2023 Miroslav Rezanina - 20230301gitf80f052277c8-4 +- edk2-OvmfPkg-Clarify-invariants-for-NestedInterruptTplLib.patch [bz#2189136] +- edk2-OvmfPkg-Relax-assertion-that-interrupts-do-not-occur.patch [bz#2189136] +- Resolves: bz#2189136 + (windows 11 installation broken with edk2-20230301gitf80f052277c8-1.el9) + * Mon May 08 2023 Miroslav Rezanina - 20230301gitf80f052277c8-3 - edk2-add-aarch64-qcow2-images.patch [bz#2186754] - edk2-update-json-files.patch [bz#2186754]