On 7/19/21 2:47 PM, Dov Murik wrote:
> On 19/07/2021 20:28, Tom Lendacky wrote:
>> On 7/6/21 3:55 AM, Dov Murik wrote:
>
>>> +[Defines]
>>> + INF_VERSION = 0x00010005
>>> + BASE_NAME = SevHashesBlobVerifierLib
>
> But is this BASE_NAME okay?
>
> Or should it be BlobVerifierLibSevHashes ?
I guess that it should probably be BlobVerifierLibSevHashes just for
consistency, but I'm not sure whether there's a convention for BASE_NAME.
Thanks,
Tom
>
>
>>> + 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 (#77924): https://edk2.groups.io/g/devel/message/77924
Mute This Topic: https://groups.io/mt/84016364/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
