From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web12.3718.1591696291772476183 for ; Tue, 09 Jun 2020 02:51:31 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=R1cd1uhH; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1591696290; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=OMeWutcPWAVL+HcqmjOpxh1vhxa0oRer+f/VvuFkQRo=; b=R1cd1uhHywoHshm3FvVU2lvAryxXar4m1+JyH8Q68V0S4NwODye73VGS0UJ3YIxdQWC3hs ViBw6CKD8rm6y2SzGS0Ey1fXrXltFHlw72jGZUMpoTYLjXJK8fv/HxuWVD0HiDoRXIXdwa DWgQmv+iH6Hfeq4bxCkcj/25l1Ag2DU= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-262-HkCqjhvGO6KHm9a8PVXg_g-1; Tue, 09 Jun 2020 05:51:18 -0400 X-MC-Unique: HkCqjhvGO6KHm9a8PVXg_g-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 1BE0D461; Tue, 9 Jun 2020 09:51:17 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-127.ams2.redhat.com [10.36.113.127]) by smtp.corp.redhat.com (Postfix) with ESMTP id 24DE48203E; Tue, 9 Jun 2020 09:51:15 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v3 14/14] OvmfPkg: use generic QEMU image loader for secure boot enabled builds To: ard.biesheuvel@linaro.org References: <20200305134607.20125-1-ard.biesheuvel@linaro.org> <20200305134607.20125-15-ard.biesheuvel@linaro.org> From: "Laszlo Ersek" Cc: devel@edk2.groups.io, =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= Message-ID: <5243a882-187f-6ca4-2450-a6c958a934e3@redhat.com> Date: Tue, 9 Jun 2020 11:51:15 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200305134607.20125-15-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Hi Ard, On 03/05/20 14:46, 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 > Reviewed-by: Laszlo Ersek > --- > 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 ec21d2f3f6cb..8916255df4df 100644 > --- a/OvmfPkg/OvmfPkgIa32.dsc > +++ b/OvmfPkg/OvmfPkgIa32.dsc > @@ -363,7 +363,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 $(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 3c80a18c6086..342ff96cc279 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.dsc > +++ b/OvmfPkg/OvmfPkgIa32X64.dsc > @@ -367,7 +367,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 $(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 63f1f935f4f3..1fb2de5e0121 100644 > --- a/OvmfPkg/OvmfPkgX64.dsc > +++ b/OvmfPkg/OvmfPkgX64.dsc > @@ -367,7 +367,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 $(TPM_ENABLE) == TRUE > Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibTcg/Tpm12DeviceLibTcg.inf > Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf > this patch (commit ced77332cab6) breaks the following use case (on X64): (1) we're defining (installing) a new libvirt domain with virt-install --location NETWORK-URL where NETWORK-URL identifies a remote (e.g. https) "exploded" Linux distro tree, (2) in the variable store template file, from which the new domain will have its variable store instantiated, the Microsoft certs had been enrolled (by running EnrollDefaultKeys), (3) the guest kernel ("vmlinuz") downloaded in step (1) is only signed by the distro vendor, not by Microsoft. When installing a distro with "virt-install" from a UEFI-bootable ISO image (exposed to the guest e.g. via virtio-scsi), that is, not from a remote "exploded" OS tree like above, then "shim" is booted first. Shim is accepted by an MS certificate. Then shim/grub boot the guest kernel, and thanks to shim, the vendor signature on that kernel is accepted. This is why "shim" exists. However, when installing the guest with "virt-install --location NETWORK-URL", then virt-install first downloads the broken-out vmlinuz and initrd files from the remote (e.g. https) OS tree, to local temporary files, and launches the guest (for installation) with a direct (fw_cfg) kernel boot. Because "vmlinuz" is only signed by the OS vendor, and "shim" is not used at all, and the varstore only has the MS certs, the guest (installer) now fails to boot. This used to work before, and there's an argument to be made that it wasn't insecure. Because -- as I understand it anyway --, the usage is not unlike "audit mode / deployed mode": - When you boot the guest for the very first time, you make the guest firmware blindly trust the guest kernel, in effect -- because, you are providing the kernel from a source whose trustworthiness is guaranteed by other means (for example, you pass such an https NETWORK-URL to virt-install on the host side that you trust). - Once the guest is installed, and either rebooted from within, or shut down altogether and relaunched, then the fw-cfg "bypass" won't happen again. It's also not needed any more, because with the installation completed, "shim" has also become part of the boot process (from the virtual disk). Do you have any ideas to make this scenario work again? Or else, can we predicate this patch for the DSC files on a build flag that's different from SECURE_BOOT_ENABLE? (Put more formally: right now, the EFI_SECURITY_VIOLATION branch, and the EFI_ACCESS_DENIED branch that I posted for TianoCore#2785 earlier, in X86QemuLoadImageLib, are dead code. Because, those conditions are only possible when SECURE_BOOT_ENABLE is defined -- but in that case, we (currently) don't use X86QemuLoadImageLib at all.) I do see that the *exact* virt-install scenario above has never worked in ArmVirtQemu / AARCH64. (Given that ArmVirtQemu has always used gBS->LoadImage() for fw_cfg kernel boot.) However, to us (RH) anyway, that difference between AARCH64 and X64 isn't a practical one. That's because we have never set SECURE_BOOT_ENABLE for ArmVirtQemu in the first place, due to not having an SMM-like pflash protection on AARCH64. ... Unfortunately, when I reviewed this patch, I failed to recall that "virt-install --location NETWORK-URL" relied on fw_cfg kernel boot. And I failed to realize that, by extension, virt-install relied on fw_cfg kernel boot bypassing Secure Boot. :( Thanks, Laszlo