Thanks for the explanations. Comments below:
On 25/07/2021 11:17, Yao, Jiewen wrote: > Thank you Dov. > Comment below: > >> -----Original Message----- >> From: Dov Murik <dovmu...@linux.ibm.com> >> Sent: Sunday, July 25, 2021 3:53 PM >> To: Yao, Jiewen <jiewen....@intel.com>; 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>; Xu, Min M <min.m...@intel.com>; >> Tom Lendacky <thomas.lenda...@amd.com>; Leif Lindholm >> <l...@nuviainc.com>; Sami Mujawar <sami.muja...@arm.com>; Dov Murik >> <dovmu...@linux.ibm.com> >> 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 it >>> should >> be OvmfPkg. >>> If a module is only required by OvmfPkg\AmdSev\AmdSevX64.{dsc,fdf}, Then 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. > > >> >> >> 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/BlobVerifierLibNull/ > 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 in >> OvmfPkg/AmdSev/BlobVerifierLibSevHashes/ >> >> Jiewen, is that what you had in mind? > [Jiewen] Let's comply with the exiting rule. > 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 > XXXPkg/FeatureYYY/AAALib. (For example, OvmfPkg/Csm/CsmSupportLib) > OK. I'll send a new version with the paths corrected. > >> >> >> >> 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 Confidential >> 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 implementation > does include SEV specific, such as SEV_HASH_TABLE_GUID, SEV_KERNEL_HASH_GUID. > > If you want to make it for generic confidential computing, then more work > should be done. For example: > 1) The instance name should be BlobVerificationLibTeeLinuxModuleHash. (TEE to > replace SEV. We need Linux keyword, because I don’t think it is applicable > for Windows) > 2) We need consider crypto agile. The current instance only consider SHA256, > which is not allowed in TDX. > 3) The GUID definition should be in OvmfPkg.dec, as the interface. > > Since we don’t have a TEE folder yet, I prefer we defer that work. > 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. > OK I'll keep that in mind for later. Also note that these require corresponding changes in QEMU. > > >> >> 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 first. > 3) We can revisit those advanced feature once we think they are mature. > > 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 year). > 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. > Sounds good. Thanks, -Dov > Thank you > Yao Jiewen > >> >> >> Thanks, >> -Dov >> >> >>> 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:43 PM >>>> To: devel@edk2.groups.io >>>> Cc: Dov Murik <dovmu...@linux.ibm.com>; 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>; Leif Lindholm <l...@nuviainc.com>; Sami >>>> Mujawar <sami.muja...@arm.com> >>>> Subject: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with >>>> kernel/initrd/cmdline >>>> >>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3457 >>>> >>>> Booting with SEV prevented the loading of kernel, initrd, and kernel >>>> command-line via QEMU fw_cfg interface because they arrive from the VMM >>>> 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 in >>>> 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 table >>>> 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 all >>>> 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 therefore >>>> 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 table >>>> 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 blob >>>> 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 reset >>>> vector. >>>> 10-11: Add the secure implementation BlobVerifierLibSevHashes and use it in >>>> AmdSevX64 builds. >>>> >>>> [1] https://lore.kernel.org/qemu-devel/20210624102040.2015280-1- >>>> dovmu...@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 tests >>>> 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 unused >>>> 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 location >> 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 <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> >>>> Cc: Leif Lindholm <l...@nuviainc.com> >>>> Cc: Sami Mujawar <sami.muja...@arm.com> >>>> >>>> --- >>>> >>>> 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 => >>>> 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/BlobVerifierLibNull.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 => >>>> PlatformBootManagerLibGrub}/QemuKernel.c (100%) >>>> >>>> -- >>>> 2.25.1 >>>> >>>> >>>> >>>> >>>> >>> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#78159): https://edk2.groups.io/g/devel/message/78159 Mute This Topic: https://groups.io/mt/84375116/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-