[PATCH RFC 0/3] EFI memory attributes

[PATCH RFC 0/3] EFI memory attributes

[Baskov Evgeniy]

UEFI specification does not guarantee that all memory would
be executable and there exist some UEFI implementations imposing 
stricter memory management policies and restricting execution
of memory where trampoline code generated by relocator is placed.
That causes page fault during boot.

Explicitly set memory attributes using DXE services for
memory region possibly occupied with trampoline code and data.

This patch works in the same way as the following patch
for the Linux kernel [1].

Later the use of DXE services can be replaced with
UEFI_MEMORY_ATTRIBUTE protocol, when it will become ready [2].

[1] https://lkml.org/lkml/2022/3/3/532
[2] https://bugzilla.tianocore.org/show_bug.cgi?id=3519

Baskov Evgeniy(3):
       efi: explicitly set memory attributes for memory
       relocator: allocate trampoline in lower 640k
       efi: provide definitions of DXE services table

 grub-core/kern/efi/efi.c             |   11 +
 grub-core/kern/efi/mm.c              |   67 ++++++++++++
 grub-core/lib/i386/relocator.c       |    5 
 grub-core/lib/relocator.c            |    1 
 grub-core/lib/x86_64/efi/relocator.c |    6 -
 grub-core/loader/i386/bsd.c          |    2 
 include/grub/efi/api.h               |  149 +++++++++++++++++++++++++++
 include/grub/efi/efi.h               |    3 
 include/grub/i386/relocator.h        |    3 
 9 files changed, 238 insertions(+), 9 deletions(-)

[PATCH RFC 1/3] efi: provide definitions of DXE services table

[Baskov Evgeniy]

DXE services can be used to change memory attributes
on systems where EFI use stricter policies about memory
access rights and sets NX flag on some pages required by
grub to executable.

Signed-off-by: Baskov Evgeniy <baskov@ispras.ru>

diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
index 40434ee9d..e164102b1 100644
--- a/grub-core/kern/efi/efi.c
+++ b/grub-core/kern/efi/efi.c
@@ -53,6 +53,17 @@ grub_efi_locate_protocol (grub_efi_guid_t *protocol, void *registration)
   return interface;
 }
 
+void *
+grub_efi_get_config_table(grub_efi_guid_t *guid)
+{
+  grub_efi_uintn_t n_entries = grub_efi_system_table->num_table_entries;
+  grub_efi_configuration_table_t *entry = grub_efi_system_table->configuration_table;
+  for (; n_entries > 0; n_entries--, entry++)
+    if (0 == grub_memcmp (&entry->vendor_guid, guid, sizeof (*guid)))
+      return entry->vendor_table;
+  return NULL;
+}
+
 /* Return the array of handles which meet the requirement. If successful,
    the number of handles is stored in NUM_HANDLES. The array is allocated
    from the heap.  */
diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
index 86db96994..90009feb6 100644
--- a/include/grub/efi/api.h
+++ b/include/grub/efi/api.h
@@ -1352,6 +1352,154 @@ struct grub_efi_runtime_services
 };
 typedef struct grub_efi_runtime_services grub_efi_runtime_services_t;
 
