Re: [PATCH v3] x86, efi: never relocate kernel below lowest acceptable address

[Kairui Song]

On Mon, Oct 14, 2019 at 6:14 PM Borislav Petkov <bp@xxxxxxxxx> wrote:
>
> On Sat, Oct 12, 2019 at 11:44:21AM +0800, Kairui Song wrote:
> > Currently, kernel fails to boot on some HyperV VMs when using EFI.
> > And it's a potential issue on all platforms.
> >
> > It's caused a broken kernel relocation on EFI systems, when below three
> > conditions are met:
> >
> > 1. Kernel image is not loaded to the default address (LOAD_PHYSICAL_ADDR)
> >    by the loader.
> > 2. There isn't enough room to contain the kernel, starting from the
> >    default load address (eg. something else occupied part the region).
> > 3. In the memmap provided by EFI firmware, there is a memory region
> >    starts below LOAD_PHYSICAL_ADDR, and suitable for containing the
> >    kernel.
> >
> > Efi stub will perform a kernel relocation when condition 1 is met. But
> > due to condition 2, efi stub can't relocate kernel to the preferred
> > address, so it fallback to query and alloc from EFI firmware for lowest
>
> Your spelling of "EFI" is like a random number generator in this
> paragraph: "Efi", "efi" and "EFI". Can you please be more careful when
> writing your commit messages? They're not some random text you hurriedly
> jot down before sending the patch but a most important description of
> why a change is being done.

Sorry I just ignored the acronym usage problems, I did double check the text but
didn't realize this is a problem... Will correct them.

>
> And if you don't see their importance now, just try doing some git
> archeology, trying to understand why a change has been done in the past
> and then encounter a commit message two-liner which doesn't say sh*t.
> Then you'll start appreciating properly written commit messages.
>
> > usable memory region.
> >
> > It's incorrect to use the lowest memory address. In later stage, kernel
> > will assume LOAD_PHYSICAL_ADDR as the minimal acceptable relocate address,
> > but efi stub will end up relocating kernel below it.
>
> Why don't you simply explain what
> choose_random_location()->find_random_virt_addr() does? That's the
> problem you're solving, right? KASLR using LOAD_PHYSICAL_ADDR as the
> minimum...
>
> > The later kernel decompressing code will forcefully correct the wrong
> > kernel load location,
>
> ... or do you mean by that the dance in
> arch/x86/boot/compressed/head_64.S where we move the kernel temporarily
> to LOAD_PHYSICAL_ADDR for the decompression?

The kernel move in arch/x86/boot/compressed/head_64.S is the problem
I'm saying here.

I thought it's a bad idea to include too much details about codes and details in
the commit message, so tried to describe it without mentioning the
implementation details.
It's making things confusing indeed.

I'll rethink about how the commit message should be composed...

>
> You can simply say that here...
>

OK, then I'll do so. Will update the commit message.

--
Best Regards,
Kairui Song