From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754005AbdF0Xdd (ORCPT ); Tue, 27 Jun 2017 19:33:33 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40956 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753935AbdF0XdY (ORCPT ); Tue, 27 Jun 2017 19:33:24 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 566EC4E03F Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=bhe@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 566EC4E03F Date: Wed, 28 Jun 2017 07:33:20 +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: <20170627233320.GB15537@x1> References: <1498314309-18502-1-git-send-email-bhe@redhat.com> <20170627232412.GA15537@x1> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170627232412.GA15537@x1> 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.38]); Tue, 27 Jun 2017 23:33:24 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 06/28/17 at 07:24am, Baoquan He wrote: > 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, randomization wont' be impacted. So below change you ~~^ of the other one > 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