From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web10.56787.1622549526322933502 for ; Tue, 01 Jun 2021 05:12:06 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=IHDTiggy; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1622549525; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=FIUFKQZUXZfNRaDhm/1TtBYh2QoVFGg79NFWQkU72WM=; b=IHDTiggy+bw+IpWSta52b1omPIVGUcsi4/plrgLcsyE6IygGL4X3qd/1imLOCWk0R5rU+B 7VyKurJEK2fj00crW3CKewlhuv9wO4HGj4JuYOhoGYcKgrFdfgfTy1kodGyUlQRj3YvMSr GSQlwC9vaArsPq+PVJYa5C0Wm1J8RkM= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-253-m9w6uTSxPuOY_WddgzsfOQ-1; Tue, 01 Jun 2021 08:12:03 -0400 X-MC-Unique: m9w6uTSxPuOY_WddgzsfOQ-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 2C980EC1A0; Tue, 1 Jun 2021 12:12:01 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-181.ams2.redhat.com [10.36.114.181]) by smtp.corp.redhat.com (Postfix) with ESMTPS id D8B725C1C4; Tue, 1 Jun 2021 12:11:57 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline To: devel@edk2.groups.io, dovmurik@linux.ibm.com, Ard Biesheuvel Cc: Tobin Feldman-Fitzthum , Tobin Feldman-Fitzthum , Jim Cadden , James Bottomley , Hubertus Franke , Jordan Justen , Ashish Kalra , Brijesh Singh , Erdem Aktas , Jiewen Yao , Min Xu , Tom Lendacky References: <20210525053116.1533673-1-dovmurik@linux.ibm.com> From: "Laszlo Ersek" Message-ID: <5d8c598e-31de-7973-df51-e913bba54587@redhat.com> Date: Tue, 1 Jun 2021 14:11:56 +0200 MIME-Version: 1.0 In-Reply-To: <20210525053116.1533673-1-dovmurik@linux.ibm.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Ard Biesheuvel > Cc: Jordan Justen > Cc: Ashish Kalra > Cc: Brijesh Singh > Cc: Erdem Aktas > Cc: James Bottomley > Cc: Jiewen Yao > Cc: Min Xu > Cc: Tom Lendacky > > 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 >