Re: [PATCH 16/16] efi/libstub: Use memory attribute protocol

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2022-10-19 10:42, Ard Biesheuvel wrote:
On Tue, 6 Sept 2022 at 12:42, Evgeniy Baskov <baskov@xxxxxxxxx> wrote:

Add EFI_MEMORY_ATTRIBUTE_PROTOCOL as preferred alternative to DXE
services for changing memory attributes in the EFISTUB.

Use DXE services only as a fallback in case aforementioned protocol
is not supported by UEFI implementation.

Move DXE services initialization code closer to the place they are used
to match EFI_MEMORY_ATTRIBUTE_PROTOCOL initialization code.

Signed-off-by: Evgeniy Baskov <baskov@xxxxxxxxx>
---
drivers/firmware/efi/libstub/mem.c | 166 ++++++++++++++++++------
 drivers/firmware/efi/libstub/x86-stub.c |  17 ---
 2 files changed, 127 insertions(+), 56 deletions(-)

diff --git a/drivers/firmware/efi/libstub/mem.c b/drivers/firmware/efi/libstub/mem.c
index 89ebc8ad2c22..8c8782993b30 100644
--- a/drivers/firmware/efi/libstub/mem.c
+++ b/drivers/firmware/efi/libstub/mem.c
@@ -5,6 +5,9 @@

 #include "efistub.h"

+const efi_dxe_services_table_t *efi_dxe_table;
+efi_memory_attribute_protocol_t *efi_mem_attrib_proto;
+
 static inline bool mmap_has_headroom(unsigned long buff_size,
                                     unsigned long map_size,
                                     unsigned long desc_size)
@@ -131,50 +134,32 @@ void efi_free(unsigned long size, unsigned long addr)
        efi_bs_call(free_pages, addr, nr_pages);
 }

-/**
- * efi_adjust_memory_range_protection() - change memory range protection attributes
- * @start:     memory range start address
- * @size:      memory range size
- *
- * Actual memory range for which memory attributes are modified is
- * the smallest ranged with start address and size aligned to EFI_PAGE_SIZE
- * that includes [start, start + size].
- *
- * @return: status code
- */
-efi_status_t efi_adjust_memory_range_protection(unsigned long start,
-                                               unsigned long size,
- unsigned long attributes)
+static void retrive_dxe_table(void)

retrieve

+{
+ efi_dxe_table = get_efi_config_table(EFI_DXE_SERVICES_TABLE_GUID);
+       if (efi_dxe_table &&
+ efi_dxe_table->hdr.signature != EFI_DXE_SERVICES_TABLE_SIGNATURE) { + efi_warn("Ignoring DXE services table: invalid signature\n");
+               efi_dxe_table = NULL;
+       }
+}
+
+static efi_status_t adjust_mem_attrib_dxe(efi_physical_addr_t rounded_start, + efi_physical_addr_t rounded_end,
+                                         unsigned long attributes)
 {
        efi_status_t status;
        efi_gcd_memory_space_desc_t desc;
-       efi_physical_addr_t end, next;
-       efi_physical_addr_t rounded_start, rounded_end;
+       efi_physical_addr_t end, next, start;
        efi_physical_addr_t unprotect_start, unprotect_size;
        int has_system_memory = 0;

-       if (efi_dxe_table == NULL)
-               return EFI_UNSUPPORTED;
+       if (!efi_dxe_table) {
+               retrive_dxe_table();

Same here


-       /*
-        * This function should not be used to modify attributes
-        * other than writable/executable.
-        */
-
-       if ((attributes & ~(EFI_MEMORY_RO | EFI_MEMORY_XP)) != 0)
-               return EFI_INVALID_PARAMETER;
-
-       /*
-        * Disallow simultaniously executable and writable memory
-        * to inforce W^X policy if direct extraction code is enabled.
-        */
-
-       if ((attributes & (EFI_MEMORY_RO | EFI_MEMORY_XP)) == 0 &&
-           IS_ENABLED(CONFIG_EFI_STUB_EXTRACT_DIRECT))
-               return EFI_INVALID_PARAMETER;
-
-       rounded_start = rounddown(start, EFI_PAGE_SIZE);
-       rounded_end = roundup(start + size, EFI_PAGE_SIZE);
+               if (!efi_dxe_table)
+                       return EFI_UNSUPPORTED;
+       }

        /*
         * Don't modify memory region attributes, they are
@@ -182,14 +167,15 @@ efi_status_t efi_adjust_memory_range_protection(unsigned long start,
         * encounter firmware bugs.
         */

-       for (end = start + size; start < end; start = next) {
+
+ for (start = rounded_start, end = rounded_end; start < end; start = next) {

                status = efi_dxe_call(get_memory_space_descriptor,
                                      start, &desc);

                if (status != EFI_SUCCESS) {
efi_warn("Unable to get memory descriptor at %lx\n",
-                                start);
+                                (unsigned long)start);
                        return status;
                }

@@ -231,3 +217,105 @@ efi_status_t efi_adjust_memory_range_protection(unsigned long start,

        return EFI_SUCCESS;
 }
+
+static void retrive_memory_attributes_proto(void)

and here

+{
+       efi_status_t status;
+       efi_guid_t guid = EFI_MEMORY_ATTRIBUTE_PROTOCOL_GUID;
+
+       status = efi_bs_call(locate_protocol, &guid, NULL,
+                            (void **)&efi_mem_attrib_proto);
+       if (status != EFI_SUCCESS)
+               efi_mem_attrib_proto = NULL;
+}
+
+/**
+ * efi_adjust_memory_range_protection() - change memory range protection attributes
+ * @start:     memory range start address
+ * @size:      memory range size
+ *
+ * Actual memory range for which memory attributes are modified is
+ * the smallest ranged with start address and size aligned to EFI_PAGE_SIZE
+ * that includes [start, start + size].
+ *
+ * This function first attempts to use EFI_MEMORY_ATTRIBUTE_PROTOCOL,
+ * that is a part of UEFI Specification since version 2.10.
+ * If the protocol is unavailable it falls back to DXE services functions.
+ *
+ * @return: status code
+ */
+efi_status_t efi_adjust_memory_range_protection(unsigned long start,
+                                               unsigned long size,
+ unsigned long attributes)
+{
+       efi_status_t status;
+       efi_physical_addr_t rounded_start, rounded_end;
+       unsigned long attr_clear;
+
+       /*
+        * This function should not be used to modify attributes
+        * other than writable/executable.
+        */
+
+       if ((attributes & ~(EFI_MEMORY_RO | EFI_MEMORY_XP)) != 0)
+               return EFI_INVALID_PARAMETER;
+
+       /*
+        * Disallow simultaniously executable and writable memory

simultaneously

+        * to inforce W^X policy if direct extraction code is enabled.

enforce

+        */
+
+       if ((attributes & (EFI_MEMORY_RO | EFI_MEMORY_XP)) == 0 &&
+           IS_ENABLED(CONFIG_EFI_STUB_EXTRACT_DIRECT))

efi_adjust_memory_range_protection() is a generic routine, but here it
depends on a x86-specific Kconfig symbol. Is that really necessary?

Since, I will make direct extraction unconditional, I will remove this as
well, since it will be always true.


+               return EFI_INVALID_PARAMETER;
+
+       rounded_start = rounddown(start, EFI_PAGE_SIZE);
+       rounded_end = roundup(start + size, EFI_PAGE_SIZE);
+
+       if (!efi_mem_attrib_proto) {
+               retrive_memory_attributes_proto();

retrieve

+
+               /* Fall back to DXE services if unsupported */
+               if (!efi_mem_attrib_proto) {
+                       return adjust_mem_attrib_dxe(rounded_start,
+                                                    rounded_end,
+                                                    attributes);
+               }
+       }
+
+       /*
+ * Unlike DXE services functions, EFI_MEMORY_ATTRIBUTE_PROTOCOL + * does not clear unset protection bit, so it needs to be cleared
+        * explcitly
+        */
+
+       attr_clear = ~attributes &
+                    (EFI_MEMORY_RO | EFI_MEMORY_XP | EFI_MEMORY_RP);
+
+       status = efi_call_proto(efi_mem_attrib_proto,
+                               clear_memory_attributes,
+                               rounded_start,
+                               rounded_end - rounded_start,
+                               attr_clear);
+       if (status != EFI_SUCCESS) {
+ efi_warn("Failed to clear memory attributes at [%08lx,%08lx]: %lx",

Need \n at the end here

+                        (unsigned long)rounded_start,
+                        (unsigned long)rounded_end,
+                        status);
+               return status;
+       }
+
+       status = efi_call_proto(efi_mem_attrib_proto,
+                               set_memory_attributes,
+                               rounded_start,
+                               rounded_end - rounded_start,
+                               attributes);
+       if (status != EFI_SUCCESS) {
+ efi_warn("Failed to set memory attributes at [%08lx,%08lx]: %lx",

and here

+                        (unsigned long)rounded_start,
+                        (unsigned long)rounded_end,
+                        status);
+       }
+
+       return status;
+}
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index 914106d547a6..dd1e1e663072 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -22,7 +22,6 @@
 #define MAXMEM_X86_64_4LEVEL (1ull << 46)

 const efi_system_table_t *efi_system_table;
-const efi_dxe_services_table_t *efi_dxe_table;
 extern u32 image_offset;
 static efi_loaded_image_t *image = NULL;

@@ -401,15 +400,6 @@ static void setup_sections_memory_protection(void *image_base,
                                             unsigned long init_size)
 {
 #ifdef CONFIG_EFI_DXE_MEM_ATTRIBUTES
- efi_dxe_table = get_efi_config_table(EFI_DXE_SERVICES_TABLE_GUID);
-
-       if (!efi_dxe_table ||
- efi_dxe_table->hdr.signature != EFI_DXE_SERVICES_TABLE_SIGNATURE) {
-               efi_warn("Unable to locate EFI DXE services table\n");
-               efi_dxe_table = NULL;
-               return;
-       }
-
        extern char _head[], _ehead[];
        extern char _compressed[], _ecompressed[];
        extern char _text[], _etext[];
@@ -791,13 +781,6 @@ unsigned long efi_main(efi_handle_t handle,
if (efi_system_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
                efi_exit(handle, EFI_INVALID_PARAMETER);

- efi_dxe_table = get_efi_config_table(EFI_DXE_SERVICES_TABLE_GUID);
-       if (efi_dxe_table &&
- efi_dxe_table->hdr.signature != EFI_DXE_SERVICES_TABLE_SIGNATURE) { - efi_warn("Ignoring DXE services table: invalid signature\n");
-               efi_dxe_table = NULL;
-       }
-
 #ifndef CONFIG_EFI_STUB_EXTRACT_DIRECT
        /*
         * If the kernel isn't already loaded at a suitable address,
--
2.35.1


Thanks, for nitpicks, I will fix all the typos. \n's are fixed by Peter's patch,
so I will rely on it.



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux