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@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;
+
+    if (def->sev &&
+        def->sev->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV &&
+        !supportsSEV)
+        return false;
+
+    return true;
+}
+
+
+static int
+qemuFirmwareEnableFeatures(virQEMUDriverPtr driver,
+                           virDomainDefPtr def,
+                           const qemuFirmware *fw)
+{
+    VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver);
+    const qemuFirmwareMappingFlash *flash = &fw->mapping.data.flash;
+    const qemuFirmwareMappingKernel *kernel = &fw->mapping.data.kernel;
+    const qemuFirmwareMappingMemory *memory = &fw->mapping.data.memory;
+    size_t i;
+
+    switch (fw->mapping.device) {
+    case QEMU_FIRMWARE_DEVICE_FLASH:
+        if (!def->os.loader &&
+            VIR_ALLOC(def->os.loader) < 0)
+            return -1;
+
+        def->os.loader->type = VIR_DOMAIN_LOADER_TYPE_PFLASH;
+        def->os.loader->readonly = VIR_TRISTATE_BOOL_YES;
+
+        if (STRNEQ(flash->executable.format, "raw")) {
+            virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+                           _("unsupported flash format '%s'"),
+                           flash->executable.format);
+            return -1;
+        }
+
+        VIR_FREE(def->os.loader->path);
+        if (VIR_STRDUP(def->os.loader->path,
+                       flash->executable.filename) < 0)
+            return -1;
+
+        if (STRNEQ(flash->nvram_template.format, "raw")) {
+            virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+                           _("unsupported nvram template format '%s'"),
+                           flash->nvram_template.format);
+            return -1;
+        }
+
+        VIR_FREE(def->os.loader->templt);
+        if (VIR_STRDUP(def->os.loader->templt,
+                       flash->nvram_template.filename) < 0)
+            return -1;
+
+        if (qemuDomainNVRAMPathGenerate(cfg, def) < 0)
+            return -1;
+
+        VIR_DEBUG("decided on firmware '%s' varstore '%s'",
+                  def->os.loader->path,
+                  def->os.loader->templt);
+        break;
+
+    case QEMU_FIRMWARE_DEVICE_KERNEL:
+        VIR_FREE(def->os.kernel);
+        if (VIR_STRDUP(def->os.kernel, kernel->filename) < 0)
+            return -1;
+
+        VIR_DEBUG("decided on kernel '%s'",
+                  def->os.kernel);
+        break;
+
+    case QEMU_FIRMWARE_DEVICE_MEMORY:
+        if (!def->os.loader &&
+            VIR_ALLOC(def->os.loader) < 0)
+            return -1;
+
+        def->os.loader->type = VIR_DOMAIN_LOADER_TYPE_ROM;
+        if (VIR_STRDUP(def->os.loader->path, memory->filename) < 0)
+            return -1;
+
+        VIR_DEBUG("decided on loader '%s'",
+                  def->os.loader->path);
+        break;
+
+    case QEMU_FIRMWARE_DEVICE_NONE:
+    case QEMU_FIRMWARE_DEVICE_LAST:
+        break;
+    }
+
+    for (i = 0; i < fw->nfeatures; i++) {
+        switch (fw->features[i]) {
+        case QEMU_FIRMWARE_FEATURE_REQUIRES_SMM:
+            VIR_DEBUG("Enabling SMM feature");
+            def->features[VIR_DOMAIN_FEATURE_SMM] = VIR_TRISTATE_SWITCH_ON;
+            break;
+
+        case QEMU_FIRMWARE_FEATURE_NONE:
+        case QEMU_FIRMWARE_FEATURE_ACPI_S3:
+        case QEMU_FIRMWARE_FEATURE_ACPI_S4:
+        case QEMU_FIRMWARE_FEATURE_AMD_SEV:
+        case QEMU_FIRMWARE_FEATURE_ENROLLED_KEYS:
+        case QEMU_FIRMWARE_FEATURE_SECURE_BOOT:
+        case QEMU_FIRMWARE_FEATURE_VERBOSE_DYNAMIC:
+        case QEMU_FIRMWARE_FEATURE_VERBOSE_STATIC:
+        case QEMU_FIRMWARE_FEATURE_LAST:
+            break;
+        }
+    }
+
+    return 0;
+}
+
+
+int
+qemuFirmwareFillDomain(virQEMUDriverPtr driver,
+                       virDomainObjPtr vm,
+                       unsigned int flags)
+{
+    VIR_AUTOPTR(virString) paths = NULL;
+    size_t npaths = 0;
+    qemuFirmwarePtr *firmwares = NULL;
+    size_t nfirmwares = 0;
+    const qemuFirmware *theone = NULL;
+    size_t i;
+    int ret = -1;
+
+    if (!(flags & VIR_QEMU_PROCESS_START_NEW))
+        return 0;
+
+    if (vm->def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE)
+        return 0;
+
+    if (qemuFirmwareFetchConfigs(&paths) < 0)
+        return -1;
+
+    npaths = virStringListLength((const char **)paths);
+
+    if (VIR_ALLOC_N(firmwares, npaths) < 0)
+        return -1;
+
+    nfirmwares = npaths;
+
+    for (i = 0; i < nfirmwares; i++) {
+        if (!(firmwares[i] = qemuFirmwareParse(paths[i])))
+            goto cleanup;
+    }
+
+    for (i = 0; i < nfirmwares; i++) {
+        if (qemuFirmwareMatchDomain(vm->def, firmwares[i])) {
+            theone = firmwares[i];
+            VIR_DEBUG("Found matching firmware (description path '%s')",
+                      paths[i]);
+            break;
+        }
+    }
+
+    if (!theone) {
+        virReportError(VIR_ERR_OPERATION_FAILED,
+                       _("Unable to find any firmware to satisfy '%s'"),
+                       virDomainOsDefFirmwareTypeToString(vm->def->os.firmware));
+        goto cleanup;
+    }
+
+
+    if (qemuFirmwareEnableFeatures(driver, vm->def, theone) < 0)
+        goto cleanup;
+
+    vm->def->os.firmware = VIR_DOMAIN_OS_DEF_FIRMWARE_NONE;
+
+    ret = 0;
+ cleanup:
+    for (i = 0; i < nfirmwares; i++)
+        qemuFirmwareFree(firmwares[i]);
+    VIR_FREE(firmwares);
+    return ret;
+}
diff --git a/src/qemu/qemu_firmware.h b/src/qemu/qemu_firmware.h
index 321169f56c..5d42b8d172 100644
--- a/src/qemu/qemu_firmware.h
+++ b/src/qemu/qemu_firmware.h
@@ -21,7 +21,9 @@
 #ifndef LIBVIRT_QEMU_FIRMWARE_H
 # define LIBVIRT_QEMU_FIRMWARE_H
 
