On Tue, 8 Jun 2021 at 12:59, Laszlo Ersek <lersek@...> wrote:

Ard,

do you have any comments please, on the topic at the bottom?

My comments follow:

On 06/08/21 11:57, Dov Murik wrote:

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.

The lib class header says "Provides interface to EFI_FILE_HANDLE
functionality", which is not too bad; but I don't immediately see what
those APIs save us -- the APIs that I believe to be relevant to this use
case all seem to be thin wrappers around EFI_FILE_PROTOCOL. (The only
instance of FileHandleLib is
"MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf".) While not
necessarily a problem, it doesn't seem an obvious win (unless it saves
you much complexity and/or code in a way that I'm missing).

In OVMF, the following executables use UefiFileHandleLib at the moment:

- MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
- ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
- ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.inf
- OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
- ShellPkg/Application/Shell/Shell.inf

The last four are shell-related, so "prior art" is really just BdsDxe...

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?

The use case that you foresee for this feature is really important here.

When you say that you don't reach QemuLoadLegacyImage(), that means your
guest kernel is built with the UEFI stub.

(1) If you can make the Linux UEFI stub a *required* part of the use
case, then:

(1a) switch the QemuLoadImageLib class resolution from
"OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf" to
"OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf" in
"OvmfPkg/AmdSev/AmdSevX64.dsc",

(1b) modify "OvmfPkg/Library/GenericQemuLoadImageLib" only (your
modifications thus far should be easy to transplant to this lib
instance); ignore "OvmfPkg/Library/X86QemuLoadImageLib". There is no
QemuLoadLegacyImage() in GenericQemuLoadImageLib.

This makes sense especially because "OvmfPkg/AmdSev/AmdSevX64.dsc" does
not offer Secure Boot support, so there's not going to be a case when
gBS->LoadImage() rejects the UEFI stubbed Linux binary due to Secure
Boot verification failing (EFI_SECURITY_VIOLATION, EFI_ACCESS_DENIED).


(2) If you cannot make the Linux UEFI stub a required part of the use
case, then X86QemuLoadImageLib needs to be modified indeed.

(2a) Unfortunately, in this case we have to add a hack, because the
synthetic filesystem only exposes the concatenated "setup data + kernel
image" blob. The following would have to be preserved (as the only
remaining QemuFwCfgLib access):

QemuFwCfgSelectItem (QemuFwCfgItemKernelSetupSize);
SetupSize = (UINTN)QemuFwCfgRead32 ();

(2b) and then the kernel blob from the synthetic fs would have to be
split in two (= setup, kernel), within QemuLoadLegacyImage().


I'm sorry for missing this aspect previously! I really hope we can go
with (1)!

The legacy x86 loader should only be kept if there is a strong need to
do so. Keeping it around for some theoretical compatibility concern is
simply not worth it, IMHO.

Having two boot paths to reason about in terms of security is not the
only problem: another concern is that the legacy fallback path is
strictly x86, and is tightly coupled with the kernel's struct
boot_params, which is only documented by the .h file that happens to
declare it (and some outdated prose under Documentation/ perhaps)

Also, the EFI stub does some non-trivial work at boot, and having this
uniform between architectures is an important goal, especially for
non-x86 folks like me. We have introduced an initrd loader mechanism
that is fully arch agnostic, and there are patches on the list to move
the measurement of the initrd into the EFI stub if it was loaded using
this mechanism.

In summary, please stick with the generic loader unless you *really* can't.

--
Ard.