Re: [PATCH 1/2] x86/boot/compressed/64: Remove .bss/.pgtable from bzImage

[Arvind Sankar <nivedita@alum.mit.edu>]

On Mon, Apr 06, 2020 at 01:20:42PM +0200, Borislav Petkov wrote:
> On Mon, Apr 06, 2020 at 11:11:21AM +0200, Ard Biesheuvel wrote:
> > Yes, it is in the PE/COFF specification. [0]
> > 
> > The whole problem is that we are conflating 'loading a PE/COFF image'
> > with 'copying a PE/COFF image into memory', which are not the same
> > thing. It is not just the layout issue, we are running into other
> > problems with things like UEFI secure boot and TPM-based measured
> > boot, where the fact that omitting the standard LoadImage() boot
> > service (which takes care of these things under the hood) means that
> > you now have to do your own checks and measurements. These things are
> > literally all over the place at the moment, shim, GRUB, systemd-boot
> > etc, with no authoritative spec that describes which component should
> > be doing what.
> 
> Sounds to me like what LoadImage() does is what the authoritative spec
> should be. Perhaps we should write it down as "Do what LoadImage()
> does... " and then enumerate the requirements.
> 
> > Commit ec93fc371f014a6fb483e3556061ecad4b40735c has the background, but ...
> 
> Nice, I like the aspect of letting firmware do only a minimum amount of
> work. :)
> 
> > ... I'll look into updating the documentation as well.
> 
> Thanks!
> 
> > Note that this stuff is hot off the press, so there may be some issues
> > lurking (like this one) that we hadn't thought of yet.
> 
> Right.
> 
> > Actually, it may be sufficient to #define __efistub_global to
> > __section(.data) like we already do for ARM, to ensure that these
> > global flags are always initialized correctly. (I'll wait for Sergey
> > to confirm that the spurious enabling of the PCI DMA protection
> > resulting from this BSS issue is causing the boot regression)

Yeah I thought of that as the easiest fix, but it might be safer to
explicitly zero-init in efi_main to avoid future problems in case
someone adds another variable in bss and isn't aware of this obscure
requirement. We actually already have sys_table in bss, but that one is
always initialized. There's also other globals that aren't annotated
(but not in bss by virtue of having initializers). What do you think?

What do you think of the other problem -- that's actually worse to fix,
as it won't just be when kaslr is disabled, the startup_64 code will do
relocation to the end of init_size and clobber the initrd before getting
to the kaslr code, so it will break as soon as the firmware loads the
"unified kernel image" at a 2Mb-aligned address. The only thing I can
think of is to just unconditionally call efi_relocate_kernel if we were
entered via handover_entry?

> 
> Cool, but let's not jinx it. :-)
> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette