Hi Dov
If this library is only needed by AmdSev build, I feel it should be in AmdSev
dir, instead of Ovmf dir. (See my question in previous email).
Thank you
Yao Jiewen
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dov Murik
> Sent: Thursday, July 22, 2021 4:45 PM
> To: devel@edk2.groups.io
> Cc: Tobin Feldman-Fitzthum <to...@linux.ibm.com>; Tobin Feldman-Fitzthum
> <to...@ibm.com>; Jim Cadden <jcad...@ibm.com>; James Bottomley
> <j...@linux.ibm.com>; Hubertus Franke <fran...@us.ibm.com>; Ard Biesheuvel
> <ardb+tianoc...@kernel.org>; Justen, Jordan L <jordan.l.jus...@intel.com>;
> Ashish Kalra <ashish.ka...@amd.com>; Brijesh Singh <brijesh.si...@amd.com>;
> Erdem Aktas <erdemak...@google.com>; Yao, Jiewen <jiewen....@intel.com>;
> Xu, Min M <min.m...@intel.com>; Tom Lendacky
> <thomas.lenda...@amd.com>; Dov Murik <dovmu...@linux.ibm.com>
> Subject: Re: [edk2-devel] [PATCH v4 10/11] OvmfPkg: add
> BlobVerifierLibSevHashes
>
>
> Here's the diff from the v3 of this patch. It's supposed to catch
> more cases of bad length fields overflowing the reserved MEMFD space or
> the declared length of the table.
>
>
>
> diff --git a/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
> b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
> index 797d63d18067..372ae6f469e7 100644
> --- a/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
> +++ b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
> @@ -94,7 +94,7 @@ VerifyBlob (
> )
> {
> CONST GUID *Guid;
> - INT32 Len;
> + INT32 Remaining;
> HASH_TABLE *Entry;
>
> if (mHashesTable == NULL || mHashesTableSize == 0) {
> @@ -111,9 +111,13 @@ VerifyBlob (
> return EFI_ACCESS_DENIED;
> }
>
> - for (Entry = mHashesTable, Len = 0;
> - Len < (INT32)mHashesTableSize;
> - Len += Entry->Len,
> + //
> + // Remaining is INT32 to catch underflow in case Entry->Len has a
> + // very high UINT16 value
> + //
> + for (Entry = mHashesTable, Remaining = mHashesTableSize;
> + Remaining >= sizeof *Entry && Remaining >= Entry->Len;
> + Remaining -= Entry->Len,
> Entry = (HASH_TABLE *)((UINT8 *)Entry + Entry->Len)) {
> UINTN EntrySize;
> EFI_STATUS Status;
> @@ -125,7 +129,7 @@ VerifyBlob (
>
> DEBUG ((DEBUG_INFO, "%a: Found GUID %g in table\n", __FUNCTION__,
> Guid));
>
> - EntrySize = Entry->Len - sizeof (Entry->Guid) - sizeof (Entry->Len);
> + 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));
> @@ -161,7 +165,8 @@ VerifyBlob (
> 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
> + @retval RETURN_SUCCESS The hashes table is set up correctly, or
> there is no
> + hashes table
> **/
> RETURN_STATUS
> EFIAPI
> @@ -175,15 +180,9 @@ BlobVerifierLibSevHashesConstructor (
> 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)) {
> + if (Ptr == NULL || Size < sizeof *Ptr ||
> + !CompareGuid (&Ptr->Guid, &SEV_HASH_TABLE_GUID) ||
> + Ptr->Len < sizeof *Ptr || Ptr->Len > Size) {
> return RETURN_SUCCESS;
> }
>
>
>
>
>
> On 22/07/2021 11:43, 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: Ard Biesheuvel <ardb+tianoc...@kernel.org>
> > Cc: Jordan Justen <jordan.l.jus...@intel.com>
> > Cc: Ashish Kalra <ashish.ka...@amd.com>
> > Cc: Brijesh Singh <brijesh.si...@amd.com>
> > Cc: Erdem Aktas <erdemak...@google.com>
> > Cc: James Bottomley <j...@linux.ibm.com>
> > Cc: Jiewen Yao <jiewen....@intel.com>
> > Cc: Min Xu <min.m...@intel.com>
> > Cc: Tom Lendacky <thomas.lenda...@amd.com>
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
> > Co-developed-by: James Bottomley <j...@linux.ibm.com>
> > Signed-off-by: James Bottomley <j...@linux.ibm.com>
> > Signed-off-by: Dov Murik <dovmu...@linux.ibm.com>
> > ---
> > OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf | 37 ++++
> > OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c | 199
> ++++++++++++++++++++
> > 2 files changed, 236 insertions(+)
> >
> > diff --git a/OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf
> b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf
> > new file mode 100644
> > index 000000000000..76ca0b8154ce
> > --- /dev/null
> > +++ b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf
> > @@ -0,0 +1,37 @@
> > +## @file
> >
> > +#
> >
> > +# Blob verifier library that uses SEV hashes table. The hashes table
> > holds the
> >
> > +# allowed hashes of the kernel, initrd, and cmdline blobs.
> >
> > +#
> >
> > +# Copyright (C) 2021, IBM Corp
> >
> > +#
> >
> > +# SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > +#
> >
> > +##
> >
> > +
> >
> > +[Defines]
> >
> > + INF_VERSION = 1.29
> >
> > + BASE_NAME = BlobVerifierLibSevHashes
> >
> > + FILE_GUID = 59e713b5-eff3-46a7-8d8b-46f4c004ad7b
> >
> > + MODULE_TYPE = BASE
> >
> > + VERSION_STRING = 1.0
> >
> > + LIBRARY_CLASS = BlobVerifierLib
> >
> > + CONSTRUCTOR = BlobVerifierLibSevHashesConstructor
> >
> > +
> >
> > +[Sources]
> >
> > + BlobVerifierSevHashes.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/BlobVerifierSevHashes.c
> b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
> > new file mode 100644
> > index 000000000000..372ae6f469e7
> > --- /dev/null
> > +++ b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
> > @@ -0,0 +1,199 @@
> > +/** @file
> >
> > +
> >
> > + Blob verifier library that uses SEV hashes table. The hashes table
> > holds the
> >
> > + allowed hashes of the kernel, initrd, and cmdline blobs.
> >
> > +
> >
> > + 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[in] BlobName The name of the blob
> >
> > + @param[in] Buf The data of the blob
> >
> > + @param[in] 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,
> >
> > + IN UINT32 BufSize
> >
> > + )
> >
> > +{
> >
> > + CONST GUID *Guid;
> >
> > + INT32 Remaining;
> >
> > + HASH_TABLE *Entry;
> >
> > +
> >
> > + 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;
> >
> > + }
> >
> > +
> >
> > + //
> >
> > + // Remaining is INT32 to catch underflow in case Entry->Len has a
> >
> > + // very high UINT16 value
> >
> > + //
> >
> > + for (Entry = mHashesTable, Remaining = mHashesTableSize;
> >
> > + Remaining >= sizeof *Entry && Remaining >= Entry->Len;
> >
> > + Remaining -= Entry->Len,
> >
> > + Entry = (HASH_TABLE *)((UINT8 *)Entry + Entry->Len)) {
> >
> > + UINTN EntrySize;
> >
> > + EFI_STATUS Status;
> >
> > + UINT8 Hash[SHA256_DIGEST_SIZE];
> >
> > +
> >
> > + if (!CompareGuid (&Entry->Guid, Guid)) {
> >
> > + continue;
> >
> > + }
> >
> > +
> >
> > + DEBUG ((DEBUG_INFO, "%a: Found GUID %g in table\n", __FUNCTION__,
> Guid));
> >
> > +
> >
> > + 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;
> >
> > + }
> >
> > +
> >
> > + //
> >
> > + // Calculate the buffer's hash and verify that it is identical to the
> >
> > + // expected hash table entry
> >
> > + //
> >
> > + Sha256HashAll (Buf, BufSize, Hash);
> >
> > +
> >
> > + 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 hashes table is set up correctly, or there
> > is
> no
> >
> > + hashes table
> >
> > +**/
> >
> > +RETURN_STATUS
> >
> > +EFIAPI
> >
> > +BlobVerifierLibSevHashesConstructor (
> >
> > + VOID
> >
> > + )
> >
> > +{
> >
> > + HASH_TABLE *Ptr = (void *)(UINTN)FixedPcdGet64
> (PcdQemuHashTableBase);
> >
> > + UINT32 Size = FixedPcdGet32 (PcdQemuHashTableSize);
> >
> > +
> >
> > + mHashesTable = NULL;
> >
> > + mHashesTableSize = 0;
> >
> > +
> >
> > + if (Ptr == NULL || Size < sizeof *Ptr ||
> >
> > + !CompareGuid (&Ptr->Guid, &SEV_HASH_TABLE_GUID) ||
> >
> > + Ptr->Len < sizeof *Ptr || Ptr->Len > Size) {
> >
> > + 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 (#78150): https://edk2.groups.io/g/devel/message/78150
Mute This Topic: https://groups.io/mt/84375131/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
