On Thu, Jul 06, 2017 at 09:22:38AM +0000, Naoya Horiguchi wrote: >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-horigu...@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-horigu...@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(®ion, 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.
OK, I got it. Thanks for your explanation. Thanks, Chao Fan > >Thanks, >Naoya Horiguchi > >
