| Message ID | 20220228114254.1099945-4-dovmurik@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Allow guest access to EFI confidential computing secret area | expand |
On 28/02/2022 14:49, Ard Biesheuvel wrote: > On Mon, 28 Feb 2022 at 12:43, Dov Murik <dovmurik@linux.ibm.com> wrote: >> >> If the efi_secret module is built, register a late_initcall in the EFI >> driver which checks whether the EFI secret area is available and >> populated, and then requests to load the efi_secret module. >> >> This will cause the <securityfs>/secrets/coco directory to appear in >> guests into which secrets were injected; in other cases, the module is >> not loaded. >> >> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com> >> Reviewed-by: Gerd Hoffmann <kraxel@redhat.com> > > It would be better to simply expose a platform device and associated > driver, instead of hooking into the module machinery directly. > > We already do something similar for the EFI rtc and the efivars > subsystem, using platform_device_register_simple() > Thanks Ard, I'll look into this. I hope the mechanism you suggest allows me to perform complex check to see if the device is really there (in this case: check for an efi variable, map memory as encrypted, verify it starts with a specific GUID -- everything before request_module() in the code below). -Dov > >> --- >> drivers/firmware/efi/Makefile | 1 + >> drivers/firmware/efi/coco.c | 58 ++++++++++++++++++++ >> drivers/virt/coco/efi_secret/Kconfig | 3 + >> 3 files changed, 62 insertions(+) >> >> diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile >> index c02ff25dd477..49c4a8c0bfc4 100644 >> --- a/drivers/firmware/efi/Makefile >> +++ b/drivers/firmware/efi/Makefile >> @@ -32,6 +32,7 @@ obj-$(CONFIG_APPLE_PROPERTIES) += apple-properties.o >> obj-$(CONFIG_EFI_RCI2_TABLE) += rci2-table.o >> obj-$(CONFIG_EFI_EMBEDDED_FIRMWARE) += embedded-firmware.o >> obj-$(CONFIG_LOAD_UEFI_KEYS) += mokvar-table.o >> +obj-$(CONFIG_EFI_COCO_SECRET) += coco.o >> >> fake_map-y += fake_mem.o >> fake_map-$(CONFIG_X86) += x86_fake_mem.o >> diff --git a/drivers/firmware/efi/coco.c b/drivers/firmware/efi/coco.c >> new file mode 100644 >> index 000000000000..f8efd240ab05 >> --- /dev/null >> +++ b/drivers/firmware/efi/coco.c >> @@ -0,0 +1,58 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Confidential computing (coco) secret area handling >> + * >> + * Copyright (C) 2021 IBM Corporation >> + * Author: Dov Murik <dovmurik@linux.ibm.com> >> + */ >> + >> +#define pr_fmt(fmt) "efi: " fmt >> + >> +#include <linux/efi.h> >> +#include <linux/init.h> >> +#include <linux/io.h> >> +#include <linux/kmod.h> >> + >> +#ifdef CONFIG_EFI_SECRET_MODULE >> + >> +/* >> + * Load the efi_secret module if the EFI secret area is populated >> + */ >> +static int __init load_efi_secret_module(void) >> +{ >> + struct linux_efi_coco_secret_area *area; >> + efi_guid_t *header_guid; >> + int ret = 0; >> + >> + if (efi.coco_secret == EFI_INVALID_TABLE_ADDR) >> + return 0; >> + >> + area = memremap(efi.coco_secret, sizeof(*area), MEMREMAP_WB); >> + if (!area) { >> + pr_err("Failed to map confidential computing secret area descriptor\n"); >> + return -ENOMEM; >> + } >> + if (!area->base_pa || area->size < sizeof(*header_guid)) >> + goto unmap_desc; >> + >> + header_guid = (void __force *)ioremap_encrypted(area->base_pa, sizeof(*header_guid)); >> + if (!header_guid) { >> + pr_err("Failed to map secret area\n"); >> + ret = -ENOMEM; >> + goto unmap_desc; >> + } >> + if (efi_guidcmp(*header_guid, EFI_SECRET_TABLE_HEADER_GUID)) >> + goto unmap_encrypted; >> + >> + ret = request_module("efi_secret"); >> + >> +unmap_encrypted: >> + iounmap((void __iomem *)header_guid); >> + >> +unmap_desc: >> + memunmap(area); >> + return ret; >> +} >> +late_initcall(load_efi_secret_module); >> + >> +#endif >> diff --git a/drivers/virt/coco/efi_secret/Kconfig b/drivers/virt/coco/efi_secret/Kconfig >> index 4404d198f3b2..dc8da2921e36 100644 >> --- a/drivers/virt/coco/efi_secret/Kconfig >> +++ b/drivers/virt/coco/efi_secret/Kconfig >> @@ -14,3 +14,6 @@ config EFI_SECRET >> >> To compile this driver as a module, choose M here. >> The module will be called efi_secret. >> + >> + The module is loaded automatically by the EFI driver if the EFI >> + secret area is populated. >> -- >> 2.25.1 >>
On Thu, 31 Mar 2022 at 11:05, Dov Murik <dovmurik@linux.ibm.com> wrote: > > Hello Ard, > > On 28/02/2022 15:15, Ard Biesheuvel wrote: > > On Mon, 28 Feb 2022 at 14:07, Dov Murik <dovmurik@linux.ibm.com> wrote: > >> > >> > >> > >> On 28/02/2022 14:49, Ard Biesheuvel wrote: > >>> On Mon, 28 Feb 2022 at 12:43, Dov Murik <dovmurik@linux.ibm.com> wrote: > >>>> > >>>> If the efi_secret module is built, register a late_initcall in the EFI > >>>> driver which checks whether the EFI secret area is available and > >>>> populated, and then requests to load the efi_secret module. > >>>> > >>>> This will cause the <securityfs>/secrets/coco directory to appear in > >>>> guests into which secrets were injected; in other cases, the module is > >>>> not loaded. > >>>> > >>>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com> > >>>> Reviewed-by: Gerd Hoffmann <kraxel@redhat.com> > >>> > >>> It would be better to simply expose a platform device and associated > >>> driver, instead of hooking into the module machinery directly. > >>> > >>> We already do something similar for the EFI rtc and the efivars > >>> subsystem, using platform_device_register_simple() > >>> > >> > >> Thanks Ard, I'll look into this. > >> > >> I hope the mechanism you suggest allows me to perform complex check to > >> see if the device is really there (in this case: check for an efi > >> variable, map memory as encrypted, verify it starts with a specific GUID > >> -- everything before request_module() in the code below). > >> > > > > There is the device part and the driver part. Some of this belongs in > > the core code that registers the platform device, and some of it > > belongs in the driver. At this point, it probably does not matter that > > much which side does what, as the platform driver simply probes and > > can perform whatever check it needs, as long as it can back out > > gracefully (although I understand that, in this particular case, there > > are reasons why the driver may decide to wipe the secret) > > I finally got to implement this, it seems like it makes the code simple. > Thanks for the advice. > > Just making sure I understand correctly: in this approach this we rely > on udev to load the efi_secret module (aliased as "platform:efi_secret") > and call its .probe() function? If there's no udev, the module will not > be loaded automatically. Did I understand that correctly? > Apologies, I am swamped in email and only spotted this now. This looks good to me: is this what you implemented in the end?
On 12/04/2022 16:08, Ard Biesheuvel wrote: > On Thu, 31 Mar 2022 at 11:05, Dov Murik <dovmurik@linux.ibm.com> wrote: >> >> Hello Ard, >> >> On 28/02/2022 15:15, Ard Biesheuvel wrote: >>> On Mon, 28 Feb 2022 at 14:07, Dov Murik <dovmurik@linux.ibm.com> wrote: >>>> >>>> >>>> >>>> On 28/02/2022 14:49, Ard Biesheuvel wrote: >>>>> On Mon, 28 Feb 2022 at 12:43, Dov Murik <dovmurik@linux.ibm.com> wrote: >>>>>> >>>>>> If the efi_secret module is built, register a late_initcall in the EFI >>>>>> driver which checks whether the EFI secret area is available and >>>>>> populated, and then requests to load the efi_secret module. >>>>>> >>>>>> This will cause the <securityfs>/secrets/coco directory to appear in >>>>>> guests into which secrets were injected; in other cases, the module is >>>>>> not loaded. >>>>>> >>>>>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com> >>>>>> Reviewed-by: Gerd Hoffmann <kraxel@redhat.com> >>>>> >>>>> It would be better to simply expose a platform device and associated >>>>> driver, instead of hooking into the module machinery directly. >>>>> >>>>> We already do something similar for the EFI rtc and the efivars >>>>> subsystem, using platform_device_register_simple() >>>>> >>>> >>>> Thanks Ard, I'll look into this. >>>> >>>> I hope the mechanism you suggest allows me to perform complex check to >>>> see if the device is really there (in this case: check for an efi >>>> variable, map memory as encrypted, verify it starts with a specific GUID >>>> -- everything before request_module() in the code below). >>>> >>> >>> There is the device part and the driver part. Some of this belongs in >>> the core code that registers the platform device, and some of it >>> belongs in the driver. At this point, it probably does not matter that >>> much which side does what, as the platform driver simply probes and >>> can perform whatever check it needs, as long as it can back out >>> gracefully (although I understand that, in this particular case, there >>> are reasons why the driver may decide to wipe the secret) >> >> I finally got to implement this, it seems like it makes the code simple. >> Thanks for the advice. >> >> Just making sure I understand correctly: in this approach this we rely >> on udev to load the efi_secret module (aliased as "platform:efi_secret") >> and call its .probe() function? If there's no udev, the module will not >> be loaded automatically. Did I understand that correctly? >> > > Apologies, I am swamped in email and only spotted this now. > > This looks good to me: is this what you implemented in the end? Yes indeed. So this old patch 3 was replaced by this much simpler code in the next iteration (v9): diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c index 378d044b2463..b92eabc554e6 100644 --- a/drivers/firmware/efi/efi.c +++ b/drivers/firmware/efi/efi.c @@ -425,6 +425,9 @@ static int __init efisubsys_init(void) if (efi_enabled(EFI_DBG) && efi_enabled(EFI_PRESERVE_BS_REGIONS)) efi_debugfs_init(); + if (efi.coco_secret != EFI_INVALID_TABLE_ADDR) + platform_device_register_simple("efi_secret", 0, NULL, 0); + return 0; err_remove_group: (this is were I missed the #ifdef CONFIG_EFI_COCO_SECRET ). Thanks again for the suggestion. -Dov
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile index c02ff25dd477..49c4a8c0bfc4 100644 --- a/drivers/firmware/efi/Makefile +++ b/drivers/firmware/efi/Makefile @@ -32,6 +32,7 @@ obj-$(CONFIG_APPLE_PROPERTIES) += apple-properties.o obj-$(CONFIG_EFI_RCI2_TABLE) += rci2-table.o obj-$(CONFIG_EFI_EMBEDDED_FIRMWARE) += embedded-firmware.o obj-$(CONFIG_LOAD_UEFI_KEYS) += mokvar-table.o +obj-$(CONFIG_EFI_COCO_SECRET) += coco.o fake_map-y += fake_mem.o fake_map-$(CONFIG_X86) += x86_fake_mem.o diff --git a/drivers/firmware/efi/coco.c b/drivers/firmware/efi/coco.c new file mode 100644 index 000000000000..f8efd240ab05 --- /dev/null +++ b/drivers/firmware/efi/coco.c @@ -0,0 +1,58 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Confidential computing (coco) secret area handling + * + * Copyright (C) 2021 IBM Corporation + * Author: Dov Murik <dovmurik@linux.ibm.com> + */ + +#define pr_fmt(fmt) "efi: " fmt + +#include <linux/efi.h> +#include <linux/init.h> +#include <linux/io.h> +#include <linux/kmod.h> + +#ifdef CONFIG_EFI_SECRET_MODULE + +/* + * Load the efi_secret module if the EFI secret area is populated + */ +static int __init load_efi_secret_module(void) +{ + struct linux_efi_coco_secret_area *area; + efi_guid_t *header_guid; + int ret = 0; + + if (efi.coco_secret == EFI_INVALID_TABLE_ADDR) + return 0; + + area = memremap(efi.coco_secret, sizeof(*area), MEMREMAP_WB); + if (!area) { + pr_err("Failed to map confidential computing secret area descriptor\n"); + return -ENOMEM; + } + if (!area->base_pa || area->size < sizeof(*header_guid)) + goto unmap_desc; + + header_guid = (void __force *)ioremap_encrypted(area->base_pa, sizeof(*header_guid)); + if (!header_guid) { + pr_err("Failed to map secret area\n"); + ret = -ENOMEM; + goto unmap_desc; + } + if (efi_guidcmp(*header_guid, EFI_SECRET_TABLE_HEADER_GUID)) + goto unmap_encrypted; + + ret = request_module("efi_secret"); + +unmap_encrypted: + iounmap((void __iomem *)header_guid); + +unmap_desc: + memunmap(area); + return ret; +} +late_initcall(load_efi_secret_module); + +#endif diff --git a/drivers/virt/coco/efi_secret/Kconfig b/drivers/virt/coco/efi_secret/Kconfig index 4404d198f3b2..dc8da2921e36 100644 --- a/drivers/virt/coco/efi_secret/Kconfig +++ b/drivers/virt/coco/efi_secret/Kconfig @@ -14,3 +14,6 @@ config EFI_SECRET To compile this driver as a module, choose M here. The module will be called efi_secret. + + The module is loaded automatically by the EFI driver if the EFI + secret area is populated.