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

Fri Jun 4 11:26:01 UTC 2021

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 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.

>
> 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.

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.

>
>
>> - 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.

>
>
>>
>> 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
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76069): https://edk2.groups.io/g/devel/message/76069
Mute This Topic: https://groups.io/mt/83074450/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-