Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751702AbcDOTBJ (ORCPT ); Fri, 15 Apr 2016 15:01:09 -0400 Received: from mail-wm0-f52.google.com ([74.125.82.52]:36096 "EHLO mail-wm0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751333AbcDOTBG (ORCPT ); Fri, 15 Apr 2016 15:01:06 -0400 MIME-Version: 1.0 In-Reply-To: <20160415074726.GC30715@gmail.com> References: <1460672954-32567-1-git-send-email-keescook@chromium.org> <1460672954-32567-3-git-send-email-keescook@chromium.org> <20160415074726.GC30715@gmail.com> Date: Fri, 15 Apr 2016 12:01:03 -0700 X-Google-Sender-Auth: lv04gXXv8jDFZIm-0C6xflDkdsg Message-ID: Subject: Re: [PATCH v5 02/21] x86, KASLR: Handle kernel relocation above 2G From: Kees Cook To: Ingo Molnar Cc: Baoquan He , Yinghai Lu , Ard Biesheuvel , Matt Redfearn , "x86@kernel.org" , "H. Peter Anvin" , Ingo Molnar , Borislav Petkov , Vivek Goyal , Andy Lutomirski , lasse.collin@tukaani.org, Andrew Morton , Dave Young , "kernel-hardening@lists.openwall.com" , LKML , Linus Torvalds , Thomas Gleixner Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6543 Lines: 165 On Fri, Apr 15, 2016 at 12:47 AM, Ingo Molnar wrote: > > * Kees Cook wrote: > >> From: Baoquan He >> >> When processing the relocation table, the offset used to calculate the >> relocation is an int. This is sufficient for calculating the physical >> address of the relocs entry on 32-bit systems and on 64-bit systems when >> the relocation is under 2G. To handle relocations above 2G (seen in >> situations like kexec, netboot, etc), this offset needs to be calculated >> using a long to avoid wrapping and miscalculating the relocation. >> >> Signed-off-by: Baoquan He >> [kees: rewrote changelog] >> Signed-off-by: Kees Cook >> --- >> arch/x86/boot/compressed/misc.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c >> index f35ad9eb1bf1..c4477d5f3fff 100644 >> --- a/arch/x86/boot/compressed/misc.c >> +++ b/arch/x86/boot/compressed/misc.c >> @@ -295,7 +295,7 @@ static void handle_relocations(void *output, unsigned long output_len) >> * So we work backwards from the end of the decompressed image. >> */ >> for (reloc = output + output_len - sizeof(*reloc); *reloc; reloc--) { >> - int extended = *reloc; >> + long extended = *reloc; >> extended += map; >> >> ptr = (unsigned long)extended; > > This patch and the code it patches is just plain sloppy. See this cast? This is an > object lesson of why type casts in C are actively dangerous, they have hidden the > 32-bit truncation bug you've fixed with this patch. > > But the primary bug, the cast, should be fixed! Together with all the other casts > of 'extended'. In my defense, there's a ton of mixing of pointers vs unsigned longs all through-out the boot code. And relocations are special since they're explicitly designed to be sign-extended, etc. I'll clean things up as best as I can. > > Also, the boot code should be reviewed for unnecessary casts, it seems to be a > disease: > > triton:~/tip> git grep -cE '\(unsigned.*;$' arch/x86/boot/compressed/ > arch/x86/boot/compressed/aslr.c:14 > arch/x86/boot/compressed/eboot.c:41 > arch/x86/boot/compressed/misc.c:5 > arch/x86/boot/compressed/misc.h:2 > arch/x86/boot/compressed/mkpiggy.c:1 > arch/x86/boot/compressed/string.c:4 > > For example the type dance and overloaded usage that choose_kernel_location() does > with the 'random' local variable in aslr.c is disgusting: > > void choose_kernel_location(unsigned char *input, > unsigned long input_size, > unsigned char **output, > unsigned long output_size, > unsigned char **virt_offset) > { > unsigned long random, min_addr; > > *virt_offset = (unsigned char *)LOAD_PHYSICAL_ADDR; > > #ifdef CONFIG_HIBERNATION > if (!cmdline_find_option_bool("kaslr")) { > debug_putstr("KASLR disabled by default...\n"); > return; > } > #else > if (cmdline_find_option_bool("nokaslr")) { > debug_putstr("KASLR disabled by cmdline...\n"); > return; > } > #endif > > real_mode->hdr.loadflags |= KASLR_FLAG; > > /* Record the various known unsafe memory ranges. */ > mem_avoid_init((unsigned long)input, input_size, > (unsigned long)*output); > > /* Low end should be the smaller of 512M or initial location. */ > min_addr = min((unsigned long)*output, 512UL << 20); > > /* Walk e820 and find a random address. */ > random = find_random_phy_addr(min_addr, output_size); > if (!random) > debug_putstr("KASLR could not find suitable E820 region...\n"); > else { > if ((unsigned long)*output != random) { > fill_pagetable(random, output_size); > switch_pagetable(); > *output = (unsigned char *)random; > } > } > > /* Pick random virtual address starting from LOAD_PHYSICAL_ADDR. */ > if (IS_ENABLED(CONFIG_X86_64)) > random = find_random_virt_offset(LOAD_PHYSICAL_ADDR, > output_size); > *virt_offset = (unsigned char *)random; > } > > Firstly, 'random' is a libc function name. We generally don't overload those. > > Secondly, it's a random what? Variable names should make it plenty obvious. So it > should probably be named 'random_addr'. > > Third: > > /* Walk e820 and find a random address. */ > random = find_random_phy_addr(min_addr, output_size); > > yeah, so what that comment tells us we knew already, due to the function name! > What the comment should _really_ talk about is the high level purpose. Something > like: 'Walk the e820 map and find a random free RAM address to which we can still > decompress the whole kernel' would work so much better ... > > Fourth, this function has seven (!!) type casts. We can sure do better. Between the e820 values, the asm linkages, the relocations, etc, there's a lot of mixing of types. As mentioned, I'll clean it up. > Fifth: > > /* Pick random virtual address starting from LOAD_PHYSICAL_ADDR. */ > if (IS_ENABLED(CONFIG_X86_64)) > random = find_random_virt_offset(LOAD_PHYSICAL_ADDR, > output_size); > *virt_offset = (unsigned char *)random; > > So the purpose of this whole function is to pick _two_ random addresses: the > random physical address to place the kernel at, and on x86_64, to also randomize > the kernel virtual address, right? So exactly which comment tells us that it's > about this? Names like 'choose_kernel_location' are singular and are actively > misleading about this ... > > ... and then I haven't even mentioned small details like the imbalanced curly > braces. I'll bite: which braces jumped out at you? I ran all this through checkpatch.pl in the hopes of finding style mistakes... Is it the mixing of single-line code with multi-line code in the if statements? > > This code sucks, and I'm not surprised at all that it was broken. It should be > improved before we can feature-extend it. > > Thanks, > > Ingo -Kees -- Kees Cook Chrome OS & Brillo Security