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

["Laszlo Ersek" <lersek@redhat.com>]

On 06/08/21 14:09, Dov Murik wrote:
> On 08/06/2021 13:59, Laszlo Ersek wrote:
>> On 06/08/21 11:57, Dov Murik wrote:

>>> 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).
>
>
> Using FileHandleGetSize() saves some handling of a EFI_FILE_INFO
> pointer and freeing it etc (for getting the size of "cmdline" and
> "initrd"). But maybe it's better not to add another dependency.

Hmm, no; you have convinced me. Please go ahead with FileHandleLib.
Thank you for taking the time to talk this through with me.

>>> 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)!
>>
>
> I'll check.
>
> But if we go with (1) -- do you (and Ard) prefer:
>
> (a) leave X86QemuLoadImageLib as it is in master;
>
> -or-
>
> (b) modify X86QemuLoadImageLib the "main" path to use the
> QemuKernelLoaderFs (what I started doing) and leave the "legacy" path
> with QemuFwCfg
>
> ?

I prefer option (a), with the extension that we need to update the
following file-top comment in the files under
"OvmfPkg/Library/X86QemuLoadImageLib":

  X86 specific implementation of QemuLoadImageLib library class interface
  with support for loading mixed mode images and non-EFI stub images

We should add a warning there that this library instance (a) depends on
fw_cfg directly, and (b) is therefore unsuitable for blob verification
purposes.

> Or, in other words, is the refactoring to read the cmdline from
> QemuKernelLoaderFs (across both QemuLoadImageLib implementations)
> beneficial even if we don't add the verification "hooks"?

My answer would be "no". IMO, the refactoring is only useful for
unifying the blob verifier in a single layer (the synthetic fs). Without
the blob verifier in the picture, I see nothing wrong with fetching
different fw_cfg blobs in different modules. The kernel command line
never needed to be available through the synthetic FS, for any
particular consumer, so removing that synthetic file was fine.

Now, we do have an internal consumer of sorts (the blob verifier). Given
that we don't want to add a whole new extra layer for it, and we also
don't want to scatter it over multiple modules, integrating it into the
synthetic FS make sense, hopefully.

I initially thought that the refactoring would benefit both
QemuLoadImageLib instances, but you've shown that I missed the
complications in the legacy path of X86QemuLoadImageLib. Namely, that
the synthetic filesystem abstraction (= the fused setup data + kernel
image blob) isn't a good match for the legacy path.

Library instances are permitted to have different features and different
limitations.

(If we *really* wanted to do refactor the legacy path cleanly, we could
further extend the QemuKernelLoaderFsDxe driver, to present -- under new
filenames -- the setup data blob isolation, and the bare kernel image
blob in isolation. But this would be a lot of wasted work, in practice,
provided that your use case requires a UEFI stub on the guest kernel at
all times (= the legacy path would never be taken). Also, let's not
forget, if we exposed such *new* synthetic files, the blob verifier
would have to verify those isolated blobs too (you'd have to provide
hashes for them from the outside); otherwise the whole exercise would be
moot, I think.)

Thanks,
Laszlo