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

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

On 06/11/20 17:05, Ard Biesheuvel wrote:
> On 6/11/20 4:55 PM, Laszlo Ersek wrote:
>> Hi Ard,
>>
>> On 06/10/20 11:32, Ard Biesheuvel wrote:
>>> On 6/10/20 11:22 AM, Laszlo Ersek wrote:
>>
>>>> (3) make it a dynamic boolean PCD, and use the recently introduced
>>>> fw_cfg control interface
>>>>
>>>>
>>>> In all three options, the OVMF DSC files would unconditionally use the
>>>> X86QemuLoadImageLib instance, we'd just predicate the fallback to the
>>>> legacy Linux/x86 boot protocol on:
>>>>
>>>>    SecureBoot disabled || FallbackPcd
>>>>
>>>> (where "SecureBoot disabled" means that the SecureBoot standard UEFI
>>>> variable doesn't exist, or has zero value).
>>>>
>>>> I think option (2) means the least churn, and it would be "good enough"
>>>> functionally.
>>>>
>>>> Does this look palatable to you?
>>>>
>>>>
>>>>> So what is the point of DEFER_EXECUTE_ON_SECURITY_VIOLATION anyway, if
>>>>> you cannot start the image anyway?
>>>>
>>>> Honestly: I'm not sure!
>>>>
>>>>> Because I would prefer it if we could
>>>>> start the image anyway, Unfortunately, the only way seems to be
>>>>
>>>> Right; it seems like the EFI_SECURITY_VIOLATION status would get saved
>>>> in the private LoadImageStatus field, and that would cause StartImage()
>>>> to fail early as well. I couldn't find a spot to "re-set"
>>>> LoadImageStatus.
>>>>
>>>>> to use
>>>>> the loaded image protocol data (base and size in memory) and do
>>>>> another
>>>>> LoadImage() after adding the hash to "db", and this is also more
>>>>> elaborate than I would like.
>>>>
>>>> Agreed. We shouldn't mess with "db".
>>>>
>>>>> Of course, the *correct* fix is to add the vendor cert to db - you are
>>>>> loading a signed image from a known source, and this whole dance with
>>>>> shim and the MS certs is ridiculous, and I am still hoping we can
>>>>> avoid
>>>>> this mess on ARM
>>>>
>>>> I considered this. It didn't look feasible to me at first sight (how
>>>> many vendors?), but now that you mention it and I've looked at it more
>>>> closely, it does look feasible. Selfishly (!), if I look at the
>>>> guest OS
>>>> types officially supported on RHEL8 hosts:
>>>>
>>>> - https://access.redhat.com/articles/973133
>>>> - https://access.redhat.com/articles/973163
>>>>
>>>> then all we (RH) would need are two patches for (upstream)
>>>> OvmfPkg/EnrollDefaultKeys. Namely, we should enroll two more DB
>>>> entries:
>>>>
>>>> - SUSE's CA certificate to which the key(s) chain that they use for
>>>> signing their kernels
>>>>
>>>> - Red Hat's CA certificate to which the key(s) chain that we use for
>>>> signing our kernels.
>>>>
>>>> That would strictly be an improvement even for upstream OVMF (more
>>>> vendors accepted).
>>>>
>>>> Ultimately I think the right way is to pick option (3) -- dynamic
>>>> PCD --
>>>> *and* add the SUSE and RH CA certs to OvmfPkg/EnrollDefaultKeys. That
>>>> means SUSE and RH signed kernels can be booted at once, with secure
>>>> verification, and users would still have a QEMU command line option for
>>>> opting *into* the legacy boot protocol fallback.
>>>>
>>>> Do you agree? Would you be OK with patches to this effect?
>>>>
>>>
>>> Yes, option #3 looks reasonable to me. And adding the certs is the right
>>> thing to do regardless - going around installing the MS certs everywhere
>>> is a recipe for disaster, and AIUI, MS now requires you to remove them
>>> in some cases?? (for DeviceGuard support or sth like that)
>>
>> I've "escalated" this internally to other virt team engineers, and they
>> seem to consider this a plain regression. They suggested we should
>> revert the patch, because:
>>
>> - the problem will affect every guest distro different from SUSE and RH
>> out there, and every host distro providing KVM hypervisor,
>>
>> - the change isn't fixing a practical security issue.
>>
>> (I'm quite stressed about bringing this internal feedback to you,
>> admittedly...)
>>
> 
> Don't worry about it.
> 
> If we can ensure that the only bootable images that are exempt from the
> secure boot checks are ones that were provided directly by the host
> userspace, then I think their position is not unreasonable, given that
> the guest is at its mercy anyway.
> 

Thanks.

So... reverting this patch would only affect the behavior of the
QemuLoadKernelImage() API, and that API only consumes fw_cfg. Fw_cfg is
under the control of the host userspace (QEMU implements the fw_cfg
"platform hardware). Does that satisfy your condition ("provided
directly by the host userspace"), or are you referring to any "farther"
origin on the host side, from where the fw_cfg content is originally taken?

IOW would you involve in this decision the question where on the network
the kernel image is downloaded from (on the host), for example? (I
wouldn't -- for me, the fact that fw_cfg is technically controlled by
QEMU is enough.)

FWIW I've reverted the patch downstream (a deadline forced my hand
before we could conclude this upstream thread, and the use cases that
had regressed are considered important), but I really dislike that
divergence from upstream. I'd like to eliminate that downstream patch
when we rebase to a subsequent stable tag.

Thanks,
Laszlo