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.web12.57505.1622553625231588163 for ; Tue, 01 Jun 2021 06:20:25 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=RJsVuGMQ; spf=pass (domain: kernel.org, ip: 198.145.29.99, mailfrom: ardb@kernel.org) Received: by mail.kernel.org (Postfix) with ESMTPSA id C2500613B1 for ; Tue, 1 Jun 2021 13:20:24 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1622553624; bh=+h17tT1JyWpGsJaSUDOspJ51Khzn/cOhftpdzKUHGw0=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=RJsVuGMQ1jhurgusoy+Lk8KJ7EKFrfKPVD1F9uZLfxAElkRfaxgZV5P5KSrcaCvJG wcgjL7ZvgTtG3/dIa4v/wkCG5eAH6e7pB5F7+dh/B5DwIbZAWWqLWzliWB/se4D1dK breX8ksf3quBiU7GG64mTA+h/vZHcWmD5u2pvI1QbCOw6rDWZX23fq5RUgvCzU6dvj 4Td/QsSCpfjzssQSrhba8ehLgFBa/y/Zk4C3ctxqxQffsYkudf+Es767SVinGdgaiG WZ2s94YUcUylk3oEYiyVFbQGk8KLGYBD82u4Hq24mhf/duigRK1TgpQHa0KqHAGc9x CBNfOnwoxUF/A== Received: by mail-oo1-f48.google.com with SMTP id o5-20020a4a2c050000b0290245d6c7b555so92383ooo.11 for ; Tue, 01 Jun 2021 06:20:24 -0700 (PDT) X-Gm-Message-State: AOAM531Vsg9TB9rYIjMQ5Vrib5gGSJnSz2X8zoIDn+qAbT7LhEQgQ/qi ejVFJ8zNhE0bhFk8xxF2Sme3uMaUuCgg3gLgUVE= X-Google-Smtp-Source: ABdhPJyPkuO0yut0y76Dtw8ai6Dx2yKXM8drtja0OBCt1MqD0TnDCFV6znGTji7F3xL1ir3hY4Y47waGgrg3k892AtA= X-Received: by 2002:a4a:9588:: with SMTP id o8mr2671715ooi.45.1622553624169; Tue, 01 Jun 2021 06:20:24 -0700 (PDT) MIME-Version: 1.0 References: <20210525053116.1533673-1-dovmurik@linux.ibm.com> <5d8c598e-31de-7973-df51-e913bba54587@redhat.com> In-Reply-To: <5d8c598e-31de-7973-df51-e913bba54587@redhat.com> From: "Ard Biesheuvel" Date: Tue, 1 Jun 2021 15:20:13 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline To: Laszlo Ersek Cc: edk2-devel-groups-io , dovmurik@linux.ibm.com, Ard Biesheuvel , 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 Content-Type: text/plain; charset="UTF-8" On Tue, 1 Jun 2021 at 14:12, Laszlo Ersek wrote: > ... > - 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 don't have any problems with that - I take it this means we can drop the QemuFwCfgLib dependency from GenericQemuLoadImageLib altogether, right? > > 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.) > This sounds like a good approach to me. > > > > > 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 > > >