Hi Naoya Horiguchi, Thanks for making this!
On 07/06/17 at 08:31am, 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? This is interesting. So you are suggesting that we should try to avoid those EFI_BOOT_SERVICES_{CODE|DATA} efi regions as long as efi map regions are available, meanwhile try to locate kernel inside mirrored regions if existed. I do know the efi work around, so it seems reasonable to me, I can add it when repost. Or you can post after mine has been merged. A little adjustment, please see the inline comment. > > [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; Here, we should define a local variable of bool type to mark if mirrored region is found. > + } > + > + 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(®ion, minimum, image_size); And can define a new global variable like efi_processed here. efi_processed = true; And yes, I missed this snippet of code to break loop if slot_area has been full, this saves time. > + 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 >