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:
> 
>> So I argue to keep the existing approach with two separate areas:
>> existing one for injected secrets, and new one for a table of approved
>> hashes (filled by QEMU and updated as initial encrypted measured guest
>> memory).
> 
> OK.
> 
>> If the issue is MEMFD space,
> 
> Yes, that's it.
> 
>> maybe we can do something like: use the
>> existing secrets page (4KB) for two uses: first 3KB for secrets, and
>> last 1KB for hashes.  If this is not enough, the hashes are even
>> smaller than 1KB; and we can even publish only one hash - the hash of
>> all 3 hashes (need to think about edge cases when there's no
>> cmdline/initrd). But all these "solutions" feel a bit hacky for me and
>> might complicate the code.
> 
> All these PCDs come in pairs -- base and size. (IIRC.) If there's no
> architectural requirement to keep these two kinds of info in different
> pages (such as different page protections or whatever), then packing
> them into a single page is something I'd like. The above 3K+1K
> subdivision sounds OK to me.
> 

I'll go with 3KB secrets + 1KB hashes.


>>
>> I don't understand your suggestion: "I'd *really* like us to extend
>> one of the existent structures. If necessary, introduce a new GUID,
>> for a table that contains both previously injected data, and the new
>> data."; does this mean to use a single MEMFD page for the injected
>> secrets and the hashes?
> 
> Yes, it's the same (say, 3K+1K) idea, just expressed differently. In one
> case, you have two GUIDed structs in the (plaintext, not compressed)
> reset vector in the pflash, and the base+size structures associated wth
> those two separate GUIDs happen to identify distinct ranges of the same
> MEMFD page. In the other case, you have just one GUIDed structure (with
> base+size contents), and the page pointed-to by this base+size pair is
> subdivided by *internal* structuring -- such as internal GUIDs and so
> on. Whichever is simpler to implement in both QEMU and edk2; I just want
> to avoid wasing a full page for three hashes.
> 

I'll go with the two GUIDed structures in the reset vector (which will
point to distinct parts of a single 4KB page).

That actually means shortening the existing secrets MEMFD area from 4KB
to 3KB. Is that OK?



>>
>> Also, in general, I don't really understand the implications of
>> running out of MEMFD place;
> 
> Here's one implication of enlarging MEMFD. It pushes BS Code, BS Data,
> Loader Code, Loader Data, perhaps some AcpiNVS and Reserved memory
> allocations to higher addresses. Then when the kernel is loaded, its
> load address may be higher too. I'm not worried about wasted guest
> memory, but abut various silent assumptions as to where the kernel
> "should be". For example, after one round of enlarging DXEFV, the
> "crash" utility stopped opening guest memory dumps, because it couldn't
> find a kernel signature in the (low) address range that it used to scan.
> The fix wasn't too difficult (the range to scan could be specified on
> the "crash" commadn line, and then my colleague Dave Anderson just
> modified "crash"), but it was a *surprise*. I don't like those.
> 
>> maybe you have other ideas around this (for example,
>> can we make MEMFD bigger only for AmdSevX64 platform?).
> 
> Yes, experimenting with a larger MEMFD in just the AmdSevX64 platform is
> fine.
> 

But now I understand that failures can appear way later in userspace
(the crash utility), so just testing that a modern AMD VM boots fine is
not enough to get confidence here.


> NB reordering various PCDs between each other, so that their relative
> relationships (orders) change, is a *lot* more risky than just enlarging
> existing areas. The code in OVMF tends not to rely on actual bases and
> sizes, but it may very well rely on a particular BasePCD + SizePCD sum
> not exceeding another particular BasePCD.
> 

Thanks for pointing this out. I'll avoid reordering.


