On Mon, 28 Feb 2022 at 14:07, Dov Murik <[email protected]> wrote:
>
>
>
> On 28/02/2022 14:49, Ard Biesheuvel wrote:
> > On Mon, 28 Feb 2022 at 12:43, Dov Murik <[email protected]> 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 <[email protected]>
> >> Reviewed-by: Gerd Hoffmann <[email protected]>
> >
> > 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 <[email protected]> wrote:
>>
>>
>>
>> On 28/02/2022 14:49, Ard Biesheuvel wrote:
>>> On Mon, 28 Feb 2022 at 12:43, Dov Murik <[email protected]> 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 <[email protected]>
>>>> Reviewed-by: Gerd Hoffmann <[email protected]>
>>>
>>> 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 <[email protected]> wrote:
>
> Hello Ard,
>
> On 28/02/2022 15:15, Ard Biesheuvel wrote:
> > On Mon, 28 Feb 2022 at 14:07, Dov Murik <[email protected]> wrote:
> >>
> >>
> >>
> >> On 28/02/2022 14:49, Ard Biesheuvel wrote:
> >>> On Mon, 28 Feb 2022 at 12:43, Dov Murik <[email protected]> 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 <[email protected]>
> >>>> Reviewed-by: Gerd Hoffmann <[email protected]>
> >>>
> >>> 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?