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

["Dov Murik" <dovmurik@linux.ibm.com>]

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