>>
>>
>>> - Modifying the QemuFwCfgLib class for this purpose is inappropriate.
>>> Even if we do our own home-brewed verifier, none of it must go into
>>> QemuFwCfgLib class. QemuFwCfgLib is for transport.
>>>
>>
>> OK, we'll take the verifier out (as you suggested below - to a
>> BlobVerifierLib with two implementations).
>>
>>
>>> [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. Then you can post the second wave, in which:
> 
> - a new "firmware config verifier" library class is introduced,
> 
> - two library instances for that class are introduced (null, and the
>   real thing),
> 
> - the AmdSevX64.dsc platform resolves the new lib class to the "real"
>   (hashing) instance,
> 
> - all other platform DSCs using QemuKernelLoaderFsDxe resolve the new
>   lib class to the null instance,
> 
> - QemuKernelLoaderFsDxe is extended with a dependency on the new class,
>   calling the proper APIs to (a) initialize the verifier, and (b) verify
>   every fw_cfg blob that is about to be exposed as a synthetic file.
> 
> Then QemuLoadImageLib needs no changes, as it will not depend on fw_cfg,
> and every synthetic file it may want to access will have been verified
> by QemuKernelLoaderFsDxe already, according to the verifier lib instance
> that's used in the respective platform DSC file.
> 
> I would recommend only posting the first patch set initially. It has a
> very well defined goal (--> hide the fw_cfg dependency in both
> QemuLoadImageLib instances behind the synthetic filesystem); we can
> validate / review that regardless of the ultimate crypto / security
> goal. Using the SimpleFs / FILE protocol APIs is not trivial IMO, so
> it's possible that just the first wave will require a v2.
> 

OK, I'll try to follow this plan.

>>
>>
>>>
>>> And then, we could eliminate the dynamic callback registration, plus
>>> the separate SevFwCfgVerifier, SevHashFinderLib, and
>>> SevQemuLoadImageLib stuff.
>>>
>>> We'd only need one new lib class, with *statically linked* hooks for
>>> QemuKernelLoaderFsDxe, and two instances of this new class, a Null
>>> one, and an actual (SEV hash verifier) one. The latter instance would
>>> locate the hash values, calculate the fresh hashes, and perform the
>>> comparisons. Only the AmdSevX64 platform would use the non-Null
>>> instance of this library class.
>>
>> OK, I'll refactor to static linking with two BlobVerifierLib
>> imlementations.
>>
>>
>>>
>>> (NB QemuKernelLoaderFsDxe is used by some ArmVirtPkg platforms, so
>>> resolutions to the Null instance would be required there too.)
>>
>> I'll need to learn how to build edk2 for Arm to test this.  Thanks for
>> the heads-up.
> 
> With regard to QemuKernelLoaderFsDxe specifically:
> 
>   build -b NOOPT -t GCC5 -p ArmVirtPkg/ArmVirtQemu.dsc               -a AARCH64
>   build -b NOOPT -t GCC5 -p ArmVirtPkg/ArmVirtQemu.dsc       -a ARM
>   build -b NOOPT -t GCC5 -p ArmVirtPkg/ArmVirtQemuKernel.dsc         -a AARCH64
>   build -b NOOPT -t GCC5 -p ArmVirtPkg/ArmVirtQemuKernel.dsc -a ARM
> 
> If you work on an x86_64 machine, you'll need cross-gcc and
> cross-binutils for this. I have the following packages installed on my
> laptop:
> 
>   binutils-aarch64-linux-gnu-2.31.1-3.el7.x86_64
>   binutils-arm-linux-gnu-2.31.1-3.el7.x86_64
>   cross-binutils-common-2.31.1-3.el7.noarch
> 
>   cross-gcc-common-9.2.1-3.el7.1.noarch
>   gcc-aarch64-linux-gnu-9.2.1-3.el7.1.x86_64
>   gcc-arm-linux-gnu-9.2.1-3.el7.1.x86_64
>   gcc-c++-aarch64-linux-gnu-9.2.1-3.el7.1.x86_64
>   gcc-c++-arm-linux-gnu-9.2.1-3.el7.1.x86_64
> 
> (I don't remember why I have the c++ cross-compiler installed.)
> 

Thanks for the details; I'll try it.

-Dov