On Tue, 1 Jun 2021 at 14:12, Laszlo Ersek <ler...@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 <ler...@redhat.com> > > Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org> > > Cc: Jordan Justen <jordan.l.jus...@intel.com> > > Cc: Ashish Kalra <ashish.ka...@amd.com> > > Cc: Brijesh Singh <brijesh.si...@amd.com> > > Cc: Erdem Aktas <erdemak...@google.com> > > Cc: James Bottomley <j...@linux.ibm.com> > > Cc: Jiewen Yao <jiewen....@intel.com> > > Cc: Min Xu <min.m...@intel.com> > > Cc: Tom Lendacky <thomas.lenda...@amd.com> > > > > 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 > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#75908): https://edk2.groups.io/g/devel/message/75908 Mute This Topic: https://groups.io/mt/83074450/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
