[edk2-devel] [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline
Dov Murik
dovmurik at linux.ibm.com
Sun Jul 25 10:06:06 UTC 2021
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 at linux.ibm.com>
>> Sent: Sunday, July 25, 2021 3:53 PM
>> To: Yao, Jiewen <jiewen.yao at intel.com>; devel at edk2.groups.io
>> Cc: Tobin Feldman-Fitzthum <tobin at linux.ibm.com>; Tobin Feldman-Fitzthum
>> <tobin at ibm.com>; Jim Cadden <jcadden at ibm.com>; James Bottomley
>> <jejb at linux.ibm.com>; Hubertus Franke <frankeh at us.ibm.com>; Ard Biesheuvel
>> <ardb+tianocore at kernel.org>; Justen, Jordan L <jordan.l.justen at intel.com>;
>> Ashish Kalra <ashish.kalra at amd.com>; Brijesh Singh <brijesh.singh at amd.com>;
>> Erdem Aktas <erdemaktas at google.com>; Xu, Min M <min.m.xu at intel.com>;
>> Tom Lendacky <thomas.lendacky at amd.com>; Leif Lindholm
>> <leif at nuviainc.com>; Sami Mujawar <sami.mujawar at arm.com>; Dov Murik
>> <dovmurik at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Dov
>> Murik
>>>> Sent: Thursday, July 22, 2021 4:43 PM
>>>> To: devel at edk2.groups.io
>>>> Cc: Dov Murik <dovmurik at linux.ibm.com>; Tobin Feldman-Fitzthum
>>>> <tobin at linux.ibm.com>; Tobin Feldman-Fitzthum <tobin at ibm.com>; Jim
>>>> Cadden <jcadden at ibm.com>; James Bottomley <jejb at linux.ibm.com>;
>>>> Hubertus Franke <frankeh at us.ibm.com>; Ard Biesheuvel
>>>> <ardb+tianocore at kernel.org>; Justen, Jordan L <jordan.l.justen at intel.com>;
>>>> Ashish Kalra <ashish.kalra at amd.com>; Brijesh Singh
>> <brijesh.singh at amd.com>;
>>>> Erdem Aktas <erdemaktas at google.com>; Yao, Jiewen
>> <jiewen.yao at intel.com>;
>>>> Xu, Min M <min.m.xu at intel.com>; Tom Lendacky
>>>> <thomas.lendacky at amd.com>; Leif Lindholm <leif at nuviainc.com>; Sami
>>>> Mujawar <sami.mujawar at 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 at 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 at kernel.org>
>>>> Cc: Jordan Justen <jordan.l.justen at intel.com>
>>>> Cc: Ashish Kalra <ashish.kalra at amd.com>
>>>> Cc: Brijesh Singh <brijesh.singh at amd.com>
>>>> Cc: Erdem Aktas <erdemaktas at google.com>
>>>> Cc: James Bottomley <jejb at linux.ibm.com>
>>>> Cc: Jiewen Yao <jiewen.yao at intel.com>
>>>> Cc: Min Xu <min.m.xu at intel.com>
>>>> Cc: Tom Lendacky <thomas.lendacky at amd.com>
>>>> Cc: Leif Lindholm <leif at nuviainc.com>
>>>> Cc: Sami Mujawar <sami.mujawar at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list