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


Dov Murik
 

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

Join devel@edk2.groups.io to automatically receive all group messages.