From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751725AbdG1GqU (ORCPT ); Fri, 28 Jul 2017 02:46:20 -0400 Received: from mail-wm0-f68.google.com ([74.125.82.68]:35535 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751659AbdG1GqS (ORCPT ); Fri, 28 Jul 2017 02:46:18 -0400 Date: Fri, 28 Jul 2017 08:46:14 +0200 From: Ingo Molnar To: Baoquan He Cc: linux-kernel@vger.kernel.org, keescook@chromium.org, matt@codeblueprint.co.uk, tglx@linutronix.de, hpa@zytor.com, izumi.taku@jp.fujitsu.com, fanc.fnst@cn.fujitsu.com, thgarnie@google.com, n-horiguchi@ah.jp.nec.com, dyoung@redhat.com Subject: Re: [PATCH v7] x86/boot/KASLR: Restrict kernel to be randomized in mirror regions Message-ID: <20170728064614.lcntuaovomcqwxx4@gmail.com> References: <1501041620-4965-1-git-send-email-bhe@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1501041620-4965-1-git-send-email-bhe@redhat.com> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Baoquan He wrote: > Currently KASLR will parse all e820 entries of RAM type and add all > candidate position into slots array. Then we will choose one slot > randomly as the new position which kernel will be decompressed into > and run at. > > On system with EFI enabled, e820 memory regions are coming from EFI > memory regions by combining adjacent regions. While these EFI memory > regions have more attributes to mark their different use. Mirror > attribute is such kind. The physical memory region whose descriptors > in EFI memory map has EFI_MEMORY_MORE_RELIABLE attribute (bit: 16) are > mirrored. The address range mirroring feature of kernel arranges such > mirror region into normal zone and other region into movable zone. And > with mirroring feature enabled, the code and date of kernel can only be > located in more reliable mirror region. However, the current KASLR code > doesn't check EFI memory entries, and could choose new position in > non-mirrored region. This will break the functionality of the address > range mirroring feature. > > So if EFI is detected, iterate EFI memory map and pick the mirror region > to process for adding candidate of randomization slot. If EFI is disabled > or no mirror region found, still process e820 memory map. > > Signed-off-by: Baoquan He > --- > v6->v7: > Ingo pointed out several incorrect line break issues and unclear > description of patch log. Correct them and rewrite patch log. > > And also rewrite the EFI warning message that if EFI memmap is above > 4G in 32bit system since 32bit system can not handle data above 4G at > kernel decompression stage. This is suggested by Ingo too. > > v5->v6: > Code style issue fix according to Kees's comment. > > This is based on tip/x86/boot, patch 1,2,3/4 in v5 post has > been put into tip/x86/boot now. > > arch/x86/boot/compressed/kaslr.c | 68 ++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 66 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c > index 99c7194f7ea6..e1dd59ea8a8f 100644 > --- a/arch/x86/boot/compressed/kaslr.c > +++ b/arch/x86/boot/compressed/kaslr.c > @@ -37,7 +37,9 @@ > #include > #include > #include > +#include > #include > +#include > > /* Macros used by the included decompressor code below. */ > #define STATIC > @@ -558,6 +560,65 @@ static void process_mem_region(struct mem_vector *entry, > } > } > > +#ifdef CONFIG_EFI > +/* > + * Returns true if mirror region found (and must have been processed > + * for slots adding) > + */ > +static bool > +process_efi_entries(unsigned long minimum, unsigned long image_size) > +{ > + struct efi_info *e = &boot_params->efi_info; > + bool efi_mirror_found = false; > + struct mem_vector region; > + efi_memory_desc_t *md; > + unsigned long pmap; > + char *signature; > + u32 nr_desc; > + int i; > + > + signature = (char *)&e->efi_loader_signature; > + if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) && > + strncmp(signature, EFI64_LOADER_SIGNATURE, 4)) > + return false; > + > +#ifdef CONFIG_X86_32 > + /* Can't handle data above 4GB at this time */ > + if (e->efi_memmap_hi) { > + warn("EFI memmap is above 4GB, can't be handled now on x86_32. EFI should be disabled.\n"); > + return false; > + } > + pmap = e->efi_memmap; > +#else > + pmap = (e->efi_memmap | ((__u64)e->efi_memmap_hi << 32)); > +#endif > + > + 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); > + efi_mirror_found = true; > + > + if (slot_area_index == MAX_SLOT_AREA) { > + debug_putstr("Aborted EFI scan (slot_areas full)!\n"); > + break; > + } > + } > + } So I suggested this before: if you treat 'md' as an array of elements (which it is), then the type cast can be moved to a more natural point, where we do address calculations anyway: md = (efi_memory_desc_t *)(e->efi_memmap | ((__u64)e->efi_memmap_hi << 32))); The 'pmap' variable can be eliminated and all places in the loop that use 'md->' can use 'md[i].'. Thanks, Ingo