From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx0b-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) by mx.groups.io with SMTP id smtpd.web12.368.1626724077458600025 for ; Mon, 19 Jul 2021 12:47:58 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@ibm.com header.s=pp1 header.b=BLiGIq8p; spf=pass (domain: linux.ibm.com, ip: 148.163.158.5, mailfrom: dovmurik@linux.ibm.com) Received: from pps.filterd (m0098421.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.43/8.16.0.43) with SMTP id 16JJYcL1036721; Mon, 19 Jul 2021 15:47:55 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=subject : to : cc : references : from : message-id : date : in-reply-to : content-type : content-transfer-encoding : mime-version; s=pp1; bh=bezO3ZNlEX4zgy6j/qCeXxz82wmwKYrzPGcG/0tmuSY=; b=BLiGIq8pNRtYb8SgV2DoLn0otpIHKtDOg1h3OUsE6MTv/pdqh7ThFD5D8cLAFNEcYmSW mxD/jtD1x1WPUmLgcKFm27QR0jdQBWbgX8qqggkYV9pAZg4jRDpODwyr2fy9clWBWi0R dVMNc5X9hgrpsz8dvW6Bsx6+t8oLnaRJkVpp33K9KkyPXqm9c4j2jR5mNGVQ7yfK1PN/ 6bpfwZYiG/aLDV8P8ZL5yxkkt4jtuVZ8a3GKVn/8v6Go+AgKEJtm4/BdDpg3BLWDdEbz zH/wALn0EVqY3cRhplldVgToigohbHBwf0oOTzM1El9QN8mWNQ/O/Aiq/WJyTmyxKZaN 0g== Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com with ESMTP id 39we61b43x-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 19 Jul 2021 15:47:55 -0400 Received: from m0098421.ppops.net (m0098421.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.43/8.16.0.43) with SMTP id 16JJZewg044532; Mon, 19 Jul 2021 15:47:55 -0400 Received: from ppma02wdc.us.ibm.com (aa.5b.37a9.ip4.static.sl-reverse.com [169.55.91.170]) by mx0a-001b2d01.pphosted.com with ESMTP id 39we61b43m-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 19 Jul 2021 15:47:54 -0400 Received: from pps.filterd (ppma02wdc.us.ibm.com [127.0.0.1]) by ppma02wdc.us.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 16JJcoPe003765; Mon, 19 Jul 2021 19:47:54 GMT Received: from b01cxnp22035.gho.pok.ibm.com (b01cxnp22035.gho.pok.ibm.com [9.57.198.25]) by ppma02wdc.us.ibm.com with ESMTP id 39vvw6wern-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 19 Jul 2021 19:47:54 +0000 Received: from b01ledav002.gho.pok.ibm.com (b01ledav002.gho.pok.ibm.com [9.57.199.107]) by b01cxnp22035.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 16JJlq8i38404386 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 19 Jul 2021 19:47:52 GMT Received: from b01ledav002.gho.pok.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id A74EB124053; Mon, 19 Jul 2021 19:47:52 +0000 (GMT) Received: from b01ledav002.gho.pok.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id C3AB9124081; Mon, 19 Jul 2021 19:47:49 +0000 (GMT) Received: from [9.65.195.237] (unknown [9.65.195.237]) by b01ledav002.gho.pok.ibm.com (Postfix) with ESMTP; Mon, 19 Jul 2021 19:47:49 +0000 (GMT) Subject: Re: [PATCH v2 10/11] OvmfPkg: add SevHashesBlobVerifierLib To: Tom Lendacky , devel@edk2.groups.io Cc: Tobin Feldman-Fitzthum , Tobin Feldman-Fitzthum , Jim Cadden , James Bottomley , Hubertus Franke , Laszlo Ersek , Ard Biesheuvel , Jordan Justen , Ashish Kalra , Brijesh Singh , Erdem Aktas , Jiewen Yao , Min Xu , Dov Murik References: <20210706085501.1260662-1-dovmurik@linux.ibm.com> <20210706085501.1260662-11-dovmurik@linux.ibm.com> <1b9d824c-56e4-588c-4a48-e2962caa7d44@amd.com> From: "Dov Murik" Message-ID: Date: Mon, 19 Jul 2021 22:47:48 +0300 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 In-Reply-To: <1b9d824c-56e4-588c-4a48-e2962caa7d44@amd.com> X-TM-AS-GCONF: 00 X-Proofpoint-ORIG-GUID: EMweryHBmu4ST1M7h3jmiFFReURxZkoP X-Proofpoint-GUID: rTaDUu6w5TCgohj58vHDNbnNp0D1LAMr X-Proofpoint-UnRewURL: 0 URL was un-rewritten MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.391,18.0.790 definitions=2021-07-19_10:2021-07-19,2021-07-19 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 impostorscore=0 bulkscore=0 spamscore=0 malwarescore=0 mlxscore=0 lowpriorityscore=0 adultscore=0 suspectscore=0 mlxlogscore=999 phishscore=0 clxscore=1015 priorityscore=1501 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2104190000 definitions=main-2107190111 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >> Cc: Ard Biesheuvel >> Cc: Jordan Justen >> Cc: Ashish Kalra >> Cc: Brijesh Singh >> Cc: Erdem Aktas >> Cc: James Bottomley >> Cc: Jiewen Yao >> Cc: Min Xu >> Cc: Tom Lendacky >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457 >> Co-developed-by: James Bottomley >> Signed-off-by: James Bottomley >> Signed-off-by: Dov Murik >> --- >> 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 >> +#include >> +#include >> +#include >> +#include >> + >> +/** >> + The SEV Hashes table must be in encrypted memory and has the table >> + and its entries described by >> + >> + |UINT16 | >> + >> + 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; >> +} >>