[pve-devel] [PATCH edk2-firmware] add patch to revert addition of EFI memory attributes protocol for x86_64

* [pve-devel] [PATCH edk2-firmware] add patch to revert addition of EFI memory attributes protocol for x86_64
@ 2025-03-27 11:53 Fiona Ebner
  2025-03-27 16:39 ` [pve-devel] applied: " Thomas Lamprecht
  0 siblings, 1 reply; 3+ messages in thread
From: Fiona Ebner @ 2025-03-27 11:53 UTC (permalink / raw)
  To: pve-devel

Because of a long-standing bug in shim [0], booting will fail for
distibutions that do not include the fix yet, like Rocky Linux 9.5
and other CentOS-based distibutions. This is cased by the addition
of the EFI_MEMORY_ATTRIBUTE_PROTOCOL in edk2 commit efaa102d00
("UefiCpuPkg: Produce EFI memory attributes protocol") for x86_64.

Even with the fix in shim, issues in commonly shipped versions of GRUB
remain [1].

This is relatively recent, i.e. in the edk2-stable202502 tag, and
since current non-minor distributions are still affected, revert the
problematic commit for now.

Once issues are less common in distributions, an option to support
disabling it (via fw_cfg on the QEMU command line) can still be added
[1]. Then, it can also be nicely documented as a known issue while
giving users guidance.

There already is a similar patch for ARM [2] inherited from the Debian
upstream version.

The problematic commit is EFI_MEMORY_ATTRIBUTE_PROTOCOL was added for x86_64 recently in the
edk2-stable202502 tag. Since current non-minor distributions are still
affected, a revert is done for now.

[0]: https://github.com/rhboot/shim/commit/c7b305152802c8db688605654f75e1195def9fd6
[1]: https://github.com/tianocore/edk2/pull/10667
[2]: ./debian/patches/ArmVirtPkg-disable-the-EFI_MEMORY_ATTRIBUTE-protocol.patch

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 ...g-Produce-EFI-memory-attributes-prot.patch | 368 ++++++++++++++++++
 debian/patches/series                         |   1 +
 2 files changed, 369 insertions(+)
 create mode 100644 debian/patches/Revert-UefiCpuPkg-Produce-EFI-memory-attributes-prot.patch

diff --git a/debian/patches/Revert-UefiCpuPkg-Produce-EFI-memory-attributes-prot.patch b/debian/patches/Revert-UefiCpuPkg-Produce-EFI-memory-attributes-prot.patch
new file mode 100644
index 0000000..40942f3
--- /dev/null
+++ b/debian/patches/Revert-UefiCpuPkg-Produce-EFI-memory-attributes-prot.patch
@@ -0,0 +1,368 @@
+Description: Revert "UefiCpuPkg: Produce EFI memory attributes protocol"
+ Because of a long-standing bug in shim [0], booting will fail for distributions
+ that do not include the fix yet, like Rocky Linux 9.5. Temporarily disable the
+ EFI_MEMORY_ATTRIBUTE_PROTOCOL again. There already is a similar patch for ARM:
+ ArmVirtPkg-disable-the-EFI_MEMORY_ATTRIBUTE-protocol.patch
+ [0]: https://github.com/rhboot/shim/commit/c7b305152802c8db688605654f75e1195def9fd6
+Author: Fiona Ebner <f.ebner@proxmox.com>
+Last-Update: 2025-03-27
+
+diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c
+index 472de55180..bf03978710 100644
+--- a/UefiCpuPkg/CpuDxe/CpuDxe.c
++++ b/UefiCpuPkg/CpuDxe/CpuDxe.c
+@@ -1033,11 +1033,6 @@ InitializeCpu (
+                   );
+   ASSERT_EFI_ERROR (Status);
+ 
+-  //
+-  // Install EFI memory attribute Protocol
+-  //
+-  InstallEfiMemoryAttributeProtocol (mCpuHandle);
+-
+   //
+   // Refresh GCD memory space map according to MTRR value.
+   //
+diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.inf b/UefiCpuPkg/CpuDxe/CpuDxe.inf
+index b2954ba234..fca74c44b3 100644
+--- a/UefiCpuPkg/CpuDxe/CpuDxe.inf
++++ b/UefiCpuPkg/CpuDxe/CpuDxe.inf
+@@ -75,7 +75,6 @@
+ 
+ [Protocols]
+   gEfiCpuArchProtocolGuid                       ## PRODUCES
+-  gEfiMemoryAttributeProtocolGuid               ## PRODUCES
+   gEfiMpServiceProtocolGuid                     ## PRODUCES
+   gEfiSmmBase2ProtocolGuid                      ## SOMETIMES_CONSUMES
+ 
+diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
+index 9a519bcb7e..c5bf2285ac 100644
+--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
++++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
+@@ -1436,298 +1436,3 @@ InitializePageTableLib (
+ 
+   return;
+ }
+-
+-/**
+-  This function set given attributes of the memory region specified by
+-  BaseAddress and Length.
+-  The valid Attributes is EFI_MEMORY_RP, EFI_MEMORY_XP, and EFI_MEMORY_RO.
+-
+-  @param  This              The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
+-  @param  BaseAddress       The physical address that is the start address of
+-                            a memory region.
+-  @param  Length            The size in bytes of the memory region.
+-  @param  Attributes        The bit mask of attributes to set for the memory
+-                            region.
+-
+-  @retval EFI_SUCCESS           The attributes were set for the memory region.
+-  @retval EFI_INVALID_PARAMETER Length is zero.
+-                                Attributes specified an illegal combination of
+-                                attributes that cannot be set together.
+-  @retval EFI_UNSUPPORTED       The processor does not support one or more
+-                                bytes of the memory resource range specified
+-                                by BaseAddress and Length.
+-                                The bit mask of attributes is not supported for
+-                                the memory resource range specified by
+-                                BaseAddress and Length.
+-  @retval EFI_OUT_OF_RESOURCES  Requested attributes cannot be applied due to lack of
+-                                system resources.
+-  @retval EFI_ACCESS_DENIED     Attributes for the requested memory region are
+-                                controlled by system firmware and cannot be updated
+-                                via the protocol.
+-**/
+-EFI_STATUS
+-EFIAPI
+-EfiSetMemoryAttributes (
+-  IN  EFI_MEMORY_ATTRIBUTE_PROTOCOL  *This,
+-  IN  EFI_PHYSICAL_ADDRESS           BaseAddress,
+-  IN  UINT64                         Length,
+-  IN  UINT64                         Attributes
+-  )
+-{
+-  RETURN_STATUS  Status;
+-  BOOLEAN        IsModified;
+-  BOOLEAN        IsSplitted;
+-
+-  DEBUG ((DEBUG_VERBOSE, "%a: 0x%lx - 0x%lx (0x%lx)\n", __func__, BaseAddress, Length, Attributes));
+-
+-  if (Attributes == 0) {
+-    DEBUG ((DEBUG_ERROR, "%a: Error - Attributes == 0\n", __func__));
+-    return EFI_INVALID_PARAMETER;
+-  }
+-
+-  if ((Attributes & ~EFI_MEMORY_ACCESS_MASK) != 0) {
+-    DEBUG ((DEBUG_ERROR, "%a: Error - Attributes(0x%lx) invalid\n", __func__, Attributes));
+-    return EFI_INVALID_PARAMETER;
+-  }
+-
+-  if (Length == 0) {
+-    DEBUG ((DEBUG_ERROR, "%a: Length is 0!\n", __func__));
+-    return RETURN_INVALID_PARAMETER;
+-  }
+-
+-  Status = ConvertMemoryPageAttributes (NULL, BaseAddress, Length, Attributes, PageActionSet, NULL, &IsSplitted, &IsModified);
+-  if (!EFI_ERROR (Status)) {
+-    if (IsModified) {
+-      //
+-      // Flush TLB as last step.
+-      //
+-      // Note: Since APs will always init CR3 register in HLT loop mode or do
+-      // TLB flush in MWAIT loop mode, there's no need to flush TLB for them
+-      // here.
+-      //
+-      CpuFlushTlb ();
+-    }
+-  } else {
+-    DEBUG ((DEBUG_ERROR, "%a: Failed in ConvertMemoryPageAttributes (%r)\n", __func__, Status));
+-  }
+-
+-  return Status;
+-}
+-
+-/**
+-  This function clears given attributes of the memory region specified by
+-  BaseAddress and Length.
+-  The valid Attributes is EFI_MEMORY_RP, EFI_MEMORY_XP, and EFI_MEMORY_RO.
+-  @param  This              The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
+-  @param  BaseAddress       The physical address that is the start address of
+-                            a memory region.
+-  @param  Length            The size in bytes of the memory region.
+-  @param  Attributes        The bit mask of attributes to clear for the memory
+-                            region.
+-
+-  @retval EFI_SUCCESS           The attributes were cleared for the memory region.
+-  @retval EFI_INVALID_PARAMETER Length is zero.
+-                                Attributes specified an illegal combination of
+-                                attributes that cannot be cleared together.
+-  @retval EFI_UNSUPPORTED       The processor does not support one or more
+-                                bytes of the memory resource range specified
+-                                by BaseAddress and Length.
+-                                The bit mask of attributes is not supported for
+-                                the memory resource range specified by
+-                                BaseAddress and Length.
+-  @retval EFI_OUT_OF_RESOURCES  Requested attributes cannot be applied due to lack of
+-                                system resources.
+-  @retval EFI_ACCESS_DENIED     Attributes for the requested memory region are
+-                                controlled by system firmware and cannot be updated
+-                                via the protocol.
+-**/
+-EFI_STATUS
+-EFIAPI
+-EfiClearMemoryAttributes (
+-  IN  EFI_MEMORY_ATTRIBUTE_PROTOCOL  *This,
+-  IN  EFI_PHYSICAL_ADDRESS           BaseAddress,
+-  IN  UINT64                         Length,
+-  IN  UINT64                         Attributes
+-  )
+-{
+-  RETURN_STATUS  Status;
+-  BOOLEAN        IsModified;
+-  BOOLEAN        IsSplitted;
+-
+-  DEBUG ((DEBUG_VERBOSE, "%a: 0x%lx - 0x%lx (0x%lx)\n", __func__, BaseAddress, Length, Attributes));
+-
+-  if (Attributes == 0) {
+-    DEBUG ((DEBUG_ERROR, "%a: Error - Attributes == 0\n", __func__));
+-    return EFI_INVALID_PARAMETER;
+-  }
+-
+-  if ((Attributes & ~EFI_MEMORY_ACCESS_MASK) != 0) {
+-    DEBUG ((DEBUG_ERROR, "%a: Error - Attributes(0x%lx) invalid\n", __func__, Attributes));
+-    return EFI_INVALID_PARAMETER;
+-  }
+-
+-  if (Length == 0) {
+-    DEBUG ((DEBUG_ERROR, "%a: Length is 0!\n", __func__));
+-    return RETURN_INVALID_PARAMETER;
+-  }
+-
+-  Status = ConvertMemoryPageAttributes (NULL, BaseAddress, Length, Attributes, PageActionClear, NULL, &IsSplitted, &IsModified);
+-  if (!EFI_ERROR (Status)) {
+-    if (IsModified) {
+-      //
+-      // Flush TLB as last step.
+-      //
+-      // Note: Since APs will always init CR3 register in HLT loop mode or do
+-      // TLB flush in MWAIT loop mode, there's no need to flush TLB for them
+-      // here.
+-      //
+-      CpuFlushTlb ();
+-    }
+-  } else {
+-    DEBUG ((DEBUG_ERROR, "%a: Failed in ConvertMemoryPageAttributes (%r)\n", __func__, Status));
+-  }
+-
+-  return Status;
+-}
+-
+-/**
+-  This function retrieves the attributes of the memory region specified by
+-  BaseAddress and Length. If different attributes are got from different part
+-  of the memory region, EFI_NO_MAPPING will be returned.
+-
+-  @param  This                The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
+-  @param  BaseAddress         The physical address that is the start address of
+-                              a memory region.
+-  @param  Length              The size in bytes of the memory region.
+-  @param  Attributes          Pointer to attributes returned.
+-
+-  @retval EFI_SUCCESS           The attributes got for the memory region.
+-  @retval EFI_INVALID_PARAMETER Length is zero.
+-                                Attributes is NULL.
+-  @retval EFI_NO_MAPPING        Attributes are not consistent cross the memory
+-                                region.
+-  @retval EFI_UNSUPPORTED       The processor does not support one or more
+-                                bytes of the memory resource range specified
+-                                by BaseAddress and Length.
+-**/
+-EFI_STATUS
+-EFIAPI
+-EfiGetMemoryAttributes (
+-  IN  EFI_MEMORY_ATTRIBUTE_PROTOCOL  *This,
+-  IN  EFI_PHYSICAL_ADDRESS           BaseAddress,
+-  IN  UINT64                         Length,
+-  OUT UINT64                         *Attributes
+-  )
+-{
+-  PAGE_TABLE_LIB_PAGING_CONTEXT  CurrentPagingContext;
+-  EFI_PHYSICAL_ADDRESS           Address;
+-  UINT64                         *PageEntry;
+-  UINT64                         MemAttr;
+-  PAGE_ATTRIBUTE                 PageAttr;
+-  INT64                          Size;
+-  UINT64                         AddressEncMask;
+-
+-  DEBUG ((DEBUG_VERBOSE, "%a: 0x%lx - 0x%lx\n", __func__, BaseAddress, Length));
+-
+-  if (!IS_ALIGNED (BaseAddress, EFI_PAGE_SIZE)) {
+-    DEBUG ((DEBUG_ERROR, "%a: BaseAddress(0x%lx) is not aligned!\n", __func__, BaseAddress));
+-    return EFI_UNSUPPORTED;
+-  }
+-
+-  if (!IS_ALIGNED (Length, EFI_PAGE_SIZE)) {
+-    DEBUG ((DEBUG_ERROR, "%a: Length(0x%lx) is not aligned!\n", __func__, Length));
+-    return EFI_UNSUPPORTED;
+-  }
+-
+-  if (Length == 0) {
+-    DEBUG ((DEBUG_ERROR, "%a: Length is 0!\n", __func__));
+-    return RETURN_INVALID_PARAMETER;
+-  }
+-
+-  if (Attributes == NULL) {
+-    DEBUG ((DEBUG_ERROR, "%a: Attributes is NULL\n", __func__));
+-    return EFI_INVALID_PARAMETER;
+-  }
+-
+-  Size    = (INT64)Length;
+-  MemAttr = (UINT64)-1;
+-
+-  // Make sure AddressEncMask is contained to smallest supported address field.
+-  //
+-  AddressEncMask = PcdGet64 (PcdPteMemoryEncryptionAddressOrMask) & PAGING_1G_ADDRESS_MASK_64;
+-
+-  GetCurrentPagingContext (&CurrentPagingContext);
+-
+-  do {
+-    PageEntry = GetPageTableEntry (&CurrentPagingContext, BaseAddress, &PageAttr);
+-    if ((PageEntry == NULL) || (PageAttr == PageNone)) {
+-      return EFI_UNSUPPORTED;
+-    }
+-
+-    //
+-    // If the memory range is cross page table boundary, make sure they
+-    // share the same attribute. Return EFI_NO_MAPPING if not.
+-    //
+-    *Attributes = GetAttributesFromPageEntry (PageEntry);
+-    if ((MemAttr != (UINT64)-1) && (*Attributes != MemAttr)) {
+-      return EFI_NO_MAPPING;
+-    }
+-
+-    switch (PageAttr) {
+-      case Page4K:
+-        Address      = *PageEntry & ~AddressEncMask & PAGING_4K_ADDRESS_MASK_64;
+-        Size        -= (EFI_PAGE_SIZE - (BaseAddress - Address));
+-        BaseAddress += (EFI_PAGE_SIZE - (BaseAddress - Address));
+-        break;
+-
+-      case Page2M:
+-        Address      = *PageEntry & ~AddressEncMask & PAGING_2M_ADDRESS_MASK_64;
+-        Size        -= SIZE_2MB - (BaseAddress - Address);
+-        BaseAddress += SIZE_2MB - (BaseAddress - Address);
+-        break;
+-
+-      case Page1G:
+-        Address      = *PageEntry & ~AddressEncMask & PAGING_1G_ADDRESS_MASK_64;
+-        Size        -= SIZE_1GB - (BaseAddress - Address);
+-        BaseAddress += SIZE_1GB - (BaseAddress - Address);
+-        break;
+-
+-      default:
+-        return EFI_UNSUPPORTED;
+-    }
+-
+-    MemAttr = *Attributes;
+-  } while (Size > 0);
+-
+-  DEBUG ((DEBUG_VERBOSE, "%a: Attributes is 0x%lx\n", __func__, *Attributes));
+-
+-  return EFI_SUCCESS;
+-}
+-
+-EFI_MEMORY_ATTRIBUTE_PROTOCOL  mMemoryAttributeProtocol = {
+-  EfiGetMemoryAttributes,
+-  EfiSetMemoryAttributes,
+-  EfiClearMemoryAttributes,
+-};
+-
+-/**
+-  Install Efi Memory Attribute Protocol.
+-
+-  @param Handle A pointer to the EFI_HANDLE on which the interface is to be installed
+-
+-**/
+-VOID
+-InstallEfiMemoryAttributeProtocol (
+-  IN EFI_HANDLE  Handle
+-  )
+-{
+-  EFI_STATUS  Status;
+-
+-  Status = gBS->InstallMultipleProtocolInterfaces (
+-                  &Handle,
+-                  &gEfiMemoryAttributeProtocolGuid,
+-                  &mMemoryAttributeProtocol,
+-                  NULL
+-                  );
+-  ASSERT_EFI_ERROR (Status);
+-}
+diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.h b/UefiCpuPkg/CpuDxe/CpuPageTable.h
+index f2694452f5..607e936b8a 100644
+--- a/UefiCpuPkg/CpuDxe/CpuPageTable.h
++++ b/UefiCpuPkg/CpuDxe/CpuPageTable.h
+@@ -10,7 +10,6 @@
+ #define _PAGE_TABLE_LIB_H_
+ 
+ #include <IndustryStandard/PeImage.h>
+-#include <Protocol/MemoryAttribute.h>
+ 
+ #define PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PSE              BIT0
+ #define PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAE              BIT1
+@@ -154,15 +153,4 @@ GetPagingDetails (
+   OUT UINT32                              **Attributes        OPTIONAL
+   );
+ 
+-/**
+-  Install Efi Memory Attribute Protocol.
+-
+-  @param Handle A pointer to the EFI_HANDLE on which the interface is to be installed
+-
+-**/
+-VOID
+-InstallEfiMemoryAttributeProtocol (
+-  IN EFI_HANDLE  Handle
+-  );
+-
+ #endif
diff --git a/debian/patches/series b/debian/patches/series
index c18b4e8..45b3050 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -3,3 +3,4 @@ brotlicompress-disable.diff
 x64-baseline-abi.patch
 Revert-ArmVirtPkg-make-EFI_LOADER_DATA-non-executabl.patch
 ArmVirtPkg-disable-the-EFI_MEMORY_ATTRIBUTE-protocol.patch
+Revert-UefiCpuPkg-Produce-EFI-memory-attributes-prot.patch
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 3+ messages in thread