Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <c7604c39-d6ca-f3b9-b1d8-fd0362216717@linux.ibm.com>
Date:   Wed, 2 Feb 2022 13:08:43 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.1
Subject: Re: [PATCH v7 4/5] efi: Load efi_secret module if EFI secret area is
 populated
Content-Language: en-US
To:     Gerd Hoffmann <kraxel@redhat.com>
Cc:     linux-efi@vger.kernel.org, Borislav Petkov <bp@suse.de>,
        Ashish Kalra <ashish.kalra@amd.com>,
        Brijesh Singh <brijesh.singh@amd.com>,
        Tom Lendacky <thomas.lendacky@amd.com>,
        Ard Biesheuvel <ardb@kernel.org>,
        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>,
        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@vger.kernel.org, Dov Murik <dovmurik@linux.ibm.com>
References: <20220201124413.1093099-1-dovmurik@linux.ibm.com>
 <20220201124413.1093099-5-dovmurik@linux.ibm.com>
 <20220202084723.ushasiekb3cxami4@sirius.home.kraxel.org>
From:   Dov Murik <dovmurik@linux.ibm.com>
In-Reply-To: <20220202084723.ushasiekb3cxami4@sirius.home.kraxel.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


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