Re: [PATCH 1/2] boot: increase stack size for kernel boot loader decompressor

[Alexander van Heukelum]

On Tue, 8 Apr 2008 10:54:15 -0700, "Yinghai Lu" <yhlu.kernel@xxxxxxxxx>
said:
> On Tue, Apr 8, 2008 at 1:23 AM, Ingo Molnar <mingo@xxxxxxx> wrote:
> >
> >  * Alexander van Heukelum <heukelum@xxxxxxxxxxxxx> wrote:
> >
> >  > I did see that the malloc space that the inflate code is using is
> >  > taken from _after_ the end of the bss. I don't see how this is
> >  > protected from being used/overwritten. Changing the stack size changes
> >  > the memory layout a bit... maybe you were so unlucky to create a
> >  > vmlinux image that was just barely smaller than some threshold and
> >  > increasing the stack size made the decompression/relocation area be
> >  > located somewhere else?
> >  >
> >  > Test patch follows.
> >
> >  that's a really interesting theory.
> >
> >  FWIIW, i've been booting allyesconfig bzImages for a long time (with
> >  only minimal amount of drivers disabled - mostly old ISA ones that
> >  assume the presence of the real hardware), and they boot and work fine
> >  on both 32-bit and 64-bit typical whitebox PCs. That means huge bzImages
> >  that decompresses into a ~41 MB kernel image. I'd expect that to be a
> >  rather severe test of the decompressor.
> 
> i don't that Alexander's patch is needed.

Hello Yinghai Lu,

Indeed, I now think it is not needed either. The decompression is
done in-place nowadays: the (compressed) image is moved to a high
memory address first, then the decompression is done starting at
the low end of the buffer. It is guaranteed that the output never
overwrites the input, and the decompression code, the stack, and
the heap are all at higher addresses than the input buffer. The
same goes for the pagetables needed for x86_64.

> also because Alex move heap before _end,
> we may need add some extra for buffer offset
> 
>         /* Replace the compressed data size with the uncompressed size */
>         subl    input_len(%ebp), %ebx
>         movl    output_len(%ebp), %eax
>         addl    %eax, %ebx
>         /* Add 8 bytes for every 32K input block */
>         shrl    $12, %eax
>         addl    %eax, %ebx
>         /* Add 32K + 18 bytes of extra slack and align on a 4K boundary
>         */
>         addl    $(32768 + 18 + 4095), %ebx
>         andl    $~4095, %ebx =============================> need add
> heap size too.
> ....

No, that size is accounted for automatically: the code computes the
buffer size needed (including slack) minus the buffer size that is
already available (in the embedded gzip-file). The image is moved
by this amount (rounded up to a page). So that part is fine.

> 
> 
>         /* Replace the compressed data size with the uncompressed size */
>         movl    input_len(%rip), %eax
>         subq    %rax, %rbx
>         movl    output_len(%rip), %eax
>         addq    %rax, %rbx
>         /* Add 8 bytes for every 32K input block */
>         shrq    $12, %rax
>         addq    %rax, %rbx
>         /* Add 32K + 18 bytes of extra slack and align on a 4K boundary
>         */
>         addq    $(32768 + 18 + 4095), %rbx
> =============================> need add heap size too.
>         andq    $~4095, %rbx
> 
> do we need to move pgtable before _end?

I just tried, but it fails: The pgtable is built and enabled in the
32-bit
setup code, but the kernel image is moved in the 64-bit part...
overwriting
the pagetable with zeroes ;).

I can't think of an obvious safe place to put the pagetables, though.
One
option is to move the image in the 32-bit code and tell the 64-bit part
somehow not to do it again... by calling into the 64-bit code at a
different
place, for example.

Greetings,
    Alexander

> YH
-- 
  Alexander van Heukelum
  heukelum@xxxxxxxxxxx

-- 
http://www.fastmail.fm - Send your email first class

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/