Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751294AbbB1Kv7 (ORCPT ); Sat, 28 Feb 2015 05:51:59 -0500 Received: from cantor2.suse.de ([195.135.220.15]:48745 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750759AbbB1Kv5 (ORCPT ); Sat, 28 Feb 2015 05:51:57 -0500 Date: Sat, 28 Feb 2015 11:50:49 +0100 From: Borislav Petkov To: "H. Peter Anvin" , Kees Cook , Jiri Kosina Cc: Ingo Molnar , Huang Ying , Jiri Kosina , LKML , LKP ML , x86-ml , Dave Young , Matt Fleming , Andy Lutomirski Subject: Re: [LKP] [x86/mm/ASLR] f47233c2d34: WARNING: CPU: 0 PID: 1 at arch/x86/mm/ioremap.c:63 __ioremap_check_ram+0x445/0x4a0() Message-ID: <20150228105049.GA11038@pd.tnic> References: <1424929021.10337.24.camel@intel.com> <20150226103040.GA3573@pd.tnic> <20150226111250.GA32024@gmail.com> <20150226121617.GB3573@pd.tnic> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20150226121617.GB3573@pd.tnic> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4538 Lines: 138 On Thu, Feb 26, 2015 at 01:16:17PM +0100, Borislav Petkov wrote: > The proper fix should be to say, don't ioremap setup_data which is > kernel memory but I'm not sure I have a good idea at the moment how to > do that *without* ioremapping the thing to inspect it first... > > More hmm... Yeah, too many hmms means this still needed staring at to find out what exactly the problem is. And the problem is that allocating that struct setup_data statically in arch/x86/boot/compressed/aslr.c works only by chance, when the kernel decompressing doesn't overwrite that memory. One thing that we could do is to stick it right below LOAD_PHYSICAL_ADDR (16M by default) which is the miminum physical address for the kernel to be loaded at and kaslr pays attention to. I.e., this struct setup_data thing lands then here: [ 0.000000] parse_setup_data: data: 0xffffe0 (va: ffffffffff200fe0) { next: 0x0, type: 0x5, len: 17, data[0]: 0x0 } which is 16M - 2*sizeof(struct setup_data). Yeah, I left some room there. Now, this approach works but I'm not sure whether this is how we want to be passing setup_data stuff from arch/x86/boot/ to kernel proper so I'd like to hear some more experienced opinions please... Thanks! --- diff --git a/arch/x86/boot/compressed/aslr.c b/arch/x86/boot/compressed/aslr.c index 7083c16cccba..9f64c64e3ebe 100644 --- a/arch/x86/boot/compressed/aslr.c +++ b/arch/x86/boot/compressed/aslr.c @@ -14,12 +14,7 @@ static const char build_str[] = UTS_RELEASE " (" LINUX_COMPILE_BY "@" LINUX_COMPILE_HOST ") (" LINUX_COMPILER ") " UTS_VERSION; -struct kaslr_setup_data { - __u64 next; - __u32 type; - __u32 len; - __u8 data[1]; -} kaslr_setup_data; +struct setup_data *ksd; #define I8254_PORT_CONTROL 0x43 #define I8254_PORT_COUNTER0 0x40 @@ -302,14 +297,20 @@ static unsigned long find_random_addr(unsigned long minimum, return slots_fetch_random(); } -static void add_kaslr_setup_data(struct boot_params *params, __u8 enabled) +static void add_kaslr_setup_data(struct boot_params *params, + u8 *output, __u8 enabled) { struct setup_data *data; - kaslr_setup_data.type = SETUP_KASLR; - kaslr_setup_data.len = 1; - kaslr_setup_data.next = 0; - kaslr_setup_data.data[0] = enabled; + /* + * Stick it right under LOAD_PHYSICAL_ADDR + */ + ksd = (struct setup_data *)(output - 2 * sizeof(struct setup_data)); + + ksd->type = SETUP_KASLR; + ksd->len = 1; + ksd->next = 0; + ksd->data[0] = enabled; data = (struct setup_data *)(unsigned long)params->hdr.setup_data; @@ -317,10 +318,9 @@ static void add_kaslr_setup_data(struct boot_params *params, __u8 enabled) data = (struct setup_data *)(unsigned long)data->next; if (data) - data->next = (unsigned long)&kaslr_setup_data; + data->next = (unsigned long)ksd; else - params->hdr.setup_data = (unsigned long)&kaslr_setup_data; - + params->hdr.setup_data = (unsigned long)ksd; } unsigned char *choose_kernel_location(struct boot_params *params, @@ -335,17 +335,17 @@ unsigned char *choose_kernel_location(struct boot_params *params, #ifdef CONFIG_HIBERNATION if (!cmdline_find_option_bool("kaslr")) { debug_putstr("KASLR disabled by default...\n"); - add_kaslr_setup_data(params, 0); + add_kaslr_setup_data(params, output, 0); goto out; } #else if (cmdline_find_option_bool("nokaslr")) { debug_putstr("KASLR disabled by cmdline...\n"); - add_kaslr_setup_data(params, 0); + add_kaslr_setup_data(params, output, 0); goto out; } #endif - add_kaslr_setup_data(params, 1); + add_kaslr_setup_data(params, output, 1); /* Record the various known unsafe memory ranges. */ mem_avoid_init((unsigned long)input, input_size, diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c index 98dc9317286e..d3b34df6e539 100644 --- a/arch/x86/kernel/setup.c +++ b/arch/x86/kernel/setup.c @@ -429,7 +429,11 @@ static void __init reserve_initrd(void) static void __init parse_kaslr_setup(u64 pa_data, u32 data_len) { - kaslr_enabled = (bool)(pa_data + sizeof(struct setup_data)); + struct setup_data *kdata; + + kdata = early_memremap(pa_data, data_len); + kaslr_enabled = kdata->data[0]; + early_iounmap(kdata, data_len); } static void __init parse_setup_data(void) --- -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/