Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <4f0ea80f-5132-0a05-ccf4-e8589a02cab4@linux.ibm.com>
Date:   Tue, 12 Apr 2022 16:18:59 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH v8 3/4] efi: Load efi_secret module if EFI secret area is
 populated
Content-Language: en-US
To:     Ard Biesheuvel <ardb@kernel.org>
Cc:     linux-efi <linux-efi@vger.kernel.org>,
        Gerd Hoffmann <kraxel@redhat.com>,
        Borislav Petkov <bp@suse.de>,
        Ashish Kalra <ashish.kalra@amd.com>,
        Brijesh Singh <brijesh.singh@amd.com>,
        Tom Lendacky <thomas.lendacky@amd.com>,
        James Morris <jmorris@namei.org>,
        "Serge E. Hallyn" <serge@hallyn.com>,
        Andi Kleen <ak@linux.intel.com>,
        Greg KH <gregkh@linuxfoundation.org>,
        Andrew Scull <ascull@google.com>,
        Dave Hansen <dave.hansen@intel.com>,
        "Dr. David Alan Gilbert" <dgilbert@redhat.com>,
        Lenny Szubowicz <lszubowi@redhat.com>,
        Peter Gonda <pgonda@google.com>,
        Matthew Garrett <mjg59@srcf.ucam.org>,
        James Bottomley <jejb@linux.ibm.com>,
        Tobin Feldman-Fitzthum <tobin@linux.ibm.com>,
        Jim Cadden <jcadden@ibm.com>,
        Daniele Buono <dbuono@linux.vnet.ibm.com>,
        linux-coco@lists.linux.dev, linux-security-module@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Dov Murik <dovmurik@linux.ibm.com>
References: <20220228114254.1099945-1-dovmurik@linux.ibm.com>
 <20220228114254.1099945-4-dovmurik@linux.ibm.com>
 <CAMj1kXFybkGtxH2U6oAi_Qeqe-i_kH-hZjUZKY3-UzPHUg55vg@mail.gmail.com>
 <f3de6cd9-9f58-7db3-e367-a6a5a5d826e2@linux.ibm.com>
 <CAMj1kXEKh-Ve3iPDnNTxi10=hcARN4ntpyQOrmb=B+4DpC24vQ@mail.gmail.com>
 <38daffb6-a72a-87f4-d927-0b857b7b6833@linux.ibm.com>
 <CAMj1kXHfeS9WzHkOyM3JGfgVLK8GQ78KBArD19NxcCMn-SDPKg@mail.gmail.com>
From:   Dov Murik <dovmurik@linux.ibm.com>
In-Reply-To: <CAMj1kXHfeS9WzHkOyM3JGfgVLK8GQ78KBArD19NxcCMn-SDPKg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 12/04/2022 16:08, Ard Biesheuvel wrote:
> On Thu, 31 Mar 2022 at 11:05, Dov Murik <dovmurik@linux.ibm.com> wrote:
>>
>> 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?
>>
> 
> Apologies, I am swamped in email and only spotted this now.
> 
> This looks good to me: is this what you implemented in the end?

Yes indeed. So this old patch 3 was replaced by this much simpler code
in the next iteration (v9):


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:


(this is were I missed the #ifdef CONFIG_EFI_COCO_SECRET ).

Thanks again for the suggestion.

-Dov