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

[Naoya Horiguchi]

On Thu, Jul 06, 2017 at 05:13:32PM +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@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?

Thank you for the review, Chao.
And you're right, I'll add break here.

# I'll post revised one tomorrow waiting for more comments.

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

We can't easily cancel process_mem_region(), so if we process a few normal
regions like EFI_CONVENTIONAL_MEMORY and then find a EFI_MEMORY_MORE_RELIABLE
region, that's a bit troublesome.
So I decided to first check whether EFI_MEMORY_MORE_RELIABLE region exists or not.

Thanks,
Naoya Horiguchi