+typedef enum
+  {
+    GRUB_EFI_GCD_MEMORY_TYPE_NON_EXISTENT,
+    GRUB_EFI_GCD_MEMORY_TYPE_RESERVED,
+    GRUB_EFI_GCD_MEMORY_TYPE_SYSTEM_MEMORY,
+    GRUB_EFI_GCD_MEMORY_TYPE_MEMORY_MAPPED_IO,
+    GRUB_EFI_GCD_MEMORY_TYPE_PERSISTENT,
+    GRUB_EFI_GCD_MEMORY_TYPE_MORE_RELIABLE,
+    GRUB_EFI_GCD_MEMORY_TYPE_MAXIMUM
+  }
+grub_efi_gcd_memory_type_t;
+
+typedef enum
+  {
+    GRUB_EFI_GCD_ALLOCATE_ANY_SEARCH_BOTTOM_UP,
+    GRUB_EFI_GCD_ALLOCATE_MAX_ADDRESS_SEARCH_BOTTOM_UP,
+    GRUB_EFI_GCD_ALLOCATE_ADDRESS,
+    GRUB_EFI_GCD_ALLOCATE_ANY_SEARCH_TOP_DOWN,
+    GRUB_EFI_GCD_ALLOCATE_MAX_ADDRESS_SEARCH_TOP_DOWN,
+    GRUB_EFI_GCD_MAX_ALLOCATE_TYPE
+  }
+grub_efi_gcd_allocate_type_t;
+
+struct grub_efi_gcd_memory_space_descriptor
+{
+  grub_efi_physical_address_t base_address;
+  grub_efi_uint64_t length;
+  grub_efi_uint64_t capabilities;
+  grub_efi_uint64_t attributes;
+  grub_efi_gcd_memory_type_t gcd_memory_type;
+  grub_efi_handle_t image_handle;
+  grub_efi_handle_t device_handle;
+};
+typedef struct grub_efi_gcd_memory_space_descriptor
+	grub_efi_gcd_memory_space_descriptor_t;
+
+typedef enum
+  {
+    GRUB_EFI_GCD_IO_TYPE_NON_EXISTENT,
+    GRUB_EFI_GCD_IO_TYPE_RESERVED,
+    GRUB_EFI_GCD_IO_TYPE_IO,
+    GRUB_EFI_GCD_IO_TYPE_MAXIMUM
+  }
+grub_efi_gcd_io_type_t;
+
+struct grub_efi_gcd_io_space_descriptor
+{
+  grub_efi_physical_address_t base_address;
+  grub_efi_uint64_t length;
+  grub_efi_gcd_io_type_t gcd_io_type;
+  grub_efi_handle_t image_handle;
+  grub_efi_handle_t device_handle;
+};
+typedef struct grub_efi_gcd_io_space_descriptor
+	grub_efi_gcd_io_space_descriptor_t;
+
+struct grub_efi_dxe_services {
+  grub_efi_table_header_t hdr;
+
+  grub_efi_status_t
+  (*add_memory_space) (grub_efi_gcd_memory_type_t gcd_memory_type,
+		       grub_efi_physical_address_t base_address,
+		       grub_efi_uint64_t length,
+		       grub_efi_uint64_t capabilities);
+
+  grub_efi_status_t
+  (*allocate_memory_space) (grub_efi_gcd_allocate_type_t gcd_allocate_type,
+			    grub_efi_gcd_memory_type_t gcd_memory_type,
+			    grub_efi_uintn_t alignment,
+			    grub_efi_uint64_t length,
+			    grub_efi_physical_address_t *base_address,
+			    grub_efi_handle_t *image_handle,
+			    grub_efi_handle_t *device_handle);
+
+  grub_efi_status_t
+  (*free_memory_space) (grub_efi_physical_address_t base_address,
+			grub_efi_uint64_t length);
+
+  grub_efi_status_t
+  (*remove_memory_space) (grub_efi_physical_address_t base_address,
+			  grub_efi_uint64_t length);
+
+  grub_efi_status_t
+  (*get_memory_space_descriptor) (grub_efi_physical_address_t base_address,
+				  grub_efi_gcd_memory_space_descriptor_t *descriptor);
+
+  grub_efi_status_t
+  (*set_memory_space_attributes) (grub_efi_physical_address_t base_address,
+				  grub_efi_uint64_t length,
+				  grub_efi_uint64_t attributes);
+
+  grub_efi_status_t
+  (*get_memory_space_map) (grub_efi_uintn_t *number_of_descriptors,
+			   grub_efi_gcd_memory_space_descriptor_t **memory_space_map);
+
+  grub_efi_status_t
+  (*add_io_space) (grub_efi_gcd_io_type_t gcd_io_type,
+		   grub_efi_physical_address_t base_address,
+		   grub_efi_uintn_t length);
+
+  grub_efi_status_t
+  (*allocate_io_space) (grub_efi_gcd_allocate_type_t allocate_type,
+			grub_efi_gcd_io_type_t gcd_io_type,
+			grub_efi_uintn_t alignment,
+			grub_efi_uint64_t length,
+			grub_efi_physical_address_t *base_address,
+			grub_efi_handle_t image_handle,
+			grub_efi_handle_t device_handle);
+
+  grub_efi_status_t
+  (*free_io_space) (grub_efi_physical_address_t base_address,
+		    grub_efi_uint64_t length);
+
+  grub_efi_status_t
+  (*remove_io_space) (grub_efi_physical_address_t base_address,
+		      grub_efi_uint64_t length);
+
+  grub_efi_status_t
+  (*get_io_space_descriptor) (grub_efi_physical_address_t base_address,
+			      grub_efi_gcd_io_space_descriptor_t *descriptor);
+
+  grub_efi_status_t
+  (*get_io_space_map) (grub_efi_uintn_t *number_of_descriptors,
+		       grub_efi_gcd_io_space_descriptor_t **io_space_map);
+
+  grub_efi_status_t
+  (*dispatch) (void);
+
+  grub_efi_status_t
+  (*schedule) (grub_efi_handle_t firmware_volume_handle,
+	       const grub_efi_guid_t *file_name);
+
+  grub_efi_status_t
+  (*trust) (grub_efi_handle_t firmware_volume_handle,
+	    const grub_efi_guid_t *file_name);
+
+  grub_efi_status_t
+  (*process_firmware_volume)(const void *firmware_volume_header,
+			     grub_efi_uintn_t size,
+			     grub_efi_handle_t *firmware_volume_handle);
+
+  grub_efi_status_t
+  (*set_memory_space_capabilities) (grub_efi_physical_address_t base_address,
+				    grub_efi_uint64_t length,
+				    grub_efi_uint64_t capabilities);
+} GRUB_PACKED;
+typedef struct grub_efi_dxe_services grub_efi_dxe_services_t;
+
 struct grub_efi_configuration_table
 {
   grub_efi_packed_guid_t vendor_guid;
@@ -1361,6 +1509,7 @@ typedef struct grub_efi_configuration_table grub_efi_configuration_table_t;
 
 #define GRUB_EFIEMU_SYSTEM_TABLE_SIGNATURE 0x5453595320494249LL
 #define GRUB_EFIEMU_RUNTIME_SERVICES_SIGNATURE 0x56524553544e5552LL
+#define GRUB_EFI_DXE_SERVICES_SIGNATURE 0x565245535f455844LL
 
 struct grub_efi_serial_io_interface
 {
diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
index eb2dfdfce..95f4ead0d 100644
--- a/include/grub/efi/efi.h
+++ b/include/grub/efi/efi.h
@@ -27,6 +27,9 @@
 /* Functions.  */
 void *EXPORT_FUNC(grub_efi_locate_protocol) (grub_efi_guid_t *protocol,
 					     void *registration);
+
+void *EXPORT_FUNC(grub_efi_get_config_table) (grub_efi_guid_t *guid);
+
 grub_efi_handle_t *
 EXPORT_FUNC(grub_efi_locate_handle) (grub_efi_locate_search_type_t search_type,
 				     grub_efi_guid_t *protocol,
-- 
2.35.1

[PATCH RFC 2/3] relocator: allocate trampoline in lower 640k

[Baskov Evgeniy]

For simplification, allocate trampoline in lower memory
for every x86 variation.

This would help setting right attributes for memory
in UEFI environments that restricts execution of some
memory regions.

Signed-off-by: Baskov Evgeniy <baskov@ispras.ru>

diff --git a/grub-core/lib/i386/relocator.c b/grub-core/lib/i386/relocator.c
index 54a1dcd8b..0b5a22dea 100644
--- a/grub-core/lib/i386/relocator.c
+++ b/grub-core/lib/i386/relocator.c
@@ -172,14 +172,13 @@ grub_relocator16_boot (struct grub_relocator *rel,
 
 grub_err_t
 grub_relocator64_boot (struct grub_relocator *rel,
-		       struct grub_relocator64_state state,
-		       grub_addr_t min_addr, grub_addr_t max_addr)
+		       struct grub_relocator64_state state)
 {
   grub_err_t err;
   void *relst;
   grub_relocator_chunk_t ch;
 
-  err = grub_relocator_alloc_chunk_align_safe (rel, &ch, min_addr, max_addr,
+  err = grub_relocator_alloc_chunk_align_safe (rel, &ch, 0x1000, 0xa0000,
 					       RELOCATOR_SIZEOF (64), 16,
 					       GRUB_RELOCATOR_PREFERENCE_NONE, 0);
   if (err)
diff --git a/grub-core/lib/relocator.c b/grub-core/lib/relocator.c
index 68ef1289a..131b6859f 100644
--- a/grub-core/lib/relocator.c
+++ b/grub-core/lib/relocator.c
@@ -24,6 +24,7 @@
 #include <grub/memory.h>
 #include <grub/dl.h>
 #include <grub/i18n.h>
+#include <grub/efi/efi.h>
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
diff --git a/grub-core/lib/x86_64/efi/relocator.c b/grub-core/lib/x86_64/efi/relocator.c
index 7d200a125..d438f9a05 100644
--- a/grub-core/lib/x86_64/efi/relocator.c
+++ b/grub-core/lib/x86_64/efi/relocator.c
@@ -47,10 +47,10 @@ grub_relocator64_efi_boot (struct grub_relocator *rel,
   grub_relocator_chunk_t ch;
 
   /*
-   * 64-bit relocator code may live above 4 GiB quite well.
-   * However, I do not want ask for problems. Just in case.
+   * GRUB relocator may be placed at any address but GRUB EFI setup code
+   * only guaranties the range [0x1000; 0xa0000) to be executable.
    */
-  err = grub_relocator_alloc_chunk_align_safe (rel, &ch, 0, 0x100000000,
+  err = grub_relocator_alloc_chunk_align_safe (rel, &ch, 0, 0xa0000,
 					       RELOCATOR_SIZEOF (64_efi), 16,
 					       GRUB_RELOCATOR_PREFERENCE_NONE, 1);
   if (err)
diff --git a/grub-core/loader/i386/bsd.c b/grub-core/loader/i386/bsd.c
index 799a24cd2..8b4ae5ba2 100644
--- a/grub-core/loader/i386/bsd.c
+++ b/grub-core/loader/i386/bsd.c
@@ -763,7 +763,7 @@ grub_freebsd_boot (void)
       stack[0] = entry;
       stack[1] = bi.tags;
       stack[2] = kern_end;
-      return grub_relocator64_boot (relocator, state, 0, 0x40000000);
+      return grub_relocator64_boot (relocator, state);
     }
   else
     {
diff --git a/include/grub/i386/relocator.h b/include/grub/i386/relocator.h
index 2a56c3b54..0dbd1f25c 100644
--- a/include/grub/i386/relocator.h
+++ b/include/grub/i386/relocator.h
@@ -87,8 +87,7 @@ grub_err_t grub_relocator32_boot (struct grub_relocator *rel,
 				  int avoid_efi_bootservices);
 
 grub_err_t grub_relocator64_boot (struct grub_relocator *rel,
-				  struct grub_relocator64_state state,
-				  grub_addr_t min_addr, grub_addr_t max_addr);
+				  struct grub_relocator64_state state);
 
 #ifdef GRUB_MACHINE_EFI
 #ifdef __x86_64__
-- 
2.35.1

[PATCH RFC 3/3] efi: explicitly set memory attributes for memory

[Baskov Evgeniy]

UEFI specification does not guarantee that memory
used for trampoline by GRUB would be executable.
Some stricter implementations set NX flag for most
regions.

Explicitly mark memory range where trampoline would
be allocated as writable and executable
using DXE services on x86-efi variations.

Avoid modification if not necessary and only modify
system memory to reduce the possibility to encounter
firmware bugs.

Signed-off-by: Baskov Evgeniy <baskov@ispras.ru>

diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
index d8e411454..f6d1170d1 100644
--- a/grub-core/kern/efi/mm.c
+++ b/grub-core/kern/efi/mm.c
@@ -230,6 +230,71 @@ stop_broadcom (void)
 
 #endif
 
+static void
+unprotect_memory_range (grub_efi_physical_address_t start,
+			 grub_efi_physical_address_t end)
+{
+  static grub_efi_guid_t dxe_guid = GRUB_EFI_DXE_SERVICES_TABLE_GUID;
+  grub_efi_physical_address_t rounded_start, rounded_end, next;
+  grub_efi_physical_address_t unprotect_start, unprotect_end;
+  grub_efi_dxe_services_t *dxe;
+  grub_efi_gcd_memory_space_descriptor_t desc;
+  grub_efi_status_t status;
+
+  dxe = grub_efi_get_config_table (&dxe_guid);
+  if (dxe == NULL || dxe->hdr.signature != GRUB_EFI_DXE_SERVICES_SIGNATURE)
+    return;
+
+  rounded_start = start & ~GRUB_EFI_PAGE_SIZE;
+  rounded_end = (end + GRUB_EFI_PAGE_SIZE - 1) & ~GRUB_EFI_PAGE_SIZE;
+
+  /*
+   * Don't modify memory region attributes, they are
+   * already suitable, to lower the possibility to
+   * encounter firmware bugs.
+   */
+
+  for (; start < end; start = next)
+    {
+      status = efi_call_2 (dxe->get_memory_space_descriptor, start, &desc);
+
+      if (status != GRUB_EFI_SUCCESS)
+        return;
+
+      next = desc.base_address + desc.length;
+
+      /*
+       * Only system memory is suitable for trampoline/kernel image placement,
+       * so only this type of memory needs its attributes to be modified.
+       */
+
+      if (desc.gcd_memory_type != GRUB_EFI_GCD_MEMORY_TYPE_SYSTEM_MEMORY ||
+          (desc.attributes & (GRUB_EFI_MEMORY_RO | GRUB_EFI_MEMORY_XP)) == 0)
+        continue;
+
+      unprotect_start = rounded_start;
+      if (unprotect_start < desc.base_address)
+        unprotect_start = desc.base_address;
+
+      unprotect_end = rounded_end;
+      if (unprotect_end > next)
+        unprotect_end = next;
+
+      status = efi_call_3 (dxe->set_memory_space_attributes,
+			   unprotect_start,
+			   unprotect_end - unprotect_start,
+			   GRUB_EFI_MEMORY_WB);
+
+      if (status != GRUB_EFI_SUCCESS)
+	{
+          grub_printf ("Cannot change memory attributes in range [0x%lX, 0x%lX): 0x%lX\n",
+		       (long)unprotect_start,
+		       (long)unprotect_end,
+		       (long)status);
+	}
+    }
+}
+
 grub_err_t
 grub_efi_finish_boot_services (grub_efi_uintn_t *outbuf_size, void *outbuf,
 			       grub_efi_uintn_t *map_key,
@@ -239,6 +304,8 @@ grub_efi_finish_boot_services (grub_efi_uintn_t *outbuf_size, void *outbuf,
   grub_efi_boot_services_t *b;
   grub_efi_status_t status;
 
+  unprotect_memory_range(0x1000, 0xa0000);
+
 #if defined (__i386__) || defined (__x86_64__)
   const grub_uint16_t apple[] = { 'A', 'p', 'p', 'l', 'e' };
   int is_apple;
-- 
2.35.1

Re: [PATCH RFC 1/3] efi: provide definitions of DXE services table

[Paul Menzel]

Dear Baskov,


Thank you for your patches.


Am 07.04.22 um 05:32 schrieb Baskov Evgeniy:
> DXE services can be used to change memory attributes
> on systems where EFI use stricter policies about memory
> access rights and sets NX flag on some pages required by
> grub to executable.

If you resend, please use (at least) 72 characters per line.

For the UEFI ignorant, could you please reference the UEFI specification 
name, revision, and section needed for review?

[…]


Kind regards,

Paul

Re: [PATCH RFC 1/3] efi: provide definitions of DXE services table

[baskov]

On 2022-04-11 10:12, Paul Menzel wrote:
> Dear Baskov,
> 
> 
> Thank you for your patches.
> 
> 
> Am 07.04.22 um 05:32 schrieb Baskov Evgeniy:
>> DXE services can be used to change memory attributes
>> on systems where EFI use stricter policies about memory
>> access rights and sets NX flag on some pages required by
>> grub to executable.
> 
> If you resend, please use (at least) 72 characters per line.

Thank you, I will take that in mind for the next version.

> 
> For the UEFI ignorant, could you please reference the UEFI
> specification name, revision, and section needed for review?
> 

DXE services are part of UEFI Platform Initialization Specification [1]
Functions used in this patch are described in section 7.2.4, Global
Coherency Domain Services, starting with page 325 (2-41) in version 
1.7A.

Actually, the fact that DXE services are not the part of the main UEFI
spec, was one of the reasons to create the new protocol.
The one remaining problem with it is that it is not supported yet by
the implementation which was the reason for this patch to exist.

[1] 
https://uefi.org/sites/default/files/resources/PI_Spec_1_7_A_final_May1.pdf

Thanks,
Baskov Evgeniy