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

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

On 06/08/21 14:49, Ard Biesheuvel wrote:
> On Tue, 8 Jun 2021 at 12:59, Laszlo Ersek <lersek@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.
> 

Awesome, thank you!
Laszlo