| Message ID | 20220228114254.1099945-4-dovmurik@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Allow guest access to EFI confidential computing secret area | expand |
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() > --- > 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 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 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)
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? In such a case, the only thing needed to add in efi.c is: 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: Does this seem OK? (before I re-spin the whole series.) Thanks, -Dov
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.