On 02/02/2022 16:31, Gerd Hoffmann wrote: > On Wed, Feb 02, 2022 at 01:08:43PM +0200, Dov Murik wrote: >> >> >> 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; >> + } > > Note I explicitly excluded that check ;) > > Checking whenever efi.coco_secret looks valid and only try load > efi_secret if that is the case (and otherwise stay silent) makes > perfect sense. The other checks should be dropped IMHO. > >> 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 think the module should fail noisily. See above for autoload. In > case the module is loaded (either manually by the admin, or because > efi.coco_secret != EFI_INVALID_TABLE_ADDR) and it can't actually load > the secrets we want know why ... > Note that the AmdSev build of OVMF always publishes LINUX_EFI_COCO_SECRET_TABLE_GUID in the EFI table. Even when LAUNCH_SECRET was not executed. In such cases the secret area will be empty. If we keep only the 'efi.coco_secret != EFI_INVALID_TABLE_ADDR' check, we'll get errors from efi_secret for every VM launch that doesn't undergo LAUNCH_SECRET. I don't think that's good. If we *do* want to check that the area starts with EFI_SECRET_TABLE_HEADER_GUID (like I think we should), we need all the checks before that, like checking that the area is big enough, and that all the memremap()s succeed -- before actually comparing the header_guid. The checks are basically prerequisites for calling efi_guidcmp() safely. -Dov