From: "Dov Murik" <dovmurik@linux.ibm.com>
To: Tom Lendacky <thomas.lendacky@amd.com>, devel@edk2.groups.io
Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>,
Tobin Feldman-Fitzthum <tobin@ibm.com>,
Jim Cadden <jcadden@ibm.com>,
James Bottomley <jejb@linux.ibm.com>,
Hubertus Franke <frankeh@us.ibm.com>,
Laszlo Ersek <lersek@redhat.com>,
Ard Biesheuvel <ardb+tianocore@kernel.org>,
Jordan Justen <jordan.l.justen@intel.com>,
Ashish Kalra <ashish.kalra@amd.com>,
Brijesh Singh <brijesh.singh@amd.com>,
Erdem Aktas <erdemaktas@google.com>,
Jiewen Yao <jiewen.yao@intel.com>, Min Xu <min.m.xu@intel.com>,
Dov Murik <dovmurik@linux.ibm.com>
Subject: Re: [PATCH v2 10/11] OvmfPkg: add SevHashesBlobVerifierLib
Date: Mon, 19 Jul 2021 22:47:48 +0300 [thread overview]
Message-ID: <a4b38ebf-abd7-1c84-c4b8-d5da3d77edf8@linux.ibm.com> (raw)
In-Reply-To: <1b9d824c-56e4-588c-4a48-e2962caa7d44@amd.com>
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@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.
>
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;
>> +}
>>
next prev parent reply other threads:[~2021-07-19 19:47 UTC|newest]
Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-06 8:54 [PATCH v2 00/11] Measured SEV boot with kernel/initrd/cmdline Dov Murik
2021-07-06 8:54 ` [PATCH v2 01/11] OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming Dov Murik
2021-07-17 15:16 ` Brijesh Singh
2021-07-06 8:54 ` [PATCH v2 02/11] OvmfPkg/AmdSev: use GenericQemuLoadImageLib in AmdSev builds Dov Murik
2021-07-17 15:18 ` Brijesh Singh
2021-07-06 8:54 ` [PATCH v2 03/11] OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg Dov Murik
2021-07-17 15:35 ` Brijesh Singh
2021-07-19 4:46 ` [edk2-devel] " Christoph Willing
2021-07-19 12:14 ` Dov Murik
2021-07-19 12:56 ` Christoph Willing
2021-07-19 17:58 ` Dov Murik
2021-07-19 22:36 ` Christoph Willing
2021-07-20 4:55 ` Dov Murik
2021-07-19 15:21 ` Lendacky, Thomas
2021-07-19 19:14 ` Dov Murik
2021-07-20 7:33 ` Dov Murik
2021-07-20 7:41 ` Ard Biesheuvel
2021-07-06 8:54 ` [PATCH v2 04/11] OvmfPkg: add library class BlobVerifierLib with null implementation Dov Murik
2021-07-17 20:16 ` Brijesh Singh
2021-07-19 15:50 ` Lendacky, Thomas
2021-07-19 19:23 ` Dov Murik
2021-07-06 8:54 ` [PATCH v2 05/11] OvmfPkg: add NullBlobVerifierLib to DSC Dov Murik
2021-07-17 20:18 ` Brijesh Singh
2021-07-06 8:54 ` [PATCH v2 06/11] ArmVirtPkg: " Dov Murik
2021-07-18 15:43 ` Brijesh Singh
2021-07-06 8:54 ` [PATCH v2 07/11] OvmfPkg/QemuKernelLoaderFsDxe: call VerifyBlob after fetch from fw_cfg Dov Murik
2021-07-18 15:47 ` Brijesh Singh
2021-07-19 12:22 ` Dov Murik
2021-07-19 15:19 ` Brijesh Singh
2021-07-19 19:54 ` Dov Murik
2021-07-19 15:57 ` Lendacky, Thomas
2021-07-19 19:30 ` Dov Murik
2021-07-06 8:54 ` [PATCH v2 08/11] OvmfPkg/AmdSev/SecretPei: build hob for full page Dov Murik
2021-07-19 16:19 ` Lendacky, Thomas
2021-07-19 19:37 ` Dov Murik
2021-07-06 8:54 ` [PATCH v2 09/11] OvmfPkg/AmdSev: reserve MEMFD space for for firmware config hashes Dov Murik
2021-07-19 16:38 ` Lendacky, Thomas
2021-07-06 8:55 ` [PATCH v2 10/11] OvmfPkg: add SevHashesBlobVerifierLib Dov Murik
2021-07-19 17:28 ` Lendacky, Thomas
2021-07-19 19:47 ` Dov Murik [this message]
2021-07-19 20:15 ` Lendacky, Thomas
2021-07-06 8:55 ` [PATCH v2 11/11] OvmfPkg/AmdSev: Enforce hash verification of kernel blobs Dov Murik
2021-07-19 17:31 ` Lendacky, Thomas
2021-07-16 17:11 ` [edk2-devel] [PATCH v2 00/11] Measured SEV boot with kernel/initrd/cmdline Ard Biesheuvel
2021-07-19 15:14 ` Lendacky, Thomas
2021-07-19 19:12 ` Dov Murik
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a4b38ebf-abd7-1c84-c4b8-d5da3d77edf8@linux.ibm.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox