[edk2-devel] [PATCH v2 10/11] OvmfPkg: add SevHashesBlobVerifierLib
Dov Murik
dovmurik at linux.ibm.com
Mon Jul 19 19:47:48 UTC 2021
On 19/07/2021 20:28, Tom Lendacky wrote:
> On 7/6/21 3:55 AM, Dov Murik wrote:
>> Add an implementation for BlobVerifierLib that locates the SEV hashes
>> table and verifies that the calculated hashes of the kernel, initrd, and
>> cmdline blobs indeed match the expected hashes stated in the hashes
>> table.
>>
>> If there's a missing hash or a hash mismatch then EFI_ACCESS_DENIED is
>> returned which will cause a failure to load a kernel image.
>>
>> Cc: Laszlo Ersek <lersek at redhat.com>
>> Cc: Ard Biesheuvel <ardb+tianocore at kernel.org>
>> Cc: Jordan Justen <jordan.l.justen at intel.com>
>> Cc: Ashish Kalra <ashish.kalra at amd.com>
>> Cc: Brijesh Singh <brijesh.singh at amd.com>
>> Cc: Erdem Aktas <erdemaktas at google.com>
>> Cc: James Bottomley <jejb at linux.ibm.com>
>> Cc: Jiewen Yao <jiewen.yao at intel.com>
>> Cc: Min Xu <min.m.xu at intel.com>
>> Cc: Tom Lendacky <thomas.lendacky at amd.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
>> Co-developed-by: James Bottomley <jejb at linux.ibm.com>
>> Signed-off-by: James Bottomley <jejb at linux.ibm.com>
>> Signed-off-by: Dov Murik <dovmurik at linux.ibm.com>
>> ---
>> OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifierLib.inf | 36 ++++
>> OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifier.c | 199 ++++++++++++++++++++
>> 2 files changed, 235 insertions(+)
>>
>> diff --git a/OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifierLib.inf b/OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifierLib.inf
>> new file mode 100644
>> index 000000000000..b060d6a1b545
>> --- /dev/null
>> +++ b/OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifierLib.inf
>> @@ -0,0 +1,36 @@
>> +## @file
>> +#
>> +# Blob verifier library that uses SEV hashes table.
>> +#
>> +# Copyright (C) 2021, IBM Corp
>> +#
>> +# SPDX-License-Identifier: BSD-2-Clause-Patent
>> +#
>> +##
>> +
>
> Same comments here as were made on the Null library instance.
>
Indeed.
>> +[Defines]
>> + INF_VERSION = 0x00010005
>> + BASE_NAME = SevHashesBlobVerifierLib
But is this BASE_NAME okay?
Or should it be BlobVerifierLibSevHashes ?
>> + FILE_GUID = 59e713b5-eff3-46a7-8d8b-46f4c004ad7b
>> + MODULE_TYPE = BASE
>> + VERSION_STRING = 1.0
>> + LIBRARY_CLASS = BlobVerifierLib
>> + CONSTRUCTOR = SevHashesBlobVerifierLibConstructor
>> +
>> +[Sources]
>> + SevHashesBlobVerifier.c
>> +
>> +[Packages]
>> + CryptoPkg/CryptoPkg.dec
>> + MdePkg/MdePkg.dec
>> + OvmfPkg/OvmfPkg.dec
>> +
>> +[LibraryClasses]
>> + BaseCryptLib
>> + BaseMemoryLib
>> + DebugLib
>> + PcdLib
>> +
>> +[FixedPcd]
>> + gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase
>> + gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize
>> diff --git a/OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifier.c b/OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifier.c
>> new file mode 100644
>> index 000000000000..961ee29f5df3
>> --- /dev/null
>> +++ b/OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifier.c
>> @@ -0,0 +1,199 @@
>> +/** @file
>> +
>> + Blob verifier library that uses SEV hashes table.
>> +
>> + Copyright (C) 2021, IBM Corporation
>> +
>> + SPDX-License-Identifier: BSD-2-Clause-Patent
>> +**/
>> +
>> +#include <Library/BaseCryptLib.h>
>> +#include <Library/BaseLib.h>
>> +#include <Library/BaseMemoryLib.h>
>> +#include <Library/DebugLib.h>
>> +#include <Library/BlobVerifierLib.h>
>> +
>> +/**
>> + The SEV Hashes table must be in encrypted memory and has the table
>> + and its entries described by
>> +
>> + <GUID>|UINT16 <len>|<data>
>> +
>> + With the whole table GUID being 9438d606-4f22-4cc9-b479-a793d411fd21
>> +
>> + The current possible table entries are for the kernel, the initrd
>> + and the cmdline:
>> +
>> + 4de79437-abd2-427f-b835-d5b172d2045b kernel
>> + 44baf731-3a2f-4bd7-9af1-41e29169781d initrd
>> + 97d02dd8-bd20-4c94-aa78-e7714d36ab2a cmdline
>> +
>> + The size of the entry is used to identify the hash, but the
>> + expectation is that it will be 32 bytes of SHA-256.
>> +**/
>> +
>> +#define SEV_HASH_TABLE_GUID \
>> + (GUID) { 0x9438d606, 0x4f22, 0x4cc9, { 0xb4, 0x79, 0xa7, 0x93, 0xd4, 0x11, 0xfd, 0x21 } }
>> +#define SEV_KERNEL_HASH_GUID \
>> + (GUID) { 0x4de79437, 0xabd2, 0x427f, { 0xb8, 0x35, 0xd5, 0xb1, 0x72, 0xd2, 0x04, 0x5b } }
>> +#define SEV_INITRD_HASH_GUID \
>> + (GUID) { 0x44baf731, 0x3a2f, 0x4bd7, { 0x9a, 0xf1, 0x41, 0xe2, 0x91, 0x69, 0x78, 0x1d } }
>> +#define SEV_CMDLINE_HASH_GUID \
>> + (GUID) { 0x97d02dd8, 0xbd20, 0x4c94, { 0xaa, 0x78, 0xe7, 0x71, 0x4d, 0x36, 0xab, 0x2a } }
>> +
>> +STATIC CONST EFI_GUID mSevKernelHashGuid = SEV_KERNEL_HASH_GUID;
>> +STATIC CONST EFI_GUID mSevInitrdHashGuid = SEV_INITRD_HASH_GUID;
>> +STATIC CONST EFI_GUID mSevCmdlineHashGuid = SEV_CMDLINE_HASH_GUID;
>> +
>> +#pragma pack (1)
>> +typedef struct {
>> + GUID Guid;
>> + UINT16 Len;
>> + UINT8 Data[];
>> +} HASH_TABLE;
>> +#pragma pack ()
>> +
>> +STATIC HASH_TABLE *mHashesTable;
>> +STATIC UINT16 mHashesTableSize;
>> +
>> +STATIC
>> +CONST GUID*
>> +FindBlobEntryGuid (
>> + IN CONST CHAR16 *BlobName
>> + )
>> +{
>> + if (StrCmp (BlobName, L"kernel") == 0) {
>> + return &mSevKernelHashGuid;
>> + } else if (StrCmp (BlobName, L"initrd") == 0) {
>> + return &mSevInitrdHashGuid;
>> + } else if (StrCmp (BlobName, L"cmdline") == 0) {
>> + return &mSevCmdlineHashGuid;
>> + } else {
>> + return NULL;
>> + }
>> +}
>> +
>> +/**
>> + Verify blob from an external source.
>> +
>> + @param BlobName The name of the blob
>> + @param Buf The data of the blob
>> + @param BufSize The size of the blob in bytes
>> +
>> + @retval EFI_SUCCESS The blob was verified successfully.
>> + @retval EFI_ACCESS_DENIED The blob could not be verified, and therefore
>> + should be considered non-secure.
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +VerifyBlob (
>> + IN CONST CHAR16 *BlobName,
>> + IN CONST VOID *Buf,
>> + UINT32 BufSize
>> + )
>> +{
>> + CONST GUID *Guid;
>> + INT32 Len;
>
> Any reason for this not to be a UINT16 like the struct or mHashesTableSize?
>
Detect overflows in the `for` loop below?
If a (bad) Entry->Len is 0xffff, then adding it to Len will overflow the
UINT16 and the Len < mHashesTableSize condition is still true.
>> + HASH_TABLE *Entry;
>> + UINT8 Hash[SHA256_DIGEST_SIZE];
>> +
>> + if (mHashesTable == NULL || mHashesTableSize == 0) {
>> + DEBUG ((DEBUG_ERROR,
>> + "%a: Verifier called but no hashes table discoverd in MEMFD\n",
>> + __FUNCTION__));
>> + return EFI_ACCESS_DENIED;
>> + }
>> +
>> + Guid = FindBlobEntryGuid (BlobName);
>> + if (Guid == NULL) {
>> + DEBUG ((DEBUG_ERROR, "%a: Unknown blob name \"%s\"\n", __FUNCTION__,
>> + BlobName));
>> + return EFI_ACCESS_DENIED;
>> + }
>> +
>> + Sha256HashAll (Buf, BufSize, Hash);
>
> Maybe search for and find the Guid (done in the for loop below) before
> calling Sha256HashAll?
>
Yep; I'll move it just before CompareMem below.
Thanks,
-Dov
> Thanks,
> Tom
>
>> +
>> + for (Entry = mHashesTable, Len = 0;
>> + Len < (INT32)mHashesTableSize;
>> + Len += Entry->Len,
>> + Entry = (HASH_TABLE *)((UINT8 *)Entry + Entry->Len)) {
>> + UINTN EntrySize;
>> + EFI_STATUS Status;
>> +
>> + if (!CompareGuid (&Entry->Guid, Guid)) {
>> + continue;
>> + }
>> +
>> + DEBUG ((DEBUG_INFO, "%a: Found GUID %g in table\n", __FUNCTION__, Guid));
>> +
>> + //
>> + // Verify that the buffer's calculated hash is identical to the expected
>> + // hash table entry
>> + //
>> + EntrySize = Entry->Len - sizeof (Entry->Guid) - sizeof (Entry->Len);
>> + if (EntrySize != SHA256_DIGEST_SIZE) {
>> + DEBUG ((DEBUG_ERROR, "%a: Hash has the wrong size %d != %d\n",
>> + __FUNCTION__, EntrySize, SHA256_DIGEST_SIZE));
>> + return EFI_ACCESS_DENIED;
>> + }
>> +
>> + if (CompareMem (Entry->Data, Hash, EntrySize) == 0) {
>> + Status = EFI_SUCCESS;
>> + DEBUG ((DEBUG_INFO, "%a: Hash comparison succeeded for \"%s\"\n",
>> + __FUNCTION__, BlobName));
>> + } else {
>> + Status = EFI_ACCESS_DENIED;
>> + DEBUG ((DEBUG_ERROR, "%a: Hash comparison failed for \"%s\"\n",
>> + __FUNCTION__, BlobName));
>> + }
>> + return Status;
>> + }
>> +
>> + DEBUG ((DEBUG_ERROR, "%a: Hash GUID %g not found in table\n", __FUNCTION__,
>> + Guid));
>> + return EFI_ACCESS_DENIED;
>> +}
>> +
>> +/**
>> + Locate the SEV hashes table.
>> +
>> + This function always returns success, even if the table can't be found. The
>> + subsequent VerifyBlob calls will fail if no table was found.
>> +
>> + @retval RETURN_SUCCESS The verifier tables were set up correctly
>> +**/
>> +RETURN_STATUS
>> +EFIAPI
>> +SevHashesBlobVerifierLibConstructor (
>> + VOID
>> + )
>> +{
>> + HASH_TABLE *Ptr = (void *)(UINTN)FixedPcdGet64 (PcdQemuHashTableBase);
>> + UINT32 Size = FixedPcdGet32 (PcdQemuHashTableSize);
>> +
>> + mHashesTable = NULL;
>> + mHashesTableSize = 0;
>> +
>> + if (Ptr == NULL || Size == 0) {
>> + return RETURN_SUCCESS;
>> + }
>> +
>> + if (!CompareGuid (&Ptr->Guid, &SEV_HASH_TABLE_GUID)) {
>> + return RETURN_SUCCESS;
>> + }
>> +
>> + if (Ptr->Len < (sizeof Ptr->Guid + sizeof Ptr->Len)) {
>> + return RETURN_SUCCESS;
>> + }
>> +
>> + DEBUG ((DEBUG_INFO, "%a: Found injected hashes table in secure location\n",
>> + __FUNCTION__));
>> +
>> + mHashesTable = (HASH_TABLE *)Ptr->Data;
>> + mHashesTableSize = Ptr->Len - sizeof Ptr->Guid - sizeof Ptr->Len;
>> +
>> + DEBUG ((DEBUG_VERBOSE, "%a: mHashesTable=0x%p, Size=%u\n", __FUNCTION__,
>> + mHashesTable, mHashesTableSize));
>> +
>> + return RETURN_SUCCESS;
>> +}
>>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77920): https://edk2.groups.io/g/devel/message/77920
Mute This Topic: https://groups.io/mt/84016364/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list