* [PATCH] Revert "OvmfPkg: use generic QEMU image loader for secure boot enabled builds"
@ 2020-06-15 14:45 Laszlo Ersek
2020-06-15 14:49 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 3+ messages in thread
From: Laszlo Ersek @ 2020-06-15 14:45 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Ard Biesheuvel, Jordan Justen, Philippe Mathieu-Daudé
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] Revert "OvmfPkg: use generic QEMU image loader for secure boot enabled builds"
2020-06-15 14:45 [PATCH] Revert "OvmfPkg: use generic QEMU image loader for secure boot enabled builds" Laszlo Ersek
@ 2020-06-15 14:49 ` Philippe Mathieu-Daudé
2020-06-16 20:40 ` [edk2-devel] " Laszlo Ersek
0 siblings, 1 reply; 3+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-15 14:49 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-groups-io; +Cc: Ard Biesheuvel, Jordan Justen
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>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [edk2-devel] [PATCH] Revert "OvmfPkg: use generic QEMU image loader for secure boot enabled builds"
2020-06-15 14:49 ` Philippe Mathieu-Daudé
@ 2020-06-16 20:40 ` Laszlo Ersek
0 siblings, 0 replies; 3+ messages in thread
From: Laszlo Ersek @ 2020-06-16 20:40 UTC (permalink / raw)
To: devel, philmd; +Cc: Ard Biesheuvel, Jordan Justen
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-06-16 20:40 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-06-15 14:45 [PATCH] Revert "OvmfPkg: use generic QEMU image loader for secure boot enabled builds" Laszlo Ersek
2020-06-15 14:49 ` Philippe Mathieu-Daudé
2020-06-16 20:40 ` [edk2-devel] " Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox