[PATCH] Revert "OvmfPkg: use generic QEMU image loader for secure boot enabled builds"

[PATCH] Revert "OvmfPkg: use generic QEMU image loader for secure boot enabled builds"

[Laszlo Ersek]

This reverts commit ced77332cab626f35fbdb36630be27303d289d79.

The command

  virt-install --location NETWORK-URL

downloads the vmlinuz and initrd files from the remote OS tree, and passes
them to the guest firmware via fw_cfg.

When used with IA32 / X64 guests, virt-install expects the guest firmware
to do two things, at the same time:

- launch the fw_cfg kernel image even if the latter does not pass SB
  verification (SB checking is supposed to be bypassed entirely in favor
  of the Linux/x86 Boot Protocol),

- still let the guest kernel perceive SB as enabled.

Commit ced77332cab6 prevented this, by removing the Linux/x86 Boot
Protocol from such an OVMF image that was built with SECURE_BOOT_ENALBE.
While that's the right thing in theory, in practice "virt-install
--location NETWORK-URL" is entrenched, and we shouldn't break it.

We can tolerate the Linux/x86 Boot Protocol as a one-of-a-kind SB bypass
for direct-booted kernels, because:

- the fw_cfg content comes from QEMU, and the guest is already at QEMU's
  mercy,

- in the guest, OS boots after the initial installation will use "shim"
  rather than an fw_cfg kernel, which we can consider somewhat similar to
  "Audit Mode / Deployed Mode" (~ trust for install, lock down after).

Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---

