[libvirt] [PATCH] conf: Fix check for chardev source path

Andrea Bolognani posted 1 patch 5 years, 7 months ago
Failed in applying to current master (apply log)
src/conf/domain_conf.c                            |  8 ++++++--
.../serial-unix-missing-source.xml                | 15 +++++++++++++++
tests/qemuxml2argvtest.c                          |  1 +
3 files changed, 22 insertions(+), 2 deletions(-)
create mode 100644 tests/qemuxml2argvdata/serial-unix-missing-source.xml
[libvirt] [PATCH] conf: Fix check for chardev source path
Posted by Andrea Bolognani 5 years, 7 months ago
Attempting to use a chardev definition like

  <serial type='unix'>
    <target type='isa-serial'/>
  </serial>

correctly results in an error being reported, since the source
path - a required piece of information - is missing; however,
the very similar

  <serial type='unix'>
    <target type='pci-serial'/>
  </serial>

was happily accepted by libvirt, only to result in libvirtd
crashing as soon as the guest was started.

The issue was caused by checking the chardev's targetType
against whitelisted values from virDomainChrChannelTargetType
without first checking the chardev's deviceType to make sure
it is actually a channel, for which the check makes sense,
rather than a different type of chardev.

The only reason this wasn't spotted earlier is that the
whitelisted values just so happen to correspond to USB and
PCI serial devices and Xen and UML consoles respectively,
all of which are fairly uncommon.

https://bugzilla.redhat.com/show_bug.cgi?id=1609720

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
I feel compelled to point out that stuff like this wouldn't
happen if libvirt was written in Rust O:-)

 src/conf/domain_conf.c                            |  8 ++++++--
 .../serial-unix-missing-source.xml                | 15 +++++++++++++++
 tests/qemuxml2argvtest.c                          |  1 +
 3 files changed, 22 insertions(+), 2 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/serial-unix-missing-source.xml

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 86199623cc..6162843028 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5467,10 +5467,14 @@ virDomainChrSourceDefValidate(const virDomainChrSourceDef *def,
         break;
 
     case VIR_DOMAIN_CHR_TYPE_UNIX:
-        /* path can be auto generated */
+        /* The source path can be auto generated for certain specific
+         * types of channels, but in most cases we should report an
+         * error if the user didn't provide it */
         if (!def->data.nix.path &&
             (!chr_def ||
-             (chr_def->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN &&
+             chr_def->deviceType != VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL ||
+             (chr_def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
+              chr_def->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN &&
               chr_def->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO))) {
             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                            _("Missing source path attribute for char device"));
diff --git a/tests/qemuxml2argvdata/serial-unix-missing-source.xml b/tests/qemuxml2argvdata/serial-unix-missing-source.xml
new file mode 100644
index 0000000000..1e1221f12d
--- /dev/null
+++ b/tests/qemuxml2argvdata/serial-unix-missing-source.xml
@@ -0,0 +1,15 @@
+<domain type='qemu'>
+  <name>guest</name>
+  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+  <memory unit='KiB'>1048576</memory>
+  <vcpu placement='static'>1</vcpu>
+  <os>
+    <type arch='aarch64' machine='virt'>hvm</type>
+  </os>
+  <devices>
+    <emulator>/usr/bin/qemu-system-aarch64</emulator>
+    <serial type='unix'>
+      <target type='pci-serial'/>
+    </serial>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 35df63b2ac..949b203998 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1417,6 +1417,7 @@ mymain(void)
     DO_TEST("serial-unix-chardev",
             QEMU_CAPS_DEVICE_ISA_SERIAL);
     DO_TEST_CAPS_LATEST("serial-unix-chardev");
+    DO_TEST_PARSE_ERROR("serial-unix-missing-source", NONE);
     DO_TEST("serial-tcp-chardev",
             QEMU_CAPS_DEVICE_ISA_SERIAL);
     DO_TEST("serial-udp-chardev",
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: Fix check for chardev source path
Posted by Ján Tomko 5 years, 7 months ago
On Fri, Sep 07, 2018 at 01:06:39PM +0200, Andrea Bolognani wrote:
>Attempting to use a chardev definition like
>
>  <serial type='unix'>
>    <target type='isa-serial'/>
>  </serial>
>
>correctly results in an error being reported, since the source
>path - a required piece of information - is missing; however,
>the very similar
>
>  <serial type='unix'>
>    <target type='pci-serial'/>
>  </serial>
>
>was happily accepted by libvirt, only to result in libvirtd
>crashing as soon as the guest was started.
>
>The issue was caused by checking the chardev's targetType
>against whitelisted values from virDomainChrChannelTargetType
>without first checking the chardev's deviceType to make sure
>it is actually a channel, for which the check makes sense,
>rather than a different type of chardev.
>
>The only reason this wasn't spotted earlier is that the
>whitelisted values just so happen to correspond to USB and
>PCI serial devices and Xen and UML consoles respectively,
>all of which are fairly uncommon.
>
>https://bugzilla.redhat.com/show_bug.cgi?id=1609720
>
>Signed-off-by: Andrea Bolognani <abologna@redhat.com>
>---
>I feel compelled to point out that stuff like this wouldn't
>happen if libvirt was written in Rust O:-)
>
> src/conf/domain_conf.c                            |  8 ++++++--
> .../serial-unix-missing-source.xml                | 15 +++++++++++++++
> tests/qemuxml2argvtest.c                          |  1 +
> 3 files changed, 22 insertions(+), 2 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/serial-unix-missing-source.xml
>
>diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>index 86199623cc..6162843028 100644
>--- a/src/conf/domain_conf.c
>+++ b/src/conf/domain_conf.c
>@@ -5467,10 +5467,14 @@ virDomainChrSourceDefValidate(const virDomainChrSourceDef *def,
>         break;
>
>     case VIR_DOMAIN_CHR_TYPE_UNIX:
>-        /* path can be auto generated */
>+        /* The source path can be auto generated for certain specific
>+         * types of channels, but in most cases we should report an
>+         * error if the user didn't provide it */
>         if (!def->data.nix.path &&
>             (!chr_def ||
>-             (chr_def->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN &&
>+             chr_def->deviceType != VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL ||
>+             (chr_def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
>+              chr_def->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN &&
>               chr_def->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO))) {

Consider:
            !(chr_def &&
              chr_def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
              (chr_def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN ||
               chr_def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO))) {


>             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                            _("Missing source path attribute for char device"));

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: Fix check for chardev source path
Posted by Andrea Bolognani 5 years, 7 months ago
On Fri, 2018-09-07 at 17:03 +0200, Ján Tomko wrote:
> On Fri, Sep 07, 2018 at 01:06:39PM +0200, Andrea Bolognani wrote:
> >         if (!def->data.nix.path &&
> >             (!chr_def ||
> > +             chr_def->deviceType != VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL ||
> > +             (chr_def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
> > +              chr_def->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN &&
> >               chr_def->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO))) {
> 
> Consider:
>             !(chr_def &&
>               chr_def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
>               (chr_def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN ||
>                chr_def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO))) {

Both make my brain hurt, but yours is a bit shorter so I guess
I'll go with that one :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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