On 03/05/19 15:38, Michal Privoznik wrote:
> On 2/28/19 12:15 PM, Laszlo Ersek wrote:
>> On 02/27/19 11:04, Michal Privoznik wrote:
>>> And finally the last missing piece. This is what puts it all
>>> together.
>>>
>>> At the beginning, qemuFirmwareFillDomain() loads all possible
>>> firmware description files based on algorithm described earlier.
>>> Then it tries to find description which matches given domain.
>>> The criteria are:
>>>
>>> - firmware is the right type (e.g. it's bios when bios was
>>> requested in domain XML)
>>> - firmware is suitable for guest architecture/machine type
>>> - firmware allows desired guest features to stay enabled (e.g.
>>> if s3/s4 is enabled for guest then firmware has to support
>>> it too)
>>>
>>> Once the desired description has been found it is then used to
>>> set various bits of virDomainDef so that proper qemu cmd line is
>>> constructed as demanded by the description file. For instance,
>>> secure boot enabled firmware might request SMM -> it will be
>>> enabled if needed.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
>>> ---
>>> src/qemu/qemu_firmware.c | 257 +++++++++++++++++++++++++++++++++++++++
>>> src/qemu/qemu_firmware.h | 7 ++
>>> 2 files changed, 264 insertions(+)
>>>
>>> diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c
>>> index 509927b154..90c611db2d 100644
>>> --- a/src/qemu/qemu_firmware.c
>>> +++ b/src/qemu/qemu_firmware.c
>>> @@ -23,6 +23,8 @@
>>> #include "qemu_firmware.h"
>>> #include "configmake.h"
>>> #include "qemu_capabilities.h"
>>> +#include "qemu_domain.h"
>>> +#include "qemu_process.h"
>>> #include "virarch.h"
>>> #include "virfile.h"
>>> #include "virhash.h"
>>> @@ -1026,3 +1028,258 @@ qemuFirmwareFetchConfigs(char ***firmwares)
>>> return 0;
>>> }
>>> +
>>> +
>>> +static bool
>>> +qemuFirmwareMatchDomain(const virDomainDef *def,
>>> + const qemuFirmware *fw)
>>> +{
>>> + size_t i;
>>> + bool supportsS3 = false;
>>> + bool supportsS4 = false;
>>> + bool supportsSecureBoot = false;
>>> + bool supportsSEV = false;
>>> +
>>> + for (i = 0; i < fw->ninterfaces; i++) {
>>> + if ((def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS &&
>>> + fw->interfaces[i] == QEMU_FIRMWARE_OS_INTERFACE_BIOS) ||
>>> + (def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_EFI &&
>>> + fw->interfaces[i] == QEMU_FIRMWARE_OS_INTERFACE_UEFI))
>>> + break;
>>> + }
>>> +
>>> + if (i == fw->ninterfaces)
>>> + return false;
>>> +
>>> + for (i = 0; i < fw->ntargets; i++) {
>>> + size_t j;
>>> +
>>> + if (def->os.arch != fw->targets[i]->architecture)
>>> + continue;
>>> +
>>> + for (j = 0; j < fw->targets[i]->nmachines; j++) {
>>> + if (virStringMatch(def->os.machine,
>>> fw->targets[i]->machines[j]))
>>> + break;
>>> + }
>>> +
>>> + if (j == fw->targets[i]->nmachines)
>>> + continue;
>>> +
>>> + break;
>>> + }
>>> +
>>> + if (i == fw->ntargets)
>>> + return false;
>>> +
>>> + for (i = 0; i < fw->nfeatures; i++) {
>>> + switch (fw->features[i]) {
>>> + case QEMU_FIRMWARE_FEATURE_ACPI_S3:
>>> + supportsS3 = true;
>>> + break;
>>> + case QEMU_FIRMWARE_FEATURE_ACPI_S4:
>>> + supportsS4 = true;
>>> + break;
>>> + case QEMU_FIRMWARE_FEATURE_SECURE_BOOT:
>>> + supportsSecureBoot = true;
>>> + break;
>>> + case QEMU_FIRMWARE_FEATURE_AMD_SEV:
>>> + supportsSEV = true;
>>> + break;
>>> + case QEMU_FIRMWARE_FEATURE_ENROLLED_KEYS:
>>> + case QEMU_FIRMWARE_FEATURE_REQUIRES_SMM:
>>> + case QEMU_FIRMWARE_FEATURE_VERBOSE_DYNAMIC:
>>> + case QEMU_FIRMWARE_FEATURE_VERBOSE_STATIC:
>>> + case QEMU_FIRMWARE_FEATURE_NONE:
>>> + case QEMU_FIRMWARE_FEATURE_LAST:
>>> + break;
>>> + }
>>> + }
>>> +
>>> + if (def->pm.s3 == VIR_TRISTATE_BOOL_YES &&
>>> + !supportsS3)
>>> + return false;
>>> +
>>> + if (def->pm.s4 == VIR_TRISTATE_BOOL_YES &&
>>> + !supportsS4)
>>> + return false;
>>> +
>>> + if (def->os.loader &&
>>> + def->os.loader->secure == VIR_TRISTATE_BOOL_YES &&
>>> + !supportsSecureBoot)
>>> + return false;
>>
>> This check doesn't seem correct. (Also the fact that
>> QEMU_FIRMWARE_FEATURE_REQUIRES_SMM is ignored above.)
>
> [This is exactly why I wanted you to take a look at these patches,
> because I was almost certain I would get this wrong. Thanks!]
>
>>
>> The @secure attribute controls whether libvirtd generates the "-global
>> driver=cfi.pflash01,property=secure,value=on" cmdline option. See
>> qemuBuildDomainLoaderCommandLine(). That means that read/write accesses
>> ("programming mode") to the pflash chips will be restricted to guest
>> code that runs in (guest) SMM.
>>
>> In other words, @secure is connected to REQUIRES_SMM, not SECURE_BOOT.
>>
>> Technically speaking, SECURE_BOOT is both independent of REQUIRES_SMM,
>> and irrelevant in itself for the QEMU command line. SECURE_BOOT is only
>> relevant to what firmware interfaces are exposed within the guest.
>>
>> Now, security-wise, there *is* a connection between SECURE_BOOT and
>> REQUIRES_SMM. Namely, it is bad practice (for production uses) for
>> firmware to offer SECURE_BOOT without also specifying REQUIRES_SMM. See
>> the explanation in the schema JSON.
>>
>> So basically, here's what I suggest:
>>
>> (1) if REQUIRES_SMM is absent from the firmware descriptor, then @secure
>> must be "no" in the domain config. Equivalently, if @secure is "yes",
>> then the firmware descriptor must come with REQUIRES_SMM.
>
> Ah okay. So @secure should be tied to REQUIRES_SMM. But that leaves
> SECURE_BOOT unchecked (i.e. unused for finding matching FW description).
That's right.
As an alternative, you could change the code so that @secure=='yes' be
satisfied by (REQUIRES_SMM && SECURE_BOOT) only. Likely much simpler for
the end-user. (Perhaps a bit restrictive for my taste.) I think this
mapping/interpretation should be decided by libvirt owners and higher
level mgmt app owners.
Thanks
Laszlo
>
>>
>> (2) if REQUIRES_SMM is present (regardless of SECURE_BOOT) in the
>> firmware descriptor, then <smm state='on'/> is required under
>> <features>, in the domain config.
>
> This is already done in qemuFirmwareEnableFeatures() (towards the end).
>
>>
>> (3) if SECURE_BOOT is present, but REQUIRES_SMM is absent, in the
>> firmware descriptor, log a big fat warning somewhere, but don't prevent
>> firmware selection or domain startup. There may be valid use cases for
>> this, so we shouldn't block that. No need to log the warning just upon
>> seeing such a firmware descriptor, but do log the warning if the
>> firmware is ultimately selected for a domain.
>>
>> (4) if REQUIRES_SMM is present, but SECURE_BOOT is absent, and the
>> firmware is ultimately selected, log another warning. This is a totally
>> valid (and safe) firmware build, but not overly useful to end-users, so
>> it may not give users what they want.
>
> Well, these can be merged into one warning like:
>
> REQUIRES_SMM and SECURE_BOOT flags mismatch in $filename
>
>
> Also, I'll have to come up with yet another FW description for my tests
> that doesn't have REQUIRES_SMM nor SECURE_BOOT to test:
>
> <os firmware='efi'>
> <type arch='x86_64' machine='pc-q35-4.0'>hvm</type>
> <loader secure='no'/>
> </os>
>
> But that should be trivial.
>
> Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
