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

["Ard Biesheuvel" <ardb@kernel.org>]

On Tue, 1 Jun 2021 at 14:12, Laszlo Ersek <lersek@redhat.com> 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?

>
> 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.


>
> >
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > 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>
> >
> > 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
> >
>