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

[Chao Fan <fanc.fnst@cn.fujitsu.com>]

On Thu, Jul 06, 2017 at 06:04:46PM +0800, Chao Fan wrote:
>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@ah.jp.nec.com>
>>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@ah.jp.nec.com>
>>---
>> 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,
>
>Sorry for one more suggestion,
>How about add:
>	if (!efi_mirror_found)
>		return false;
>at this place, between the two cycles.
>
>Because if there are no mirror regions found, I think we can return
>directly and go to walk the e820 entries.
>I don't know whether my understanding is right.

Sorry for disturbing, it's my misunderstanding.
My suggestion is useless.

Since efi entries have been walked, we can use them directly,
no need to walk e820 entries again.

The logic of your codes is good.
Sorry again.

Thanks,
Chao Fan

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