conf: Validate firmware configuration more thoroughly

Generally speaking, when firmware autoselection is in use we
don't want any information to be provided manually. There are
two exceptions:

  * we still want the path to the NVRAM file to be customizable;

  * using <loader secure='yes'/> was how you would ask for a
    firmware that implements the Secure Boot feature in the
    original approach to firmware autoselection, so we want to
    keep that working.

Anything else should result in a descriptive error.

Resolves: https://gitlab.com/libvirt/libvirt/-/issues/327
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index a67c46255c42ae28e4dc2bf9b2ae3a998781cdc9..355668b04293941b3d2a1a1390add952c310017d 100644 (file)
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -1606,6 +1606,54 @@ virDomainDefOSValidate(const virDomainDef *def,
                            _("firmware auto selection not implemented for this driver"));
             return -1;
         }
+
+        if (!loader)
+            return 0;
+
+        if (loader->readonly) {
+            virReportError(VIR_ERR_XML_DETAIL, "%s",
+                           _("loader attribute 'readonly' cannot be specified "
+                             "when firmware autoselection is enabled"));
+            return -1;
+        }
+        if (loader->type) {
+            virReportError(VIR_ERR_XML_DETAIL, "%s",
+                           _("loader attribute 'type' cannot be specified "
+                             "when firmware autoselection is enabled"));
+            return -1;
+        }
+        if (loader->path) {
+            virReportError(VIR_ERR_XML_DETAIL, "%s",
+                           _("loader path cannot be specified "
+                             "when firmware autoselection is enabled"));
+            return -1;
+        }
+        if (loader->nvramTemplate) {
+            virReportError(VIR_ERR_XML_DETAIL, "%s",
+                           _("nvram attribute 'template' cannot be specified "
+                             "when firmware autoselection is enabled"));
+            return -1;
+        }
+
+        /* We need to accept 'yes' here because the initial implementation
+         * of firmware autoselection used it as a way to request a firmware
+         * with Secure Boot support, so the error message is technically
+         * incorrect; however, we want to discourage people from using this
+         * attribute at all, so it's fine to be a bit more aggressive than
+         * it would be strictly required :) */
+        if (loader->secure == VIR_TRISTATE_BOOL_NO) {
+            virReportError(VIR_ERR_XML_DETAIL, "%s",
+                           _("loader attribute 'secure' cannot be specified "
+                             "when firmware autoselection is enabled"));
+            return -1;
+        }
+
+        if (loader->nvram && def->os.firmware != VIR_DOMAIN_OS_DEF_FIRMWARE_EFI) {
+            virReportError(VIR_ERR_XML_DETAIL,
+                           _("firmware type '%s' does not support nvram"),
+                           virDomainOsDefFirmwareTypeToString(def->os.firmware));
+            return -1;
+        }
     } else {
         if (!loader)
             return 0;

diff --git a/tests/qemuxml2argvdata/firmware-auto-bios-nvram.x86_64-latest.err b/tests/qemuxml2argvdata/firmware-auto-bios-nvram.x86_64-latest.err
new file mode 100644 (file)
index 0000000..772beb4
--- /dev/null
+++ b/tests/qemuxml2argvdata/firmware-auto-bios-nvram.x86_64-latest.err
@@ -0,0 +1 @@
+firmware type 'bios' does not support nvram

diff --git a/tests/qemuxml2argvdata/firmware-auto-bios-nvram.xml b/tests/qemuxml2argvdata/firmware-auto-bios-nvram.xml
new file mode 100644 (file)
index 0000000..6dad1e1
--- /dev/null
+++ b/tests/qemuxml2argvdata/firmware-auto-bios-nvram.xml
@@ -0,0 +1,18 @@
+<domain type='kvm'>
+  <name>fedora</name>
+  <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid>
+  <memory unit='KiB'>8192</memory>
+  <vcpu placement='static'>1</vcpu>
+  <os firmware='bios'>
+    <type arch='x86_64' machine='pc-q35-4.0'>hvm</type>
+    <nvram>/path/to/fedora_VARS.fd</nvram>
+  </os>
+  <features>
+    <acpi/>
+  </features>
+  <devices>
+    <emulator>/usr/bin/qemu-system-x86_64</emulator>
+    <controller type='usb' model='none'/>
+    <memballoon model='none'/>
+  </devices>
+</domain>

diff --git a/tests/qemuxml2argvdata/firmware-auto-efi-loader-insecure.x86_64-latest.err b/tests/qemuxml2argvdata/firmware-auto-efi-loader-insecure.x86_64-latest.err
new file mode 100644 (file)
index 0000000..564f0e6
--- /dev/null
+++ b/tests/qemuxml2argvdata/firmware-auto-efi-loader-insecure.x86_64-latest.err
@@ -0,0 +1 @@
+loader attribute 'secure' cannot be specified when firmware autoselection is enabled

diff --git a/tests/qemuxml2argvdata/firmware-auto-efi-loader-insecure.xml b/tests/qemuxml2argvdata/firmware-auto-efi-loader-insecure.xml
new file mode 100644 (file)
index 0000000..33bd7b0
--- /dev/null
+++ b/tests/qemuxml2argvdata/firmware-auto-efi-loader-insecure.xml
@@ -0,0 +1,18 @@
+<domain type='kvm'>
+  <name>fedora</name>
+  <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid>
+  <memory unit='KiB'>8192</memory>
+  <vcpu placement='static'>1</vcpu>
+  <os firmware='efi'>
+    <type arch='x86_64' machine='pc-q35-4.0'>hvm</type>
+    <loader secure='no'/>
+  </os>
+  <features>
+    <acpi/>
+  </features>
+  <devices>
+    <emulator>/usr/bin/qemu-system-x86_64</emulator>
+    <controller type='usb' model='none'/>
+    <memballoon model='none'/>
+  </devices>
+</domain>

diff --git a/tests/qemuxml2argvdata/firmware-auto-efi-loader-path.x86_64-latest.err b/tests/qemuxml2argvdata/firmware-auto-efi-loader-path.x86_64-latest.err
new file mode 100644 (file)
index 0000000..e551faf
--- /dev/null
+++ b/tests/qemuxml2argvdata/firmware-auto-efi-loader-path.x86_64-latest.err
@@ -0,0 +1 @@
+loader attribute 'type' cannot be specified when firmware autoselection is enabled

diff --git a/tests/qemuxml2argvdata/firmware-auto-efi-loader-path.xml b/tests/qemuxml2argvdata/firmware-auto-efi-loader-path.xml
new file mode 100644 (file)
index 0000000..a40f5e7
--- /dev/null
+++ b/tests/qemuxml2argvdata/firmware-auto-efi-loader-path.xml
@@ -0,0 +1,18 @@
+<domain type='kvm'>
+  <name>fedora</name>
+  <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid>
+  <memory unit='KiB'>8192</memory>
+  <vcpu placement='static'>1</vcpu>
+  <os firmware='efi'>
+    <type arch='x86_64' machine='pc-q35-4.0'>hvm</type>
+    <loader>/path/to/OVMF_CODE.fd</loader>
+  </os>
+  <features>
+    <acpi/>
+  </features>
+  <devices>
+    <emulator>/usr/bin/qemu-system-x86_64</emulator>
+    <controller type='usb' model='none'/>
+    <memballoon model='none'/>
+  </devices>
+</domain>

diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index b01ad8d4e9e77db4b50b2a1f57cd2b50299e86a2..473e00ffa7efa72fc209a078228896329fe7e815 100644 (file)
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1217,9 +1217,12 @@ mymain(void)
     DO_TEST_NOCAPS("firmware-manual-noefi-noacpi-q35");
 
     DO_TEST_CAPS_LATEST("firmware-auto-bios");
+    DO_TEST_CAPS_LATEST_PARSE_ERROR("firmware-auto-bios-nvram");
     DO_TEST_CAPS_LATEST("firmware-auto-efi");
     DO_TEST_CAPS_LATEST("firmware-auto-efi-nvram");
     DO_TEST_CAPS_LATEST("firmware-auto-efi-loader-secure");
+    DO_TEST_CAPS_LATEST_PARSE_ERROR("firmware-auto-efi-loader-insecure");
+    DO_TEST_CAPS_LATEST_PARSE_ERROR("firmware-auto-efi-loader-path");
     DO_TEST_CAPS_LATEST("firmware-auto-efi-secboot");
     DO_TEST_CAPS_LATEST("firmware-auto-efi-no-secboot");
     DO_TEST_CAPS_LATEST("firmware-auto-efi-enrolled-keys");