Message-ID: <20170627233320.GB15537@x1>
Date:   Wed, 28 Jun 2017 07:33:20 +0800
From:   Baoquan He <bhe@...hat.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "Kees Cook <keescook@...omium.org, Yinghai Lu" <yinghai@...nel.org>,
        Arnd Bergmann <arnd@...db.de>,
        Thomas Garnier <thgarnie@...gle.com>
Subject: Re: [PATCH] x86/boot/KASLR: Skip relocation handling in no kaslr case

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@...hat.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

Powered by blists - more mailing lists