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

["Laszlo Ersek" <lersek@redhat.com>]

Ard,

I'll have a specific question for you below; please feel free to jump
forward (search for your name). Thanks.

Dov, my comments below:

On 05/25/21 07:31, Dov Murik wrote:
> 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 declares a new page in MEMFD 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
> theses hashes are part of the SEV measurement (which has to be approved
> by the Guest Owner for secret injection, for example).  Note that this
> requires a new QEMU patch which will be submitted soon.
> 
> OVMF parses the table of hashes populated by QEMU (patch 5), and as it
> reads the fw_cfg blobs from QEMU, it will verify each one against the
> expected hash (kernel and initrd verifiers are introduced in patch 6,
> and command-line verifier is introduced in patches 7+8).  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.

Please catch the error in my reasoning below.

The goal is for the guest firmware to ensure the authenticity
(integrity) of kernel, initrd, cmdline.

This is not really different from a normal Secure Boot setup, where the
guest firmware verifies the kernel image (presented as a UEFI
application, i.e. with the UEFI stub). It is up to the kernel to verify
the integrity of the initrd. The command line is not particularly
verified (as far as I know?), but if that's a problem, it should be
solved even for bare metal Secure Boot use cases. (Because, if the
"root" user is compromised on a running Linux system, they can modify
the kernel params for next boot in the grub config.)

The AmdSevX64 platform uses a unified firmware image (executable +
varstore are presented as one blob, no separate CODE and VARS). There is
one pflash chip, and the initial guest-owner-side measurement covers the
whole blob, including the varstore.

This suggests that the guest owner could boot the unified firmware image
in a trusted guest environment first, and use UEFI-level tools to enroll
various SB certificates. Then this modified image would be deployed
every time to the untrusted cloud.

The AmdSevX64 platform could adopt a PlatformBootManagerLib instance
where the TryRunningQemuKernel() call is reinstated, backed by the usual
QemuLoadImageLib class APIs QemuLoadKernelImage() and
QemuStartKernelImage().

edk2 offers two QemuLoadImageLib instances, GenericQemuLoadImageLib and
X86QemuLoadImageLib. The former strictly enforces SB verification. That
was in fact a *problem* for the traditional OvmfPkg platforms; please
refer to commit 82808b422617 ("Revert "OvmfPkg: use generic QEMU image
loader for secure boot enabled ..."", 2020-06-16). But the same rigor
seems just right here, for the AmdSevX64 platform.

Where I see a gap in all this myself -- and of course there could be
plenty other gaps that I just don't see -- is the varstore's protection
from the hypervisor, once the guest is up and running. Can we discuss
that perhaps?

If necessary, we could perhaps rework the AmdSevX64 platform to drop the
pflash-backed variable driver stack, and use in-RAM (memory-only)
variable emulation. Actual persistence / non-volatility of UEFI
variables may not really be relevant for the remotely attested platform,
but keeping all the variables in RAM would subject the varstore to
memory encryption / protection. And perhaps we could integrate the
enrollment of SB certificates into the *code* part of the firmware, with
gRT->SetVariable() calls. (Normally this would be absolutely horrible,
but for the remotely attested platform, anything goes.)

I simply dislike adding brand new code for a use case which at least
*appears* to significantly overlap with that of Secure Boot. Secure Boot
is about image verification, and it's rooted in tamper-resistant storage
of certificates and/or image hashes. If we can figure out "tamper
resistant" in the current context, we could perhaps reuse much of the
existent SB infrastructure.

----*----

Considering the particular approach in this set:

- To reiterate Brijesh's point, I feel a new MEMFD page is wasteful. If
we really need the MEMFD approach, I'd *really* like us to extend one of
the existent structures. If necessary, introduce a new GUID, for a table
that contains both previously injected data, and the new data. If all
that's impossible or too awkward, please document why.

- Modifying the QemuFwCfgLib class for this purpose is inappropriate.
Even if we do our own home-brewed verifier, none of it must go into
QemuFwCfgLib class. QemuFwCfgLib is for transport.

[Ard, please see this one question:]

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


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

Thanks
Laszlo



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