2:40 p.m.
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(a)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