+# include "domain_conf.h"
 # include "viralloc.h"
+# include "qemu_conf.h"
 
 typedef struct _qemuFirmware qemuFirmware;
 typedef qemuFirmware *qemuFirmwarePtr;
@@ -40,4 +42,9 @@ qemuFirmwareFormat(qemuFirmwarePtr fw);
 int
 qemuFirmwareFetchConfigs(char ***firmwares);
 
+int
+qemuFirmwareFillDomain(virQEMUDriverPtr driver,
+                       virDomainObjPtr vm,
+                       unsigned int flags);
+
 #endif /* LIBVIRT_QEMU_FIRMWARE_H */
-- 
2.19.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

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.

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

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

> +
> +    if (def->sev &&
> +        def->sev->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV &&
> +        !supportsSEV)
> +        return false;
> +
> +    return true;
> +}
> +
> +
> +static int
> +qemuFirmwareEnableFeatures(virQEMUDriverPtr driver,
> +                           virDomainDefPtr def,
> +                           const qemuFirmware *fw)
> +{
> +    VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver);
> +    const qemuFirmwareMappingFlash *flash = &fw->mapping.data.flash;
> +    const qemuFirmwareMappingKernel *kernel = &fw->mapping.data.kernel;
> +    const qemuFirmwareMappingMemory *memory = &fw->mapping.data.memory;
> +    size_t i;
> +
> +    switch (fw->mapping.device) {
> +    case QEMU_FIRMWARE_DEVICE_FLASH:
> +        if (!def->os.loader &&
> +            VIR_ALLOC(def->os.loader) < 0)
> +            return -1;
> +
> +        def->os.loader->type = VIR_DOMAIN_LOADER_TYPE_PFLASH;
> +        def->os.loader->readonly = VIR_TRISTATE_BOOL_YES;
> +
> +        if (STRNEQ(flash->executable.format, "raw")) {
> +            virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> +                           _("unsupported flash format '%s'"),
> +                           flash->executable.format);
> +            return -1;
> +        }
> +
> +        VIR_FREE(def->os.loader->path);
> +        if (VIR_STRDUP(def->os.loader->path,
> +                       flash->executable.filename) < 0)
> +            return -1;
> +
> +        if (STRNEQ(flash->nvram_template.format, "raw")) {
> +            virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> +                           _("unsupported nvram template format '%s'"),
> +                           flash->nvram_template.format);
> +            return -1;
> +        }
> +
> +        VIR_FREE(def->os.loader->templt);
> +        if (VIR_STRDUP(def->os.loader->templt,
> +                       flash->nvram_template.filename) < 0)
> +            return -1;
> +
> +        if (qemuDomainNVRAMPathGenerate(cfg, def) < 0)
> +            return -1;
> +
> +        VIR_DEBUG("decided on firmware '%s' varstore '%s'",

Please log the string "varstore template" instead.

> +                  def->os.loader->path,
> +                  def->os.loader->templt);
> +        break;
> +
> +    case QEMU_FIRMWARE_DEVICE_KERNEL:
> +        VIR_FREE(def->os.kernel);
> +        if (VIR_STRDUP(def->os.kernel, kernel->filename) < 0)
> +            return -1;
> +
> +        VIR_DEBUG("decided on kernel '%s'",
> +                  def->os.kernel);
> +        break;
> +
> +    case QEMU_FIRMWARE_DEVICE_MEMORY:
> +        if (!def->os.loader &&
> +            VIR_ALLOC(def->os.loader) < 0)
> +            return -1;
> +
> +        def->os.loader->type = VIR_DOMAIN_LOADER_TYPE_ROM;
> +        if (VIR_STRDUP(def->os.loader->path, memory->filename) < 0)
> +            return -1;
> +
> +        VIR_DEBUG("decided on loader '%s'",
> +                  def->os.loader->path);
> +        break;
> +
> +    case QEMU_FIRMWARE_DEVICE_NONE:
> +    case QEMU_FIRMWARE_DEVICE_LAST:
> +        break;
> +    }
> +
> +    for (i = 0; i < fw->nfeatures; i++) {
> +        switch (fw->features[i]) {
> +        case QEMU_FIRMWARE_FEATURE_REQUIRES_SMM:
> +            VIR_DEBUG("Enabling SMM feature");
> +            def->features[VIR_DOMAIN_FEATURE_SMM] = VIR_TRISTATE_SWITCH_ON;
> +            break;
> +
> +        case QEMU_FIRMWARE_FEATURE_NONE:
> +        case QEMU_FIRMWARE_FEATURE_ACPI_S3:
> +        case QEMU_FIRMWARE_FEATURE_ACPI_S4:
> +        case QEMU_FIRMWARE_FEATURE_AMD_SEV:
> +        case QEMU_FIRMWARE_FEATURE_ENROLLED_KEYS:
> +        case QEMU_FIRMWARE_FEATURE_SECURE_BOOT:
> +        case QEMU_FIRMWARE_FEATURE_VERBOSE_DYNAMIC:
> +        case QEMU_FIRMWARE_FEATURE_VERBOSE_STATIC:
> +        case QEMU_FIRMWARE_FEATURE_LAST:
> +            break;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +
> +int
> +qemuFirmwareFillDomain(virQEMUDriverPtr driver,
> +                       virDomainObjPtr vm,
> +                       unsigned int flags)
> +{
> +    VIR_AUTOPTR(virString) paths = NULL;
> +    size_t npaths = 0;
> +    qemuFirmwarePtr *firmwares = NULL;
> +    size_t nfirmwares = 0;
> +    const qemuFirmware *theone = NULL;
> +    size_t i;
> +    int ret = -1;
> +
> +    if (!(flags & VIR_QEMU_PROCESS_START_NEW))
> +        return 0;
> +
> +    if (vm->def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE)
> +        return 0;
> +
> +    if (qemuFirmwareFetchConfigs(&paths) < 0)
> +        return -1;
> +
> +    npaths = virStringListLength((const char **)paths);
> +
> +    if (VIR_ALLOC_N(firmwares, npaths) < 0)
> +        return -1;
> +
> +    nfirmwares = npaths;
> +
> +    for (i = 0; i < nfirmwares; i++) {
> +        if (!(firmwares[i] = qemuFirmwareParse(paths[i])))
> +            goto cleanup;
> +    }
> +
> +    for (i = 0; i < nfirmwares; i++) {
> +        if (qemuFirmwareMatchDomain(vm->def, firmwares[i])) {
> +            theone = firmwares[i];
> +            VIR_DEBUG("Found matching firmware (description path '%s')",
> +                      paths[i]);
> +            break;
> +        }
> +    }
> +
> +    if (!theone) {
> +        virReportError(VIR_ERR_OPERATION_FAILED,
> +                       _("Unable to find any firmware to satisfy '%s'"),
> +                       virDomainOsDefFirmwareTypeToString(vm->def->os.firmware));
> +        goto cleanup;
> +    }
> +
> +
> +    if (qemuFirmwareEnableFeatures(driver, vm->def, theone) < 0)
> +        goto cleanup;
> +
> +    vm->def->os.firmware = VIR_DOMAIN_OS_DEF_FIRMWARE_NONE;
> +
> +    ret = 0;
> + cleanup:
> +    for (i = 0; i < nfirmwares; i++)
> +        qemuFirmwareFree(firmwares[i]);
> +    VIR_FREE(firmwares);
> +    return ret;
> +}
> diff --git a/src/qemu/qemu_firmware.h b/src/qemu/qemu_firmware.h
> index 321169f56c..5d42b8d172 100644
> --- a/src/qemu/qemu_firmware.h
> +++ b/src/qemu/qemu_firmware.h
> @@ -21,7 +21,9 @@
>  #ifndef LIBVIRT_QEMU_FIRMWARE_H
>  # define LIBVIRT_QEMU_FIRMWARE_H
>  
> +# include "domain_conf.h"
>  # include "viralloc.h"
> +# include "qemu_conf.h"
>  
>  typedef struct _qemuFirmware qemuFirmware;
>  typedef qemuFirmware *qemuFirmwarePtr;
> @@ -40,4 +42,9 @@ qemuFirmwareFormat(qemuFirmwarePtr fw);
>  int
>  qemuFirmwareFetchConfigs(char ***firmwares);
>  
> +int
> +qemuFirmwareFillDomain(virQEMUDriverPtr driver,
> +                       virDomainObjPtr vm,
> +                       unsigned int flags);
> +
>  #endif /* LIBVIRT_QEMU_FIRMWARE_H */
> 

Thanks
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list