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

["Dov Murik" <dovmurik@linux.ibm.com>]

On 08/06/2021 13:59, Laszlo Ersek 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).


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.


> 
> 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)!
> 

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

?


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"?


-Dov

> Thanks,
> Laszlo
>