Message-ID: <20170706132827.GJ19994@x1>
Date:   Thu, 6 Jul 2017 21:28:27 +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 07/05/17 at 12:06pm, Kees Cook wrote:
> On Tue, Jun 27, 2017 at 4:24 PM, Baoquan He <bhe@...hat.com> wrote:
> > 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:
> 
> Well, it's only sometimes separated. When it's still tied together,
> the (identity-mapped) physical randomization _is_ the virtual
> randomization.

I am a little confused about this. I can't think of a case that
in x86 64 virtual address and physical address need be tied together to
do randomization. And in what conditions they are separated, could you
give cases with a detailed address value?

But in x86 32bit, it does couple physical and virtual address
randomization together. Because it just randomizes to get an value
between 16M and 512M, then use the delta between 16M and the random
value to do the relocation handling for virtual address.

Just write my understanding at below, please add comment or point out
mistakes of mine.
> 
> > 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;
> >         }
> 
> On x86_64, the kernel is built so that if the virt address matches
> LOAD_PHYSICAL_ADDR, we don't have to do relocations.

Agreed.

> 
> > 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.
> 
> Let's address this specific issue (below).
> 
> >
> >>
> >> 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'.
> 
> So, this isn't always true. The output address is the address assigned
> by head_*.S for the extraction destination. We can't just change that,
> since it's based on where to perform the extraction, etc. All the
> logic in there is designed to make sure we're actually >=
> LOAD_PHYSICAL_ADDR:
> 
>         cmpl    $LOAD_PHYSICAL_ADDR, %ebx
>         jge     1f
> #endif
>         movl    $LOAD_PHYSICAL_ADDR, %ebx
> 1:

Yes, here it's trying to do an adjustment. To align the original loading
address if it's not aligned to CONFIG_PHYSICAL_ALIGN, and to make it be
LOAD_PHYSICAL_ADDR if it's smaller than LOAD_PHYSICAL_ADDR. Other than
those, nothing else be done. So let me conclude the possible original
'output' value:
1) default case
  It's LOAD_PHYSICAL_ADDR Which is the default loading address.
2) kexec/kdump
  The output will be at the top of the available physical ram.
3) Modified bootloader
  Which could put kernel anywhere by modifying code of bootloader. This
  is the case that we need do above alignment and adjustment to avoid
  unpleasant kernel error.

Except of above 3 cases, physical address of kernel could be changed by
physical address randomization.
4) physical address randomization

However, for virtual address, there are only two chances to decide:
1) arch/x86/kernel/vmlinux.lds.S
  we just map .text at __START_KERNEL
#define __PHYSICAL_START        ALIGN(CONFIG_PHYSICAL_START, \                                                                                   
                                      CONFIG_PHYSICAL_ALIGN)

#define __START_KERNEL          (__START_KERNEL_map + __PHYSICAL_START)
2) handle_relocations :: boot/compressed/misc.c
In handle_relocations(), we DO pass in the physical 'output', but that
is used to calculate 'map' which is used to position those memory
unit where the referenced address need be relocated. 
  
So later, in arch/x86/kernel/head_64.S, we will store the delta which is
between 16M and the actual kernel loading address into phys_base. So as
long as we can translate physical address and virtual address of kernel
by below formula, everything is going very well. 

    y = x - __START_KERNEL_map + phys_base.
   
We really don't need to tie physical address and virtual address of
kernel together in any cases. They can be randomized or not randomized
completely separately. As long as we keep above translation formula
working well, everything need not be worried.
> 
> We can't undo that since the calculation performed there is critical
> to getting it into the right place.
> 
> >> >
> >> >         /* 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
> 
> So, the code in question is this:
> 
> static void handle_relocations(void *output, unsigned long output_len,
>                                unsigned long virt_addr)
> {
>         int *reloc;
>         unsigned long delta, map, ptr;
>         unsigned long min_addr = (unsigned long)output;
>         unsigned long max_addr = min_addr + (VO___bss_start - VO__text);
> 
>         /*
>          * Calculate the delta between where vmlinux was linked to load
>          * and where it was actually loaded.
>          */
>         delta = min_addr - LOAD_PHYSICAL_ADDR;
> 
>         /*
>          * The kernel contains a table of relocation addresses. Those
>          * addresses have the final load address of the kernel in virtual
>          * memory. We are currently working in the self map. So we need to
>          * create an adjustment for kernel memory addresses to the self map.
>          * This will involve subtracting out the base address of the kernel.
>          */
>         map = delta - __START_KERNEL_map;
> 
>         /*
>          * 32-bit always performs relocations. 64-bit relocations are only
>          * needed if KASLR has chosen a different starting address offset
>          * from __START_KERNEL_map.
>          */
>         if (IS_ENABLED(CONFIG_X86_64))
>                 delta = virt_addr - LOAD_PHYSICAL_ADDR;
> 
>         if (!delta) {
>                 debug_putstr("No relocation needed... ");
>                 return;
>         }
>         debug_putstr("Performing relocations... ");
> 
> The first use of delta is to figure out the correct relocation offset,
> which is based on the relocation tables, etc.
Yes
> 
> The second use is for decoupled phys/virt (x86_64 only), but I don't
> think these need to be separate, do they? I think "delta" should only
> ever operate on the virt_addr, yes?

