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%)
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-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 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+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> Cc: Leif Lindholm <leif@nuviainc.com> Cc: Sami Mujawar <sami.mujawar@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 (#78075): https://edk2.groups.io/g/devel/message/78075 Mute This Topic: https://groups.io/mt/84375116/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi I am reviewing this patch series. I am OK with most parts. 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? 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 <dovmurik@linux.ibm.com>; 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>; Ard Biesheuvel > <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>; > Ashish Kalra <ashish.kalra@amd.com>; Brijesh Singh <brijesh.singh@amd.com>; > Erdem Aktas <erdemaktas@google.com>; Yao, Jiewen <jiewen.yao@intel.com>; > Xu, Min M <min.m.xu@intel.com>; Tom Lendacky > <thomas.lendacky@amd.com>; Leif Lindholm <leif@nuviainc.com>; Sami > Mujawar <sami.mujawar@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- > 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 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+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> > Cc: Leif Lindholm <leif@nuviainc.com> > Cc: Sami Mujawar <sami.mujawar@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 (#78149): https://edk2.groups.io/g/devel/message/78149 Mute This Topic: https://groups.io/mt/84375116/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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. If that is indeed the case, then I need to: 1. Modify patch 4: put the code of the null implementation in OvmfPkf/BlobVerifierLibNull/ 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? 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. 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. 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 <dovmurik@linux.ibm.com>; 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>; Ard Biesheuvel >> <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>; >> Ashish Kalra <ashish.kalra@amd.com>; Brijesh Singh <brijesh.singh@amd.com>; >> Erdem Aktas <erdemaktas@google.com>; Yao, Jiewen <jiewen.yao@intel.com>; >> Xu, Min M <min.m.xu@intel.com>; Tom Lendacky >> <thomas.lendacky@amd.com>; Leif Lindholm <leif@nuviainc.com>; Sami >> Mujawar <sami.mujawar@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- >> 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 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+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> >> Cc: Leif Lindholm <leif@nuviainc.com> >> Cc: Sami Mujawar <sami.mujawar@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 (#78157): https://edk2.groups.io/g/devel/message/78157 Mute This Topic: https://groups.io/mt/84375116/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Thank you Dov. Comment below: > -----Original Message----- > From: Dov Murik <dovmurik@linux.ibm.com> > Sent: Sunday, July 25, 2021 3:53 PM > To: Yao, Jiewen <jiewen.yao@intel.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>; Ard Biesheuvel > <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>; > Ashish Kalra <ashish.kalra@amd.com>; Brijesh Singh <brijesh.singh@amd.com>; > Erdem Aktas <erdemaktas@google.com>; Xu, Min M <min.m.xu@intel.com>; > Tom Lendacky <thomas.lendacky@amd.com>; Leif Lindholm > <leif@nuviainc.com>; Sami Mujawar <sami.mujawar@arm.com>; Dov Murik > <dovmurik@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/ > > 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) > > > > 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. > > 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. 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 <dovmurik@linux.ibm.com>; 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>; Ard Biesheuvel > >> <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>; > >> Ashish Kalra <ashish.kalra@amd.com>; Brijesh Singh > <brijesh.singh@amd.com>; > >> Erdem Aktas <erdemaktas@google.com>; Yao, Jiewen > <jiewen.yao@intel.com>; > >> Xu, Min M <min.m.xu@intel.com>; Tom Lendacky > >> <thomas.lendacky@amd.com>; Leif Lindholm <leif@nuviainc.com>; Sami > >> Mujawar <sami.mujawar@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- > >> 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 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+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> > >> Cc: Leif Lindholm <leif@nuviainc.com> > >> Cc: Sami Mujawar <sami.mujawar@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 (#78158): https://edk2.groups.io/g/devel/message/78158 Mute This Topic: https://groups.io/mt/84375116/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 <dovmurik@linux.ibm.com> >> Sent: Sunday, July 25, 2021 3:53 PM >> To: Yao, Jiewen <jiewen.yao@intel.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>; Ard Biesheuvel >> <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>; >> Ashish Kalra <ashish.kalra@amd.com>; Brijesh Singh <brijesh.singh@amd.com>; >> Erdem Aktas <erdemaktas@google.com>; Xu, Min M <min.m.xu@intel.com>; >> Tom Lendacky <thomas.lendacky@amd.com>; Leif Lindholm >> <leif@nuviainc.com>; Sami Mujawar <sami.mujawar@arm.com>; Dov Murik >> <dovmurik@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 <dovmurik@linux.ibm.com>; 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>; Ard Biesheuvel >>>> <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>; >>>> Ashish Kalra <ashish.kalra@amd.com>; Brijesh Singh >> <brijesh.singh@amd.com>; >>>> Erdem Aktas <erdemaktas@google.com>; Yao, Jiewen >> <jiewen.yao@intel.com>; >>>> Xu, Min M <min.m.xu@intel.com>; Tom Lendacky >>>> <thomas.lendacky@amd.com>; Leif Lindholm <leif@nuviainc.com>; Sami >>>> Mujawar <sami.mujawar@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- >>>> 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 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+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> >>>> Cc: Leif Lindholm <leif@nuviainc.com> >>>> Cc: Sami Mujawar <sami.mujawar@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/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Sun, 2021-07-25 at 10:52 +0300, Dov Murik wrote: > 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. The original reason for the AmdSev package was actually for attestation: The only way to get attested boot using a standard VM image for SEV and SEV-ES was to pull grub inside the measurement envelope and have a stripped down hard failing boot path, so if the key didn't decode the encrypted boot volume for some reason, the whole thing would fail without revealing the injected secret. This stripped down hard failing boot path is much easier to construct as a separate target. Essentially that means that lots of SEV exists outside the AmdSev directory and things should only be in it if they're either modified to support the encrypted volume boot path or are only required by it. However, this ran into problems when it was decided AmdSev shouldn't have it's own Library, so the modified boot path now lives in OvmfPkg/Library/PlatformBootManagerLibGrub, so now it's unclear even to me what the criteria are. James -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#78160): https://edk2.groups.io/g/devel/message/78160 Mute This Topic: https://groups.io/mt/84375116/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi James "However, this ran into problems when it was decided AmdSev shouldn't have it's own Library." I am not clear on the history. Would you please clarify why AmdSev should not have its own library? It looks not reasonable to me. AmdSev is just a feature. A feature may have its own library. We have enough examples. Also, the instance name "Grub" is very confusing. I compared PlatformBootManagerLib and PlatformBootManagerLibGrub. This is just a customized PlatformBootManagerLib. For example, XEN feature removing and PIIX4 difference has nothing to do with Grub... ================= PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x60), 0x0b); // A PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x61), 0x0b); // B PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x62), 0x0a); // C PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x63), 0x0a); // D ================= It is a big misleading. Can we move the PlatformBootManagerLibGrub To AmdSev now? > -----Original Message----- > From: James Bottomley <jejb@linux.ibm.com> > Sent: Monday, July 26, 2021 5:10 AM > To: devel@edk2.groups.io; dovmurik@linux.ibm.com; Yao, Jiewen > <jiewen.yao@intel.com> > Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>; Tobin Feldman-Fitzthum > <tobin@ibm.com>; Jim Cadden <jcadden@ibm.com>; Hubertus Franke > <frankeh@us.ibm.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, > Jordan L <jordan.l.justen@intel.com>; Ashish Kalra <ashish.kalra@amd.com>; > Brijesh Singh <brijesh.singh@amd.com>; Erdem Aktas > <erdemaktas@google.com>; Xu, Min M <min.m.xu@intel.com>; Tom Lendacky > <thomas.lendacky@amd.com>; Leif Lindholm <leif@nuviainc.com>; Sami > Mujawar <sami.mujawar@arm.com> > Subject: Re: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with > kernel/initrd/cmdline > > On Sun, 2021-07-25 at 10:52 +0300, Dov Murik wrote: > > 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. > > The original reason for the AmdSev package was actually for > attestation: The only way to get attested boot using a standard VM > image for SEV and SEV-ES was to pull grub inside the measurement > envelope and have a stripped down hard failing boot path, so if the key > didn't decode the encrypted boot volume for some reason, the whole > thing would fail without revealing the injected secret. This stripped > down hard failing boot path is much easier to construct as a separate > target. > > Essentially that means that lots of SEV exists outside the AmdSev > directory and things should only be in it if they're either modified to > support the encrypted volume boot path or are only required by it. > However, this ran into problems when it was decided AmdSev shouldn't > have it's own Library, so the modified boot path now lives in > OvmfPkg/Library/PlatformBootManagerLibGrub, so now it's unclear even to > me what the criteria are. > > James > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#78161): https://edk2.groups.io/g/devel/message/78161 Mute This Topic: https://groups.io/mt/84375116/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Mon, 2021-07-26 at 00:55 +0000, Yao, Jiewen wrote: > Hi James > "However, this ran into problems when it was decided AmdSev shouldn't > have it's own Library." > > I am not clear on the history. Would you please clarify why AmdSev > should not have its own library? The history predates me. It was already done for the Bhyve package which also has a modified PlatformBootManagerLib when I came along with this. However, only having Library in the top level package seems to be a common edk2 pattern if you run a find. > It looks not reasonable to me. AmdSev is just a feature. A feature > may have its own library. We have enough examples. We do? Running find . -name Library -print only turns up ./FmpDevicePkg/Test/UnitTest/Library As not following the top level package only pattern. > Also, the instance name "Grub" is very confusing. I compared > PlatformBootManagerLib and PlatformBootManagerLibGrub. This is just a > customized PlatformBootManagerLib. It's called Grub because it places Grub in the Fv for combined pre- attestation. Either SEV or TDX could use this (Although TDX looks likely not to want to). > For example, XEN feature removing and PIIX4 difference has nothing to > do with Grub... > ================= > PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x60), 0x0b); // A > PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x61), 0x0b); // B > PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x62), 0x0a); // C > PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x63), 0x0a); // D > ================= It's part of the boot path stripping to make sure there's a hard failure if Grub fails to execute. There's a Bugzilla requiring more of this because a grub only booting platform library needs fewer extraneous things which could constitute an attack surface for the injected secret. > It is a big misleading. Can we move the PlatformBootManagerLibGrub To > AmdSev now? I think you probably want to ask around older edk2 package maintainers and see if there's any reason for this pattern, which seems to be strongly enforced. If no-one can remember, then likely it can be broken. James -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#78175): https://edk2.groups.io/g/devel/message/78175 Mute This Topic: https://groups.io/mt/84375116/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi James You are right that initially EDKII did recommend to put a lib under Library dir. But with more and more feature, people start realizing that it is NOT efficient way to identify a *feature*. We changed the direction later - if we can define a feature dir, we can lib to feature dir. I used " find . -name *Lib*.inf -print", because I cannot assume the Lib is under Library dir for feature. I can find lots of stuff. Most of them are under Library, below is NOT in *Pkg/Library/ ============= ./edk2/ArmPkg/Drivers/ArmGic/ArmGicLib.inf ./edk2/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf ./edk2/ArmPlatformPkg/PlatformPei/PlatformPeiLib.inf ./edk2/CryptoPkg/Test/UnitTest/Library/BaseCryptLib/TestBaseCryptLibHost.inf ./edk2/CryptoPkg/Test/UnitTest/Library/BaseCryptLib/TestBaseCryptLibShell.inf ./edk2/FmpDevicePkg/FmpDxe/FmpDxeLib.inf ./edk2/FmpDevicePkg/Test/UnitTest/Library/FmpDependencyLib/FmpDependencyLibUnitTestsHost.inf ./edk2/FmpDevicePkg/Test/UnitTest/Library/FmpDependencyLib/FmpDependencyLibUnitTestsUefi.inf ./edk2/MdePkg/Test/UnitTest/Library/BaseLib/BaseLibUnitTestsHost.inf ./edk2/MdePkg/Test/UnitTest/Library/BaseLib/BaseLibUnitTestsUefi.inf ./edk2/MdePkg/Test/UnitTest/Library/BaseSafeIntLib/TestBaseSafeIntLibDxe.inf ./edk2/MdePkg/Test/UnitTest/Library/BaseSafeIntLib/TestBaseSafeIntLibHost.inf ./edk2/MdePkg/Test/UnitTest/Library/BaseSafeIntLib/TestBaseSafeIntLibPei.inf ./edk2/MdePkg/Test/UnitTest/Library/BaseSafeIntLib/TestBaseSafeIntLibSmm.inf ./edk2/MdePkg/Test/UnitTest/Library/BaseSafeIntLib/TestBaseSafeIntLibUefiShell.inf ./edk2/OvmfPkg/Csm/CsmSupportLib/CsmSupportLib.inf ./edk2/OvmfPkg/Csm/LegacyBootMaintUiLib/LegacyBootMaintUiLib.inf ./edk2/OvmfPkg/Csm/LegacyBootManagerLib/LegacyBootManagerLib.inf ============ If I search on edk2-platform (https://github.com/tianocore/edk2-platforms), I can find more: ============ ./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/BaseFuncLib/BaseFuncLib.inf ./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/BaseGpioCheckConflictLib/BaseGpioCheckConflictLib.inf ./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/BaseGpioCheckConflictLibNull/BaseGpioCheckConflictLibNull.inf ./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/BasePlatformHookLib/BasePlatformHookLib.inf ./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/BoardAcpiLib/SmmBoardAcpiEnableLib.inf ./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/BoardAcpiLib/SmmMultiBoardAcpiSupportLib.inf ./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/BoardInitLib/PeiBoardInitPostMemLib.inf ./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/BoardInitLib/PeiBoardInitPreMemLib.inf ./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/BoardInitLib/PeiMultiBoardInitPostMemLib.inf ./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/BoardInitLib/PeiMultiBoardInitPreMemLib.inf ./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/DxePolicyBoardConfigLib/DxePolicyBoardConfigLib.inf ./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/PeiPolicyBoardConfigLib/PeiPolicyBoardConfigLib.inf ./Platform/Intel/CometlakeOpenBoardPkg/Features/Tbt/Library/DxeTbtPolicyLib/DxeTbtPolicyLib.inf ./Platform/Intel/CometlakeOpenBoardPkg/Features/Tbt/Library/PeiDxeSmmTbtCommonLib/TbtCommonLib.inf ./Platform/Intel/CometlakeOpenBoardPkg/Features/Tbt/Library/PeiTbtPolicyLib/PeiTbtPolicyLib.inf ./Platform/Intel/CometlakeOpenBoardPkg/Features/Tbt/Library/Private/PeiDTbtInitLib/PeiDTbtInitLib.inf ./Platform/Intel/CometlakeOpenBoardPkg/FspWrapper/Library/PeiFspPolicyInitLib/PeiFspPolicyInitLib.inf ./Platform/Intel/CometlakeOpenBoardPkg/FspWrapper/Library/PeiSiliconPolicyUpdateLibFsp/PeiSiliconPolicyUpdateLibFsp.inf ./Platform/Intel/CometlakeOpenBoardPkg/Policy/Library/DxePolicyUpdateLib/DxePolicyUpdateLib.inf ./Platform/Intel/CometlakeOpenBoardPkg/Policy/Library/PeiPolicyInitLib/PeiPolicyInitLib.inf ./Platform/Intel/CometlakeOpenBoardPkg/Policy/Library/PeiPolicyUpdateLib/PeiPolicyUpdateLib.inf ./Platform/Intel/KabylakeOpenBoardPkg/Features/Tbt/Library/DxeTbtPolicyLib/DxeTbtPolicyLib.inf ./Platform/Intel/KabylakeOpenBoardPkg/Features/Tbt/Library/PeiDxeSmmTbtCommonLib/TbtCommonLib.inf ./Platform/Intel/KabylakeOpenBoardPkg/Features/Tbt/Library/PeiTbtPolicyLib/PeiTbtPolicyLib.inf ./Platform/Intel/KabylakeOpenBoardPkg/Features/Tbt/Library/Private/PeiDTbtInitLib/PeiDTbtInitLib.inf ./Platform/Intel/KabylakeOpenBoardPkg/FspWrapper/Library/PeiSiliconPolicyNotifyLib/PeiPreMemSiliconPolicyNotifyLib.inf ./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/FspWrapper/Library/PeiSiliconPolicyNotifyLib/PeiPreMemSiliconPolicyNotifyLib.inf ./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/FspWrapper/Library/PeiSiliconPolicyUpdateLibFsp/PeiSiliconPolicyUpdateLibFsp.inf ./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Library/BasePlatformHookLib/BasePlatformHookLib.inf ./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Library/BoardAcpiLib/DxeBoardAcpiTableLib.inf ./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Library/BoardAcpiLib/DxeMultiBoardAcpiSupportLib.inf ./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Library/BoardAcpiLib/SmmBoardAcpiEnableLib.inf ./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Library/BoardAcpiLib/SmmMultiBoardAcpiSupportLib.inf ./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Library/BoardInitLib/PeiBoardInitPostMemLib.inf ./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Library/BoardInitLib/PeiBoardInitPreMemLib.inf ./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Library/BoardInitLib/PeiMultiBoardInitPostMemLib.inf ./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Library/BoardInitLib/PeiMultiBoardInitPreMemLib.inf ./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Policy/Library/DxeSiliconPolicyUpdateLib/DxeSiliconPolicyUpdateLib.inf ./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/FspWrapper/Library/PeiSiliconPolicyUpdateLibFsp/PeiSiliconPolicyUpdateLibFsp.inf ./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Library/BasePlatformHookLib/BasePlatformHookLib.inf ./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Library/BoardAcpiLib/DxeBoardAcpiTableLib.inf ./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Library/BoardAcpiLib/DxeMultiBoardAcpiSupportLib.inf ./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Library/BoardAcpiLib/SmmBoardAcpiEnableLib.inf ./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Library/BoardAcpiLib/SmmMultiBoardAcpiSupportLib.inf ./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Library/BoardInitLib/PeiBoardInitPostMemLib.inf ./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Library/BoardInitLib/PeiBoardInitPreMemLib.inf ./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Library/BoardInitLib/PeiMultiBoardInitPostMemLib.inf ./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Library/BoardInitLib/PeiMultiBoardInitPreMemLib.inf ./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Policy/Library/DxeSiliconPolicyUpdateLib/DxeSiliconPolicyUpdateLib.inf ./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Policy/Library/PeiSiliconPolicyUpdateLib/PeiSiliconPolicyUpdateLib.inf ./Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiEnableLibNull/BoardAcpiEnableLibNull.inf ./Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiTableLibNull/BoardAcpiTableLibNull.inf ./Platform/Intel/MinPlatformPkg/Acpi/Library/DxeAslUpdateLib/DxeAslUpdateLib.inf ./Platform/Intel/MinPlatformPkg/Acpi/Library/MultiBoardAcpiSupportLib/DxeMultiBoardAcpiSupportLib.inf ./Platform/Intel/MinPlatformPkg/Acpi/Library/MultiBoardAcpiSupportLib/SmmMultiBoardAcpiSupportLib.inf ./Platform/Intel/MinPlatformPkg/Bds/Library/BoardBootManagerLibNull/BoardBootManagerLibNull.inf ./Platform/Intel/MinPlatformPkg/Bds/Library/DxePlatformBootManagerLib/DxePlatformBootManagerLib.inf ./Platform/Intel/MinPlatformPkg/Flash/Library/SpiFlashCommonLibNull/SpiFlashCommonLibNull.inf ./Platform/Intel/MinPlatformPkg/FspWrapper/Library/DxeFspWrapperPlatformLib/DxeFspWrapperPlatformLib.inf ./Platform/Intel/MinPlatformPkg/FspWrapper/Library/PeiFspWrapperHobProcessLib/PeiFspWrapperHobProcessLib.inf ./Platform/Intel/MinPlatformPkg/FspWrapper/Library/PeiFspWrapperPlatformLib/PeiFspWrapperPlatformLib.inf ./Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatformSecLib/SecFspWrapperPlatformSecLib.inf ./Platform/Intel/MinPlatformPkg/Pci/Library/PciHostBridgeLibSimple/PciHostBridgeLibSimple.inf ./Platform/Intel/MinPlatformPkg/Pci/Library/PciSegmentInfoLibSimple/PciSegmentInfoLibSimple.inf ./Platform/Intel/MinPlatformPkg/PlatformInit/Library/BoardInitLibNull/BoardInitLibNull.inf ./Platform/Intel/MinPlatformPkg/PlatformInit/Library/MultiBoardInitSupportLib/DxeMultiBoardInitSupportLib.inf ./Platform/Intel/MinPlatformPkg/PlatformInit/Library/MultiBoardInitSupportLib/PeiMultiBoardInitSupportLib.inf ./Platform/Intel/MinPlatformPkg/PlatformInit/Library/PeiReportFvLib/PeiReportFvLib.inf ./Platform/Intel/MinPlatformPkg/PlatformInit/Library/ReportCpuHobLib/ReportCpuHobLib.inf ./Platform/Intel/MinPlatformPkg/PlatformInit/Library/SecBoardInitLibNull/SecBoardInitLibNull.inf ./Platform/Intel/MinPlatformPkg/PlatformInit/Library/SiliconPolicyInitLibNull/SiliconPolicyInitLibNull.inf ./Platform/Intel/MinPlatformPkg/PlatformInit/Library/SiliconPolicyUpdateLibNull/SiliconPolicyUpdateLibNull.inf ./Platform/Intel/MinPlatformPkg/Tcg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf ./Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeTestPointCheckLib.inf ./Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiTestPointCheckLib.inf ./Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/SecTestPointCheckLib.inf ./Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/SmmTestPointCheckLib.inf ./Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLibNull/TestPointCheckLibNull.inf ./Platform/Intel/MinPlatformPkg/Test/Library/TestPointLib/DxeTestPointLib.inf ./Platform/Intel/MinPlatformPkg/Test/Library/TestPointLib/PeiTestPointLib.inf ./Platform/Intel/MinPlatformPkg/Test/Library/TestPointLib/SmmTestPointLib.inf ...... ============ > -----Original Message----- > From: James Bottomley <jejb@linux.ibm.com> > Sent: Monday, July 26, 2021 10:50 PM > To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io; > dovmurik@linux.ibm.com > Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>; Tobin Feldman-Fitzthum > <tobin@ibm.com>; Jim Cadden <jcadden@ibm.com>; Hubertus Franke > <frankeh@us.ibm.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, > Jordan L <jordan.l.justen@intel.com>; Ashish Kalra <ashish.kalra@amd.com>; > Brijesh Singh <brijesh.singh@amd.com>; Erdem Aktas > <erdemaktas@google.com>; Xu, Min M <min.m.xu@intel.com>; Tom Lendacky > <thomas.lendacky@amd.com>; Leif Lindholm <leif@nuviainc.com>; Sami > Mujawar <sami.mujawar@arm.com> > Subject: Re: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with > kernel/initrd/cmdline > > On Mon, 2021-07-26 at 00:55 +0000, Yao, Jiewen wrote: > > Hi James > > "However, this ran into problems when it was decided AmdSev shouldn't > > have it's own Library." > > > > I am not clear on the history. Would you please clarify why AmdSev > > should not have its own library? > > The history predates me. It was already done for the Bhyve package > which also has a modified PlatformBootManagerLib when I came along with > this. However, only having Library in the top level package seems to > be a common edk2 pattern if you run a find. > > > It looks not reasonable to me. AmdSev is just a feature. A feature > > may have its own library. We have enough examples. > > We do? Running > > find . -name Library -print > > only turns up > > ./FmpDevicePkg/Test/UnitTest/Library > > As not following the top level package only pattern. > > > Also, the instance name "Grub" is very confusing. I compared > > PlatformBootManagerLib and PlatformBootManagerLibGrub. This is just a > > customized PlatformBootManagerLib. > > It's called Grub because it places Grub in the Fv for combined pre- > attestation. Either SEV or TDX could use this (Although TDX looks > likely not to want to). > > > For example, XEN feature removing and PIIX4 difference has nothing to > > do with Grub... > > ================= > > PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x60), 0x0b); // A > > PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x61), 0x0b); // B > > PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x62), 0x0a); // C > > PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x63), 0x0a); // D > > ================= > > It's part of the boot path stripping to make sure there's a hard > failure if Grub fails to execute. There's a Bugzilla requiring more of > this because a grub only booting platform library needs fewer > extraneous things which could constitute an attack surface for the > injected secret. > > > It is a big misleading. Can we move the PlatformBootManagerLibGrub To > > AmdSev now? > > I think you probably want to ask around older edk2 package maintainers > and see if there's any reason for this pattern, which seems to be > strongly enforced. If no-one can remember, then likely it can be > broken. > > James > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#78178): https://edk2.groups.io/g/devel/message/78178 Mute This Topic: https://groups.io/mt/84375116/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.