Re: [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline


Laszlo Ersek
 

On 06/01/21 15:20, Ard Biesheuvel wrote:
On Tue, 1 Jun 2021 at 14:12, Laszlo Ersek <lersek@...> wrote:
...
- A major complication for hashing all three of: kernel, initrd,
cmdline, is that the *fetching* of this triplet is split between two
places. (Well, it is split between *three* places in fact, but I'm
going to ignore LinuxInitrdDynamicShellCommand for now, because the
AmdSevX64 platform sets BUILD_SHELL to FALSE for production.)

The kernel and the initrd are fetched in QemuKernelLoaderFsDxe, but
the command line is fetched in (both) QemuLoadImageLib instances.
This requires that all these modules be littered with hashing as
well, which I find *really bad*. Even if we factor out the actual
logic, I strongly dislike having *just hooks* for hashing in multiple
modules.

Now, please refer to efc52d67e157 ("OvmfPkg/QemuKernelLoaderFsDxe:
don't expose kernel command line", 2020-03-05). If we first

(a) reverted that commit, and

(b) modified *both* QemuLoadImageLib instances, to load the kernel
command line from the *synthetic filesystem* (rather than directly
from fw_cfg),

then we could centralize the hashing to just QemuKernelLoaderFsDxe.

Ard -- what's your thought on this?
I don't have any problems with that - I take it this means we can drop
the QemuFwCfgLib dependency from GenericQemuLoadImageLib altogether,
right?
A bit more work is needed for that (but I agree it should be done),
because we have this additionally:

QemuFwCfgSelectItem (QemuFwCfgItemInitrdSize);
InitrdSize = (UINTN)QemuFwCfgRead32 ();

if (InitrdSize > 0) {
//
// Append ' initrd=initrd' in UTF-16.
//
KernelLoadedImage->LoadOptionsSize += sizeof (L" initrd=initrd") - 2;
}

This should also be rebased on top of the synthetic filesystem [*], and
then no more QemuFwCfgLib calls should remain.

[*] From StubFileOpen() in "QemuKernelLoaderFsDxe.c", it seems that we
successfully open zero-sized fw_cfg files. (We also list them, when
reading the root directory, in StubFileRead()). That's not a problem
at all, but it means that, after opening the initrd file temporarily
in QemuLoadImageLib, EFI_FILE_PROTOCOL.GetInfo() should be used for
fetching an EFI_FILE_INFO, and then EFI_FILE_INFO.FileSize needs to
be compared to 0.




And then, we could eliminate the dynamic callback registration, plus
the separate SevFwCfgVerifier, SevHashFinderLib, and
SevQemuLoadImageLib stuff.

We'd only need one new lib class, with *statically linked* hooks for
QemuKernelLoaderFsDxe, and two instances of this new class, a Null
one, and an actual (SEV hash verifier) one. The latter instance would
locate the hash values, calculate the fresh hashes, and perform the
comparisons. Only the AmdSevX64 platform would use the non-Null
instance of this library class.

(NB QemuKernelLoaderFsDxe is used by some ArmVirtPkg platforms, so
resolutions to the Null instance would be required there too.)
This sounds like a good approach to me.
Thank you!
Laszlo





Cc: Laszlo Ersek <lersek@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Ashish Kalra <ashish.kalra@...>
Cc: Brijesh Singh <brijesh.singh@...>
Cc: Erdem Aktas <erdemaktas@...>
Cc: James Bottomley <jejb@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Min Xu <min.m.xu@...>
Cc: Tom Lendacky <thomas.lendacky@...>

James Bottomley (8):
OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming
OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg
OvmfPkg/AmdSev: add a page to the MEMFD for firmware config hashes
OvmfPkg/QemuKernelLoaderFsDxe: Add ability to verify loaded items
OvmfPkg/AmdSev: Add library to find encrypted hashes for the FwCfg
device
OvmfPkg/AmdSev: Add firmware file plugin to verifier
OvmfPkg: GenericQemuLoadImageLib: Allow verifying fw_cfg command line
OvmfPkg/AmdSev: add SevQemuLoadImageLib

OvmfPkg/OvmfPkg.dec | 10 ++
OvmfPkg/AmdSev/AmdSevX64.dsc | 9 +-
OvmfPkg/AmdSev/AmdSevX64.fdf | 3 +
OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf | 30 +++++
OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf | 34 ++++++
OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf | 30 +++++
OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf | 2 +
OvmfPkg/ResetVector/ResetVector.inf | 2 +
OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h | 47 ++++++++
OvmfPkg/Include/Library/QemuFwCfgLib.h | 35 ++++++
OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h | 11 ++
OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c | 60 ++++++++++
OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c | 126 ++++++++++++++++++++
OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c | 52 ++++++++
OvmfPkg/AmdSev/SecretDxe/SecretDxe.c | 2 +-
OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c | 29 +++++
OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c | 5 +
OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c | 50 ++++++++
OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c | 31 +++++
OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 20 ++++
OvmfPkg/ResetVector/ResetVector.nasmb | 2 +
21 files changed, 587 insertions(+), 3 deletions(-)
create mode 100644 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf
create mode 100644 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf
create mode 100644 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf
create mode 100644 OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h
create mode 100644 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c
create mode 100644 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c
create mode 100644 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c
create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c



Join devel@edk2.groups.io to automatically receive all group messages.