Message-ID: <CAMj1kXG+34-bK1XuxX5VopkRt1SV1ewUAEmif+aQj5cJQ=9vbA@mail.gmail.com>
Date:   Mon, 6 Apr 2020 15:29:18 +0200
From:   Ard Biesheuvel <ardb@...nel.org>
To:     Arvind Sankar <nivedita@...m.mit.edu>
Cc:     Borislav Petkov <bp@...en8.de>, Sergey Shatunov <me@...k.pw>,
        hpa@...or.com,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        mingo@...hat.com, Thomas Gleixner <tglx@...utronix.de>,
        x86@...nel.org, linux-efi <linux-efi@...r.kernel.org>,
        initramfs@...r.kernel.org,
        Donovan Tremura <neurognostic@...tonmail.ch>,
        Harald Hoyer <harald@...er.xyz>
Subject: Re: [PATCH 1/2] x86/boot/compressed/64: Remove .bss/.pgtable from bzImage

On Mon, 6 Apr 2020 at 15:22, Arvind Sankar <nivedita@...m.mit.edu> wrote:
>
> 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?
>

*If* we zero init BSS, I'd prefer for it to be done in the EFI
handover protocol entrypoint only. But does that fix the issue that
BSS lives outside of the memory footprint of the kernel image?


> 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?
>

Yes, that seems to be the most robust approach.

Powered by blists - more mailing lists