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

From: Ard Biesheuvel
Date: Wed Oct 19 2022 - 03:42:52 EST

Next message: Rolf Eike Beer: "[PATCH] binfmt_elf: fix documented return value for load_elf_phdrs()"
Previous message: Jeff Vanhoof: "Re: [PATCH v3 2/6] usb: dwc3: gadget: cancel requests instead of release after missed isoc"
Next in thread: Evgeniy Baskov: "Re: [PATCH 16/16] efi/libstub: Use memory attribute protocol"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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?

> + 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
>

Next message: Rolf Eike Beer: "[PATCH] binfmt_elf: fix documented return value for load_elf_phdrs()"
Previous message: Jeff Vanhoof: "Re: [PATCH v3 2/6] usb: dwc3: gadget: cancel requests instead of release after missed isoc"
Next in thread: Evgeniy Baskov: "Re: [PATCH 16/16] efi/libstub: Use memory attribute protocol"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]