[libvirt] [PATCH v1 11/15] qemu_firmware: Introduce qemuFirmwareFillDomain()

[Michal Privoznik]

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 at redhat.com>
>> ---
>>   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).

> 
> (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