From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754038AbdF0XYW (ORCPT ); Tue, 27 Jun 2017 19:24:22 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44190 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752792AbdF0XYQ (ORCPT ); Tue, 27 Jun 2017 19:24:16 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 49F4361BAA Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=bhe@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 49F4361BAA Date: Wed, 28 Jun 2017 07:24:12 +0800 From: Baoquan He To: Kees Cook Cc: LKML , "H. Peter Anvin" , Thomas Gleixner , Ingo Molnar , "x86@kernel.org" , "Kees Cook , Arnd Bergmann , Thomas Garnier Subject: Re: [PATCH] x86/boot/KASLR: Skip relocation handling in no kaslr case Message-ID: <20170627232412.GA15537@x1> References: <1498314309-18502-1-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.39]); Tue, 27 Jun 2017 23:24:16 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Kees, On 06/27/17 at 03:42pm, Kees Cook wrote: > On Sat, Jun 24, 2017 at 7:25 AM, Baoquan He wrote: > > Kdump kernel will reset to firmware after crash is trigered when > > crashkernel=xxM,high is added to kernel command line. Kexec has the > > same phenomenon. This only happened on system with kaslr code > > compiled in and kernel option 'nokaslr'is added. Both of them works > > well when kaslr is enabled. > > > > When crashkernel high is set or kexec case, kexec/kdump kernel will be > > put above 4G. Since we assign the original loading address of kernel to > > virt_addr as initial value, the virt_addr will be larger than 1G if kaslr > > is disabled, it exceeds the kernel mapping size which is only 1G. Then > > it will cause relocation handling error in handle_relocations(). > > > > In fact this is a known issue and fixed in commit: > > > > ... f285f4a ("x86, boot: Skip relocs when load address unchanged") > > > > But above fix was lost carelessly in later commit: > > > > ... 8391c73 ("x86/KASLR: Randomize virtual address separately") > > Hrm, this changes more than just fixing this missing logic. How about > just simply: Below code was added to fix the kexec/kdump kernel with kaslr disabled, at that time kernel kaslr physical address and virtual address randomization are coupled. What it was doing is to randomize physical address in 1G range and add the delta onto the starting address of virtual address, 0xffffffff80000000. But now the physical and virtual address randomization has been separated. It means that whether each of them is changed or not randomly, the randomization wont' be impacted. So below change you provided will has two problems: 1st, the 'virt_addr' represents the offset of virtual address randomization between 0xffffffff80000000 and 0xffffffffc0000000, should not get a initial value '(unsigned long)output'. The 'output' could be any value between 16M and 64T. It makes no sense to make the assignment and could bring confusion to code readers. 2nd, for x86 64, we do the relocation handling if and only if virtual address is randomized to a different value, namely the offset 'virt_addr' has a value which is not 16M. You can see this in handle_relocations(). if (IS_ENABLED(CONFIG_X86_64)) delta = virt_addr - LOAD_PHYSICAL_ADDR; if (!delta) { debug_putstr("No relocation needed... "); return; } Now below code will bring a bug that if physical address randomization fail to get a different value, but virtual address randomization may succeed to get one, then it won't do relocation handling. This contradicts with the design and implementation of the current code. > > diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c > index b3c5a5f030ce..4496a05d1f8a 100644 > --- a/arch/x86/boot/compressed/misc.c > +++ b/arch/x86/boot/compressed/misc.c > @@ -339,6 +339,7 @@ asmlinkage __visible void *extract_kernel(void > *rmode, memptr heap, > { > const unsigned long kernel_total_size = VO__end - VO__text; > unsigned long virt_addr = (unsigned long)output; > + unsigned long virt_addr_orig = virt_addr; > > /* Retain x86 boot parameters pointer passed from startup_32/64. */ > boot_params = rmode; > @@ -405,7 +406,12 @@ asmlinkage __visible void *extract_kernel(void > *rmode, memptr heap, > __decompress(input_data, input_len, NULL, NULL, output, output_len, > NULL, error); > parse_elf(output); > - handle_relocations(output, output_len, virt_addr); > + /* > + * 32-bit always performs relocations. 64-bit relocations are only > + * needed if kASLR has chosen a different load address. > + */ > + if (!IS_ENABLED(CONFIG_X86_64) || virt_addr != virt_addr_orig) > + handle_relocations(output, output_len, virt_addr); > debug_putstr("done.\nBooting the kernel.\n"); > return output; > } > > > diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c > > index b3c5a5f0..c945acd 100644 > > --- a/arch/x86/boot/compressed/misc.c > > +++ b/arch/x86/boot/compressed/misc.c > > @@ -338,7 +338,7 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap, > > unsigned long output_len) > > { > > const unsigned long kernel_total_size = VO__end - VO__text; > > - unsigned long virt_addr = (unsigned long)output; > > + unsigned long virt_addr = LOAD_PHYSICAL_ADDR; > > I don't like this being hard-coded. The logic for the output address > is already handled in the .S files, and may not be LOAD_PHYSICAL_ADDR. > I'd prefer just adding back the simple test of virt_addr changing. Do you mean the handling in boot/compressed/head_64.S? Whatever it does, it's only for physical address. The virtual address mapping is not touched. Here virt_addr respresents the offset between 0xffffffff80000000, 0xffffffffc0000000. And if skip the relocation handling, we can see that the '_text' will be mapped to 0xffffffff81000000 forever, no matter where physical address of '_text' is. So here LOAD_PHYSICAL_ADDR is default value of 'virt_addr'. > > > > /* Retain x86 boot parameters pointer passed from startup_32/64. */ > > boot_params = rmode; > > @@ -397,7 +397,7 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap, > > #ifndef CONFIG_RELOCATABLE > > if ((unsigned long)output != LOAD_PHYSICAL_ADDR) > > error("Destination address does not match LOAD_PHYSICAL_ADDR"); > > - if ((unsigned long)output != virt_addr) > > + if (virt_addr != LOAD_PHYSICAL_ADDR) > > error("Destination virtual address changed when not relocatable"); > > #endif > > > > diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h > > index 1c8355e..766a521 100644 > > --- a/arch/x86/boot/compressed/misc.h > > +++ b/arch/x86/boot/compressed/misc.h > > @@ -81,8 +81,6 @@ static inline void choose_random_location(unsigned long input, > > unsigned long output_size, > > unsigned long *virt_addr) > > { > > - /* No change from existing output location. */ > > - *virt_addr = *output; > > } > > #endif > > > > -- > > 2.5.5 > > > > > > -- > Kees Cook > Pixel Security