From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.156.1]) by mx.groups.io with SMTP id smtpd.web09.16129.1627207582763258021 for ; Sun, 25 Jul 2021 03:06:22 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@ibm.com header.s=pp1 header.b=JxaM+VTU; spf=pass (domain: linux.ibm.com, ip: 148.163.156.1, mailfrom: dovmurik@linux.ibm.com) Received: from pps.filterd (m0098393.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.43/8.16.0.43) with SMTP id 16PA2cTM046014; Sun, 25 Jul 2021 06:06:17 -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=msuY24fqBv3O4daB4wEyJfjLi+6i6BhujbA0VAWYD0A=; b=JxaM+VTU1dfDViCCq5qpIlUmLtJ5ObREaIivohcnNwYDIIflLvYPIAeoeF/08xe9MJzG dL8VVCRuXG4MwsPbcVlfL0WDSwvrMN9u2/HtP8UjFbe4TwBtrpsc5GpPZrR0J8Qc9CSD n1fORFO61BdZik89V85b/XLwRj5YVzXWCYvpcdwrbFzlcVcckFfH20Gnlf+1gA6W0oPp bM1nro5xxWx61/8U1mdXFUyRxu+GfMKevAJSZkPBRdUYiv9y7oYIcZ3mBSQ+b5BsL2Xo KGNW6ZPQO7g8ya3p3nsQ7Od5kLxYqWTRvEp7Eqc6Gf+ozk1GxcKvuUXe5pNrjSTAZRgB aw== Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com with ESMTP id 3a15js8aun-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Sun, 25 Jul 2021 06:06:16 -0400 Received: from m0098393.ppops.net (m0098393.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.43/8.16.0.43) with SMTP id 16PA5xxN057738; Sun, 25 Jul 2021 06:06:16 -0400 Received: from ppma04wdc.us.ibm.com (1a.90.2fa9.ip4.static.sl-reverse.com [169.47.144.26]) by mx0a-001b2d01.pphosted.com with ESMTP id 3a15js8ath-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Sun, 25 Jul 2021 06:06:16 -0400 Received: from pps.filterd (ppma04wdc.us.ibm.com [127.0.0.1]) by ppma04wdc.us.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 16PA4xPk031135; Sun, 25 Jul 2021 10:06:14 GMT Received: from b03cxnp08025.gho.boulder.ibm.com (b03cxnp08025.gho.boulder.ibm.com [9.17.130.17]) by ppma04wdc.us.ibm.com with ESMTP id 3a0ag9r71c-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Sun, 25 Jul 2021 10:06:14 +0000 Received: from b03ledav003.gho.boulder.ibm.com (b03ledav003.gho.boulder.ibm.com [9.17.130.234]) by b03cxnp08025.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 16PA6CYB47382880 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Sun, 25 Jul 2021 10:06:12 GMT Received: from b03ledav003.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 734896A04F; Sun, 25 Jul 2021 10:06:12 +0000 (GMT) Received: from b03ledav003.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id E40ED6A051; Sun, 25 Jul 2021 10:06:07 +0000 (GMT) Received: from [9.160.123.143] (unknown [9.160.123.143]) by b03ledav003.gho.boulder.ibm.com (Postfix) with ESMTP; Sun, 25 Jul 2021 10:06:07 +0000 (GMT) Subject: Re: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline To: "Yao, Jiewen" , "devel@edk2.groups.io" Cc: Tobin Feldman-Fitzthum , Tobin Feldman-Fitzthum , Jim Cadden , James Bottomley , Hubertus Franke , Ard Biesheuvel , "Justen, Jordan L" , Ashish Kalra , Brijesh Singh , Erdem Aktas , "Xu, Min M" , Tom Lendacky , Leif Lindholm , Sami Mujawar , Dov Murik References: <20210722084307.2890952-1-dovmurik@linux.ibm.com> <711c0ad9-9ebe-0eaa-e04b-28e7e7f69ef4@linux.ibm.com> From: "Dov Murik" Message-ID: Date: Sun, 25 Jul 2021 13:06:06 +0300 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 In-Reply-To: X-TM-AS-GCONF: 00 X-Proofpoint-ORIG-GUID: oDUw6IV9V3GyWfTZu4UnG5uoCgVKRrId X-Proofpoint-GUID: YfTlKs-ZEHBw0A0OxYFs1ftQK68d2H0s 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-25_02:2021-07-23,2021-07-25 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 phishscore=0 malwarescore=0 adultscore=0 priorityscore=1501 clxscore=1015 impostorscore=0 spamscore=0 lowpriorityscore=0 suspectscore=0 mlxlogscore=999 mlxscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2104190000 definitions=main-2107250071 X-MIME-Autoconverted: from 8bit to quoted-printable by mx0a-001b2d01.pphosted.com id 16PA2cTM046014 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable Thanks for the explanations. Comments below: On 25/07/2021 11:17, Yao, Jiewen wrote: > Thank you Dov. > Comment below: >=20 >> -----Original Message----- >> From: Dov Murik >> Sent: Sunday, July 25, 2021 3:53 PM >> To: Yao, Jiewen ; devel@edk2.groups.io >> Cc: Tobin Feldman-Fitzthum ; Tobin Feldman-Fitzthu= m >> ; Jim Cadden ; James Bottomley >> ; Hubertus Franke ; Ard Biesheu= vel >> ; Justen, Jordan L ; >> Ashish Kalra ; Brijesh Singh ; >> Erdem Aktas ; Xu, Min M ; >> Tom Lendacky ; Leif Lindholm >> ; Sami Mujawar ; Dov Murik >> >> Subject: Re: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with >> kernel/initrd/cmdline >> >> Hi Jiewen, >> >> On 25/07/2021 5:43, Yao, Jiewen wrote: >>> Hi >>> I am reviewing this patch series. I am OK with most parts. >> >> Thank you. >> >>> >>> And I do have one question: >>> May I know what is criteria to put a SEV module to OvmfPkg\AmdSev or >> OvmfPkg directly? >>> >>> My original understanding is: >>> If a module is required by OvmfPkg{Ia32,Ia32X64,X64}.{dsc,fdf}, then i= t should >> be OvmfPkg. >>> If a module is only required by OvmfPkg\AmdSev\AmdSevX64.{dsc,fdf}, Th= en it >> should be in OvmfPkg\AmdSev. >>> >>> Am I right? >>> >> >> I actually don't know the criteria. What you say sounds reasonable. >> I'll also let James (who introduced the AmdSevX64 target) say what he >> thinks. > [Jiewen] OK. >=20 >=20 >> >> >> If that is indeed the case, then I need to: >> >> 1. Modify patch 4: put the code of the null implementation in >> OvmfPkf/BlobVerifierLibNull/ > [Jiewen] Since this is a library, I expect to be OvmfPkf/Library/BlobVer= ifierLibNull/ >=20 Yes, you're right, I missed that "/Library/" in my description. >> >> 2. Modify patches 5+6: use the new path of the BlobVerifierLibNull inf = file >> >> 3. Modify patches 10+11: put the code of the SevHashes implementation i= n >> OvmfPkg/AmdSev/BlobVerifierLibSevHashes/ >> >> Jiewen, is that what you had in mind? > [Jiewen] Let's comply with the exiting rule.=20 > 1) A library in a packet should be in XXXPkg/Library/AAALib in general. = (For example, OvmfPkg/Library) > 2) If a library in a packet belongs to a feature, then it should be XXXP= kg/FeatureYYY/AAALib. (For example, OvmfPkg/Csm/CsmSupportLib) >=20 OK. I'll send a new version with the paths corrected. >=20 >> >> >> >> Two other things to consider: >> >> A. The blob verification by hashes just uses initially-measured memory, >> and no other features of SEV. It might be useful for other Confidentia= l >> Computing implementations. But maybe if that need arises then we'll >> extract it from the AmdSev folder. > [Jiewen] I think so. Because it is generic, that is why I agree that the= *library class* name does not include SEV keyword. > The *library instance* should add *Sev* keyword, because the implementat= ion does include SEV specific, such as SEV_HASH_TABLE_GUID, SEV_KERNEL_HAS= H_GUID. >=20 > If you want to make it for generic confidential computing, then more wor= k should be done. For example: > 1) The instance name should be BlobVerificationLibTeeLinuxModuleHash. (T= EE to replace SEV. We need Linux keyword, because I don=E2=80=99t think it = is applicable for Windows) > 2) We need consider crypto agile. The current instance only consider SHA= 256, which is not allowed in TDX. > 3) The GUID definition should be in OvmfPkg.dec, as the interface. >=20 > Since we don=E2=80=99t have a TEE folder yet, I prefer we defer that wor= k. > Later, when we finish all Sev/Tdx work, we can consider create a common = Tee dir or event a package. But that may happen in next year. >=20 OK I'll keep that in mind for later. Also note that these require corresponding changes in QEMU. >=20 >=20 >> >> B. There were talks about reducing the number of targets, and maybe >> unifying AmdSevX64 into OvmfPkgX64. If this is indeed the direction >> we're going to, then there's no need to separate the code. > [Jiewen] I think we are following what Laslo suggested. > 1) The OvmfPkg includes the *basic* feature at first. > 2) The advanced feature is checked into SEV folder or TDX folder, at fir= st. > 3) We can revisit those advanced feature once we think they are mature. >=20 > We agree that direction, and we should follow that. > Let's keep #1 and #2 at first to finish the feature at first (in this ye= ar). Then we can see how to enhance in #3 (maybe next year). > The more we know each other, the better we can create an architecture to= support TEE. >=20 Sounds good. Thanks, -Dov > Thank you > Yao Jiewen >=20 >> >> >> Thanks, >> -Dov >> >> >>> Thank you >>> Yao Jiewen >>> >>> >>>> -----Original Message----- >>>> From: devel@edk2.groups.io On Behalf Of Dov >> Murik >>>> Sent: Thursday, July 22, 2021 4:43 PM >>>> To: devel@edk2.groups.io >>>> Cc: Dov Murik ; Tobin Feldman-Fitzthum >>>> ; Tobin Feldman-Fitzthum ; Jim >>>> Cadden ; James Bottomley ; >>>> Hubertus Franke ; Ard Biesheuvel >>>> ; Justen, Jordan L ; >>>> Ashish Kalra ; Brijesh Singh >> ; >>>> Erdem Aktas ; Yao, Jiewen >> ; >>>> Xu, Min M ; Tom Lendacky >>>> ; Leif Lindholm ; Sami >>>> Mujawar >>>> Subject: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with >>>> kernel/initrd/cmdline >>>> >>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3457 >>>> >>>> Booting with SEV prevented the loading of kernel, initrd, and kernel >>>> command-line via QEMU fw_cfg interface because they arrive from the V= MM >>>> which is untrusted in SEV. >>>> >>>> However, in some cases the kernel, initrd, and cmdline are not secret >>>> but should not be modified by the host. In such a case, we want to >>>> verify inside the trusted VM that the kernel, initrd, and cmdline are >>>> indeed the ones expected by the Guest Owner, and only if that is the >>>> case go on and boot them up (removing the need for grub inside OVMF i= n >>>> that mode). >>>> >>>> This patch series reserves an area in MEMFD (previously the last 1KB = of >>>> the launch secret page) which will contain the hashes of these three >>>> blobs (kernel, initrd, cmdline), each under its own GUID entry. This >>>> tables of hashes is populated by QEMU before launch, and encrypted as >>>> part of the initial VM memory; this makes sure these hashes are part = of >>>> the SEV measurement (which has to be approved by the Guest Owner for >>>> secret injection, for example). Note that populating the hashes tabl= e >>>> requires QEMU support [1]. >>>> >>>> OVMF parses the table of hashes populated by QEMU (patch 10), and as = it >>>> reads the fw_cfg blobs from QEMU, it will verify each one against the >>>> expected hash. This is all done inside the trusted VM context. If a= ll >>>> the hashes are correct, boot of the kernel is allowed to continue. >>>> >>>> Any attempt by QEMU to modify the kernel, initrd, cmdline (including >>>> dropping one of them), or to modify the OVMF code that verifies those >>>> hashes, will cause the initial SEV measurement to change and therefor= e >>>> will be detectable by the Guest Owner during launch before secret >>>> injection. >>>> >>>> Relevant part of OVMF serial log during boot with AmdSevX86 build and >>>> QEMU with -kernel/-initrd/-append: >>>> >>>> ... >>>> BlobVerifierLibSevHashesConstructor: Found injected hashes table in= secure >>>> location >>>> Select Item: 0x17 >>>> Select Item: 0x8 >>>> FetchBlob: loading 7379328 bytes for "kernel" >>>> Select Item: 0x18 >>>> Select Item: 0x11 >>>> VerifyBlob: Found GUID 4DE79437-ABD2-427F-B835-D5B172D2045B in >> table >>>> VerifyBlob: Hash comparison succeeded for "kernel" >>>> Select Item: 0xB >>>> FetchBlob: loading 12483878 bytes for "initrd" >>>> Select Item: 0x12 >>>> VerifyBlob: Found GUID 44BAF731-3A2F-4BD7-9AF1-41E29169781D in tabl= e >>>> VerifyBlob: Hash comparison succeeded for "initrd" >>>> Select Item: 0x14 >>>> FetchBlob: loading 86 bytes for "cmdline" >>>> Select Item: 0x15 >>>> VerifyBlob: Found GUID 97D02DD8-BD20-4C94-AA78-E7714D36AB2A in >> table >>>> VerifyBlob: Hash comparison succeeded for "cmdline" >>>> ... >>>> >>>> The patch series is organized as follows: >>>> >>>> 1: Simple comment fix in adjacent area in the code. >>>> 2: Use GenericQemuLoadImageLib to gain one location for fw_cfg bl= ob >>>> fetching. >>>> 3: Allow the (previously blocked) usage of -kernel in AmdSevX64. >>>> 4-7: Add BlobVerifierLib with null implementation and use it in the= correct >>>> location in QemuKernelLoaderFsDxe. >>>> 8-9: Reserve memory for hashes table, declare this area in the rese= t vector. >>>> 10-11: Add the secure implementation BlobVerifierLibSevHashes and use= it in >>>> AmdSevX64 builds. >>>> >>>> [1] https://lore.kernel.org/qemu-devel/20210624102040.2015280-1- >>>> dovmurik@linux.ibm.com/ >>>> >>>> Code is at >>>> https://github.com/confidential-containers-demo/edk2/tree/sev-hashes-= v4 >>>> >>>> v4 changes: >>>> - BlobVerifierSevHashes (patch 10): more comprehensive overflow test= s >>>> when parsing the SEV hashes table structure >>>> >>>> v3: https://edk2.groups.io/g/devel/message/77955 >>>> v3 changes: >>>> - Rename to BlobVerifierLibNull, use decimal INF_VERSION, remove unu= sed >>>> DebugLib reference, fix doxygen comments, add missing IN attribute >>>> - Rename to BlobVerifierLibSevHashes, use decimal INF_VERSION, fix >>>> doxygen comments, add missing IN attribute, >>>> calculate buffer hash only when the guid is found in hashes table >>>> - SecretPei: use ALIGN_VALUE to round the hob size >>>> - Coding style fixes >>>> - Add missing 'Ref:' in patch 1 commit message >>>> - Fix phrasing and typos in commit messages >>>> - Remove Cc: Laszlo from series >>>> >>>> v2: https://edk2.groups.io/g/devel/message/77505 >>>> v2 changes: >>>> - Use the last 1KB of the existing SEV launch secret page for hashes= table >>>> (instead of reserving a whole new MEMFD page). >>>> - Build on top of commit cf203024745f >> ("OvmfPkg/GenericQemuLoadImageLib: >>>> Read >>>> cmdline from QemuKernelLoaderFs", 2021-06-28) to have a single loc= ation >> in >>>> which all of kernel/initrd/cmdline are fetched from QEMU. >>>> - Use static linking of the two BlobVerifierLib implemenatations. >>>> - Reorganize series. >>>> >>>> v1: https://edk2.groups.io/g/devel/message/75567 >>>> >>>> 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 >>>> Cc: Leif Lindholm >>>> Cc: Sami Mujawar >>>> >>>> --- >>>> >>>> Ard: please review patch 6 (ArmVirtPkg). Thanks. >>>> >>>> Tom, Brijesh: I'll also send the diff for patch 10. Thanks. >>>> >>>> --- >>>> >>>> Dov Murik (8): >>>> OvmfPkg/AmdSev: use GenericQemuLoadImageLib in AmdSev builds >>>> OvmfPkg: add library class BlobVerifierLib with null implementation >>>> OvmfPkg: add BlobVerifierLibNull to DSC >>>> ArmVirtPkg: add BlobVerifierLibNull to DSC >>>> OvmfPkg/QemuKernelLoaderFsDxe: call VerifyBlob after fetch from fw_= cfg >>>> OvmfPkg/AmdSev/SecretPei: build hob for full page >>>> OvmfPkg: add BlobVerifierLibSevHashes >>>> OvmfPkg/AmdSev: Enforce hash verification of kernel blobs >>>> >>>> James Bottomley (3): >>>> OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming >>>> OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_= cfg >>>> OvmfPkg/AmdSev: reserve MEMFD space for for firmware config hashes >>>> >>>> OvmfPkg/OvmfPkg.dec = | 9 + >>>> ArmVirtPkg/ArmVirtQemu.dsc = | 5 +- >>>> ArmVirtPkg/ArmVirtQemuKernel.dsc = | 5 +- >>>> OvmfPkg/AmdSev/AmdSevX64.dsc = | 9 +- >>>> OvmfPkg/OvmfPkgIa32.dsc = | 5 +- >>>> OvmfPkg/OvmfPkgIa32X64.dsc = | 5 +- >>>> OvmfPkg/OvmfPkgX64.dsc = | 5 +- >>>> OvmfPkg/AmdSev/AmdSevX64.fdf = | 5 +- >>>> OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibNull.inf = | 24 >>>> +++ >>>> OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf = | >> 37 >>>> ++++ >>>> >>>> >> OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub. >>>> inf | 2 + >>>> OvmfPkg/ResetVector/ResetVector.inf = | 2 + >>>> OvmfPkg/Include/Library/BlobVerifierLib.h = | 38 ++++ >>>> OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h >> | >>>> 11 ++ >>>> OvmfPkg/AmdSev/SecretDxe/SecretDxe.c = | 2 +- >>>> OvmfPkg/AmdSev/SecretPei/SecretPei.c = | 3 +- >>>> OvmfPkg/Library/BlobVerifierLib/BlobVerifierNull.c = | 33 >> ++++ >>>> OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c = | >> 199 >>>> ++++++++++++++++++++ >>>> OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c >> | >>>> 5 + >>>> OvmfPkg/Library/{PlatformBootManagerLib =3D> >>>> PlatformBootManagerLibGrub}/QemuKernel.c | 0 >>>> OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c >>>> | 9 + >>>> OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm = | 20 >> ++ >>>> OvmfPkg/ResetVector/ResetVector.nasmb = | 2 + >>>> 23 files changed, 425 insertions(+), 10 deletions(-) >>>> create mode 100644 OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibNu= ll.inf >>>> create mode 100644 >>>> OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf >>>> create mode 100644 OvmfPkg/Include/Library/BlobVerifierLib.h >>>> create mode 100644 OvmfPkg/Library/BlobVerifierLib/BlobVerifierNull.= c >>>> create mode 100644 >> OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c >>>> copy OvmfPkg/Library/{PlatformBootManagerLib =3D> >>>> PlatformBootManagerLibGrub}/QemuKernel.c (100%) >>>> >>>> -- >>>> 2.25.1 >>>> >>>> >>>> >>>>=20 >>>> >>>