[PATCH] efi/libstub: Disable PCI DMA before grabbing the EFI memory map

[PATCH] efi/libstub: Disable PCI DMA before grabbing the EFI memory map

[Ard Biesheuvel]

Currently, the EFI stub will disable PCI DMA as the very last thing it
does before calling ExitBootServices(), to avoid interfering with the
firmware's normal operation as much as possible.

However, the stub will invoke DisconnectController() on all endpoints
downstream of the PCI bridges it disables, and this may affect the
layout of the EFI memory map, making it likely that ExitBootServices()
will fail the first time around, and that the EFI memory map needs to be
reloaded.

This, in turn, increases the likelihood that the slack space we
allocated is insufficient (and we can no longer allocate memory via boot
services after having called ExitBootServices() once), causing the
second call to GetMemoryMap (and therefore the boot) to fail. This makes
the PCI DMA disable feature a bit more fragile than it already is, so
let's make it more robust, by allocating the space for the EFI memory
map after disabling PCI DMA.

Cc: Matthew Garrett <mjg59@srcf.ucam.org>
Cc: Daniel Kiper <dkiper@net-space.pl>
Reported-by: Glenn Washburn <development@efficientek.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/libstub/efi-stub-helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 51779279fbff21b5..bfa30625f5d03167 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -380,6 +380,9 @@ efi_status_t efi_exit_boot_services(void *handle, void *priv,
 	struct efi_boot_memmap *map;
 	efi_status_t status;
 
+	if (efi_disable_pci_dma)
+		efi_pci_disable_bridge_busmaster();
+
 	status = efi_get_memory_map(&map, true);
 	if (status != EFI_SUCCESS)
 		return status;
@@ -390,9 +393,6 @@ efi_status_t efi_exit_boot_services(void *handle, void *priv,
 		return status;
 	}
 
-	if (efi_disable_pci_dma)
-		efi_pci_disable_bridge_busmaster();
-
 	status = efi_bs_call(exit_boot_services, handle, map->map_key);
 
 	if (status == EFI_INVALID_PARAMETER) {
-- 
2.39.2

Re: [PATCH] efi/libstub: Disable PCI DMA before grabbing the EFI memory map

[Matthew Garrett]

On Tue, Jun 27, 2023 at 09:41:32AM +0200, Ard Biesheuvel wrote:

> However, the stub will invoke DisconnectController() on all endpoints
> downstream of the PCI bridges it disables, and this may affect the
> layout of the EFI memory map, making it likely that ExitBootServices()
> will fail the first time around, and that the EFI memory map needs to be
> reloaded.

Isn't it always likely that ExitBootServices() will fail the first time 
around, but disable_early_pci_dma makes it more likely it'll have 
changed by enough that we need a bigger map? Other than that potential 
quibble over the changelog,

Acked-by: Matthew Garrett <mjg59@srcf.ucam.org>

Re: [PATCH] efi/libstub: Disable PCI DMA before grabbing the EFI memory map

[Ard Biesheuvel]

On Tue, 27 Jun 2023 at 10:00, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>
> On Tue, Jun 27, 2023 at 09:41:32AM +0200, Ard Biesheuvel wrote:
>
> > However, the stub will invoke DisconnectController() on all endpoints
> > downstream of the PCI bridges it disables, and this may affect the
> > layout of the EFI memory map, making it likely that ExitBootServices()
> > will fail the first time around, and that the EFI memory map needs to be
> > reloaded.
>
> Isn't it always likely that ExitBootServices() will fail the first time
> around, but disable_early_pci_dma makes it more likely it'll have
> changed by enough that we need a bigger map?

Not quite. It should only fail the first time if the memory map
changed since the last call to GetMemoryMap(), and normally, this will
only happen if some kind of asynchronous event was triggered after
GetMemoryMap() but before ExitBootServices(). (This is why calling
ExitBootServices() at most twice should always suffice: the first call
disables the timer interrupt, so the second time around, no events
will fire in the mean time)

In this case, we explicitly invoke boot services between
GetMemoryMap() and ExitBootServices(), making the first failure
substantially more likely.

> Other than that potential
> quibble over the changelog,
>
> Acked-by: Matthew Garrett <mjg59@srcf.ucam.org>

Thanks

Re: [PATCH] efi/libstub: Disable PCI DMA before grabbing the EFI memory map

[Matthew Garrett]

On Tue, Jun 27, 2023 at 10:14:16AM +0200, Ard Biesheuvel wrote:

> Not quite. It should only fail the first time if the memory map
> changed since the last call to GetMemoryMap(), and normally, this will
> only happen if some kind of asynchronous event was triggered after
> GetMemoryMap() but before ExitBootServices(). (This is why calling
> ExitBootServices() at most twice should always suffice: the first call
> disables the timer interrupt, so the second time around, no events
> will fire in the mean time)

Can't driver shutdown code also end up altering it? I've certainly had 
extremely deterministic requirements to call it twice, which doesn't 
line up terribly well with it just being down to async events.

Re: [PATCH] efi/libstub: Disable PCI DMA before grabbing the EFI memory map

[Ard Biesheuvel]

On Tue, 27 Jun 2023 at 10:17, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>
> On Tue, Jun 27, 2023 at 10:14:16AM +0200, Ard Biesheuvel wrote:
>
> > Not quite. It should only fail the first time if the memory map
> > changed since the last call to GetMemoryMap(), and normally, this will
> > only happen if some kind of asynchronous event was triggered after
> > GetMemoryMap() but before ExitBootServices(). (This is why calling
> > ExitBootServices() at most twice should always suffice: the first call
> > disables the timer interrupt, so the second time around, no events
> > will fire in the mean time)
>
> Can't driver shutdown code also end up altering it?

Yes, but doing so violates the UEFI spec:
EVT_SIGNAL_EXIT_BOOT_SERVICES is documented as not permitting the use
of memory allocation services, either directly or indirectly (via the
use of other external code that might use them)

> I've certainly had
> extremely deterministic requirements to call it twice, which doesn't
> line up terribly well with it just being down to async events.

Yeah, I guess you can still get the Windows sticker if you violate
this requirement :-)

Re: [PATCH] efi/libstub: Disable PCI DMA before grabbing the EFI memory map

[Matthew Garrett]

On Tue, Jun 27, 2023 at 10:32:36AM +0200, Ard Biesheuvel wrote:
> On Tue, 27 Jun 2023 at 10:17, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> >
> > On Tue, Jun 27, 2023 at 10:14:16AM +0200, Ard Biesheuvel wrote:
> >
> > > Not quite. It should only fail the first time if the memory map
> > > changed since the last call to GetMemoryMap(), and normally, this will
> > > only happen if some kind of asynchronous event was triggered after
> > > GetMemoryMap() but before ExitBootServices(). (This is why calling
> > > ExitBootServices() at most twice should always suffice: the first call
> > > disables the timer interrupt, so the second time around, no events
> > > will fire in the mean time)
> >
> > Can't driver shutdown code also end up altering it?
> 
> Yes, but doing so violates the UEFI spec:
> EVT_SIGNAL_EXIT_BOOT_SERVICES is documented as not permitting the use
> of memory allocation services, either directly or indirectly (via the
> use of other external code that might use them)

Maybe people have become better at observing that restriction! Anyway, 
feel free to ignore my nit in that case.

Re: [PATCH] efi/libstub: Disable PCI DMA before grabbing the EFI memory map

[Ard Biesheuvel]

On Tue, 27 Jun 2023 at 10:37, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>
> On Tue, Jun 27, 2023 at 10:32:36AM +0200, Ard Biesheuvel wrote:
> > On Tue, 27 Jun 2023 at 10:17, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > >
> > > On Tue, Jun 27, 2023 at 10:14:16AM +0200, Ard Biesheuvel wrote:
> > >
> > > > Not quite. It should only fail the first time if the memory map
> > > > changed since the last call to GetMemoryMap(), and normally, this will
> > > > only happen if some kind of asynchronous event was triggered after
> > > > GetMemoryMap() but before ExitBootServices(). (This is why calling
> > > > ExitBootServices() at most twice should always suffice: the first call
> > > > disables the timer interrupt, so the second time around, no events
> > > > will fire in the mean time)
> > >
> > > Can't driver shutdown code also end up altering it?
> >
> > Yes, but doing so violates the UEFI spec:
> > EVT_SIGNAL_EXIT_BOOT_SERVICES is documented as not permitting the use
> > of memory allocation services, either directly or indirectly (via the
> > use of other external code that might use them)
>
> Maybe people have become better at observing that restriction! Anyway,
> feel free to ignore my nit in that case.

I haven't dealt with actual production x86 hardware built to boot
Windows via EFI (or CSM) as much as you have, so my world view tends
to be a bit naive when it comes to actual EFI compliance :-)