Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1676235 > unrolled thread

Re: [PATCH] x86/boot/KASLR: Skip relocation handling in no kaslr case

Started byKees Cook <keescook@chromium.org>
First post2017-06-28 00:50 +0200
Last post2017-06-28 01:40 +0200
Articles 3 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] x86/boot/KASLR: Skip relocation handling in no kaslr case Kees Cook <keescook@chromium.org> - 2017-06-28 00:50 +0200
    Re: [PATCH] x86/boot/KASLR: Skip relocation handling in no kaslr case Baoquan He <bhe@redhat.com> - 2017-06-28 01:30 +0200
      Re: [PATCH] x86/boot/KASLR: Skip relocation handling in no kaslr case Baoquan He <bhe@redhat.com> - 2017-06-28 01:40 +0200

#1676235 — Re: [PATCH] x86/boot/KASLR: Skip relocation handling in no kaslr case

FromKees Cook <keescook@chromium.org>
Date2017-06-28 00:50 +0200
SubjectRe: [PATCH] x86/boot/KASLR: Skip relocation handling in no kaslr case
Message-ID<tX7t0-6Yu-29@gated-at.bofh.it>
On Sat, Jun 24, 2017 at 7:25 AM, Baoquan He <bhe@redhat.com> wrote:
> Kdump kernel will reset to firmware after crash is trigered when
> crashkernel=xxM,high is added to kernel command line. Kexec has the
> same phenomenon. This only happened on system with kaslr code
> compiled in and kernel option 'nokaslr'is added. Both of them works
> well when kaslr is enabled.
>
> When crashkernel high is set or kexec case, kexec/kdump kernel will be
> put above 4G. Since we assign the original loading address of kernel to
> virt_addr as initial value, the virt_addr will be larger than 1G if kaslr
> is disabled, it exceeds the kernel mapping size which is only 1G. Then
> it will cause relocation handling error in handle_relocations().
>
> In fact this is a known issue and fixed in commit:
>
> ... f285f4a ("x86, boot: Skip relocs when load address unchanged")
>
> But above fix was lost carelessly in later commit:
>
> ... 8391c73 ("x86/KASLR: Randomize virtual address separately")

Hrm, this changes more than just fixing this missing logic. How about
just simply:

diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index b3c5a5f030ce..4496a05d1f8a 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -339,6 +339,7 @@ asmlinkage __visible void *extract_kernel(void
*rmode, memptr heap,
 {
        const unsigned long kernel_total_size = VO__end - VO__text;
        unsigned long virt_addr = (unsigned long)output;
+       unsigned long virt_addr_orig = virt_addr;

        /* Retain x86 boot parameters pointer passed from startup_32/64. */
        boot_params = rmode;
@@ -405,7 +406,12 @@ asmlinkage __visible void *extract_kernel(void
*rmode, memptr heap,
        __decompress(input_data, input_len, NULL, NULL, output, output_len,
                        NULL, error);
        parse_elf(output);
-       handle_relocations(output, output_len, virt_addr);
+       /*
+        * 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) || virt_addr != virt_addr_orig)
+               handle_relocations(output, output_len, virt_addr);
        debug_putstr("done.\nBooting the kernel.\n");
        return output;
 }


>
> To fix it, just assign LOAD_PHYSICAL_ADDR to virt_addr as initial value.
> If no kaslr is taken, it will skip the relocation handling and jump out.
>
> Fixes: 8391c73 ("x86/KASLR: Randomize virtual address separately")
> Tested-by: Dave Young <dyoung@redhat.com>
> Signed-off-by: Baoquan He <bhe@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: x86@kernel.org
> Cc: Kees Cook <keescook@chromium.org
> Cc: Yinghai Lu <yinghai@kernel.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Thomas Garnier <thgarnie@google.com>
> ---
>  arch/x86/boot/compressed/kaslr.c | 3 ---
>  arch/x86/boot/compressed/misc.c  | 4 ++--
>  arch/x86/boot/compressed/misc.h  | 2 --
>  3 files changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index fe318b4..91f27ab 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -625,9 +625,6 @@ void choose_random_location(unsigned long input,
>  {
>         unsigned long random_addr, min_addr;
>
> -       /* By default, keep output position unchanged. */
> -       *virt_addr = *output;
> -
>         if (cmdline_find_option_bool("nokaslr")) {
>                 warn("KASLR disabled: 'nokaslr' on cmdline.");
>                 return;
> diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> index b3c5a5f0..c945acd 100644
> --- a/arch/x86/boot/compressed/misc.c
> +++ b/arch/x86/boot/compressed/misc.c
> @@ -338,7 +338,7 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
>                                   unsigned long output_len)
>  {
>         const unsigned long kernel_total_size = VO__end - VO__text;
> -       unsigned long virt_addr = (unsigned long)output;
> +       unsigned long virt_addr = LOAD_PHYSICAL_ADDR;

I don't like this being hard-coded. The logic for the output address
is already handled in the .S files, and may not be LOAD_PHYSICAL_ADDR.
I'd prefer just adding back the simple test of virt_addr changing.

-Kees

>
>         /* Retain x86 boot parameters pointer passed from startup_32/64. */
>         boot_params = rmode;
> @@ -397,7 +397,7 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
>  #ifndef CONFIG_RELOCATABLE
>         if ((unsigned long)output != LOAD_PHYSICAL_ADDR)
>                 error("Destination address does not match LOAD_PHYSICAL_ADDR");
> -       if ((unsigned long)output != virt_addr)
> +       if (virt_addr != LOAD_PHYSICAL_ADDR)
>                 error("Destination virtual address changed when not relocatable");
>  #endif
>
> diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
> index 1c8355e..766a521 100644
> --- a/arch/x86/boot/compressed/misc.h
> +++ b/arch/x86/boot/compressed/misc.h
> @@ -81,8 +81,6 @@ static inline void choose_random_location(unsigned long input,
>                                           unsigned long output_size,
>                                           unsigned long *virt_addr)
>  {
> -       /* No change from existing output location. */
> -       *virt_addr = *output;
>  }
>  #endif
>
> --
> 2.5.5
>



-- 
Kees Cook
Pixel Security

[toc] | [next] | [standalone]

#1676258

FromBaoquan He <bhe@redhat.com>
Date2017-06-28 01:30 +0200
Message-ID<tX85H-7qz-1@gated-at.bofh.it>
In reply to#1676235
Hi Kees,

On 06/27/17 at 03:42pm, Kees Cook wrote:
> On Sat, Jun 24, 2017 at 7:25 AM, Baoquan He <bhe@redhat.com> wrote:
> > Kdump kernel will reset to firmware after crash is trigered when
> > crashkernel=xxM,high is added to kernel command line. Kexec has the
> > same phenomenon. This only happened on system with kaslr code
> > compiled in and kernel option 'nokaslr'is added. Both of them works
> > well when kaslr is enabled.
> >
> > When crashkernel high is set or kexec case, kexec/kdump kernel will be
> > put above 4G. Since we assign the original loading address of kernel to
> > virt_addr as initial value, the virt_addr will be larger than 1G if kaslr
> > is disabled, it exceeds the kernel mapping size which is only 1G. Then
> > it will cause relocation handling error in handle_relocations().
> >
> > In fact this is a known issue and fixed in commit:
> >
> > ... f285f4a ("x86, boot: Skip relocs when load address unchanged")
> >
> > But above fix was lost carelessly in later commit:
> >
> > ... 8391c73 ("x86/KASLR: Randomize virtual address separately")
> 
> Hrm, this changes more than just fixing this missing logic. How about
> just simply:

Below code was added to fix the kexec/kdump kernel with kaslr disabled,
at that time kernel kaslr physical address and virtual address
randomization are coupled. What it was doing is to randomize physical
address in 1G range and add the delta onto the starting address of
virtual address, 0xffffffff80000000.

But now the physical and virtual address randomization has been
separated. It means that whether each of them is changed or not
randomly, the randomization wont' be impacted. So below change you
provided will has two problems:

1st, the 'virt_addr' represents the offset of virtual address
randomization between 0xffffffff80000000 and 0xffffffffc0000000, should
not get a initial value '(unsigned long)output'. The 'output' could be
any value between 16M and 64T. It makes no sense to make the assignment
and could bring confusion to code readers.

2nd, for x86 64, we do the relocation handling if and only if virtual
address is randomized to a different value, namely the offset 'virt_addr'
has a value which is not 16M. You can see this in handle_relocations().

	if (IS_ENABLED(CONFIG_X86_64))  
                delta = virt_addr - LOAD_PHYSICAL_ADDR; 
  
        if (!delta) {          
                debug_putstr("No relocation needed... ");
                return;
        }
Now below code will bring a bug that if physical address randomization fail
to get a different value, but virtual address randomization may succeed to
get one, then it won't do relocation handling. This contradicts with the
design and implementation of the current code.

> 
> diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> index b3c5a5f030ce..4496a05d1f8a 100644
> --- a/arch/x86/boot/compressed/misc.c
> +++ b/arch/x86/boot/compressed/misc.c
> @@ -339,6 +339,7 @@ asmlinkage __visible void *extract_kernel(void
> *rmode, memptr heap,
>  {
>         const unsigned long kernel_total_size = VO__end - VO__text;
>         unsigned long virt_addr = (unsigned long)output;
> +       unsigned long virt_addr_orig = virt_addr;
> 
>         /* Retain x86 boot parameters pointer passed from startup_32/64. */
>         boot_params = rmode;
> @@ -405,7 +406,12 @@ asmlinkage __visible void *extract_kernel(void
> *rmode, memptr heap,
>         __decompress(input_data, input_len, NULL, NULL, output, output_len,
>                         NULL, error);
>         parse_elf(output);
> -       handle_relocations(output, output_len, virt_addr);
> +       /*
> +        * 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) || virt_addr != virt_addr_orig)
> +               handle_relocations(output, output_len, virt_addr);
>         debug_putstr("done.\nBooting the kernel.\n");
>         return output;
>  }
> 
> > diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> > index b3c5a5f0..c945acd 100644
> > --- a/arch/x86/boot/compressed/misc.c
> > +++ b/arch/x86/boot/compressed/misc.c
> > @@ -338,7 +338,7 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
> >                                   unsigned long output_len)
> >  {
> >         const unsigned long kernel_total_size = VO__end - VO__text;
> > -       unsigned long virt_addr = (unsigned long)output;
> > +       unsigned long virt_addr = LOAD_PHYSICAL_ADDR;
> 
> I don't like this being hard-coded. The logic for the output address
> is already handled in the .S files, and may not be LOAD_PHYSICAL_ADDR.
> I'd prefer just adding back the simple test of virt_addr changing.

Do you mean the handling in boot/compressed/head_64.S? Whatever it does,
it's only for physical address. The virtual address mapping is not
touched. Here virt_addr respresents the offset between
0xffffffff80000000, 0xffffffffc0000000. And if skip the relocation
handling, we can see that the '_text' will be mapped to
0xffffffff81000000 forever, no matter where physical address of '_text'
is. So here LOAD_PHYSICAL_ADDR is default value of 'virt_addr'.

> >
> >         /* Retain x86 boot parameters pointer passed from startup_32/64. */
> >         boot_params = rmode;
> > @@ -397,7 +397,7 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
> >  #ifndef CONFIG_RELOCATABLE
> >         if ((unsigned long)output != LOAD_PHYSICAL_ADDR)
> >                 error("Destination address does not match LOAD_PHYSICAL_ADDR");
> > -       if ((unsigned long)output != virt_addr)
> > +       if (virt_addr != LOAD_PHYSICAL_ADDR)
> >                 error("Destination virtual address changed when not relocatable");
> >  #endif
> >
> > diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
> > index 1c8355e..766a521 100644
> > --- a/arch/x86/boot/compressed/misc.h
> > +++ b/arch/x86/boot/compressed/misc.h
> > @@ -81,8 +81,6 @@ static inline void choose_random_location(unsigned long input,
> >                                           unsigned long output_size,
> >                                           unsigned long *virt_addr)
> >  {
> > -       /* No change from existing output location. */
> > -       *virt_addr = *output;
> >  }
> >  #endif
> >
> > --
> > 2.5.5
> >
> 
> 
> 
> -- 
> Kees Cook
> Pixel Security

[toc] | [prev] | [next] | [standalone]

#1676264

FromBaoquan He <bhe@redhat.com>
Date2017-06-28 01:40 +0200
Message-ID<tX8fo-7uy-21@gated-at.bofh.it>
In reply to#1676258
On 06/28/17 at 07:24am, Baoquan He wrote:
> Hi Kees,
> 
> On 06/27/17 at 03:42pm, Kees Cook wrote:
> > On Sat, Jun 24, 2017 at 7:25 AM, Baoquan He <bhe@redhat.com> wrote:
> > > Kdump kernel will reset to firmware after crash is trigered when
> > > crashkernel=xxM,high is added to kernel command line. Kexec has the
> > > same phenomenon. This only happened on system with kaslr code
> > > compiled in and kernel option 'nokaslr'is added. Both of them works
> > > well when kaslr is enabled.
> > >
> > > When crashkernel high is set or kexec case, kexec/kdump kernel will be
> > > put above 4G. Since we assign the original loading address of kernel to
> > > virt_addr as initial value, the virt_addr will be larger than 1G if kaslr
> > > is disabled, it exceeds the kernel mapping size which is only 1G. Then
> > > it will cause relocation handling error in handle_relocations().
> > >
> > > In fact this is a known issue and fixed in commit:
> > >
> > > ... f285f4a ("x86, boot: Skip relocs when load address unchanged")
> > >
> > > But above fix was lost carelessly in later commit:
> > >
> > > ... 8391c73 ("x86/KASLR: Randomize virtual address separately")
> > 
> > Hrm, this changes more than just fixing this missing logic. How about
> > just simply:
> 
> Below code was added to fix the kexec/kdump kernel with kaslr disabled,
> at that time kernel kaslr physical address and virtual address
> randomization are coupled. What it was doing is to randomize physical
> address in 1G range and add the delta onto the starting address of
> virtual address, 0xffffffff80000000.
> 
> But now the physical and virtual address randomization has been
> separated. It means that whether each of them is changed or not
> randomly, randomization wont' be impacted. So below change you
		      ~~^ of the other one
> provided will has two problems:
> 
> 1st, the 'virt_addr' represents the offset of virtual address
> randomization between 0xffffffff80000000 and 0xffffffffc0000000, should
> not get a initial value '(unsigned long)output'. The 'output' could be
> any value between 16M and 64T. It makes no sense to make the assignment
> and could bring confusion to code readers.
> 
> 2nd, for x86 64, we do the relocation handling if and only if virtual
> address is randomized to a different value, namely the offset 'virt_addr'
> has a value which is not 16M. You can see this in handle_relocations().
> 
> 	if (IS_ENABLED(CONFIG_X86_64))  
>                 delta = virt_addr - LOAD_PHYSICAL_ADDR; 
>   
>         if (!delta) {          
>                 debug_putstr("No relocation needed... ");
>                 return;
>         }
> Now below code will bring a bug that if physical address randomization fail
> to get a different value, but virtual address randomization may succeed to
> get one, then it won't do relocation handling. This contradicts with the
> design and implementation of the current code.
> 
> > 
> > diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> > index b3c5a5f030ce..4496a05d1f8a 100644
> > --- a/arch/x86/boot/compressed/misc.c
> > +++ b/arch/x86/boot/compressed/misc.c
> > @@ -339,6 +339,7 @@ asmlinkage __visible void *extract_kernel(void
> > *rmode, memptr heap,
> >  {
> >         const unsigned long kernel_total_size = VO__end - VO__text;
> >         unsigned long virt_addr = (unsigned long)output;
> > +       unsigned long virt_addr_orig = virt_addr;
> > 
> >         /* Retain x86 boot parameters pointer passed from startup_32/64. */
> >         boot_params = rmode;
> > @@ -405,7 +406,12 @@ asmlinkage __visible void *extract_kernel(void
> > *rmode, memptr heap,
> >         __decompress(input_data, input_len, NULL, NULL, output, output_len,
> >                         NULL, error);
> >         parse_elf(output);
> > -       handle_relocations(output, output_len, virt_addr);
> > +       /*
> > +        * 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) || virt_addr != virt_addr_orig)
> > +               handle_relocations(output, output_len, virt_addr);
> >         debug_putstr("done.\nBooting the kernel.\n");
> >         return output;
> >  }
> > 
> > > diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> > > index b3c5a5f0..c945acd 100644
> > > --- a/arch/x86/boot/compressed/misc.c
> > > +++ b/arch/x86/boot/compressed/misc.c
> > > @@ -338,7 +338,7 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
> > >                                   unsigned long output_len)
> > >  {
> > >         const unsigned long kernel_total_size = VO__end - VO__text;
> > > -       unsigned long virt_addr = (unsigned long)output;
> > > +       unsigned long virt_addr = LOAD_PHYSICAL_ADDR;
> > 
> > I don't like this being hard-coded. The logic for the output address
> > is already handled in the .S files, and may not be LOAD_PHYSICAL_ADDR.
> > I'd prefer just adding back the simple test of virt_addr changing.
> 
> Do you mean the handling in boot/compressed/head_64.S? Whatever it does,
> it's only for physical address. The virtual address mapping is not
> touched. Here virt_addr respresents the offset between
> 0xffffffff80000000, 0xffffffffc0000000. And if skip the relocation
> handling, we can see that the '_text' will be mapped to
> 0xffffffff81000000 forever, no matter where physical address of '_text'
> is. So here LOAD_PHYSICAL_ADDR is default value of 'virt_addr'.
> 
> > >
> > >         /* Retain x86 boot parameters pointer passed from startup_32/64. */
> > >         boot_params = rmode;
> > > @@ -397,7 +397,7 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
> > >  #ifndef CONFIG_RELOCATABLE
> > >         if ((unsigned long)output != LOAD_PHYSICAL_ADDR)
> > >                 error("Destination address does not match LOAD_PHYSICAL_ADDR");
> > > -       if ((unsigned long)output != virt_addr)
> > > +       if (virt_addr != LOAD_PHYSICAL_ADDR)
> > >                 error("Destination virtual address changed when not relocatable");
> > >  #endif
> > >
> > > diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
> > > index 1c8355e..766a521 100644
> > > --- a/arch/x86/boot/compressed/misc.h
> > > +++ b/arch/x86/boot/compressed/misc.h
> > > @@ -81,8 +81,6 @@ static inline void choose_random_location(unsigned long input,
> > >                                           unsigned long output_size,
> > >                                           unsigned long *virt_addr)
> > >  {
> > > -       /* No change from existing output location. */
> > > -       *virt_addr = *output;
> > >  }
> > >  #endif
> > >
> > > --
> > > 2.5.5
> > >
> > 
> > 
> > 
> > -- 
> > Kees Cook
> > Pixel Security

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web