Re: [PATCH v2 10/11] OvmfPkg: add SevHashesBlobVerifierLib

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

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@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ashish Kalra <ashish.kalra@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
> Co-developed-by: James Bottomley <jejb@linux.ibm.com>
> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
> Signed-off-by: Dov Murik <dovmurik@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.

> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = SevHashesBlobVerifierLib
> +  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?

> +  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?

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;
> +}
>