From 74e5313dfa6719f7990c7e175e035d17c9b3f657 Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Fri, 5 Jun 2020 23:44:43 +0200
Subject: Revert "OvmfPkg: use generic QEMU image loader for secure boot
 enabled builds"

Notes about the RHEL-8.2/20190904-37eef91017ad [edk2-stable201908] ->
RHEL-8.3/20200603-ca407c7246bf [edk2-stable202005] rebase:

- new patch (to be dropped later, hopefully)

This reverts commit ced77332cab626f35fbdb36630be27303d289d79.

Upstream commit ced77332cab6 ("OvmfPkg: use generic QEMU image loader for
secure boot enabled builds", 2020-03-05) changes the "Secure Boot threat
model" in a way that is incompatible with at least two use cases.

Namely, OVMF has always considered kernel images direct-booted via fw_cfg
as trusted, bypassing Secure Boot validation. While that approach is
rooted in a technicality (namely, OVMF doesn't load such images with the
LoadImage() UEFI boot service / through the UEFI stub, but with the
Linux/x86 Boot Protocol), that doesn't mean it's wrong. The direct-booted
kernel from fw_cfg comes from the host side, and Secure Boot in the guest
is a barrier between the guest firmware and the guest operating system --
it's not a barrier between host and guest.

Upstream commit ced77332cab6 points out that the above (historical) OVMF
behavior differs from ArmVirtQemu's -- the latter direct-boots kernels
from fw_cfg with the LoadImage() / StartImage() boot services. While that
difference indeed exists between OVMF and ArmVirtQemu, it's not relevant
for RHEL downstream. That's because we never build the ArmVirtQemu
firmware with the Secure Boot feature, so LoadImage() can never reject the
direct-booted kernel due to a signing issue.

Subjecting a kernel direct-booted via fw_cfg to Secure Boot verification
breaks at least two use cases with OVMF:

- It breaks the %check stage in the SPEC file.

  In that stage, we use the "ovmf-vars-generator" utility from the
  "qemu-ovmf-secureboot" project, for verifying whether the Secure Boot
  operational mode is enabled. The guest kernel is supposed to boot, and
  to print "Secure boot enabled".

  As guest kernel, we pick whatever host kernel is available in the Brew
  build root. The kernel in question may be a publicly released RHEL
  kernel, signed with "Red Hat Secure Boot (signing key 1)", or a
  development build, signed for example with "Red Hat Secure Boot Signing
  3 (beta)". Either way, none of these keys are accepted by the
  certificates that were enrolled by "ovmf-vars-generator" /
  "EnrollDefaultKeys.efi" in the %build stage. Therefore, the %check stage
  fails.

- It breaks "virt-install --location NETWORK-URL" Linux guest
  installations, if the variable store template used for the new domain
  has the Secure Boot operational mode enabled. "virt-install --location"
  fetches the kernel from the remote OS tree, and passes it to the guest
  firmware via fw_cfg. Therefore the above symptom appears (even for
  publicly released OSes).

  Importantly, if the user downloads the installer ISO of the publicly
  released Fedora / RHEL OS, and exposes the ISO to the guest for example
  as a virtio-scsi CD-ROM, then the installation with "virt-install"
  (without "--location") does succeed. That's because that way, "shim" is
  booted first, from the UEFI-bootable CD-ROM. "Shim" does pass Secure
  Boot verification against the Microsoft certificates, and then it is
  "shim" that accepts the "Red Hat Secure Boot (signing key 1)" signature
  on the guest kernel.

Some ways to approach this problem (without reverting upstream commit
ced77332cab6):

- Equip "ovmf-vars-generator" / "EnrollDefaultKeys.efi" to enroll the
  public half of "Red Hat Secure Boot (signing key 1)" in the %build
  stage. Use a publicly released RHEL kernel in the %check stage.

  Downsides:

  - The Brew build root does not offer any particular released RHEL
    kernel, so either the %check stage would have to download it, or the
    SRPM would have to bundle it. However, Brew build environments do not
    have unfettered network access (rightly so), so the download wouldn't
    work. Furthermore, for bundling with the SRPM, such a kernel image
    could be considered too large.

  - Does not solve the "virt-install --location" issue for other vendors'
    signed kernels.

- Invoke "ovmf-vars-generator" / "EnrollDefaultKeys.efi" multiple times
  during %build, to create multiple varstore templates. One that would
  accept publicly released RHEL kernels, and another to accept development
  kernels. Don't try to use a particular guest kernel for verification;
  instead, check what kernel Brew offers in the build environment, and use
  the varstore template matching *that* kernel.

  Downsides:

  - It may be considered useless to perform %check with a varstore
    template that is *not* the one that we ship.

  - Does not solve the "virt-install --location" issue for other vendors'
    signed kernels.

- Sign the RHEL kernels such that the currently enrolled certificates
  accept them.

  Downsides:

  - Not feasible at all; it would require Microsoft to sign our kernels.
    "Shim" exists exactly to eliminate such signing requirements.

- Modify "virt-install --location NETWORK-URL" such that it download a
  complete (UEFI-bootable) installer ISO image, rather than broken-out
  vmlinuz / initrd files. In other words, replace direct (fw_cfg) kernel
  boot with a CD-ROM / "shim" boot, internally to "virt-install".

  Downsides:

  - Defeats the goal of "virt-install --location NETWORK-URL", and defeats
    the network installation method of (for example) Anaconda.

For now, revert upstream commit ced77332cab6, in order to return to the
model we had used in RHEL-8.2 and before. The following ticket has been
filed to investigate the problem separately:
<https://bugzilla.redhat.com/show_bug.cgi?id=1844653>.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/OvmfPkgIa32.dsc    | 4 ----
 OvmfPkg/OvmfPkgIa32X64.dsc | 4 ----
 OvmfPkg/OvmfPkgX64.dsc     | 4 ----
 3 files changed, 12 deletions(-)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index e8868136d8..5b1e757cb9 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -379,11 +379,7 @@
   PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
   MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
-!if $(SECURE_BOOT_ENABLE) == TRUE
-  QemuLoadImageLib|OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
-!else
   QemuLoadImageLib|OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
-!endif
 !if $(TPM_ENABLE) == TRUE
   Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibTcg/Tpm12DeviceLibTcg.inf
   Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index d05275a324..5dffc32105 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -383,11 +383,7 @@
   PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
   MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
-!if $(SECURE_BOOT_ENABLE) == TRUE
-  QemuLoadImageLib|OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
-!else
   QemuLoadImageLib|OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
-!endif
 !if $(TPM_ENABLE) == TRUE
   Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibTcg/Tpm12DeviceLibTcg.inf
   Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index cac4cecf18..a2a76fdeea 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -383,11 +383,7 @@
   PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
   MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
-!if $(SECURE_BOOT_ENABLE) == TRUE
-  QemuLoadImageLib|OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
-!else
   QemuLoadImageLib|OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
-!endif
 !if $(TPM_ENABLE) == TRUE
   Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibTcg/Tpm12DeviceLibTcg.inf
   Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
-- 
2.18.1