From: Kees Cook <keescook@chromium.org> To: Jiri Kosina <jkosina@suse.cz> Cc: "H. Peter Anvin" <hpa@linux.intel.com>, LKML <linux-kernel@vger.kernel.org>, live-patching@vger.kernel.org, Linux-MM <linux-mm@kvack.org>, "x86@kernel.org" <x86@kernel.org> Subject: Re: [PATCH v2] x86, kaslr: propagate base load address calculation Date: Fri, 13 Feb 2015 15:25:26 -0800 [thread overview] Message-ID: <CAGXu5jL3UMkeHpAxe1RBpnQhLWGquR1NJQx1AsukiwA31AA78g@mail.gmail.com> (raw) In-Reply-To: <alpine.LNX.2.00.1502132316320.4925@pobox.suse.cz> On Fri, Feb 13, 2015 at 2:20 PM, Jiri Kosina <jkosina@suse.cz> wrote: > 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 <jkosina@suse.cz> >> >> 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? No, no; I agree: a malicious boot loader is a lost cause. I mean mostly from a misbehavior perspective. Like, someone sees "kaslr" in the setup args and thinks they can set it to 1 and boot a kernel, etc. Or they set it to 0, but they lack HIBERNATION and "1" gets appended, but the setup_data parser sees the boot-loader one set to 0, etc. I'm just curious if we should avoid getting some poor system into a confusing state. > > 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 :) Yeah, I think so too. :) > > [ ... 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. Great! -Kees > > Thanks, > > -- > Jiri Kosina > SUSE Labs -- Kees Cook Chrome OS Security
WARNING: multiple messages have this Message-ID (diff)
From: Kees Cook <keescook@chromium.org> To: Jiri Kosina <jkosina@suse.cz> Cc: "H. Peter Anvin" <hpa@linux.intel.com>, LKML <linux-kernel@vger.kernel.org>, live-patching@vger.kernel.org, Linux-MM <linux-mm@kvack.org>, "x86@kernel.org" <x86@kernel.org> Subject: Re: [PATCH v2] x86, kaslr: propagate base load address calculation Date: Fri, 13 Feb 2015 15:25:26 -0800 [thread overview] Message-ID: <CAGXu5jL3UMkeHpAxe1RBpnQhLWGquR1NJQx1AsukiwA31AA78g@mail.gmail.com> (raw) In-Reply-To: <alpine.LNX.2.00.1502132316320.4925@pobox.suse.cz> On Fri, Feb 13, 2015 at 2:20 PM, Jiri Kosina <jkosina@suse.cz> wrote: > 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 <jkosina@suse.cz> >> >> 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? No, no; I agree: a malicious boot loader is a lost cause. I mean mostly from a misbehavior perspective. Like, someone sees "kaslr" in the setup args and thinks they can set it to 1 and boot a kernel, etc. Or they set it to 0, but they lack HIBERNATION and "1" gets appended, but the setup_data parser sees the boot-loader one set to 0, etc. I'm just curious if we should avoid getting some poor system into a confusing state. > > 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 :) Yeah, I think so too. :) > > [ ... 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. Great! -Kees > > Thanks, > > -- > Jiri Kosina > SUSE Labs -- Kees Cook Chrome OS Security -- 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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
next prev parent reply other threads:[~2015-02-13 23:25 UTC|newest] Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-02-10 13:17 [PATCH] x86, kaslr: propagate base load address calculation Jiri Kosina 2015-02-10 13:17 ` Jiri Kosina 2015-02-10 17:25 ` Kees Cook 2015-02-10 17:25 ` Kees Cook 2015-02-10 23:07 ` Jiri Kosina 2015-02-10 23:07 ` Jiri Kosina 2015-02-10 23:13 ` Jiri Kosina 2015-02-10 23:13 ` Jiri Kosina 2015-02-13 15:04 ` [PATCH v2] " Jiri Kosina 2015-02-13 15:04 ` Jiri Kosina 2015-02-13 17:49 ` Kees Cook 2015-02-13 17:49 ` Kees Cook 2015-02-13 22:20 ` Jiri Kosina 2015-02-13 22:20 ` Jiri Kosina 2015-02-13 23:25 ` Kees Cook [this message] 2015-02-13 23:25 ` Kees Cook 2015-02-16 11:55 ` Borislav Petkov 2015-02-16 11:55 ` Borislav Petkov 2015-02-16 19:27 ` Kees Cook 2015-02-16 19:27 ` Kees Cook 2015-02-16 19:42 ` Borislav Petkov 2015-02-16 19:42 ` Borislav Petkov 2015-02-17 10:44 ` Borislav Petkov 2015-02-17 10:44 ` Borislav Petkov 2015-02-17 12:21 ` Jiri Kosina 2015-02-17 12:21 ` Jiri Kosina 2015-02-17 12:39 ` Borislav Petkov 2015-02-17 12:39 ` Borislav Petkov 2015-02-17 16:45 ` Kees Cook 2015-02-17 16:45 ` Kees Cook 2015-02-17 22:31 ` Borislav Petkov 2015-02-17 22:31 ` Borislav Petkov 2015-02-18 3:33 ` Kees Cook 2015-02-18 3:33 ` Kees Cook 2015-02-18 8:32 ` Borislav Petkov 2015-02-18 8:32 ` Borislav Petkov 2015-02-18 10:46 ` Jiri Kosina 2015-02-18 10:46 ` Jiri Kosina
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAGXu5jL3UMkeHpAxe1RBpnQhLWGquR1NJQx1AsukiwA31AA78g@mail.gmail.com \ --to=keescook@chromium.org \ --cc=hpa@linux.intel.com \ --cc=jkosina@suse.cz \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=live-patching@vger.kernel.org \ --cc=x86@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.