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

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

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.

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?


Thanks for the guidance,

-Dov