Yes. And if yes, for x86_64, there's nothing related to physical
address, here physical address is only used to find out the ram unit
where relocation need be done.

> 
> in misc.c, this chooses a physical location and a virtual:
> 
>         choose_random_location((unsigned long)input_data, input_len,
>                                 (unsigned long *)&output,
>                                 max(output_len, kernel_total_size),
>                                 &virt_addr);
> 
> Then we place the kernel in the physical location:
> 
>         __decompress(input_data, input_len, NULL, NULL, output, output_len,
>                         NULL, error);
> 
> And finally handle relocations:
> 
>         handle_relocations(output, output_len, virt_addr);
> 
> "output" is the identity-mapped physical (and possibly virtual)
> location, and virt_addr is the virtual.

No, output here is physical address, and yes., it's also virtual address
which is identity mappped. But when we do the relocation handling, we
can only think it as physical address. Here it's virtual only because we
need to operate on ram unit under protection mode with paging enabled.

> 
> So, it looks to me like the calculations at the top of
> handle_relocations() may not be correct for the case you're hitting?
> 
> What are the values there in that case? We must not hard-code output,
> so handle_relocations() has to be entirely self-contained with its
> logic.

Well, I may get a little bit what confusion you might have. First of
all, that 'virt_addr' is not the real virtual address, it's just an
offset in [0xffffffff80000000], 0xffffffffc0000000). Its value range is
between 16M and 1G. And for physical address 'output', we need avoid
many things, initramfs, kernel cmdline, etc. While for virt_addr, we
don't need to avoid anything, because it's just an offset and we can map
kernel from any places to 'virt_addr'. And secondly, hard-coding 'virt_add'
is only hard-coding the virtual randomzation offset, it won't impact
'output' at all. 'ouput' doesn't rely on and isn't related to
'virt_addr' at all, for x86 64.

> 
> I actually think the bug might be that choose_random_location() in the
> nokaslr case doesn't explicitly set virt_addr to output, like the
> no-op does:


> 
> void choose_random_location(unsigned long input,
>                             unsigned long input_size,
>                             unsigned long *output,
>                             unsigned long output_size,
>                             unsigned long *virt_addr)
> {
>         unsigned long random_addr, min_addr;
> 
>         if (cmdline_find_option_bool("nokaslr")) {
>                 warn("KASLR disabled: 'nokaslr' on cmdline.");
>                 return;
>         }
> 
> Perhaps we need a few fixes:
> 
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index 91f27ab970ef..e1e57c626093 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -627,6 +627,7 @@ void choose_random_location(unsigned long input,
> 
>         if (cmdline_find_option_bool("nokaslr")) {
>                 warn("KASLR disabled: 'nokaslr' on cmdline.");
> +               *virt_addr = *output;
>                 return;
>         }
> 
> diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> index 00241c815524..3eb754e4f823 100644
> --- a/arch/x86/boot/compressed/misc.c
> +++ b/arch/x86/boot/compressed/misc.c
> @@ -198,12 +198,13 @@ static void handle_relocations(void *output,
> unsigned long output_len,
>          * needed if KASLR has chosen a different starting address offset
>          * from __START_KERNEL_map.
>          */
> -       if (IS_ENABLED(CONFIG_X86_64))
> +       if (IS_ENABLED(CONFIG_X86_64)) {
>                 delta = virt_addr - LOAD_PHYSICAL_ADDR;
> 
> -       if (!delta) {
> -               debug_putstr("No relocation needed... ");
> -               return;
> +               if (!delta) {
> +                       debug_putstr("No relocation needed... ");
> +                       return;
> +               }
>         }
>         debug_putstr("Performing relocations... ");
> 
> 
> 
> 
> 
> -- 
> Kees Cook
> Pixel Security

Powered by blists - more mailing lists