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


Dov Murik
 

Thank you Laszlo for reviewing this.


On 01/06/2021 15:11, Laszlo Ersek wrote:
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.)
James explained the difference from Secure Boot setup and our choice of
hash validation of kernel+initrd+cmdline.


[Skipping...]



----*----

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.
Brijesh's approach mandates the guest owner use of SEV secret injection,
because the approved hashes are injected into the secrets page at
launch. There are two disadvantages for this approach:

(a) It requires the use of SEV's LAUNCH_SECRET during launch, thus tying
together measurement of approved boot binaries and the secrets (which in
this case will be used by later kernel or userspace processes). Note
also that the hashes are not confidential (in fact, the host has access
to the full kernel+initrd).

(b) AMD SEV-SNP doesn't support LAUNCH_SECRET any more; instead, in SNP
(/me hand-waving) there's a mechanism to securely verify measurement
later (e.g. during initrd) and if measurement matches then there's a
secure way to retrieve secrets.

My hope is that the approach we proposed (QEMU is building a "hashes
page" which is measured at launch) will be useful also for
secure-booting SNP (and TDX?) guests. The idea is that in SNP the
initial encrypted memory will include OVMF and the hahses page (as in
SEV); this will be measured at launch and saved by SNP; at a later step,
a userspace process requests the initial measurement from the PSP
hardware (in a secure way - that's only possible in SNP); if that
measurement matches then the userspace process may request some secrets.
That said, I'm only beginning to learn about these new generations.

So I argue to keep the existing approach with two separate areas:
existing one for injected secrets, and new one for a table of approved
hashes (filled by QEMU and updated as initial encrypted measured guest
memory).

If the issue is MEMFD space, maybe we can do something like: use the
existing secrets page (4KB) for two uses: first 3KB for secrets, and
last 1KB for hashes. If this is not enough, the hashes are even smaller
than 1KB; and we can even publish only one hash - the hash of all 3
hashes (need to think about edge cases when there's no cmdline/initrd).
But all these "solutions" feel a bit hacky for me and might complicate
the code.

I don't understand your suggestion: "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.";
does this mean to use a single MEMFD page for the injected secrets and
the hashes?

Also, in general, I don't really understand the implications of running
out of MEMFD place; maybe you have other ideas around this (for example,
can we make MEMFD bigger only for AmdSevX64 platform?).


- 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.
OK, we'll take the verifier out (as you suggested below - to a
BlobVerifierLib with two implementations).


[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?
I understand there's agreement here, and that both this suggested change
(use the synthetic filesystem) and my patch series (add hash
verification) touch the same code areas. How do you envision this
process in the mailing list? Seperate patch serieses with dependency?
One long patch series with both changes? What goes first?



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.
OK, I'll refactor to static linking with two BlobVerifierLib imlementations.



(NB QemuKernelLoaderFsDxe is used by some ArmVirtPkg platforms, so
resolutions to the Null instance would be required there too.)
I'll need to learn how to build edk2 for Arm to test this. Thanks for
the heads-up.


-Dov



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

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