lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 6 Jul 2017 21:28:27 +0800 From: Baoquan He <bhe@...hat.com> To: Kees Cook <keescook@...omium.org> Cc: LKML <linux-kernel@...r.kernel.org>, "H. Peter Anvin" <hpa@...or.com>, Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, "x86@...nel.org" <x86@...nel.org>, "Kees Cook <keescook@...omium.org, Yinghai Lu" <yinghai@...nel.org>, Arnd Bergmann <arnd@...db.de>, Thomas Garnier <thgarnie@...gle.com> Subject: Re: [PATCH] x86/boot/KASLR: Skip relocation handling in no kaslr case On 07/05/17 at 12:06pm, Kees Cook wrote: > On Tue, Jun 27, 2017 at 4:24 PM, Baoquan He <bhe@...hat.com> wrote: > > 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: > > Well, it's only sometimes separated. When it's still tied together, > the (identity-mapped) physical randomization _is_ the virtual > randomization. I am a little confused about this. I can't think of a case that in x86 64 virtual address and physical address need be tied together to do randomization. And in what conditions they are separated, could you give cases with a detailed address value? But in x86 32bit, it does couple physical and virtual address randomization together. Because it just randomizes to get an value between 16M and 512M, then use the delta between 16M and the random value to do the relocation handling for virtual address. Just write my understanding at below, please add comment or point out mistakes of mine. > > > 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; > > } > > On x86_64, the kernel is built so that if the virt address matches > LOAD_PHYSICAL_ADDR, we don't have to do relocations. Agreed. > > > 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. > > Let's address this specific issue (below). > > > > >> > >> 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'. > > So, this isn't always true. The output address is the address assigned > by head_*.S for the extraction destination. We can't just change that, > since it's based on where to perform the extraction, etc. All the > logic in there is designed to make sure we're actually >= > LOAD_PHYSICAL_ADDR: > > cmpl $LOAD_PHYSICAL_ADDR, %ebx > jge 1f > #endif > movl $LOAD_PHYSICAL_ADDR, %ebx > 1: Yes, here it's trying to do an adjustment. To align the original loading address if it's not aligned to CONFIG_PHYSICAL_ALIGN, and to make it be LOAD_PHYSICAL_ADDR if it's smaller than LOAD_PHYSICAL_ADDR. Other than those, nothing else be done. So let me conclude the possible original 'output' value: 1) default case It's LOAD_PHYSICAL_ADDR Which is the default loading address. 2) kexec/kdump The output will be at the top of the available physical ram. 3) Modified bootloader Which could put kernel anywhere by modifying code of bootloader. This is the case that we need do above alignment and adjustment to avoid unpleasant kernel error. Except of above 3 cases, physical address of kernel could be changed by physical address randomization. 4) physical address randomization However, for virtual address, there are only two chances to decide: 1) arch/x86/kernel/vmlinux.lds.S we just map .text at __START_KERNEL #define __PHYSICAL_START ALIGN(CONFIG_PHYSICAL_START, \ CONFIG_PHYSICAL_ALIGN) #define __START_KERNEL (__START_KERNEL_map + __PHYSICAL_START) 2) handle_relocations :: boot/compressed/misc.c In handle_relocations(), we DO pass in the physical 'output', but that is used to calculate 'map' which is used to position those memory unit where the referenced address need be relocated. So later, in arch/x86/kernel/head_64.S, we will store the delta which is between 16M and the actual kernel loading address into phys_base. So as long as we can translate physical address and virtual address of kernel by below formula, everything is going very well. y = x - __START_KERNEL_map + phys_base. We really don't need to tie physical address and virtual address of kernel together in any cases. They can be randomized or not randomized completely separately. As long as we keep above translation formula working well, everything need not be worried. > > We can't undo that since the calculation performed there is critical > to getting it into the right place. > > >> > > >> > /* 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 > > So, the code in question is this: > > static void handle_relocations(void *output, unsigned long output_len, > unsigned long virt_addr) > { > int *reloc; > unsigned long delta, map, ptr; > unsigned long min_addr = (unsigned long)output; > unsigned long max_addr = min_addr + (VO___bss_start - VO__text); > > /* > * Calculate the delta between where vmlinux was linked to load > * and where it was actually loaded. > */ > delta = min_addr - LOAD_PHYSICAL_ADDR; > > /* > * The kernel contains a table of relocation addresses. Those > * addresses have the final load address of the kernel in virtual > * memory. We are currently working in the self map. So we need to > * create an adjustment for kernel memory addresses to the self map. > * This will involve subtracting out the base address of the kernel. > */ > map = delta - __START_KERNEL_map; > > /* > * 32-bit always performs relocations. 64-bit relocations are only > * needed if KASLR has chosen a different starting address offset > * from __START_KERNEL_map. > */ > if (IS_ENABLED(CONFIG_X86_64)) > delta = virt_addr - LOAD_PHYSICAL_ADDR; > > if (!delta) { > debug_putstr("No relocation needed... "); > return; > } > debug_putstr("Performing relocations... "); > > The first use of delta is to figure out the correct relocation offset, > which is based on the relocation tables, etc. Yes > > The second use is for decoupled phys/virt (x86_64 only), but I don't > think these need to be separate, do they? I think "delta" should only > ever operate on the virt_addr, yes? Yes. And if yes, for x86_64, there's nothing related to physical address, here physical address is only used to find out the ram unit where relocation need be done. > > in misc.c, this chooses a physical location and a virtual: > > choose_random_location((unsigned long)input_data, input_len, > (unsigned long *)&output, > max(output_len, kernel_total_size), > &virt_addr); > > Then we place the kernel in the physical location: > > __decompress(input_data, input_len, NULL, NULL, output, output_len, > NULL, error); > > And finally handle relocations: > > handle_relocations(output, output_len, virt_addr); > > "output" is the identity-mapped physical (and possibly virtual) > location, and virt_addr is the virtual. No, output here is physical address, and yes., it's also virtual address which is identity mappped. But when we do the relocation handling, we can only think it as physical address. Here it's virtual only because we need to operate on ram unit under protection mode with paging enabled. > > So, it looks to me like the calculations at the top of > handle_relocations() may not be correct for the case you're hitting? > > What are the values there in that case? We must not hard-code output, > so handle_relocations() has to be entirely self-contained with its > logic. Well, I may get a little bit what confusion you might have. First of all, that 'virt_addr' is not the real virtual address, it's just an offset in [0xffffffff80000000], 0xffffffffc0000000). Its value range is between 16M and 1G. And for physical address 'output', we need avoid many things, initramfs, kernel cmdline, etc. While for virt_addr, we don't need to avoid anything, because it's just an offset and we can map kernel from any places to 'virt_addr'. And secondly, hard-coding 'virt_add' is only hard-coding the virtual randomzation offset, it won't impact 'output' at all. 'ouput' doesn't rely on and isn't related to 'virt_addr' at all, for x86 64. > > I actually think the bug might be that choose_random_location() in the > nokaslr case doesn't explicitly set virt_addr to output, like the > no-op does: > > void choose_random_location(unsigned long input, > unsigned long input_size, > unsigned long *output, > unsigned long output_size, > unsigned long *virt_addr) > { > unsigned long random_addr, min_addr; > > if (cmdline_find_option_bool("nokaslr")) { > warn("KASLR disabled: 'nokaslr' on cmdline."); > return; > } > > Perhaps we need a few fixes: > > diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c > index 91f27ab970ef..e1e57c626093 100644 > --- a/arch/x86/boot/compressed/kaslr.c > +++ b/arch/x86/boot/compressed/kaslr.c > @@ -627,6 +627,7 @@ void choose_random_location(unsigned long input, > > if (cmdline_find_option_bool("nokaslr")) { > warn("KASLR disabled: 'nokaslr' on cmdline."); > + *virt_addr = *output; > return; > } > > diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c > index 00241c815524..3eb754e4f823 100644 > --- a/arch/x86/boot/compressed/misc.c > +++ b/arch/x86/boot/compressed/misc.c > @@ -198,12 +198,13 @@ static void handle_relocations(void *output, > unsigned long output_len, > * needed if KASLR has chosen a different starting address offset > * from __START_KERNEL_map. > */ > - if (IS_ENABLED(CONFIG_X86_64)) > + if (IS_ENABLED(CONFIG_X86_64)) { > delta = virt_addr - LOAD_PHYSICAL_ADDR; > > - if (!delta) { > - debug_putstr("No relocation needed... "); > - return; > + if (!delta) { > + debug_putstr("No relocation needed... "); > + return; > + } > } > debug_putstr("Performing relocations... "); > > > > > > -- > Kees Cook > Pixel Security
Powered by blists - more mailing lists