From: Hans de Goede <hdegoede@redhat.com>
To: Ard Biesheuvel <ardb@kernel.org>, linux-efi@vger.kernel.org
Cc: Matthew Garrett <matthewgarrett@google.com>,
Ingo Molnar <mingo@kernel.org>, Andy Lutomirski <luto@kernel.org>,
Thomas Gleixner <tglx@linutronix.de>,
Arvind Sankar <nivedita@alum.mit.edu>,
Matthew Garrett <mjg59@google.com>
Subject: Re: [PATCH v2 21/21] efi: Allow disabling PCI busmastering on bridges during boot
Date: Thu, 6 Feb 2020 15:30:59 +0100 [thread overview]
Message-ID: <0a31756e-d554-ff1c-5809-130d516714ae@redhat.com> (raw)
In-Reply-To: <20191218170139.9468-22-ardb@kernel.org>
Hi,
On 12/18/19 6:01 PM, Ard Biesheuvel wrote:
> From: Matthew Garrett <matthewgarrett@google.com>
>
> Add an option to disable the busmaster bit in the control register on
> all PCI bridges during the invocation of ExitBootServices() and passing
> control to the runtime kernel. System firmware may configure the IOMMU
> to prevent malicious PCI devices from being able to attack the OS via DMA.
> However, since firmware can't guarantee that the OS is IOMMU-aware, it
> will tear down IOMMU configuration when ExitBootServices() is called.
> This leaves a window between where a hostile device could still cause
> damage before Linux configures the IOMMU again.
>
> If CONFIG_EFI_DISABLE_PCI_DMA is enabled or the "efi=disable_pci_dma"
> command line argument is passed, the EFI stub will clear the busmaster
> bit on all PCI bridges before ExitBootServices() completes. This will
> prevent any malicious PCI devices from being able to perform DMA until
> the kernel reenables busmastering after configuring the IOMMU.
>
> This option is disabled when in EFI mixed mode environments (ie, 64-bit
> kernels with a 32-bit EFI implementation), given that the use of EFI
> events is not supported in this case.
>
> This option may cause failures with some poorly behaved hardware and
> should not be enabled without testing. The kernel commandline options
> "efi=disable_pci_dma" or "efi=no_disable_pci_dma" may be used to
> override the default.
>
> Co-developed-by: Matthew Garrett <mjg59@google.com>
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> [ardb: use EFI events to defer DMA disabling to the end of ExitBootServices()]
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
I guess this might not be the latest version of this patch, but
this does seem to be the thread where most discussion on it
has happened.
My personal kernel tree atm consists of v5.5 + efi/next + my own patches
and yesterday I noticed that will not boot on a Lenovo X1 7th gen connected
to a Lenovo thunderbolt 3 gen 2 dock.
My first hunch was that I have CONFIG_EFI_DISABLE_PCI_DMA=y and that that
was causing it and indeed that is the problem.
So as (somewhat) expected CONFIG_EFI_DISABLE_PCI_DMA=y indeed stops the kernel
from booting on some systems.
When I hit this problem the efistub prints 2 messages and then the system
just hangs:
exit_boot() failed!
efi_main() failed!
When I boot the system without it being connected to the thunderbolt dock
then efi=disable_pci_dma works fine.
Let me know if I can do anything to help and getting booting while
connected to the dock to work with efi=disable_pci_dma.
Regards,
Hans
> ---
> Documentation/admin-guide/kernel-parameters.txt | 4 +
> arch/x86/boot/compressed/eboot.c | 2 +
> drivers/firmware/efi/Kconfig | 22 ++++
> drivers/firmware/efi/libstub/Makefile | 2 +-
> drivers/firmware/efi/libstub/arm-stub.c | 1 +
> drivers/firmware/efi/libstub/efi-stub-helper.c | 17 +++
> drivers/firmware/efi/libstub/efistub.h | 1 +
> drivers/firmware/efi/libstub/pci.c | 116 ++++++++++++++++++++
> include/linux/efi.h | 2 +
> 9 files changed, 166 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 5a92d89a1bd4..06fd8d2c9a2c 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1180,6 +1180,10 @@
> claim. Specify efi=nosoftreserve to disable this
> reservation and treat the memory by its base type
> (i.e. EFI_CONVENTIONAL_MEMORY / "System RAM").
> + disable_pci_dma: Disable the busmaster bit on all
> + PCI bridges while in the EFI boot stub
> + no_disable_pci_dma: Leave the busmaster bit set on all
> + PCI bridges while in the EFI boot stub
>
> efi_no_storage_paranoia [EFI; X86]
> Using this parameter you can use more than 50% of
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index a3000283dc7a..79c3c3308195 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -126,6 +126,8 @@ static void setup_efi_pci(struct boot_params *params)
> efi_handle_t h;
> int i;
>
> + efi_pci_disable_bridge_busmaster();
> +
> status = efi_call_early(locate_handle,
> EFI_LOCATE_BY_PROTOCOL,
> &pci_proto, NULL, &size, pci_handle);
> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> index bcc378c19ebe..da2f5fc862aa 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -215,6 +215,28 @@ config EFI_RCI2_TABLE
>
> Say Y here for Dell EMC PowerEdge systems.
>
> +config EFI_DISABLE_PCI_DMA
> + bool "Clear Busmaster bit on PCI bridges during ExitBootServices()"
> + help
> + Disable the busmaster bit in the control register on all PCI bridges
> + while calling ExitBootServices() and passing control to the runtime
> + kernel. System firmware may configure the IOMMU to prevent malicious
> + PCI devices from being able to attack the OS via DMA. However, since
> + firmware can't guarantee that the OS is IOMMU-aware, it will tear
> + down IOMMU configuration when ExitBootServices() is called. This
> + leaves a window between where a hostile device could still cause
> + damage before Linux configures the IOMMU again.
> +
> + If you say Y here, the EFI stub will clear the busmaster bit on all
> + PCI bridges before ExitBootServices() completes. This will prevent
> + any malicious PCI devices from being able to perform DMA until the
> + kernel reenables busmastering after configuring the IOMMU.
> +
> + This option will cause failures with some poorly behaved hardware
> + and should not be enabled without testing. The kernel commandline
> + options "efi=disable_pci_dma" or "efi=no_disable_pci_dma" may be
> + used to override this option.
> +
> endmenu
>
> config UEFI_CPER
> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> index c35f893897e1..98a81576213d 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -39,7 +39,7 @@ OBJECT_FILES_NON_STANDARD := y
> KCOV_INSTRUMENT := n
>
> lib-y := efi-stub-helper.o gop.o secureboot.o tpm.o \
> - random.o
> + random.o pci.o
>
> # include the stub's generic dependencies from lib/ when building for ARM/arm64
> arm-deps-y := fdt_rw.c fdt_ro.c fdt_wip.c fdt.c fdt_empty_tree.c fdt_sw.c
> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> index e1ec0b2cde29..aa2929098d60 100644
> --- a/drivers/firmware/efi/libstub/arm-stub.c
> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> @@ -257,6 +257,7 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table_arg,
> }
>
> install_memreserve_table();
> + efi_pci_disable_bridge_busmaster();
>
> new_fdt_addr = fdt_addr;
> status = allocate_new_fdt_and_exit_boot(handle,
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index 8b0d59f5c8cd..4b79bfff1fd4 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -33,6 +33,8 @@ static bool __section(.data) efi_nokaslr;
> static bool __section(.data) efi_quiet;
> static bool __section(.data) efi_novamap;
> static bool __section(.data) efi_nosoftreserve;
> +static bool __section(.data) efi_disable_pci_dma =
> + IS_ENABLED(CONFIG_EFI_DISABLE_PCI_DMA);
>
> bool __pure nokaslr(void)
> {
> @@ -51,6 +53,11 @@ bool __pure __efi_soft_reserve_enabled(void)
> return !efi_nosoftreserve;
> }
>
> +bool __pure disable_pci_dma(void)
> +{
> + return efi_disable_pci_dma;
> +}
> +
> #define EFI_MMAP_NR_SLACK_SLOTS 8
>
> struct file_info {
> @@ -499,6 +506,16 @@ efi_status_t efi_parse_options(char const *cmdline)
> efi_nosoftreserve = true;
> }
>
> + if (!strncmp(str, "disable_pci_dma", 15)) {
> + str += strlen("disable_pci_dma");
> + efi_disable_pci_dma = true;
> + }
> +
> + if (!strncmp(str, "no_disable_pci_dma", 18)) {
> + str += strlen("no_disable_pci_dma");
> + efi_disable_pci_dma = false;
> + }
> +
> /* Group words together, delimited by "," */
> while (*str && *str != ' ' && *str != ',')
> str++;
> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> index 72e60b6afbf8..679268f67521 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -28,6 +28,7 @@
> extern bool __pure nokaslr(void);
> extern bool __pure is_quiet(void);
> extern bool __pure novamap(void);
> +extern bool __pure disable_pci_dma(void);
>
> extern __pure efi_system_table_t *efi_system_table(void);
>
> diff --git a/drivers/firmware/efi/libstub/pci.c b/drivers/firmware/efi/libstub/pci.c
> new file mode 100644
> index 000000000000..e2f71f890dd1
> --- /dev/null
> +++ b/drivers/firmware/efi/libstub/pci.c
> @@ -0,0 +1,116 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PCI-related functions used by the EFI stub on multiple
> + * architectures.
> + *
> + * Copyright 2019 Google, LLC
> + */
> +
> +#include <linux/efi.h>
> +#include <linux/pci.h>
> +
> +#include <asm/efi.h>
> +
> +#include "efistub.h"
> +
> +static efi_event_t __section(.data) exit_boot_services_event;
> +static efi_event_t __section(.data) disable_pci_dma_event;
> +
> +static efi_handle_t *__section(.data) pci_handle;
> +static unsigned long __section(.data) pci_handle_size;
> +
> +static efi_guid_t pci_proto = EFI_PCI_IO_PROTOCOL_GUID;
> +
> +static void __efiapi handle_exit_boot_services_event(efi_event_t event,
> + void *context)
> +{
> + efi_call_early(signal_event, disable_pci_dma_event);
> +}
> +
> +static void __efiapi handle_disable_pci_dma_event(efi_event_t event,
> + void *context)
> +{
> + efi_handle_t handle;
> + efi_status_t status;
> + u16 class, command;
> + int i;
> +
> + for_each_efi_handle(handle, pci_handle, pci_handle_size, i) {
> + efi_pci_io_protocol_t *pci = NULL;
> +
> + status = efi_call_early(handle_protocol, handle,
> + &pci_proto, (void **)&pci);
> + if (status != EFI_SUCCESS || !pci)
> + continue;
> +
> + status = efi_call_proto(pci, pci.read, EfiPciIoWidthUint16,
> + PCI_CLASS_DEVICE, 1, &class);
> +
> + if (status != EFI_SUCCESS || class != PCI_CLASS_BRIDGE_PCI)
> + continue;
> +
> + /* Disable busmastering */
> + status = efi_call_proto(pci, pci.read, EfiPciIoWidthUint16,
> + PCI_COMMAND, 1, &command);
> + if (status != EFI_SUCCESS || !(command & PCI_COMMAND_MASTER))
> + continue;
> +
> + command &= ~PCI_COMMAND_MASTER;
> + status = efi_call_proto(pci, pci.write, EfiPciIoWidthUint16,
> + PCI_COMMAND, 1, &command);
> + if (status != EFI_SUCCESS)
> + pr_efi_err("Failed to disable PCI busmastering\n");
> + }
> +}
> +
> +void efi_pci_disable_bridge_busmaster(void)
> +{
> + efi_status_t status;
> +
> + if (!disable_pci_dma() || !efi_is_native())
> + return;
> +
> + status = efi_call_early(locate_handle, EFI_LOCATE_BY_PROTOCOL,
> + &pci_proto, NULL, &pci_handle_size, pci_handle);
> +
> + if (status != EFI_BUFFER_TOO_SMALL) {
> + if (status != EFI_SUCCESS && status != EFI_NOT_FOUND)
> + pr_efi_err("Failed to locate PCI I/O handles'\n");
> + return;
> + }
> +
> + status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
> + pci_handle_size, (void **)&pci_handle);
> + if (status != EFI_SUCCESS) {
> + pr_efi_err("Failed to allocate memory for 'pci_handle'\n");
> + return;
> + }
> +
> + status = efi_call_early(locate_handle, EFI_LOCATE_BY_PROTOCOL,
> + &pci_proto, NULL, &pci_handle_size, pci_handle);
> + if (status != EFI_SUCCESS) {
> + pr_efi_err("Failed to locate PCI I/O handles'\n");
> + goto free_handle;
> + }
> +
> + status = efi_call_early(create_event, EVT_NOTIFY_SIGNAL,
> + TPL_CALLBACK, handle_disable_pci_dma_event,
> + NULL, &disable_pci_dma_event);
> + if (status != EFI_SUCCESS) {
> + pr_efi_err("Failed to create event to disable PCI DMA\n");
> + goto free_handle;
> + }
> +
> + status = efi_call_early(create_event, EVT_SIGNAL_EXIT_BOOT_SERVICES,
> + TPL_CALLBACK, handle_exit_boot_services_event,
> + NULL, &exit_boot_services_event);
> + if (status != EFI_SUCCESS) {
> + pr_efi_err("Failed to register for EBS() event\n");
> + goto free_handle;
> + }
> +
> + return;
> +
> +free_handle:
> + efi_call_early(free_pool, pci_handle);
> +}
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 81bd1e8c201f..2864633abff0 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1786,4 +1786,6 @@ struct linux_efi_memreserve {
> #define EFI_MEMRESERVE_COUNT(size) (((size) - sizeof(struct linux_efi_memreserve)) \
> / sizeof(((struct linux_efi_memreserve *)0)->entry[0]))
>
> +void efi_pci_disable_bridge_busmaster(void);
> +
> #endif /* _LINUX_EFI_H */
>
next prev parent reply other threads:[~2020-02-06 14:31 UTC|newest]
Thread overview: 87+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-18 17:01 [PATCH v2 00/21] efi/x86: confine type unsafe casting to mixed mode Ard Biesheuvel
2019-12-18 17:01 ` [PATCH v2 01/21] efi/libstub: remove unused __efi_call_early() macro Ard Biesheuvel
2019-12-18 17:01 ` [PATCH v2 02/21] efi/x86: rename efi_is_native() to efi_is_mixed() Ard Biesheuvel
2019-12-18 17:01 ` [PATCH v2 03/21] efi/libstub: use a helper to iterate over a EFI handle array Ard Biesheuvel
2019-12-18 17:01 ` [PATCH v2 04/21] efi/libstub: extend native protocol definitions with mixed_mode aliases Ard Biesheuvel
2019-12-18 17:01 ` [PATCH v2 05/21] efi/libstub: distinguish between native/mixed not 32/64 bit Ard Biesheuvel
2019-12-18 17:01 ` [PATCH v2 06/21] efi/libstub/x86: use mixed mode helpers to populate efi_config Ard Biesheuvel
2019-12-18 17:01 ` [PATCH v2 07/21] efi/libstub: drop explicit 32/64-bit protocol definitions Ard Biesheuvel
2019-12-18 17:01 ` [PATCH v2 08/21] efi/libstub: use stricter typing for firmware function pointers Ard Biesheuvel
2019-12-18 17:01 ` [PATCH v2 09/21] efi/libstub: annotate firmware routines as __efiapi Ard Biesheuvel
2019-12-18 17:01 ` [PATCH v2 10/21] efi/libstub/x86: avoid thunking for native firmware calls Ard Biesheuvel
2019-12-21 21:22 ` Hans de Goede
2019-12-22 12:02 ` Ard Biesheuvel
2019-12-22 12:37 ` Ard Biesheuvel
2019-12-22 12:46 ` Andy Lutomirski
2019-12-22 15:29 ` Ard Biesheuvel
2019-12-22 21:12 ` Arvind Sankar
2019-12-22 21:25 ` Ard Biesheuvel
2019-12-23 11:49 ` Hans de Goede
2019-12-23 12:00 ` Ard Biesheuvel
2019-12-18 17:01 ` [PATCH v2 11/21] efi/libstub: get rid of 'sys_table_arg' macro parameter Ard Biesheuvel
2019-12-18 17:01 ` [PATCH v2 12/21] efi/libstub: unify the efi_char16_printk implementations Ard Biesheuvel
2019-12-18 17:01 ` [PATCH v2 13/21] efi/libstub/x86: drop __efi_early() export of efi_config struct Ard Biesheuvel
2019-12-24 19:34 ` Hans de Goede
2019-12-25 14:42 ` Ard Biesheuvel
2019-12-27 22:44 ` Hans de Goede
2019-12-27 22:51 ` Ard Biesheuvel
2019-12-31 23:04 ` Arvind Sankar
2020-01-01 18:13 ` Ard Biesheuvel
2020-01-01 19:08 ` Arvind Sankar
2020-01-02 7:33 ` Ard Biesheuvel
2020-01-02 14:06 ` Arvind Sankar
2020-01-02 15:20 ` Ard Biesheuvel
2020-01-02 15:51 ` Arvind Sankar
2020-01-02 15:58 ` Ard Biesheuvel
2020-01-02 16:28 ` Ard Biesheuvel
2020-01-02 16:59 ` Ard Biesheuvel
2020-01-02 17:26 ` Arvind Sankar
2020-01-02 17:30 ` Ard Biesheuvel
2020-01-02 17:41 ` Arvind Sankar
2020-01-02 17:48 ` Ard Biesheuvel
2020-01-02 18:10 ` Arvind Sankar
2020-01-02 18:38 ` Ard Biesheuvel
2020-01-03 14:16 ` Arvind Sankar
2020-01-03 14:23 ` Ard Biesheuvel
2020-01-02 18:38 ` Arvind Sankar
2020-01-02 16:59 ` Arvind Sankar
2020-01-02 17:03 ` Ard Biesheuvel
2020-01-02 17:21 ` Arvind Sankar
2019-12-18 17:01 ` [PATCH v2 14/21] efi/libstub: drop sys_table_arg from printk routines Ard Biesheuvel
2019-12-18 17:01 ` [PATCH v2 15/21] efi/libstub: remove 'sys_table_arg' from all function prototypes Ard Biesheuvel
2019-12-18 17:01 ` [PATCH v2 16/21] efi/libstub: drop protocol argument from efi_call_proto() macro Ard Biesheuvel
2019-12-18 17:01 ` [PATCH v2 17/21] efi/libstub: drop 'table' argument from efi_table_attr() macro Ard Biesheuvel
2019-12-18 17:01 ` [PATCH v2 18/21] efi/libstub: use 'func' not 'f' as macro parameter Ard Biesheuvel
2019-12-31 16:51 ` Arvind Sankar
2019-12-31 17:06 ` Ard Biesheuvel
2019-12-31 17:36 ` Arvind Sankar
2019-12-18 17:01 ` [PATCH v2 19/21] efi/libstub: tidy up types and names of global cmdline variables Ard Biesheuvel
2019-12-18 17:01 ` [PATCH v2 20/21] efi/libstub: import type definitions for creating and signalling events Ard Biesheuvel
2019-12-18 17:01 ` [PATCH v2 21/21] efi: Allow disabling PCI busmastering on bridges during boot Ard Biesheuvel
2019-12-19 2:50 ` Andy Lutomirski
2019-12-19 13:17 ` Ard Biesheuvel
2019-12-19 20:04 ` Matthew Garrett
2019-12-19 20:04 ` Matthew Garrett
2019-12-20 7:06 ` Ard Biesheuvel
2019-12-20 7:17 ` Andy Lutomirski
2019-12-20 8:11 ` Ard Biesheuvel
2019-12-20 19:41 ` Arvind Sankar
2020-01-02 14:46 ` Laszlo Ersek
2020-01-02 15:40 ` Ard Biesheuvel
2019-12-20 20:43 ` Matthew Garrett
2019-12-21 16:44 ` Ard Biesheuvel
2019-12-21 21:24 ` Matthew Garrett
2019-12-21 22:54 ` Arvind Sankar
2019-12-23 14:02 ` Ard Biesheuvel
2019-12-23 15:46 ` Arvind Sankar
2019-12-23 15:58 ` Ard Biesheuvel
2019-12-23 16:12 ` Arvind Sankar
2019-12-23 20:57 ` Matthew Garrett
2020-02-06 14:30 ` Hans de Goede [this message]
2020-02-06 14:35 ` Ard Biesheuvel
2020-03-04 10:38 ` Hans de Goede
2020-03-04 18:26 ` Ard Biesheuvel
2020-03-04 18:49 ` Hans de Goede
2020-03-04 21:59 ` Ard Biesheuvel
2019-12-19 11:12 ` [PATCH v2 00/21] efi/x86: confine type unsafe casting to mixed mode Hans de Goede
2019-12-19 13:22 ` Ard Biesheuvel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0a31756e-d554-ff1c-5809-130d516714ae@redhat.com \
--to=hdegoede@redhat.com \
--cc=ardb@kernel.org \
--cc=linux-efi@vger.kernel.org \
--cc=luto@kernel.org \
--cc=matthewgarrett@google.com \
--cc=mingo@kernel.org \
--cc=mjg59@google.com \
--cc=nivedita@alum.mit.edu \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).