Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752115AbdGFJhN convert rfc822-to-8bit (ORCPT ); Thu, 6 Jul 2017 05:37:13 -0400 Received: from tyo161.gate.nec.co.jp ([114.179.232.161]:38878 "EHLO tyo161.gate.nec.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751011AbdGFJhL (ORCPT ); Thu, 6 Jul 2017 05:37:11 -0400 From: Naoya Horiguchi To: Baoquan He CC: Kees Cook , LKML , "x86@kernel.org" , Thomas Gleixner , "H. Peter Anvin" , Ingo Molnar , "izumi.taku@jp.fujitsu.com" , Thomas Garnier , "fanc.fnst@cn.fujitsu.com" , Matt Fleming , Junichi Nomura Subject: Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice Thread-Topic: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice Thread-Index: AQHS9jI2mWWdXsD8kkSmPgVylBsaHqJF7kyAgAAFIIA= Date: Thu, 6 Jul 2017 09:36:29 +0000 Message-ID: <20170706093629.GB24491@hori1.linux.bs1.fc.nec.co.jp> References: <20170706083106.GA21796@hori1.linux.bs1.fc.nec.co.jp> <20170706091809.GI19994@x1> In-Reply-To: <20170706091809.GI19994@x1> Accept-Language: en-US, ja-JP Content-Language: ja-JP X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.128.101.10] Content-Type: text/plain; charset="iso-2022-jp" Content-ID: <506B4724EA7445458708F9BC9FAFC4E9@gisp.nec.co.jp> Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 X-TM-AS-MML: disable Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6190 Lines: 159 On Thu, Jul 06, 2017 at 05:18:09PM +0800, Baoquan He wrote: > 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. Thank you for the positive response, Baoquan. > > 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 > > 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 > > --- > > 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. OK. efi_mirror_found need not be a global variable any longer. > > + } > > + > > + 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; I intented that this is represented by the return value of process_efi_entry(), but if you have more readable/clearer options, I'm fine with that. > > > And yes, I missed this snippet of code to break loop if slot_area has > been full, this saves time. No problem, feel free to move this snippet to your 2/2 patch. Thanks, Naoya Horiguchi