Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice

From: Chao Fan
Date: Thu Jul 06 2017 - 05:14:04 EST


On Thu, Jul 06, 2017 at 08:31:07AM +0000, Naoya Horiguchi wrote:
>Hi Baoquan, everyone,
>
>I'm also interested in KASLR/EFI related issue (but not the same issue
>with yours, so I separated the thread.)
>
>This patch is based on Baoquan's recent patches[1], adding more code
>on the new function process_efi_entry().
>If it's OK, could you queue this onto your tree/series?
>
>[1] "[PATCH v3 0/2] x86/boot/KASLR: Restrict kernel to be randomized"
> https://lkml.org/lkml/2017/7/5/98
>
>Thanks,
>Naoya Horiguchi
>---
>From: Naoya Horiguchi <n-horiguchi@xxxxxxxxxxxxx>
>Date: Thu, 6 Jul 2017 16:40:52 +0900
>Subject: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from
> KASLR's choice
>
>KASLR chooses kernel location from E820_TYPE_RAM regions by walking over
>e820 entries now. E820_TYPE_RAM includes EFI_BOOT_SERVICES_CODE and
>EFI_BOOT_SERVICES_DATA, so those regions can be the target. According to
>UEFI spec, all memory regions marked as EfiBootServicesCode and
>EfiBootServicesData are available for free memory after the first call
>of ExitBootServices(). So such regions should be usable for kernel on
>spec basis.
>
>In x86, however, we have some workaround for broken firmware, where we
>keep such regions reserved until SetVirtualAddressMap() is done.
>See the following code in should_map_region():
>
> static bool should_map_region(efi_memory_desc_t *md)
> {
> ...
> /*
> * Map boot services regions as a workaround for buggy
> * firmware that accesses them even when they shouldn't.
> *
> * See efi_{reserve,free}_boot_services().
> */
> if (md->type == EFI_BOOT_SERVICES_CODE ||
> md->type == EFI_BOOT_SERVICES_DATA)
> return false;
>
>This workaround suppressed a boot crash, but potential issues still
>remain because no one prevents the regions from overlapping with kernel
>image by KASLR.
>
>So let's make sure that EFI_BOOT_SERVICES_{CODE|DATA} regions are never
>chosen as kernel memory for the workaround to work fine.
>
>Signed-off-by: Naoya Horiguchi <n-horiguchi@xxxxxxxxxxxxx>
>---
> arch/x86/boot/compressed/kaslr.c | 41 +++++++++++++++++++++++++++++++---------
> 1 file changed, 32 insertions(+), 9 deletions(-)
>
>diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>index 94f08fd375ae..f43fed0441a6 100644
>--- a/arch/x86/boot/compressed/kaslr.c
>+++ b/arch/x86/boot/compressed/kaslr.c
>@@ -563,7 +563,8 @@ static void process_mem_region(struct mem_vector *entry,
> /* Marks if efi mirror regions have been found and handled. */
> static bool efi_mirror_found;
>
>-static void process_efi_entry(unsigned long minimum, unsigned long image_size)
>+/* Returns true if we really enter efi memmap walk, otherwise returns false. */
>+static bool process_efi_entry(unsigned long minimum, unsigned long image_size)
> {
> struct efi_info *e = &boot_params->efi_info;
> struct mem_vector region;
>@@ -577,13 +578,13 @@ static void process_efi_entry(unsigned long minimum, unsigned long image_size)
> signature = (char *)&boot_params->efi_info.efi_loader_signature;
> if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) &&
> strncmp(signature, EFI64_LOADER_SIGNATURE, 4))
>- return;
>+ return false;
>
> #ifdef CONFIG_X86_32
> /* Can't handle data above 4GB at this time */
> if (e->efi_memmap_hi) {
> warn("Memory map is above 4GB, EFI should be disabled.\n");
>- return;
>+ return false;
> }
> pmap = e->efi_memmap;
> #else
>@@ -593,13 +594,36 @@ static void process_efi_entry(unsigned long minimum, unsigned long image_size)
> nr_desc = e->efi_memmap_size / e->efi_memdesc_size;
> for (i = 0; i < nr_desc; i++) {
> md = (efi_memory_desc_t *)(pmap + (i * e->efi_memdesc_size));
>- if (md->attribute & EFI_MEMORY_MORE_RELIABLE) {
>- region.start = md->phys_addr;
>- region.size = md->num_pages << EFI_PAGE_SHIFT;
>- process_mem_region(&region, minimum, image_size);
>+ if (md->attribute & EFI_MEMORY_MORE_RELIABLE)
> efi_mirror_found = true;

Hi Horiguchi-san,

If efi_mirror_found is changed to be true, we won't need to walk other
entries, so I think:
if (md->attribute & EFI_MEMORY_MORE_RELIABLE) {
efi_mirror_found = true;
break;
}
will be enough to show that mirror regions exist. And will walk
less entries. How do you think about this?

Another question: what's the benifit of putting this part of
"efi_mirror_found = true" to a independent cycle.

Thanks,
Chao Fan

>+ }
>+
>+ for (i = 0; i < nr_desc; i++) {
>+ md = (efi_memory_desc_t *)(pmap + (i * e->efi_memdesc_size));
>+
>+ /*
>+ * EFI_BOOT_SERVICES_{CODE|DATA} are avoided because boot
>+ * services regions could be accessed after ExitBootServices()
>+ * due to the workaround for buggy firmware.
>+ */
>+ if (!(md->type == EFI_LOADER_CODE ||
>+ md->type == EFI_LOADER_DATA ||
>+ md->type == EFI_CONVENTIONAL_MEMORY))
>+ continue;
>+
>+ if (efi_mirror_found &&
>+ !(md->attribute & EFI_MEMORY_MORE_RELIABLE))
>+ continue;
>+
>+ region.start = md->phys_addr;
>+ region.size = md->num_pages << EFI_PAGE_SHIFT;
>+ process_mem_region(&region, minimum, image_size);
>+ if (slot_area_index == MAX_SLOT_AREA) {
>+ debug_putstr("Aborted EFI scan (slot_areas full)!\n");
>+ break;
> }
> }
>+ return true;
> }
>
> static void process_e820_entry(unsigned long minimum, unsigned long image_size)
>@@ -637,8 +661,7 @@ static unsigned long find_random_phys_addr(unsigned long minimum,
> minimum = ALIGN(minimum, CONFIG_PHYSICAL_ALIGN);
>
> #ifdef CONFIG_EFI
>- process_efi_entry(minimum, image_size);
>- if (efi_mirror_found)
>+ if (process_efi_entry(minimum, image_size))
> return slots_fetch_random();
> #endif
>
>--
>2.7.4
>
>
>