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

[Laszlo Ersek]

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

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