Hello Ard,

On 28/02/2022 15:15, Ard Biesheuvel wrote:

On Mon, 28 Feb 2022 at 14:07, Dov Murik [off-list ref] wrote:

quoted

On 28/02/2022 14:49, Ard Biesheuvel wrote:

quoted

On Mon, 28 Feb 2022 at 12:43, Dov Murik [off-list ref] wrote:

quoted

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 <redacted>
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