[PATCH] x86, boot: skip relocs when load address unchanged

[PATCH] x86, boot: skip relocs when load address unchanged

[Kees Cook]

On 64-bit, relocation is not required unless the load address gets
changed. Without this, relocations do unexpected things when the kernel
is above 4G.

Reported-by: Baoquan He <bhe@redhat.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: stable@vger.kernel.org
---
This is a reimplementation of Baoquan's "kaslr: check if kernel location is
changed", which performs the check without needing to change the function
declaration. This should have exactly the same effect, but I dropped Vivek's
Ack and Thomas's Test, since it's technically a different patch.
---
 arch/x86/boot/compressed/misc.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index dcc1c536cc21..a950864a64da 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -373,6 +373,8 @@ asmlinkage __visible void *decompress_kernel(void *rmode, memptr heap,
 				  unsigned long output_len,
 				  unsigned long run_size)
 {
+	unsigned char *output_orig = output;
+
 	real_mode = rmode;
 
 	sanitize_boot_params(real_mode);
@@ -421,7 +423,12 @@ asmlinkage __visible void *decompress_kernel(void *rmode, memptr heap,
 	debug_putstr("\nDecompressing Linux... ");
 	decompress(input_data, input_len, NULL, NULL, output, NULL, error);
 	parse_elf(output);
-	handle_relocations(output, output_len);
+	/*
+	 * 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) || output != output_orig)
+		handle_relocations(output, output_len);
 	debug_putstr("done.\nBooting the kernel.\n");
 	return output;
 }
-- 
1.9.1


-- 
Kees Cook
Chrome OS Security

Re: [PATCH] x86, boot: skip relocs when load address unchanged

[Thomas D.]

Hi,

Kees Cook wrote:
> This is a reimplementation of Baoquan's "kaslr: check if kernel location is
> changed", which performs the check without needing to change the function
> declaration. This should have exactly the same effect, but I dropped Vivek's
> Ack and Thomas's Test, since it's technically a different patch.

Your patch works for me, too.

Tested with linux-3.19-rc4 on a 64-bit system.

Thanks!


-Thomas

[tip:x86/urgent] x86, boot: Skip relocs when load address unchanged

[tip-bot for Kees Cook]

Commit-ID:  f285f4a21c3253887caceed493089ece17579d59
Gitweb:     http://git.kernel.org/tip/f285f4a21c3253887caceed493089ece17579d59
Author:     Kees Cook <keescook@chromium.org>
AuthorDate: Thu, 15 Jan 2015 16:51:46 -0800
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 20 Jan 2015 12:37:23 +0100

x86, boot: Skip relocs when load address unchanged

On 64-bit, relocation is not required unless the load address gets
changed. Without this, relocations do unexpected things when the kernel
is above 4G.

Reported-by: Baoquan He <bhe@redhat.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Tested-by: Thomas D. <whissi@whissi.de>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Junjie Mao <eternal.n08@gmail.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: stable@vger.kernel.org
Link: http://lkml.kernel.org/r/20150116005146.GA4212@www.outflux.net
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/boot/compressed/misc.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index dcc1c53..a950864 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -373,6 +373,8 @@ asmlinkage __visible void *decompress_kernel(void *rmode, memptr heap,
 				  unsigned long output_len,
 				  unsigned long run_size)
 {
+	unsigned char *output_orig = output;
+
 	real_mode = rmode;
 
 	sanitize_boot_params(real_mode);
@@ -421,7 +423,12 @@ asmlinkage __visible void *decompress_kernel(void *rmode, memptr heap,
 	debug_putstr("\nDecompressing Linux... ");
 	decompress(input_data, input_len, NULL, NULL, output, NULL, error);
 	parse_elf(output);
-	handle_relocations(output, output_len);
+	/*
+	 * 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) || output != output_orig)
+		handle_relocations(output, output_len);
 	debug_putstr("done.\nBooting the kernel.\n");
 	return output;
 }

Re: [PATCH] x86, boot: skip relocs when load address unchanged

[Baoquan He]

On 01/15/15 at 04:51pm, Kees Cook wrote:
> On 64-bit, relocation is not required unless the load address gets
> changed. Without this, relocations do unexpected things when the kernel
> is above 4G.

This patch works for me. And good to see it's being merged. About the
patch log, I would say relocations do unexpected things when the kernel
is above 1G since randomization is done from 16M to 1G, namely
CONFIG_RANDOMIZE_BASE_MAX_OFFSET. So above 1G kernel text mapping will
step into kernel module mapping region.

BTW, I am working on separate randomization of kernel physical and virtual
address , will post it. But it won't conflict with this because I don't
think it can be accepted in a short time. Before that this patch truly
fix the kexec/kdump bug when kaslr is compiled in.

Thanks
Baoquan

> 
> Reported-by: Baoquan He <bhe@redhat.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@vger.kernel.org
> ---
> This is a reimplementation of Baoquan's "kaslr: check if kernel location is
> changed", which performs the check without needing to change the function
> declaration. This should have exactly the same effect, but I dropped Vivek's
> Ack and Thomas's Test, since it's technically a different patch.
> ---
>  arch/x86/boot/compressed/misc.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> index dcc1c536cc21..a950864a64da 100644
> --- a/arch/x86/boot/compressed/misc.c
> +++ b/arch/x86/boot/compressed/misc.c
> @@ -373,6 +373,8 @@ asmlinkage __visible void *decompress_kernel(void *rmode, memptr heap,
>  				  unsigned long output_len,
>  				  unsigned long run_size)
>  {
> +	unsigned char *output_orig = output;
> +
>  	real_mode = rmode;
>  
>  	sanitize_boot_params(real_mode);
> @@ -421,7 +423,12 @@ asmlinkage __visible void *decompress_kernel(void *rmode, memptr heap,
>  	debug_putstr("\nDecompressing Linux... ");
>  	decompress(input_data, input_len, NULL, NULL, output, NULL, error);
>  	parse_elf(output);
> -	handle_relocations(output, output_len);
> +	/*
> +	 * 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) || output != output_orig)
> +		handle_relocations(output, output_len);
>  	debug_putstr("done.\nBooting the kernel.\n");
>  	return output;
>  }
> -- 
> 1.9.1
> 
> 
> -- 
> Kees Cook
> Chrome OS Security

Re: [PATCH] x86, boot: skip relocs when load address unchanged

[MegaBrutal]

Thanks for this patch, and good to see it in mainline!

This actually fixes the problem I reported here:
https://lkml.org/lkml/2014/12/1/15

I wish it to be backported into the Ubuntu Utopic kernel asap.

> This patch works for me. And good to see it's being merged. About the
> patch log, I would say relocations do unexpected things when the kernel
> is above 1G since randomization is done from 16M to 1G, namely
> CONFIG_RANDOMIZE_BASE_MAX_OFFSET. So above 1G kernel text mapping will
> step into kernel module mapping region.

I'm just speculating, but is it the reason why I only get problems
when I boot with kexec? Maybe it's only then when the kernel gets
above 1G. Otherwise, the kernels boot properly when they are booted
from GRUB.


2015-01-20 14:04 GMT+01:00 Baoquan He <bhe@redhat.com>:
>
> On 01/15/15 at 04:51pm, Kees Cook wrote:
> > On 64-bit, relocation is not required unless the load address gets
> > changed. Without this, relocations do unexpected things when the kernel
> > is above 4G.
>
> This patch works for me. And good to see it's being merged. About the
> patch log, I would say relocations do unexpected things when the kernel
> is above 1G since randomization is done from 16M to 1G, namely
> CONFIG_RANDOMIZE_BASE_MAX_OFFSET. So above 1G kernel text mapping will
> step into kernel module mapping region.
>
> BTW, I am working on separate randomization of kernel physical and virtual
> address , will post it. But it won't conflict with this because I don't
> think it can be accepted in a short time. Before that this patch truly
> fix the kexec/kdump bug when kaslr is compiled in.
>
> Thanks
> Baoquan
>
> >
> > Reported-by: Baoquan He <bhe@redhat.com>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > Cc: stable@vger.kernel.org
> > ---
> > This is a reimplementation of Baoquan's "kaslr: check if kernel location is
> > changed", which performs the check without needing to change the function
> > declaration. This should have exactly the same effect, but I dropped Vivek's
> > Ack and Thomas's Test, since it's technically a different patch.
> > ---
> >  arch/x86/boot/compressed/misc.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> > index dcc1c536cc21..a950864a64da 100644
> > --- a/arch/x86/boot/compressed/misc.c
> > +++ b/arch/x86/boot/compressed/misc.c
> > @@ -373,6 +373,8 @@ asmlinkage __visible void *decompress_kernel(void *rmode, memptr heap,
> >                                 unsigned long output_len,
> >                                 unsigned long run_size)
> >  {
> > +     unsigned char *output_orig = output;
> > +
> >       real_mode = rmode;
> >
> >       sanitize_boot_params(real_mode);
> > @@ -421,7 +423,12 @@ asmlinkage __visible void *decompress_kernel(void *rmode, memptr heap,
> >       debug_putstr("\nDecompressing Linux... ");
> >       decompress(input_data, input_len, NULL, NULL, output, NULL, error);
> >       parse_elf(output);
> > -     handle_relocations(output, output_len);
> > +     /*
> > +      * 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) || output != output_orig)
> > +             handle_relocations(output, output_len);
> >       debug_putstr("done.\nBooting the kernel.\n");
> >       return output;
> >  }
> > --
> > 1.9.1
> >
> >
> > --
> > Kees Cook
> > Chrome OS Security
> --
> 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/

Re: [PATCH] x86, boot: skip relocs when load address unchanged

[Baoquan He]

On 02/26/15 at 07:29am, MegaBrutal wrote:
> Thanks for this patch, and good to see it in mainline!
> 
> This actually fixes the problem I reported here:
> https://lkml.org/lkml/2014/12/1/15
> 
> I wish it to be backported into the Ubuntu Utopic kernel asap.
> 
> > This patch works for me. And good to see it's being merged. About the
> > patch log, I would say relocations do unexpected things when the kernel
> > is above 1G since randomization is done from 16M to 1G, namely
> > CONFIG_RANDOMIZE_BASE_MAX_OFFSET. So above 1G kernel text mapping will
> > step into kernel module mapping region.
> 
> I'm just speculating, but is it the reason why I only get problems
> when I boot with kexec? Maybe it's only then when the kernel gets
> above 1G. Otherwise, the kernels boot properly when they are booted
> from GRUB.

Yeah, kexec loads kernel at the top of physical memory. And above 1G
kernel mapping will step into module mapping area and collapse system.
Grub doesn't incur this problem.

Re: [PATCH] x86, boot: skip relocs when load address unchanged

[Ingo Molnar]

* MegaBrutal <megabrutal@gmail.com> wrote:

> Thanks for this patch, and good to see it in mainline!
> 
> This actually fixes the problem I reported here:
> https://lkml.org/lkml/2014/12/1/15
> 
> I wish it to be backported into the Ubuntu Utopic kernel asap.

We marked the commit Cc: stable, so it ought to be picked 
up by the stable kernel and then it should show up in 
Ubuntu if they refresh their kernel package to the latest 
stable kernel.

Thanks,

	Ingo