Re: [edk2-devel] [PATCH 13/13] OvmfPkg: use generic QEMU image loader for secure boot enabled builds

["Laszlo Ersek" <lersek@redhat.com>]

On 03/02/20 08:29, Ard Biesheuvel wrote:
> The QemuLoadImageLib implementation we currently use for all OVMF
> builds copies the behavior of the QEMU loader code that precedes it,
> which is to disregard UEFI secure boot policies entirely when it comes
> to loading kernel images that have been specified on the QEMU command
> line. This behavior deviates from ArmVirtQemu based builds, which do
> take UEFI secure boot policies into account, and refuse to load images
> from the command line that cannot be authenticated.
> 
> The disparity was originally due to the fact that the QEMU command line
> kernel loader did not use LoadImage and StartImage at all, but this
> changed recently, and now, there are only a couple of reasons left to
> stick with the legacy loader:
> - it permits loading images that lack a valid PE/COFF header,
> - it permits loading X64 kernels on IA32 firmware running on a X64
>   capable system.
> 
> Since every non-authentic PE/COFF image can trivially be converted into
> an image that lacks a valid PE/COFF header, the former case can simply
> not be supported in a UEFI secure boot context. The latter case is highly
> theoretical, given that one could easily switch to native X64 firmware in
> a VM scenario.
> 
> That leaves us with little justification to use the legacy loader at all
> when UEFI secure boot policies are in effect, so let's switch to the
> generic loader for UEFI secure boot enabled builds.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2566
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  OvmfPkg/OvmfPkgIa32.dsc    | 4 ++++
>  OvmfPkg/OvmfPkgIa32X64.dsc | 4 ++++
>  OvmfPkg/OvmfPkgX64.dsc     | 4 ++++
>  3 files changed, 12 insertions(+)
> 
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 2cc924a6986a..eceddb71948f 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -361,7 +361,11 @@ [LibraryClasses.common.DXE_DRIVER]
>    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 $(TPM2_ENABLE) == TRUE
>    Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
>  !endif
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 21d1f156973b..8bdf2e692b00 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -365,7 +365,11 @@ [LibraryClasses.common.DXE_DRIVER]
>    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 $(TPM2_ENABLE) == TRUE
>    Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
>  !endif
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index f3d0f18db7e2..bc0a3e438d2a 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -365,7 +365,11 @@ [LibraryClasses.common.DXE_DRIVER]
>    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 $(TPM2_ENABLE) == TRUE
>    Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
>  !endif
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thank you!
Laszlo