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

Tue Jun 8 12:49:14 UTC 2021

On Tue, 8 Jun 2021 at 12:59, Laszlo Ersek <lersek at redhat.com> wrote:
>
> Ard,
>
> do you have any comments please, on the topic at the bottom?
>
> My comments follow:
>
> On 06/08/21 11:57, Dov Murik wrote:
> >
> >
> > On 04/06/2021 14:26, Laszlo Ersek wrote:
> >> On 06/04/21 12:30, Dov Murik wrote:
> >>
> >
> > ...
> >
> >>>
> >>>> [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?
> >>
> >> Good point. I do have a kind of patch order laid out in my mind, but
> >> I didn't think of whether we should have the patches in one patch
> >> series, or in two "waves".
> >>
> >> OK, let's go with two patch sets.
> >>
> >> In the first set, we should just focus on the above steps (a) and
> >> (b). Step (a) shouldn't be too hard. In step (b), you'd modify both
> >> QemuLoadImageLib instances (two separate patches), replacing the
> >> QemuFwCfgLib APIs for fetching the cmdline with
> >> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL and EFI_FILE_PROTOCOL APIs.
> >>
> >> Speaking from memory, the synthetic filesystem has a unique device
> >> path, so the first step would be calling gBS->LocateDevicePath(), for
> >> finding SimpleFs on the unique device path. Once you have the
> >> SimpleFs interface, you can call OpenVolume, then open the "cmdline"
> >> file using the EFI_FILE_PROTOCOL output by OpenVolume.
> >>
> >> Once we merge this series (basically just three patches), there is no
> >> QemuFwCfgLib dependency left in either QemuLoadImageLib instance, I
> >> reckon.
> >
> > I started working on that, and managed to remove all QemuFwCfg* calls
> > in the main path of QemuLoadKernelImage (so far working on
> > X86QemuLoadImageLib.c).  That works fine: I read the content of the
> > "cmdline" synthetic file, and I check the size of the synthetic
> > "initrd" file.  I used Library/FileHandleLib.h; I hope that's fine.
>
> The lib class header says "Provides interface to EFI_FILE_HANDLE
> functionality", which is not too bad; but I don't immediately see what
> those APIs save us -- the APIs that I believe to be relevant to this use
> case all seem to be thin wrappers around EFI_FILE_PROTOCOL. (The only
> instance of FileHandleLib is
> "MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf".) While not
> necessarily a problem, it doesn't seem an obvious win (unless it saves
> you much complexity and/or code in a way that I'm missing).
>
> In OVMF, the following executables use UefiFileHandleLib at the moment:
>
> - MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
> - ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
> - ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.inf
> - OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
> - ShellPkg/Application/Shell/Shell.inf
>
> The last four are shell-related, so "prior art" is really just BdsDxe...
>
> > However, there's another path (which I don't reach with my test
> > setup), which is the call to QemuLoadLegacyImage, which has a lot of
> > calls to QemuFwCfg* in its body.
> >
> > Am I expected to change that legacy path as well?
> > Or is it in a "it's working don't touch" state?
> > If I modify this, how do I test it?
>
> The use case that you foresee for this feature is really important here.
>
> When you say that you don't reach QemuLoadLegacyImage(), that means your
> guest kernel is built with the UEFI stub.
>
> (1) If you can make the Linux UEFI stub a *required* part of the use
> case, then:
>
> (1a) switch the QemuLoadImageLib class resolution from
> "OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf" to
> "OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf" in
> "OvmfPkg/AmdSev/AmdSevX64.dsc",
>
> (1b) modify "OvmfPkg/Library/GenericQemuLoadImageLib" only (your
> modifications thus far should be easy to transplant to this lib
> instance); ignore "OvmfPkg/Library/X86QemuLoadImageLib". There is no
> QemuLoadLegacyImage() in GenericQemuLoadImageLib.
>
> This makes sense especially because "OvmfPkg/AmdSev/AmdSevX64.dsc" does
> not offer Secure Boot support, so there's not going to be a case when
> gBS->LoadImage() rejects the UEFI stubbed Linux binary due to Secure
> Boot verification failing (EFI_SECURITY_VIOLATION, EFI_ACCESS_DENIED).
>
>
> (2) If you cannot make the Linux UEFI stub a required part of the use
> case, then X86QemuLoadImageLib needs to be modified indeed.
>
> (2a) Unfortunately, in this case we have to add a hack, because the
> synthetic filesystem only exposes the concatenated "setup data + kernel
> image" blob. The following would have to be preserved (as the only
> remaining QemuFwCfgLib access):
>
>   QemuFwCfgSelectItem (QemuFwCfgItemKernelSetupSize);
>   SetupSize = (UINTN)QemuFwCfgRead32 ();
>
> (2b) and then the kernel blob from the synthetic fs would have to be
> split in two (= setup, kernel), within QemuLoadLegacyImage().
>
>
> I'm sorry for missing this aspect previously! I really hope we can go
> with (1)!
>

The legacy x86 loader should only be kept if there is a strong need to
do so. Keeping it around for some theoretical compatibility concern is
simply not worth it, IMHO.

Having two boot paths to reason about in terms of security is not the
only problem: another concern is that the legacy fallback path is
strictly x86, and is tightly coupled with the kernel's struct
boot_params, which is only documented by the .h file that happens to
declare it (and some outdated prose under Documentation/ perhaps)

Also, the EFI stub does some non-trivial work at boot, and having this
uniform between architectures is an important goal, especially for
non-x86 folks like me. We have introduced an initrd loader mechanism
that is fully arch agnostic, and there are patches on the list to move
the measurement of the initrd into the EFI stub if it was loaded using
this mechanism.

In summary, please stick with the generic loader unless you *really* can't.

-- 
Ard.

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76212): https://edk2.groups.io/g/devel/message/76212
Mute This Topic: https://groups.io/mt/83074450/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-