Re: [PATCH] x86/efistub: Add missing boot_params for mixed mode compat entry

[Ard Biesheuvel]

On Mon, 25 Mar 2024 at 15:18, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> Hi,
>
> On 3/25/24 9:39 AM, Ard Biesheuvel wrote:
> > From: Ard Biesheuvel <ardb@xxxxxxxxxx>
> >
> > The native EFI stub entry point does not take a struct boot_params from
> > the boot loader, but creates it from scratch, and populates only the
> > fields that still have meaning in this context (command line, initrd
> > base and size, etc)
> >
> > The original mixed mode implementation used the EFI handover protocol,
> > where the boot loader (i.e., GRUB) populates a struct boot_params and
> > passes it to a special EFI entry point that takes the struct boot_params
> > pointer as the third argument.
> >
> > When the new mixed mode implementation was introduced, using a special
> > 32-bit PE entrypoint in the 64-bit kernel, it adopted the usual
> > prototype, and relied on the EFI stub to create the struct boot_params
> > as usual. This is preferred because it makes the bootloader side much
> > easier to implement, as it does not need any x86-specific knowledge on
> > how struct boot_params and struct setup_header are put together.
> >
> > However, one thing was missed: EFI mixed mode goes through startup_32()
> > *before* entering the 64-bit EFI stub, which is difficult to avoid given
> > that 64-bit execution requires page tables, which can only be populated
> > using 32-bit code, and this piece is what the mixed mode EFI stub relies
> > on. startup_32() accesses a couple of struct boot_params fields to
> > decide where to place the page tables.
> >
> > startup_32() turns out to be quite tolerant to bogus struct boot_params,
> > given that ESI used to contain junk when entering via the new mixed mode
> > protocol. Only when commit
> >
> >   e2ab9eab324c ("x86/boot/compressed: Move 32-bit entrypoint code into .text section")
> >
> > started to zero ESI explicitly when entering via this boot path, boot
> > failures started to appear on some systems, presumably ones that unmap
> > page 0x0 or map it read-only.
> >
> > The solution is to pass a special, temporary struct boot_params to
> > startup_32() via ESI, one that is sufficient for getting it to create
> > the page tables correctly and is discarded right after. This means
> > setting a minimal alignment of 4k, only to get the statically allocated
> > page tables line up correctly, and setting init_size to the executable
> > image size (_end - startup_32). This ensures that the page tables are
> > covered by the static footprint of the PE image.
> >
> > Given that EFI boot no longer calls the decompressor and no longer pads
> > the image to permit the decompressor to execute in place, the same
> > temporary struct boot_params should be used in the EFI handover protocol
> > based mixed mode implementation as well, to prevent the page tables from
> > being placed outside of allocated memory.
> >
> > Cc: Hans de Goede <hdegoede@xxxxxxxxxx>
> > Fixes: e2ab9eab324c ("x86/boot/compressed: Move 32-bit entrypoint code into .text section")
> > Closes: https://lore.kernel.org/all/20240321150510.GI8211@xxxxxxxxxxxxx/
> > Reported-by: Clayton Craft <clayton@xxxxxxxxxxxxx>
> > Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
>
> I have given this a test run (on top of 6.9-rc1) on one of my
> Bay Trail mixed mode tablets and the tablet still boots fine:
>
> Tested-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>

Many thanks Hans.