| Message ID | 20220201124413.1093099-5-dovmurik@linux.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Allow guest access to EFI confidential computing secret area | expand |
On Tue, Feb 01, 2022 at 12:44:12PM +0000, Dov Murik 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. > + 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; Why these sanity checks are here and not in the efi_secret module? take care, Gerd
On 02/02/2022 10:47, Gerd Hoffmann wrote: > On Tue, Feb 01, 2022 at 12:44:12PM +0000, Dov Murik 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. > >> + 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; > > Why these sanity checks are here and not in the efi_secret module? > The same checks indeed appear in the efi_secret module (see in patch 3: efi_secret_map_area() and the beginning of efi_secret_securityfs_setup()). However, in the efi_secret module, the checks are noisy, because they expect the secret area to be populated. For example: + if (efi.coco_secret == EFI_INVALID_TABLE_ADDR) { + pr_err("Secret area address is not available\n"); + return -EINVAL; + } ... + if (efi_guidcmp(h->guid, EFI_SECRET_TABLE_HEADER_GUID)) { + pr_err("EFI secret area does not start with correct GUID\n"); + ret = -EINVAL; + goto err_cleanup; + } ... Whereas here (patch 4, in efi/coco.c) the EFI driver checks if there a populated secret area, and if there is one, triggers the efi_secret module load. Another approach could be to just try to load the module anyway, and the module will fail (silently? noisily?) if there's no designated secret area or it's not populated. I feel that will be harder to understand what's going on. I'm open to suggestions in case this duplication is too bad (maybe efi_secret can expose some kind of probe() function that can be called before actually initializing it?) Thanks, -Dov
On Wed, Feb 02, 2022 at 01:08:43PM +0200, Dov Murik wrote: > > > On 02/02/2022 10:47, Gerd Hoffmann wrote: > > On Tue, Feb 01, 2022 at 12:44:12PM +0000, Dov Murik 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. > > > >> + 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; > > > > Why these sanity checks are here and not in the efi_secret module? > > The same checks indeed appear in the efi_secret module (see in patch 3: > efi_secret_map_area() and the beginning of efi_secret_securityfs_setup()). > > However, in the efi_secret module, the checks are noisy, because they > expect the secret area to be populated. For example: > > + if (efi.coco_secret == EFI_INVALID_TABLE_ADDR) { > + pr_err("Secret area address is not available\n"); > + return -EINVAL; > + } Note I explicitly excluded that check ;) Checking whenever efi.coco_secret looks valid and only try load efi_secret if that is the case (and otherwise stay silent) makes perfect sense. The other checks should be dropped IMHO. > Another approach could be to just try to load the module anyway, and > the module will fail (silently? noisily?) if there's no designated > secret area or it's not populated. I feel that will be harder to > understand what's going on. I think the module should fail noisily. See above for autoload. In case the module is loaded (either manually by the admin, or because efi.coco_secret != EFI_INVALID_TABLE_ADDR) and it can't actually load the secrets we want know why ... take care, Gerd
On 02/02/2022 16:31, Gerd Hoffmann wrote: > On Wed, Feb 02, 2022 at 01:08:43PM +0200, Dov Murik wrote: >> >> >> On 02/02/2022 10:47, Gerd Hoffmann wrote: >>> On Tue, Feb 01, 2022 at 12:44:12PM +0000, Dov Murik 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. >>> >>>> + 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; >>> >>> Why these sanity checks are here and not in the efi_secret module? >> >> The same checks indeed appear in the efi_secret module (see in patch 3: >> efi_secret_map_area() and the beginning of efi_secret_securityfs_setup()). >> >> However, in the efi_secret module, the checks are noisy, because they >> expect the secret area to be populated. For example: >> >> + if (efi.coco_secret == EFI_INVALID_TABLE_ADDR) { >> + pr_err("Secret area address is not available\n"); >> + return -EINVAL; >> + } > > Note I explicitly excluded that check ;) > > Checking whenever efi.coco_secret looks valid and only try load > efi_secret if that is the case (and otherwise stay silent) makes > perfect sense. The other checks should be dropped IMHO. > >> Another approach could be to just try to load the module anyway, and >> the module will fail (silently? noisily?) if there's no designated >> secret area or it's not populated. I feel that will be harder to >> understand what's going on. > > I think the module should fail noisily. See above for autoload. In > case the module is loaded (either manually by the admin, or because > efi.coco_secret != EFI_INVALID_TABLE_ADDR) and it can't actually load > the secrets we want know why ... > Note that the AmdSev build of OVMF always publishes LINUX_EFI_COCO_SECRET_TABLE_GUID in the EFI table. Even when LAUNCH_SECRET was not executed. In such cases the secret area will be empty. If we keep only the 'efi.coco_secret != EFI_INVALID_TABLE_ADDR' check, we'll get errors from efi_secret for every VM launch that doesn't undergo LAUNCH_SECRET. I don't think that's good. If we *do* want to check that the area starts with EFI_SECRET_TABLE_HEADER_GUID (like I think we should), we need all the checks before that, like checking that the area is big enough, and that all the memremap()s succeed -- before actually comparing the header_guid. The checks are basically prerequisites for calling efi_guidcmp() safely. -Dov
Hi, > > I think the module should fail noisily. See above for autoload. In > > case the module is loaded (either manually by the admin, or because > > efi.coco_secret != EFI_INVALID_TABLE_ADDR) and it can't actually load > > the secrets we want know why ... > > Note that the AmdSev build of OVMF always publishes > LINUX_EFI_COCO_SECRET_TABLE_GUID in the EFI table. Even when > LAUNCH_SECRET was not executed. In such cases the secret area will be > empty. Hmm, ok. Why? I assume the secret area is filled by the host and ovmf doesn't even look at it? > If we keep only the 'efi.coco_secret != EFI_INVALID_TABLE_ADDR' check, > we'll get errors from efi_secret for every VM launch that doesn't > undergo LAUNCH_SECRET. I don't think that's good. Well, if that is a common case the module could either print nothing or log KERN_INFO level instead of KERN_ERROR. > If we *do* want to check that the area starts with > EFI_SECRET_TABLE_HEADER_GUID (like I think we should), we need all the > checks before that, like checking that the area is big enough, and that > all the memremap()s succeed -- before actually comparing the > header_guid. The checks are basically prerequisites for calling > efi_guidcmp() safely. It is still not fully clear to me why you want do that check twice. take care, Gerd
On 03/02/2022 8:16, Gerd Hoffmann wrote: > Hi, > >>> I think the module should fail noisily. See above for autoload. In >>> case the module is loaded (either manually by the admin, or because >>> efi.coco_secret != EFI_INVALID_TABLE_ADDR) and it can't actually load >>> the secrets we want know why ... >> >> Note that the AmdSev build of OVMF always publishes >> LINUX_EFI_COCO_SECRET_TABLE_GUID in the EFI table. Even when >> LAUNCH_SECRET was not executed. In such cases the secret area will be >> empty. > > Hmm, ok. Why? I assume the secret area is filled by the host and ovmf > doesn't even look at it? > Exactly. OVMF just reserves this area, and puts its address+size in the EFI config table. It doesn't care about its format and usage. There are currently two "users" for the actual data in this memory area: 1. grub's efisecret module (which reads the disk passphrase from an entry in the secret area) 2. linux's efi_secret module (which we're discussing here) >> If we keep only the 'efi.coco_secret != EFI_INVALID_TABLE_ADDR' check, >> we'll get errors from efi_secret for every VM launch that doesn't >> undergo LAUNCH_SECRET. I don't think that's good. > > Well, if that is a common case the module could either print nothing or > log KERN_INFO level instead of KERN_ERROR. > What if the user doesn't inject a secret and doesn't include the efi_secret module at all in the initrd? request_module("efi_secret") will fail. I can ignore the error code of request_module("efi_secret") but that feels bad. >> If we *do* want to check that the area starts with >> EFI_SECRET_TABLE_HEADER_GUID (like I think we should), we need all the >> checks before that, like checking that the area is big enough, and that >> all the memremap()s succeed -- before actually comparing the >> header_guid. The checks are basically prerequisites for calling >> efi_guidcmp() safely. > > It is still not fully clear to me why you want do that check twice. > I want to load the module only if secrets were injected by the Guest Owner. Again, I'm open to ideas on how to de-duplicate these early checks, if that's important. -Dov
Hi, > >> If we keep only the 'efi.coco_secret != EFI_INVALID_TABLE_ADDR' check, > >> we'll get errors from efi_secret for every VM launch that doesn't > >> undergo LAUNCH_SECRET. I don't think that's good. > > > > Well, if that is a common case the module could either print nothing or > > log KERN_INFO level instead of KERN_ERROR. > > What if the user doesn't inject a secret and doesn't include the > efi_secret module at all in the initrd? request_module("efi_secret") > will fail. > > I can ignore the error code of request_module("efi_secret") but that > feels bad. Looking at the error code returned by request_module should help to figure what happened (module load failed / no secret present / something else). But, yes, module load errors are harmless in case there is no secret present in the first place. Hmm, tricky. I don't see a way to solve that without duplicating the checks. I withdraw my objections. Reviewed-by: Gerd Hoffmann <kraxel@redhat.com> take care, Gerd
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 ed181797ed86..068fdd1fccd7 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.
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>/coco/efi_secret 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> --- drivers/firmware/efi/Makefile | 1 + drivers/firmware/efi/coco.c | 58 ++++++++++++++++++++++++++++ drivers/virt/coco/efi_secret/Kconfig | 3 ++ 3 files changed, 62 insertions(+) create mode 100644 drivers/firmware/efi/coco.c