From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by mx.groups.io with SMTP id smtpd.web11.276.1626802066198729281 for ; Tue, 20 Jul 2021 10:27:46 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=jTGWWGAQ; spf=pass (domain: kernel.org, ip: 198.145.29.99, mailfrom: ardb@kernel.org) Received: by mail.kernel.org (Postfix) with ESMTPSA id AAF0E6120C for ; Tue, 20 Jul 2021 17:27:45 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1626802065; bh=2PFMIF1oJnSlTnToCAwMG9WeIUsUviClKt43Msir+rM=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=jTGWWGAQzdxLrM7zvccLmsz5q763j2vG6AKQCNxfoK7K2y8Yht1AgFahS0q3zihl/ 7EQDpquE6qOKJj9NUoqy7uuARRElHtdrVFwNGqlvZ9zbgMdoP88IQOuEMxaI60LaAe 13MzkrHurrONkhgKYsxi0bYS9oJ8SHlNmV47KnlldT+viXMgApvMJKytjuNnD6NJXu V9zBlBGqrvhN1ZOxeBubF43cZJbGm+SbS6DO1HZJGYT+1nBUkVtcCY/YiCwk2bZVqp EeAld042RzvqzRbPURB/daBc5TR4He1l9nP895g/OqyGvPV+9BgqAX9kTp6Ru+wY1G DqLETzpcklaTw== Received: by mail-oi1-f182.google.com with SMTP id u15so25335995oiw.3 for ; Tue, 20 Jul 2021 10:27:45 -0700 (PDT) X-Gm-Message-State: AOAM533i+gqTHJbDnqcpttFF+L1bhIa5/3K+Kppm3Ak1cxIBSGn7uQAN FDXc7mgWqn3AL2AJvD0O5MiS5egkIP/8XYn6bDM= X-Google-Smtp-Source: ABdhPJy1FbsppP7fYfirKeXRwOcD2ZSCaXPX1OE/4BkvqvdWU0ra2emT8nZL3ssPWQgNJCs/2IjCvmc3QTnQyUhkjIs= X-Received: by 2002:aca:5a04:: with SMTP id o4mr21584982oib.33.1626802064929; Tue, 20 Jul 2021 10:27:44 -0700 (PDT) MIME-Version: 1.0 References: <20210720080401.3662854-1-dovmurik@linux.ibm.com> In-Reply-To: From: "Ard Biesheuvel" Date: Tue, 20 Jul 2021 19:27:33 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v3 00/11] Measured SEV boot with kernel/initrd/cmdline To: Tom Lendacky Cc: Dov Murik , edk2-devel-groups-io , Tobin Feldman-Fitzthum , Tobin Feldman-Fitzthum , Jim Cadden , James Bottomley , Hubertus Franke , Ard Biesheuvel , Jordan Justen , Ashish Kalra , Brijesh Singh , Erdem Aktas , Jiewen Yao , Min Xu , Leif Lindholm , Sami Mujawar Content-Type: text/plain; charset="UTF-8" On Tue, 20 Jul 2021 at 19:22, Tom Lendacky wrote: > > On 7/20/21 3:03 AM, Dov Murik wrote: > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3457 > > I believe the convention is that this line be in the individual patch > commit messages just like this (i.e. with the BZ: tag and the first line), > not as a Ref: tag at the end of the commit message. > > I'll let Ard decide on that, though. > Using Ref: in the signoff section of the patch is perfectly fine with me. I'll go over this series on Thursday and merge it if everything looks ok. Thanks all for the review. > > > > 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 reserves an area in MEMFD (previously the last 1KB of > > the launch secret page) 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 these hashes are part of > > the SEV measurement (which has to be approved by the Guest Owner for > > secret injection, for example). Note that populating the hashes table > > requires QEMU support [1]. > > > > OVMF parses the table of hashes populated by QEMU (patch 10), and as it > > reads the fw_cfg blobs from QEMU, it will verify each one against the > > expected hash. 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. > > > > Relevant part of OVMF serial log during boot with AmdSevX86 build and > > QEMU with -kernel/-initrd/-append: > > > > ... > > BlobVerifierLibSevHashesConstructor: Found injected hashes table in secure location > > Select Item: 0x17 > > Select Item: 0x8 > > FetchBlob: loading 7379328 bytes for "kernel" > > Select Item: 0x18 > > Select Item: 0x11 > > VerifyBlob: Found GUID 4DE79437-ABD2-427F-B835-D5B172D2045B in table > > VerifyBlob: Hash comparison succeeded for "kernel" > > Select Item: 0xB > > FetchBlob: loading 12483878 bytes for "initrd" > > Select Item: 0x12 > > VerifyBlob: Found GUID 44BAF731-3A2F-4BD7-9AF1-41E29169781D in table > > VerifyBlob: Hash comparison succeeded for "initrd" > > Select Item: 0x14 > > FetchBlob: loading 86 bytes for "cmdline" > > Select Item: 0x15 > > VerifyBlob: Found GUID 97D02DD8-BD20-4C94-AA78-E7714D36AB2A in table > > VerifyBlob: Hash comparison succeeded for "cmdline" > > ... > > > > The patch series is organized as follows: > > > > 1: Simple comment fix in adjacent area in the code. > > 2: Use GenericQemuLoadImageLib to gain one location for fw_cfg blob > > fetching. > > 3: Allow the (previously blocked) usage of -kernel in AmdSevX64. > > 4-7: Add BlobVerifierLib with null implementation and use it in the correct > > location in QemuKernelLoaderFsDxe. > > 8-9: Reserve memory for hashes table, declare this area in the reset vector. > > 10-11: Add the secure implementation BlobVerifierLibSevHashes and use it in > > AmdSevX64 builds. > > > > [1] https://lore.kernel.org/qemu-devel/20210624102040.2015280-1-dovmurik@linux.ibm.com/ > > > > Code is at > > https://github.com/confidential-containers-demo/edk2/tree/sev-hashes-v3 > > > > v3 changes: > > - Rename to BlobVerifierLibNull, use decimal INF_VERSION, remove unused > > DebugLib reference, fix doxygen comments, add missing IN attribute > > - Rename to BlobVerifierLibSevHashes, use decimal INF_VERSION, fix > > doxygen comments, add missing IN attribute, > > calculate buffer hash only when the guid is found in hashes table > > - SecretPei: use ALIGN_VALUE to round the hob size > > - Coding style fixes > > - Add missing 'Ref:' in patch 1 commit message > > - Fix phrasing and typos in commit messages > > - Remove Cc: Laszlo from series > > > > v2: https://edk2.groups.io/g/devel/message/77505 > > v2 changes: > > - Use the last 1KB of the existing SEV launch secret page for hashes table > > (instead of reserving a whole new MEMFD page). > > - Build on top of commit cf203024745f ("OvmfPkg/GenericQemuLoadImageLib: Read > > cmdline from QemuKernelLoaderFs", 2021-06-28) to have a single location in > > which all of kernel/initrd/cmdline are fetched from QEMU. > > - Use static linking of the two BlobVerifierLib implemenatations. > > - Reorganize series. > > > > v1: https://edk2.groups.io/g/devel/message/75567 > > > > 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 > > Cc: Leif Lindholm > > Cc: Sami Mujawar > > > > Dov Murik (8): > > OvmfPkg/AmdSev: use GenericQemuLoadImageLib in AmdSev builds > > OvmfPkg: add library class BlobVerifierLib with null implementation > > OvmfPkg: add BlobVerifierLibNull to DSC > > ArmVirtPkg: add BlobVerifierLibNull to DSC > > OvmfPkg/QemuKernelLoaderFsDxe: call VerifyBlob after fetch from fw_cfg > > OvmfPkg/AmdSev/SecretPei: build hob for full page > > OvmfPkg: add BlobVerifierLibSevHashes > > OvmfPkg/AmdSev: Enforce hash verification of kernel blobs > > > > James Bottomley (3): > > OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming > > OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg > > OvmfPkg/AmdSev: reserve MEMFD space for for firmware config hashes > > > > OvmfPkg/OvmfPkg.dec | 9 + > > ArmVirtPkg/ArmVirtQemu.dsc | 5 +- > > ArmVirtPkg/ArmVirtQemuKernel.dsc | 5 +- > > OvmfPkg/AmdSev/AmdSevX64.dsc | 9 +- > > OvmfPkg/OvmfPkgIa32.dsc | 5 +- > > OvmfPkg/OvmfPkgIa32X64.dsc | 5 +- > > OvmfPkg/OvmfPkgX64.dsc | 5 +- > > OvmfPkg/AmdSev/AmdSevX64.fdf | 5 +- > > OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibNull.inf | 24 +++ > > OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf | 37 ++++ > > OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf | 2 + > > OvmfPkg/ResetVector/ResetVector.inf | 2 + > > OvmfPkg/Include/Library/BlobVerifierLib.h | 38 ++++ > > OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h | 11 ++ > > OvmfPkg/AmdSev/SecretDxe/SecretDxe.c | 2 +- > > OvmfPkg/AmdSev/SecretPei/SecretPei.c | 3 +- > > OvmfPkg/Library/BlobVerifierLib/BlobVerifierNull.c | 33 ++++ > > OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c | 200 ++++++++++++++++++++ > > OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c | 5 + > > OvmfPkg/Library/{PlatformBootManagerLib => PlatformBootManagerLibGrub}/QemuKernel.c | 0 > > OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c | 9 + > > OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 20 ++ > > OvmfPkg/ResetVector/ResetVector.nasmb | 2 + > > 23 files changed, 426 insertions(+), 10 deletions(-) > > create mode 100644 OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibNull.inf > > create mode 100644 OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf > > create mode 100644 OvmfPkg/Include/Library/BlobVerifierLib.h > > create mode 100644 OvmfPkg/Library/BlobVerifierLib/BlobVerifierNull.c > > create mode 100644 OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c > > copy OvmfPkg/Library/{PlatformBootManagerLib => PlatformBootManagerLibGrub}/QemuKernel.c (100%) > >