From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753913AbbBMWVI (ORCPT ); Fri, 13 Feb 2015 17:21:08 -0500 Received: from cantor2.suse.de ([195.135.220.15]:52814 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752071AbbBMWVH (ORCPT ); Fri, 13 Feb 2015 17:21:07 -0500 Date: Fri, 13 Feb 2015 23:20:59 +0100 (CET) From: Jiri Kosina To: Kees Cook cc: "H. Peter Anvin" , LKML , live-patching@vger.kernel.org, Linux-MM , "x86@kernel.org" Subject: Re: [PATCH v2] x86, kaslr: propagate base load address calculation In-Reply-To: Message-ID: References: User-Agent: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 13 Feb 2015, Kees Cook wrote: > > Commit e2b32e678 ("x86, kaslr: randomize module base load address") makes > > the base address for module to be unconditionally randomized in case when > > CONFIG_RANDOMIZE_BASE is defined and "nokaslr" option isn't present on the > > commandline. > > > > This is not consistent with how choose_kernel_location() decides whether > > it will randomize kernel load base. > > > > Namely, CONFIG_HIBERNATION disables kASLR (unless "kaslr" option is > > explicitly specified on kernel commandline), which makes the state space > > larger than what module loader is looking at. IOW CONFIG_HIBERNATION && > > CONFIG_RANDOMIZE_BASE is a valid config option, kASLR wouldn't be applied > > by default in that case, but module loader is not aware of that. > > > > Instead of fixing the logic in module.c, this patch takes more generic > > aproach. It introduces a new bootparam setup data_type SETUP_KASLR and > > uses that to pass the information whether kaslr has been applied during > > kernel decompression, and sets a global 'kaslr_enabled' variable > > accordingly, so that any kernel code (module loading, livepatching, ...) > > can make decisions based on its value. > > > > x86 module loader is converted to make use of this flag. > > > > Signed-off-by: Jiri Kosina > > Thanks for working on this! If others are happy with the setup_data > approach, I think this is fine. This is for x86 folks to decide. I hope my original CC covers this, so let's wait for their verdict. > My only concern is confusion over seeing SETUP_KASLR that was added by a > boot loader. Well, so you are concerned about bootloader that is evil on purpose? If you have such bootloader, you are screwed anyway, because it's free to setup asynchronous events that will corrupt your kernel anyway (DMA that will happen only after the loaded kernel is already active, for example). If you want to avoid evil bootloaders, secure boot is currently The option, I am afraid. > Another way to handle it might be to do some kind of relocs-like poking > of a value into the decompressed kernel? This is so hackish that I'd like to avoid it in favor of the boot params aproach as much as possbile :) [ ... snip ... ] > > diff --git a/arch/x86/boot/compressed/aslr.c b/arch/x86/boot/compressed/aslr.c > > index bb13763..d9d1da9 100644 > > --- a/arch/x86/boot/compressed/aslr.c > > +++ b/arch/x86/boot/compressed/aslr.c > > @@ -14,6 +14,13 @@ > > static const char build_str[] = UTS_RELEASE " (" LINUX_COMPILE_BY "@" > > LINUX_COMPILE_HOST ") (" LINUX_COMPILER ") " UTS_VERSION; > > > > +struct kaslr_setup_data { > > Should this be "static"? Good catch. So let's wait what x86 folks have to say. I'll either update in in v3, or hopefully someone will fix this when applying the patch for -tip. Thanks, -- Jiri Kosina SUSE Labs From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-we0-f170.google.com (mail-we0-f170.google.com [74.125.82.170]) by kanga.kvack.org (Postfix) with ESMTP id 730BD6B008C for ; Fri, 13 Feb 2015 17:21:07 -0500 (EST) Received: by mail-we0-f170.google.com with SMTP id q59so19269099wes.1 for ; Fri, 13 Feb 2015 14:21:06 -0800 (PST) Received: from mx2.suse.de (cantor2.suse.de. [195.135.220.15]) by mx.google.com with ESMTPS id eu19si6308008wid.10.2015.02.13.14.21.04 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Fri, 13 Feb 2015 14:21:05 -0800 (PST) Date: Fri, 13 Feb 2015 23:20:59 +0100 (CET) From: Jiri Kosina Subject: Re: [PATCH v2] x86, kaslr: propagate base load address calculation In-Reply-To: Message-ID: References: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-linux-mm@kvack.org List-ID: To: Kees Cook Cc: "H. Peter Anvin" , LKML , live-patching@vger.kernel.org, Linux-MM , "x86@kernel.org" On Fri, 13 Feb 2015, Kees Cook wrote: > > Commit e2b32e678 ("x86, kaslr: randomize module base load address") makes > > the base address for module to be unconditionally randomized in case when > > CONFIG_RANDOMIZE_BASE is defined and "nokaslr" option isn't present on the > > commandline. > > > > This is not consistent with how choose_kernel_location() decides whether > > it will randomize kernel load base. > > > > Namely, CONFIG_HIBERNATION disables kASLR (unless "kaslr" option is > > explicitly specified on kernel commandline), which makes the state space > > larger than what module loader is looking at. IOW CONFIG_HIBERNATION && > > CONFIG_RANDOMIZE_BASE is a valid config option, kASLR wouldn't be applied > > by default in that case, but module loader is not aware of that. > > > > Instead of fixing the logic in module.c, this patch takes more generic > > aproach. It introduces a new bootparam setup data_type SETUP_KASLR and > > uses that to pass the information whether kaslr has been applied during > > kernel decompression, and sets a global 'kaslr_enabled' variable > > accordingly, so that any kernel code (module loading, livepatching, ...) > > can make decisions based on its value. > > > > x86 module loader is converted to make use of this flag. > > > > Signed-off-by: Jiri Kosina > > Thanks for working on this! If others are happy with the setup_data > approach, I think this is fine. This is for x86 folks to decide. I hope my original CC covers this, so let's wait for their verdict. > My only concern is confusion over seeing SETUP_KASLR that was added by a > boot loader. Well, so you are concerned about bootloader that is evil on purpose? If you have such bootloader, you are screwed anyway, because it's free to setup asynchronous events that will corrupt your kernel anyway (DMA that will happen only after the loaded kernel is already active, for example). If you want to avoid evil bootloaders, secure boot is currently The option, I am afraid. > Another way to handle it might be to do some kind of relocs-like poking > of a value into the decompressed kernel? This is so hackish that I'd like to avoid it in favor of the boot params aproach as much as possbile :) [ ... snip ... ] > > diff --git a/arch/x86/boot/compressed/aslr.c b/arch/x86/boot/compressed/aslr.c > > index bb13763..d9d1da9 100644 > > --- a/arch/x86/boot/compressed/aslr.c > > +++ b/arch/x86/boot/compressed/aslr.c > > @@ -14,6 +14,13 @@ > > static const char build_str[] = UTS_RELEASE " (" LINUX_COMPILE_BY "@" > > LINUX_COMPILE_HOST ") (" LINUX_COMPILER ") " UTS_VERSION; > > > > +struct kaslr_setup_data { > > Should this be "static"? Good catch. So let's wait what x86 folks have to say. I'll either update in in v3, or hopefully someone will fix this when applying the patch for -tip. Thanks, -- Jiri Kosina SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org