Re: [PATCH v8 3/4] efi: Load efi_secret module if EFI secret area is populated

[Dov Murik <dovmurik@linux.ibm.com>]

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