On 06/04/21 12:30, Dov Murik wrote:
So I argue to keep the existing approach with two separate areas:OK.
If the issue is MEMFD space,Yes, that's it.
maybe we can do something like: use theAll 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.
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.
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,Yes, experimenting with a larger MEMFD in just the AmdSevX64 platform is
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.
Good point. I do have a kind of patch order laid out in my mind, but I- Modifying the QemuFwCfgLib class for this purpose is inappropriate.OK, we'll take the verifier out (as you suggested below - to a
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
- the AmdSevX64.dsc platform resolves the new lib class to the "real"
- 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.
With regard to QemuKernelLoaderFsDxe specifically:OK, I'll refactor to static linking with two BlobVerifierLib
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
(I don't remember why I have the c++ cross-compiler installed.)