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 <[email protected]>
---
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
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 <[email protected]>
+ */
+
+#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.
--
2.25.1
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
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 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 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 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 <[email protected]>
take care,
Gerd