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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 2 Feb 2022 13:08:43 +0200
From:   Dov Murik <dovmurik@...ux.ibm.com>
To:     Gerd Hoffmann <kraxel@...hat.com>
Cc:     linux-efi@...r.kernel.org, Borislav Petkov <bp@...e.de>,
        Ashish Kalra <ashish.kalra@....com>,
        Brijesh Singh <brijesh.singh@....com>,
        Tom Lendacky <thomas.lendacky@....com>,
        Ard Biesheuvel <ardb@...nel.org>,
        James Morris <jmorris@...ei.org>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        Andrew Scull <ascull@...gle.com>,
        Dave Hansen <dave.hansen@...el.com>,
        "Dr. David Alan Gilbert" <dgilbert@...hat.com>,
        Lenny Szubowicz <lszubowi@...hat.com>,
        Peter Gonda <pgonda@...gle.com>,
        James Bottomley <jejb@...ux.ibm.com>,
        Tobin Feldman-Fitzthum <tobin@...ux.ibm.com>,
        Jim Cadden <jcadden@....com>,
        Daniele Buono <dbuono@...ux.vnet.ibm.com>,
        linux-coco@...ts.linux.dev, linux-security-module@...r.kernel.org,
        linux-kernel@...r.kernel.org, Dov Murik <dovmurik@...ux.ibm.com>
Subject: Re: [PATCH v7 4/5] efi: Load efi_secret module if EFI secret area is
 populated



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