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

[Chao Fan]

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