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

[Baoquan He <bhe@redhat.com>]

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