Notes:
    - pick up Ard's ACK from
    
      http://mid.mail-archive.com/c06ee730-e421-0aa5-882f-bc09ae9c546f@arm.com
      https://edk2.groups.io/g/devel/message/61169
    
    - posting to the list to enable feedback on the commit message (I intend
      to push the patch in one or two days)
    
    - repo:   https://pagure.io/lersek/edk2.git
      branch: reenable_fwcfg_x86_boot_proto

 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 d0df9cbbfb2b..16103d177374 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -379,11 +379,7 @@ [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 $(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 b3ae62fee92b..9597ef6721da 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -383,11 +383,7 @@ [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 $(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 f7fe75ebf531..a6e585c03d41 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -383,11 +383,7 @@ [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 $(TPM_ENABLE) == TRUE
   Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibTcg/Tpm12DeviceLibTcg.inf
   Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
-- 
2.19.1.3.g30247aa5d201

Re: [PATCH] Revert "OvmfPkg: use generic QEMU image loader for secure boot enabled builds"

[Philippe Mathieu-Daudé]

On 6/15/20 4:45 PM, Laszlo Ersek wrote:
> This reverts commit ced77332cab626f35fbdb36630be27303d289d79.
> 
> The command
> 
>   virt-install --location NETWORK-URL
> 
> downloads the vmlinuz and initrd files from the remote OS tree, and passes
> them to the guest firmware via fw_cfg.
> 
> When used with IA32 / X64 guests, virt-install expects the guest firmware
> to do two things, at the same time:
> 
> - launch the fw_cfg kernel image even if the latter does not pass SB
>   verification (SB checking is supposed to be bypassed entirely in favor
>   of the Linux/x86 Boot Protocol),
> 
> - still let the guest kernel perceive SB as enabled.
> 
> Commit ced77332cab6 prevented this, by removing the Linux/x86 Boot
> Protocol from such an OVMF image that was built with SECURE_BOOT_ENALBE.
> While that's the right thing in theory, in practice "virt-install
> --location NETWORK-URL" is entrenched, and we shouldn't break it.
> 
> We can tolerate the Linux/x86 Boot Protocol as a one-of-a-kind SB bypass
> for direct-booted kernels, because:
> 
> - the fw_cfg content comes from QEMU, and the guest is already at QEMU's
>   mercy,
> 
> - in the guest, OS boots after the initial installation will use "shim"
>   rather than an fw_cfg kernel, which we can consider somewhat similar to
>   "Audit Mode / Deployed Mode" (~ trust for install, lock down after).
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
> 
> Notes:
>     - pick up Ard's ACK from
>     
>       http://mid.mail-archive.com/c06ee730-e421-0aa5-882f-bc09ae9c546f@arm.com
>       https://edk2.groups.io/g/devel/message/61169
>     
>     - posting to the list to enable feedback on the commit message (I intend
>       to push the patch in one or two days)
>     
>     - repo:   https://pagure.io/lersek/edk2.git
>       branch: reenable_fwcfg_x86_boot_proto
> 
>  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 d0df9cbbfb2b..16103d177374 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -379,11 +379,7 @@ [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 $(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 b3ae62fee92b..9597ef6721da 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -383,11 +383,7 @@ [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 $(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 f7fe75ebf531..a6e585c03d41 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -383,11 +383,7 @@ [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 $(TPM_ENABLE) == TRUE
>    Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibTcg/Tpm12DeviceLibTcg.inf
>    Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
> 

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

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

[Laszlo Ersek]

On 06/15/20 16:49, Philippe Mathieu-Daudé wrote:
> On 6/15/20 4:45 PM, Laszlo Ersek wrote:
>> This reverts commit ced77332cab626f35fbdb36630be27303d289d79.
>>
>> The command
>>
>>   virt-install --location NETWORK-URL
>>
>> downloads the vmlinuz and initrd files from the remote OS tree, and passes
>> them to the guest firmware via fw_cfg.
>>
>> When used with IA32 / X64 guests, virt-install expects the guest firmware
>> to do two things, at the same time:
>>
>> - launch the fw_cfg kernel image even if the latter does not pass SB
>>   verification (SB checking is supposed to be bypassed entirely in favor
>>   of the Linux/x86 Boot Protocol),
>>
>> - still let the guest kernel perceive SB as enabled.
>>
>> Commit ced77332cab6 prevented this, by removing the Linux/x86 Boot
>> Protocol from such an OVMF image that was built with SECURE_BOOT_ENALBE.
>> While that's the right thing in theory, in practice "virt-install
>> --location NETWORK-URL" is entrenched, and we shouldn't break it.
>>
>> We can tolerate the Linux/x86 Boot Protocol as a one-of-a-kind SB bypass
>> for direct-booted kernels, because:
>>
>> - the fw_cfg content comes from QEMU, and the guest is already at QEMU's
>>   mercy,
>>
>> - in the guest, OS boots after the initial installation will use "shim"
>>   rather than an fw_cfg kernel, which we can consider somewhat similar to
>>   "Audit Mode / Deployed Mode" (~ trust for install, lock down after).
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> ---
>>
>> Notes:
>>     - pick up Ard's ACK from
>>     
>>       http://mid.mail-archive.com/c06ee730-e421-0aa5-882f-bc09ae9c546f@arm.com
>>       https://edk2.groups.io/g/devel/message/61169
>>     
>>     - posting to the list to enable feedback on the commit message (I intend
>>       to push the patch in one or two days)
>>     
>>     - repo:   https://pagure.io/lersek/edk2.git
>>       branch: reenable_fwcfg_x86_boot_proto
>>
>>  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 d0df9cbbfb2b..16103d177374 100644
>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>> @@ -379,11 +379,7 @@ [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 $(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 b3ae62fee92b..9597ef6721da 100644
>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>> @@ -383,11 +383,7 @@ [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 $(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 f7fe75ebf531..a6e585c03d41 100644
>> --- a/OvmfPkg/OvmfPkgX64.dsc
>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>> @@ -383,11 +383,7 @@ [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 $(TPM_ENABLE) == TRUE
>>    Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibTcg/Tpm12DeviceLibTcg.inf
>>    Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
>>
> 
> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

Thanks!

Merged as commit 82808b422617 ('Revert "OvmfPkg: use generic QEMU image
loader for secure boot enabled ..."', 2020-06-16) via
<https://github.com/tianocore/edk2/pull/703>, after truncating the
subject line (originally auto-generated by git-revert), to pacify
PatchCheck.py.

Laszlo