On Tue, 6 Sept 2022 at 12:42, Evgeniy Baskov <baskov@xxxxxxxxx> wrote:
>
> efi_adjust_memory_range_protection() can be useful outside x86-stub.c.
>
> Move it to mem.c, where memory related code resides and make it
> non-static.
>
> Change its behavior to setup exact attibutes and disallow making memory
> regions readable and writable simultaniosly for supported
> configurations.
>
> Signed-off-by: Evgeniy Baskov <baskov@xxxxxxxxx>
Acked-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> ---
> drivers/firmware/efi/libstub/efistub.h | 4 +
> drivers/firmware/efi/libstub/mem.c | 101 ++++++++++++++++++++++++
> drivers/firmware/efi/libstub/x86-stub.c | 67 ++--------------
> 3 files changed, 111 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> index b0ae0a454404..22fe28385db7 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -907,6 +907,10 @@ efi_status_t efi_relocate_kernel(unsigned long *image_addr,
> unsigned long alignment,
> unsigned long min_addr);
>
> +efi_status_t efi_adjust_memory_range_protection(unsigned long start,
> + unsigned long size,
> + unsigned long attributes);
> +
> efi_status_t efi_parse_options(char const *cmdline);
>
> void efi_parse_option_graphics(char *option);
> diff --git a/drivers/firmware/efi/libstub/mem.c b/drivers/firmware/efi/libstub/mem.c
> index feef8d4be113..89ebc8ad2c22 100644
> --- a/drivers/firmware/efi/libstub/mem.c
> +++ b/drivers/firmware/efi/libstub/mem.c
> @@ -130,3 +130,104 @@ void efi_free(unsigned long size, unsigned long addr)
> nr_pages = round_up(size, EFI_ALLOC_ALIGN) / EFI_PAGE_SIZE;
> 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)
> +{
> + 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 unprotect_start, unprotect_size;
> + int has_system_memory = 0;
> +
> + if (efi_dxe_table == NULL)
> + return EFI_UNSUPPORTED;
> +
> + /*
> + * 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);
> +
> + /*
> + * Don't modify memory region attributes, they are
> + * already suitable, to lower the possibility to
> + * encounter firmware bugs.
> + */
> +
> + for (end = start + size; 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);
> + return status;
> + }
> +
> + 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 != EfiGcdMemoryTypeSystemMemory) {
> + efi_warn("Attempted to change protection of special memory range\n");
> + return EFI_UNSUPPORTED;
> + }
> +
> + if (((desc.attributes ^ attributes) &
> + (EFI_MEMORY_RO | EFI_MEMORY_XP)) == 0)
> + continue;
> +
> + desc.attributes &= ~(EFI_MEMORY_RO | EFI_MEMORY_XP);
> + desc.attributes |= attributes;
> +
> + unprotect_start = max(rounded_start, desc.base_address);
> + unprotect_size = min(rounded_end, next) - unprotect_start;
> +
> + status = efi_dxe_call(set_memory_space_attributes,
> + unprotect_start, unprotect_size,
> + desc.attributes);
> +
> + if (status != EFI_SUCCESS) {
> + efi_warn("Unable to unprotect memory range [%08lx,%08lx]: %lx\n",
> + (unsigned long)unprotect_start,
> + (unsigned long)(unprotect_start + unprotect_size),
> + status);
> + return status;
> + }
> + }
> +
> + return EFI_SUCCESS;
> +}
> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> index 05ae8bcc9d67..678f9c2ccafc 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -212,62 +212,6 @@ static void retrieve_apple_device_properties(struct boot_params *boot_params)
> }
> }
>
> -static void
> -adjust_memory_range_protection(unsigned long start, unsigned long size)
> -{
> - efi_status_t status;
> - efi_gcd_memory_space_desc_t desc;
> - unsigned long end, next;
> - unsigned long rounded_start, rounded_end;
> - unsigned long unprotect_start, unprotect_size;
> - int has_system_memory = 0;
> -
> - if (efi_dxe_table == NULL)
> - return;
> -
> - rounded_start = rounddown(start, EFI_PAGE_SIZE);
> - rounded_end = roundup(start + size, EFI_PAGE_SIZE);
> -
> - /*
> - * Don't modify memory region attributes, they are
> - * already suitable, to lower the possibility to
> - * encounter firmware bugs.
> - */
> -
> - for (end = start + size; start < end; start = next) {
> -
> - status = efi_dxe_call(get_memory_space_descriptor, start, &desc);
> -
> - if (status != 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 != EfiGcdMemoryTypeSystemMemory ||
> - (desc.attributes & (EFI_MEMORY_RO | EFI_MEMORY_XP)) == 0)
> - continue;
> -
> - unprotect_start = max(rounded_start, (unsigned long)desc.base_address);
> - unprotect_size = min(rounded_end, next) - unprotect_start;
> -
> - status = efi_dxe_call(set_memory_space_attributes,
> - unprotect_start, unprotect_size,
> - EFI_MEMORY_WB);
> -
> - if (status != EFI_SUCCESS) {
> - efi_warn("Unable to unprotect memory range [%08lx,%08lx]: %lx\n",
> - unprotect_start,
> - unprotect_start + unprotect_size,
> - status);
> - }
> - }
> -}
> -
> /*
> * Trampoline takes 2 pages and can be loaded in first megabyte of memory
> * with its end placed between 128k and 640k where BIOS might start.
> @@ -291,12 +235,12 @@ setup_memory_protection(unsigned long image_base, unsigned long image_size)
> * and relocated kernel image.
> */
>
> - adjust_memory_range_protection(TRAMPOLINE_PLACEMENT_BASE,
> - TRAMPOLINE_PLACEMENT_SIZE);
> + efi_adjust_memory_range_protection(TRAMPOLINE_PLACEMENT_BASE,
> + TRAMPOLINE_PLACEMENT_SIZE, 0);
>
> #ifdef CONFIG_64BIT
> if (image_base != (unsigned long)startup_32)
> - adjust_memory_range_protection(image_base, image_size);
> + efi_adjust_memory_range_protection(image_base, image_size, 0);
> #else
> /*
> * Clear protection flags on a whole range of possible
> @@ -306,8 +250,9 @@ setup_memory_protection(unsigned long image_base, unsigned long image_size)
> * need to remove possible protection on relocated image
> * itself disregarding further relocations.
> */
> - adjust_memory_range_protection(LOAD_PHYSICAL_ADDR,
> - KERNEL_IMAGE_SIZE - LOAD_PHYSICAL_ADDR);
> + efi_adjust_memory_range_protection(LOAD_PHYSICAL_ADDR,
> + KERNEL_IMAGE_SIZE - LOAD_PHYSICAL_ADDR,
> + 0);
> #endif
> }
>
> --
> 2.35.1
>
