From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752725AbdGFBVG (ORCPT ); Wed, 5 Jul 2017 21:21:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44170 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752440AbdGFBVF (ORCPT ); Wed, 5 Jul 2017 21:21:05 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com AD9AB74870 Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=bhe@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com AD9AB74870 Date: Thu, 6 Jul 2017 09:21:01 +0800 From: Baoquan He To: Kees Cook Cc: LKML , "x86@kernel.org" , "H. Peter Anvin" , Thomas Gleixner , Ingo Molnar , Thomas Garnier , caoj.fnst@cn.fujitsu.com, izumi.taku@jp.fujitsu.com Subject: Re: [PATCH v2 1/2] x86/boot/KASLR: Adapt process_e820_entry for any type of memory entry Message-ID: <20170706012101.GH19994@x1> References: <1499155442-17467-1-git-send-email-bhe@redhat.com> <1499155442-17467-2-git-send-email-bhe@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.7.0 (2016-08-17) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Thu, 06 Jul 2017 01:21:04 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07/05/17 at 03:06pm, Kees Cook wrote: > On Tue, Jul 4, 2017 at 1:04 AM, Baoquan He wrote: > > Now function process_e820_entry is only used to process e820 memory > > entries. Adapt it for any type of memory entry, not just for e820. > > Later we will use it to process efi mirror regions. > > > > So rename the old process_e820_entry to process_mem_region, and > > extract and wrap the e820 specific processing code into process_e820_entry. > > This mixes changing the structure internals (.addr vs .start) with > changing the processing logic (adding a wrapper). I would prefer this > was done in several stages to make review easier: OK, will do following below steps. Thanks! > > 1) lift e820 walking into a new function process_e820_entries(), and > move TYPE_RAM logic there. > > 2) switch process_e820_entry() from struct boot_e820_entry to struct mem_vector. > > 3) rename (and adjust comments!) of process_e820_entry() into > process_mem_region(). > > -Kees > > > > > Signed-off-by: Baoquan He > > --- > > arch/x86/boot/compressed/kaslr.c | 60 ++++++++++++++++++++++------------------ > > 1 file changed, 33 insertions(+), 27 deletions(-) > > > > diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c > > index 91f27ab970ef..85c360eec4a6 100644 > > --- a/arch/x86/boot/compressed/kaslr.c > > +++ b/arch/x86/boot/compressed/kaslr.c > > @@ -479,35 +479,31 @@ static unsigned long slots_fetch_random(void) > > return 0; > > } > > > > -static void process_e820_entry(struct boot_e820_entry *entry, > > +static void process_mem_region(struct mem_vector *entry, > > unsigned long minimum, > > unsigned long image_size) > > { > > struct mem_vector region, overlap; > > struct slot_area slot_area; > > unsigned long start_orig, end; > > - struct boot_e820_entry cur_entry; > > - > > - /* Skip non-RAM entries. */ > > - if (entry->type != E820_TYPE_RAM) > > - return; > > + struct mem_vector cur_entry; > > > > /* On 32-bit, ignore entries entirely above our maximum. */ > > - if (IS_ENABLED(CONFIG_X86_32) && entry->addr >= KERNEL_IMAGE_SIZE) > > + if (IS_ENABLED(CONFIG_X86_32) && entry->start >= KERNEL_IMAGE_SIZE) > > return; > > > > /* Ignore entries entirely below our minimum. */ > > - if (entry->addr + entry->size < minimum) > > + if (entry->start + entry->size < minimum) > > return; > > > > /* Ignore entries above memory limit */ > > - end = min(entry->size + entry->addr, mem_limit); > > - if (entry->addr >= end) > > + end = min(entry->size + entry->start, mem_limit); > > + if (entry->start >= end) > > return; > > - cur_entry.addr = entry->addr; > > - cur_entry.size = end - entry->addr; > > + cur_entry.start = entry->start; > > + cur_entry.size = end - entry->start; > > > > - region.start = cur_entry.addr; > > + region.start = cur_entry.start; > > region.size = cur_entry.size; > > > > /* Give up if slot area array is full. */ > > @@ -522,7 +518,7 @@ static void process_e820_entry(struct boot_e820_entry *entry, > > region.start = ALIGN(region.start, CONFIG_PHYSICAL_ALIGN); > > > > /* Did we raise the address above this e820 region? */ > > - if (region.start > cur_entry.addr + cur_entry.size) > > + if (region.start > cur_entry.start + cur_entry.size) > > return; > > > > /* Reduce size by any delta from the original address. */ > > @@ -562,12 +558,31 @@ static void process_e820_entry(struct boot_e820_entry *entry, > > } > > } > > > > -static unsigned long find_random_phys_addr(unsigned long minimum, > > - unsigned long image_size) > > +static void process_e820_entry(unsigned long minimum, unsigned long image_size) > > { > > int i; > > - unsigned long addr; > > + struct mem_vector region; > > + struct boot_e820_entry *entry; > > + > > + /* Verify potential e820 positions, appending to slots list. */ > > + for (i = 0; i < boot_params->e820_entries; i++) { > > + entry = &boot_params->e820_table[i]; > > + /* Skip non-RAM entries. */ > > + if (entry->type != E820_TYPE_RAM) > > + continue; > > + region.start = entry->addr; > > + region.size = entry->size; > > + process_mem_region(®ion, minimum, image_size); > > + if (slot_area_index == MAX_SLOT_AREA) { > > + debug_putstr("Aborted e820 scan (slot_areas full)!\n"); > > + break; > > + } > > + } > > +} > > > > +static unsigned long find_random_phys_addr(unsigned long minimum, > > + unsigned long image_size) > > +{ > > /* Check if we had too many memmaps. */ > > if (memmap_too_large) { > > debug_putstr("Aborted e820 scan (more than 4 memmap= args)!\n"); > > @@ -577,16 +592,7 @@ static unsigned long find_random_phys_addr(unsigned long minimum, > > /* Make sure minimum is aligned. */ > > minimum = ALIGN(minimum, CONFIG_PHYSICAL_ALIGN); > > > > - /* Verify potential e820 positions, appending to slots list. */ > > - for (i = 0; i < boot_params->e820_entries; i++) { > > - process_e820_entry(&boot_params->e820_table[i], minimum, > > - image_size); > > - if (slot_area_index == MAX_SLOT_AREA) { > > - debug_putstr("Aborted e820 scan (slot_areas full)!\n"); > > - break; > > - } > > - } > > - > > + process_e820_entry(minimum, image_size); > > return slots_fetch_random(); > > } > > > > -- > > 2.5.5 > > > > > > -- > Kees Cook > Pixel Security