Re: [PATCH v3 02/19] x86, boot: Move compressed kernel to end of buffer before decompressing

[Baoquan He]

Hi Kees,

Many thanks for your review and suggestions!

On 03/07/16 at 02:35pm, Kees Cook wrote:
> > Current code uses extract_offset to decide the copied ZO position, it
> > put ZO starting from extract_offset. When INIT_SIZE is bigger than
> > VO_INIT_SIZE copied ZO occupies space from extract_offset to the end
> > of decompressing buffer, just like below.
> >
> > 0                       extract_offset                       init_size
> > |-----------|---------------|------------------------|---------|
> >             |               |                        |         |
> >           VO__text      startup_32 of ZO         VO__end     ZO__end
> >
> > However when INIT_SIZE is euqal to VO_INIT_SIZE there's still space left
> 
> typo: "equal"

Yes, will change it in next post.

> 
> > from end of ZO to the end of decompressing buffer, like below.
> >
> > 0                       extract_offset                       init_size
> > |-----------|---------------|------------------------|---------|
> >             |               |                        |         |
> >           VO__text      startup_32 of ZO         ZO__end     VO__end
> >
> > It's better to always put ZO right in the end of decompressing buffer.
> > Anyway, who doesn't like it to be better.
> 
> It looks to me like it's just cleaner to always have ZO__end ==
> VO__end? Is that the motivation for moving it? What problem is solved
> with this change?

As you can see there are 2 cases. The first case is depicted in the 1st
graph. In that case ZO__end is after VO__end. In the 2nd case, yes, we
move ZO__end to make that happen ZO__end == VO__end. In fact this patch
is not trying to make them equal, what it is trying to do is adjusting
the behaviour. We always put ZO just in the end by getting init_size
which is the deompressing buffer and ZO_INIT_SIZE, not like before we
just put ZO starting from extract_offset. We change a different way to
get the starting position of ZO.
> 
> -Kees
> 
> >
> > So in this patch add BP_init_size into asm-offsets.c to make it visible
> > to asm modules. And find the starting position of copied ZO by init_size
> > and ZO run size, namely (ZO__end - startup_32) when move ZO.
> >
> > Signed-off-by: Yinghai Lu <yinghai@xxxxxxxxxx>
> > ---
> > v2->v3:
> >     Adjust the patch log.
> >
> >  arch/x86/boot/compressed/head_32.S     | 11 +++++++++--
> >  arch/x86/boot/compressed/head_64.S     |  8 ++++++--
> >  arch/x86/boot/compressed/mkpiggy.c     |  3 ---
> >  arch/x86/boot/compressed/vmlinux.lds.S |  1 +
> >  arch/x86/kernel/asm-offsets.c          |  1 +
> >  arch/x86/kernel/vmlinux.lds.S          |  1 +
> >  6 files changed, 18 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S
> > index 8ef964d..0c140f9 100644
> > --- a/arch/x86/boot/compressed/head_32.S
> > +++ b/arch/x86/boot/compressed/head_32.S
> > @@ -148,7 +148,9 @@ preferred_addr:
> >  1:
> >
> >         /* Target address to relocate to for decompression */
> > -       addl    $z_extract_offset, %ebx
> > +       movl    BP_init_size(%esi), %eax
> > +       subl    $_end, %eax
> > +       addl    %eax, %ebx
> >
> >         /* Set up the stack */
> >         leal    boot_stack_end(%ebx), %esp
> > @@ -210,8 +212,13 @@ relocated:
> >                                 /* push arguments for decompress_kernel: */
> >         pushl   $z_run_size     /* size of kernel with .bss and .brk */
> >         pushl   $z_output_len   /* decompressed length, end of relocs */
> > -       leal    z_extract_offset_negative(%ebx), %ebp
> > +
> > +       movl    BP_init_size(%esi), %eax
> > +       subl    $_end, %eax
> > +       movl    %ebx, %ebp
> > +       subl    %eax, %ebp
> >         pushl   %ebp            /* output address */
> > +
> >         pushl   $z_input_len    /* input_len */
> >         leal    input_data(%ebx), %eax
> >         pushl   %eax            /* input_data */
> > diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
> > index b0c0d16..67dd8d3 100644
> > --- a/arch/x86/boot/compressed/head_64.S
> > +++ b/arch/x86/boot/compressed/head_64.S
> > @@ -102,7 +102,9 @@ ENTRY(startup_32)
> >  1:
> >
> >         /* Target address to relocate to for decompression */
> > -       addl    $z_extract_offset, %ebx
> > +       movl    BP_init_size(%esi), %eax
> > +       subl    $_end, %eax
> > +       addl    %eax, %ebx
> >
> >  /*
> >   * Prepare for entering 64 bit mode
> > @@ -330,7 +332,9 @@ preferred_addr:
> >  1:
> >
> >         /* Target address to relocate to for decompression */
> > -       leaq    z_extract_offset(%rbp), %rbx
> > +       movl    BP_init_size(%rsi), %ebx
> > +       subl    $_end, %ebx
> > +       addq    %rbp, %rbx
> >
> >         /* Set up the stack */
> >         leaq    boot_stack_end(%rbx), %rsp
> > diff --git a/arch/x86/boot/compressed/mkpiggy.c b/arch/x86/boot/compressed/mkpiggy.c
> > index d8222f2..b980046 100644
> > --- a/arch/x86/boot/compressed/mkpiggy.c
> > +++ b/arch/x86/boot/compressed/mkpiggy.c
> > @@ -85,9 +85,6 @@ int main(int argc, char *argv[])
> >         printf("z_output_len = %lu\n", (unsigned long)olen);
> >         printf(".globl z_extract_offset\n");
> >         printf("z_extract_offset = 0x%lx\n", offs);
> > -       /* z_extract_offset_negative allows simplification of head_32.S */
> > -       printf(".globl z_extract_offset_negative\n");
> > -       printf("z_extract_offset_negative = -0x%lx\n", offs);
> >         printf(".globl z_run_size\n");
> >         printf("z_run_size = %lu\n", run_size);
> >
> > diff --git a/arch/x86/boot/compressed/vmlinux.lds.S b/arch/x86/boot/compressed/vmlinux.lds.S
> > index 34d047c..e24e0a0 100644
> > --- a/arch/x86/boot/compressed/vmlinux.lds.S
> > +++ b/arch/x86/boot/compressed/vmlinux.lds.S
> > @@ -70,5 +70,6 @@ SECTIONS
> >                 _epgtable = . ;
> >         }
> >  #endif
> > +       . = ALIGN(PAGE_SIZE);   /* keep ZO size page aligned */
> >         _end = .;
> >  }
> > diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
> > index 84a7524..506de14 100644
> > --- a/arch/x86/kernel/asm-offsets.c
> > +++ b/arch/x86/kernel/asm-offsets.c
> > @@ -81,6 +81,7 @@ void common(void) {
> >         OFFSET(BP_hardware_subarch, boot_params, hdr.hardware_subarch);
> >         OFFSET(BP_version, boot_params, hdr.version);
> >         OFFSET(BP_kernel_alignment, boot_params, hdr.kernel_alignment);
> > +       OFFSET(BP_init_size, boot_params, hdr.init_size);
> >         OFFSET(BP_pref_address, boot_params, hdr.pref_address);
> >         OFFSET(BP_code32_start, boot_params, hdr.code32_start);
> >
> > diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
> > index 74e4bf1..217d5b1 100644
> > --- a/arch/x86/kernel/vmlinux.lds.S
> > +++ b/arch/x86/kernel/vmlinux.lds.S
> > @@ -325,6 +325,7 @@ SECTIONS
> >                 __brk_limit = .;
> >         }
> >
> > +       . = ALIGN(PAGE_SIZE);           /* keep VO_INIT_SIZE page aligned */
> >         _end = .;
> >
> >          STABS_DEBUG
> > --
> > 2.5.0
> >
> 
> 
> 
> -- 
> Kees Cook
> Chrome OS & Brillo Security