From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.156.1]) by mx.groups.io with SMTP id smtpd.web12.6552.1622802625522665815 for ; Fri, 04 Jun 2021 03:30:25 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@ibm.com header.s=pp1 header.b=qUoa4p2d; spf=pass (domain: linux.ibm.com, ip: 148.163.156.1, mailfrom: dovmurik@linux.ibm.com) Received: from pps.filterd (m0098393.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.43/8.16.0.43) with SMTP id 154A41w3126875; Fri, 4 Jun 2021 06:30:24 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=subject : to : cc : references : from : message-id : date : mime-version : in-reply-to : content-type : content-transfer-encoding; s=pp1; bh=kItGrBWx4TCNZ6+lMdXA8yY6IgTdMduHGMeD85ch+FM=; b=qUoa4p2duDIPfFWaiJPLEqLTnT8BpkpDzevK02mkNdZ24mRItMs4GkPAfVxvakWar/XM bV8J+cPUohcQk0e/uohMgvdH3NQACw6mEF6GAI9pMNfm3qOyEGOZ599WeRcml5WuCi/p nTeIEiYZXGwMOX3jin5neTAEaqg6eN2BBFyqM1UHLFH4XeP+1xvM5UsgPXcJYvmy7ZHG 9E7sge9l/Ea3KsekbmjQCAwaYAnTKWp60+JEP8BxMGFWa6eeT9TE2MEOCmUyNbrK/tR2 rZeTvFdruLy2YdbMr2Y72vKyNL3aLT/dFYly+e8HAHrN8KUN38zS1/csJaCkALfG/mdh xw== Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com with ESMTP id 38ygsmju7q-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 04 Jun 2021 06:30:23 -0400 Received: from m0098393.ppops.net (m0098393.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.43/8.16.0.43) with SMTP id 154A4QjI127939; Fri, 4 Jun 2021 06:30:23 -0400 Received: from ppma02fra.de.ibm.com (47.49.7a9f.ip4.static.sl-reverse.com [159.122.73.71]) by mx0a-001b2d01.pphosted.com with ESMTP id 38ygsmju6b-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 04 Jun 2021 06:30:23 -0400 Received: from pps.filterd (ppma02fra.de.ibm.com [127.0.0.1]) by ppma02fra.de.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 154ASQEK023745; Fri, 4 Jun 2021 10:30:20 GMT Received: from b06avi18626390.portsmouth.uk.ibm.com (b06avi18626390.portsmouth.uk.ibm.com [9.149.26.192]) by ppma02fra.de.ibm.com with ESMTP id 38w413s9xx-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 04 Jun 2021 10:30:20 +0000 Received: from d06av24.portsmouth.uk.ibm.com (d06av24.portsmouth.uk.ibm.com [9.149.105.60]) by b06avi18626390.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 154ATcYs12452140 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 4 Jun 2021 10:29:38 GMT Received: from d06av24.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id A806842049; Fri, 4 Jun 2021 10:30:14 +0000 (GMT) Received: from d06av24.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 8C55742042; Fri, 4 Jun 2021 10:30:10 +0000 (GMT) Received: from [9.65.213.35] (unknown [9.65.213.35]) by d06av24.portsmouth.uk.ibm.com (Postfix) with ESMTP; Fri, 4 Jun 2021 10:30:10 +0000 (GMT) Subject: Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline To: Laszlo Ersek , devel@edk2.groups.io, 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> <5d8c598e-31de-7973-df51-e913bba54587@redhat.com> From: "Dov Murik" Message-ID: Date: Fri, 4 Jun 2021 13:30:08 +0300 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <5d8c598e-31de-7973-df51-e913bba54587@redhat.com> X-TM-AS-GCONF: 00 X-Proofpoint-ORIG-GUID: U6P0o_3tE9t4uJ9Ck6KO_0zNfvFTDPIv X-Proofpoint-GUID: EUkqSj0q7DI1EGvxk2WpPs6tjx6TinyQ X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.391,18.0.761 definitions=2021-06-04_05:2021-06-04,2021-06-04 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 malwarescore=0 priorityscore=1501 mlxscore=0 spamscore=0 phishscore=0 clxscore=1015 adultscore=0 lowpriorityscore=0 suspectscore=0 impostorscore=0 bulkscore=0 mlxlogscore=999 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2104190000 definitions=main-2106040080 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >> 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 >> >