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

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

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