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

From: Ard Biesheuvel <ardb@kernel.org>
To: Arvind Sankar <nivedita@alum.mit.edu>
Cc: Sergey Shatunov <me@prok.pw>,
	bp@alien8.de, hpa@zytor.com,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	mingo@redhat.com, Thomas Gleixner <tglx@linutronix.de>,
	x86@kernel.org, linux-efi <linux-efi@vger.kernel.org>,
	initramfs@vger.kernel.org,
	Donovan Tremura <neurognostic@protonmail.ch>,
	Harald Hoyer <harald@hoyer.xyz>
Subject: Re: [PATCH 1/2] x86/boot/compressed/64: Remove .bss/.pgtable from bzImage
Date: Mon, 6 Apr 2020 09:32:47 +0200	[thread overview]
Message-ID: <CAMj1kXEUhyv886CjyKvjw2F12WaZxZRUWF6t_XzP4C2TJPdpeg@mail.gmail.com> (raw)
In-Reply-To: <20200406035110.GA3241052@rani.riverdale.lan>

On Mon, 6 Apr 2020 at 05:51, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Mon, Apr 06, 2020 at 07:00:39AM +0700, Sergey Shatunov wrote:
> > On Sun, 2020-04-05 at 19:18 -0400, Arvind Sankar wrote:
> > > I'm not familiar with systemd-boot: when you say systemd-boot stub,
> > > is
> > > that something different from the kernel's EFI_STUB option? Or is it
> > > just a kernel with EFI_STUB enabled and with builtin initramfs +
> > > builtin
> > > cmdline?
> > Basicaly systemd-boot stub is efi application with packed EFI_STUB-
> > enabled kernel, initrd and cmdline into single file. Source can be
> > found here:
> > https://github.com/systemd/systemd/blob/master/src/boot/efi/stub.c
> >
> > It doesn't do anything unusual, just extracting data from sections and
> > calling efi handover.
> >
> > Final image created by objcopy'ing precompiled stub and adding sections with that stuff:
> >
> >     objcopy \
> >         --add-section .osrel=os_release --change-section-vma
> > '.osrel=0x20000' \
> >         --add-section .cmdline=cmdline --change-section-vma
> > '.cmdline=0x30000' \
> >         --add-section .linux=vmlinuz --change-section-vma
> > '.linux=0x2000000' \
> >         --add-section .initrd=initrd --change-section-vma
> > '.initrd=0x3000000' \
> >         /usr/lib/systemd/boot/efi/linuxx64.efi.stub output.efi
>
> So this embeds the bzImage which is a PE executable inside another PE
> executable. Before my patch, the bss section was explicitly part of the
> bzImage and so would have been zero, now it isn't any more and the PE
> loader is expected to zero it out before executing. systemd-boot's stub
> loader doesn't do that prior to jumping to the EFI handover entry, so
> the issue must be because bss contains garbage.  I'm not 100% sure why
> that leads to a crash, as the only variables in bss in the EFI stub are
> for some boolean EFI command line arguments, so it ought to still have
> worked, just as though it was invoked with random arguments. Anyway we
> need to handle an uninitialized bss to get this to work properly.
>

The EFI handover protocol strikes again :-(

It seems we did not include any guidance in the documentation in
Documentation/x86/boot.rst regarding zero-initializing BSS, and come
to think of it, we don't include any other requirements either, i.e.,
regarding placement wrt section alignment etc. This is a serious bug.
Even though EFI usually lays out PE/COFF images in files the exact way
they appear in memory, this is not actually required by the spec. Most
notably, the virtual size can be smaller than the file size, and the
loader is expected to zero-initialize the difference as well.

Since the EFI handover protocol should be considered deprecated at
this point (and is never going to be supported in upstream GRUB
either, for instance), I would recommend the systemd-boot developers
to start looking into deprecating this as well, and switch to the
ordinary PE/COFF entry point, and use the new initrd callback protocol
for initrd loading.

On the Linux/x86 side, we should at least add some code to the EFI
handover protocol entry point to zero initialize BSS, and ensure that
it is either not needed in other places, or add the code to deal with
those as well.

> I also see from systemd [0] and dracut source [1] that these VMA's seem
> to be hardcoded with no checking for how big the files actually are, and
> objcopy doesn't seem to complain if sections end up overlapping.
>
> So since [2] in dracut, the space available for the .linux section
> containing the bzImage shrank from ~48MiB to 16MiB. This will hopefully
> still fit the compressed kernel (although an allyesconfig bzImage is far
> bigger than even 48MiB), but in-place decompression is unlikely to be
> possible even for a normal config, which will break another patchset
> that got merged into mainline for 5.7 [3,4], which tries to avoid
> copying the kernel unless necessary, and has a good chance of triggering
> in-place decompression if kaslr is disabled.
>
> I'll get systemd-boot installed here so I can reproduce and implement
> some workarounds for both issues. I should hopefully have a fix in a day
> or two.
>

Thanks Arvind.