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)! Thanks, Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#76194): https://edk2.groups.io/g/devel/message/76194 Mute This Topic: https://groups.io/mt/83074450/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-