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

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

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...)

Thanks,
Laszlo