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