1. Patchew
  2. Libvirt
  3. [PATCH 0/2] qemu: Don't crash on empty USB cdrom hotplug
  4. View patch

[PATCH 2/2] qemu: validate: Reject empty USB disks

Peter Krempa posted 2 patches 3 months, 1 week ago
[PATCH 2/2] qemu: validate: Reject empty USB disks
Posted by Peter Krempa 3 months, 1 week ago 
Attempting to start qemu with or hotplug an empty 'usb-storage' based
disk results in the following error:

 qemu-system-x86_64: -device {"driver":"usb-storage","bus":"usb.0","port":"2","id":"usb-disk1","removable":true}: drive property not set

Reject such config at validation step and adjust tests.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/qemu/qemu_validate.c                      |  6 ++++
 .../disk-cdrom-bus-other.x86_64-latest.args   |  5 ++--
 .../disk-cdrom-bus-other.x86_64-latest.xml    |  5 ----
 .../qemuxmlconfdata/disk-cdrom-bus-other.xml  |  5 ----
 .../disk-cdrom-usb-empty.x86_64-latest.err    |  1 +
 .../qemuxmlconfdata/disk-cdrom-usb-empty.xml  | 29 +++++++++++++++++++
 tests/qemuxmlconftest.c                       |  1 +
 7 files changed, 39 insertions(+), 13 deletions(-)
 create mode 100644 tests/qemuxmlconfdata/disk-cdrom-usb-empty.x86_64-latest.err
 create mode 100644 tests/qemuxmlconfdata/disk-cdrom-usb-empty.xml

diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index b885fe7c77..48abc3f850 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -3037,6 +3037,12 @@ qemuValidateDomainDeviceDefDiskFrontend(const virDomainDiskDef *disk,
             return -1;
         }

+        if (virStorageSourceIsEmpty(disk->src)) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("'usb' disk must not be empty"));
+            return -1;
+        }
+
         if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
             disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB) {
             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
diff --git a/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.args b/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.args
index e0d2452a10..e1406af663 100644
--- a/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.args
+++ b/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.args
@@ -27,9 +27,8 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
 -no-shutdown \
 -boot strict=on \
 -device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \
--blockdev '{"driver":"file","filename":"/root/boot.iso","node-name":"libvirt-2-storage","read-only":true}' \
--device '{"driver":"usb-storage","bus":"usb.0","port":"1","drive":"libvirt-2-storage","id":"usb-disk0","removable":false}' \
--device '{"driver":"usb-storage","bus":"usb.0","port":"2","id":"usb-disk1","removable":false}' \
+-blockdev '{"driver":"file","filename":"/root/boot.iso","node-name":"libvirt-1-storage","read-only":true}' \
+-device '{"driver":"usb-storage","bus":"usb.0","port":"1","drive":"libvirt-1-storage","id":"usb-disk0","removable":false}' \
 -audiodev '{"id":"audio1","driver":"none"}' \
 -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
 -msg timestamp=on
diff --git a/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.xml b/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.xml
index 312e735947..f16fe63057 100644
--- a/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.xml
+++ b/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.xml
@@ -23,11 +23,6 @@
       <target dev='sda' bus='usb'/>
       <readonly/>
     </disk>
-    <disk type='file' device='cdrom'>
-      <driver name='qemu' type='raw'/>
-      <target dev='sdb' bus='usb'/>
-      <readonly/>
-    </disk>
     <controller type='usb' index='0' model='piix3-uhci'>
       <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
     </controller>
diff --git a/tests/qemuxmlconfdata/disk-cdrom-bus-other.xml b/tests/qemuxmlconfdata/disk-cdrom-bus-other.xml
index 9564dc11be..119ce297a0 100644
--- a/tests/qemuxmlconfdata/disk-cdrom-bus-other.xml
+++ b/tests/qemuxmlconfdata/disk-cdrom-bus-other.xml
@@ -20,11 +20,6 @@
       <target dev='sda' bus='usb'/>
       <readonly/>
     </disk>
-    <disk type='file' device='cdrom'>
-      <driver name='qemu' type='raw'/>
-      <target dev='sdb' bus='usb'/>
-      <readonly/>
-    </disk>
     <controller type='usb' index='0'/>
     <controller type='ide' index='0'/>
     <controller type='pci' index='0' model='pci-root'/>
diff --git a/tests/qemuxmlconfdata/disk-cdrom-usb-empty.x86_64-latest.err b/tests/qemuxmlconfdata/disk-cdrom-usb-empty.x86_64-latest.err
new file mode 100644
index 0000000000..a00cb0a4b2
--- /dev/null
+++ b/tests/qemuxmlconfdata/disk-cdrom-usb-empty.x86_64-latest.err
@@ -0,0 +1 @@
+unsupported configuration: 'usb' disk must not be empty
diff --git a/tests/qemuxmlconfdata/disk-cdrom-usb-empty.xml b/tests/qemuxmlconfdata/disk-cdrom-usb-empty.xml
new file mode 100644
index 0000000000..fc1cf45085
--- /dev/null
+++ b/tests/qemuxmlconfdata/disk-cdrom-usb-empty.xml
@@ -0,0 +1,29 @@
+<domain type='qemu'>
+  <name>QEMUGuest1</name>
+  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+  <memory unit='KiB'>219100</memory>
+  <currentMemory unit='KiB'>219100</currentMemory>
+  <vcpu placement='static'>1</vcpu>
+  <os>
+    <type arch='x86_64' machine='pc'>hvm</type>
+    <boot dev='hd'/>
+  </os>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>destroy</on_crash>
+  <devices>
+    <emulator>/usr/bin/qemu-system-x86_64</emulator>
+    <disk type='file' device='cdrom'>
+      <driver name='qemu' type='raw'/>
+      <target dev='sdb' bus='usb'/>
+      <readonly/>
+    </disk>
+    <controller type='usb' index='0'/>
+    <controller type='ide' index='0'/>
+    <controller type='pci' index='0' model='pci-root'/>
+    <input type='mouse' bus='ps2'/>
+    <input type='keyboard' bus='ps2'/>
+    <memballoon model='none'/>
+  </devices>
+</domain>
diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c
index 89292af300..436d58b977 100644
--- a/tests/qemuxmlconftest.c
+++ b/tests/qemuxmlconftest.c
@@ -1536,6 +1536,7 @@ mymain(void)
     DO_TEST_CAPS_LATEST("disk-cdrom");
     DO_TEST_CAPS_LATEST("disk-cdrom-empty-network-invalid");
     DO_TEST_CAPS_LATEST("disk-cdrom-bus-other");
+    DO_TEST_CAPS_LATEST_PARSE_ERROR("disk-cdrom-usb-empty");
     DO_TEST_CAPS_LATEST("disk-cdrom-network");
     DO_TEST_CAPS_LATEST("disk-cdrom-tray");
     DO_TEST_CAPS_LATEST("disk-floppy");
-- 
2.46.0
Re: [PATCH 2/2] qemu: validate: Reject empty USB disks
Posted by Martin Kletzander 3 months, 1 week ago 
On Fri, Aug 16, 2024 at 04:42:33PM +0200, Peter Krempa wrote:
>Attempting to start qemu with or hotplug an empty 'usb-storage' based
>disk results in the following error:
>
> qemu-system-x86_64: -device {"driver":"usb-storage","bus":"usb.0","port":"2","id":"usb-disk1","removable":true}: drive property not set
>
>Reject such config at validation step and adjust tests.
>

It's weird that we even support usb cdrom as qemu cannot even emulate
that, at least to my knowledge.  Other than that the patch looks fine,
but I cannot shake off the feeling of fishiness with this one.  Could
you bring me up to speed how this even happened to be "supported"?

Thanks,
Martin

>Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>---
> src/qemu/qemu_validate.c                      |  6 ++++
> .../disk-cdrom-bus-other.x86_64-latest.args   |  5 ++--
> .../disk-cdrom-bus-other.x86_64-latest.xml    |  5 ----
> .../qemuxmlconfdata/disk-cdrom-bus-other.xml  |  5 ----
> .../disk-cdrom-usb-empty.x86_64-latest.err    |  1 +
> .../qemuxmlconfdata/disk-cdrom-usb-empty.xml  | 29 +++++++++++++++++++
> tests/qemuxmlconftest.c                       |  1 +
> 7 files changed, 39 insertions(+), 13 deletions(-)
> create mode 100644 tests/qemuxmlconfdata/disk-cdrom-usb-empty.x86_64-latest.err
> create mode 100644 tests/qemuxmlconfdata/disk-cdrom-usb-empty.xml
>
>diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
>index b885fe7c77..48abc3f850 100644
>--- a/src/qemu/qemu_validate.c
>+++ b/src/qemu/qemu_validate.c
>@@ -3037,6 +3037,12 @@ qemuValidateDomainDeviceDefDiskFrontend(const virDomainDiskDef *disk,
>             return -1;
>         }
>
>+        if (virStorageSourceIsEmpty(disk->src)) {
>+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>+                           _("'usb' disk must not be empty"));
>+            return -1;
>+        }
>+
>         if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
>             disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB) {
>             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>diff --git a/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.args b/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.args
>index e0d2452a10..e1406af663 100644
>--- a/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.args
>+++ b/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.args
>@@ -27,9 +27,8 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
> -no-shutdown \
> -boot strict=on \
> -device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \
>--blockdev '{"driver":"file","filename":"/root/boot.iso","node-name":"libvirt-2-storage","read-only":true}' \
>--device '{"driver":"usb-storage","bus":"usb.0","port":"1","drive":"libvirt-2-storage","id":"usb-disk0","removable":false}' \
>--device '{"driver":"usb-storage","bus":"usb.0","port":"2","id":"usb-disk1","removable":false}' \
>+-blockdev '{"driver":"file","filename":"/root/boot.iso","node-name":"libvirt-1-storage","read-only":true}' \
>+-device '{"driver":"usb-storage","bus":"usb.0","port":"1","drive":"libvirt-1-storage","id":"usb-disk0","removable":false}' \
> -audiodev '{"id":"audio1","driver":"none"}' \
> -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
> -msg timestamp=on
>diff --git a/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.xml b/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.xml
>index 312e735947..f16fe63057 100644
>--- a/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.xml
>+++ b/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.xml
>@@ -23,11 +23,6 @@
>       <target dev='sda' bus='usb'/>
>       <readonly/>
>     </disk>
>-    <disk type='file' device='cdrom'>
>-      <driver name='qemu' type='raw'/>
>-      <target dev='sdb' bus='usb'/>
>-      <readonly/>
>-    </disk>
>     <controller type='usb' index='0' model='piix3-uhci'>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
>     </controller>
>diff --git a/tests/qemuxmlconfdata/disk-cdrom-bus-other.xml b/tests/qemuxmlconfdata/disk-cdrom-bus-other.xml
>index 9564dc11be..119ce297a0 100644
>--- a/tests/qemuxmlconfdata/disk-cdrom-bus-other.xml
>+++ b/tests/qemuxmlconfdata/disk-cdrom-bus-other.xml
>@@ -20,11 +20,6 @@
>       <target dev='sda' bus='usb'/>
>       <readonly/>
>     </disk>
>-    <disk type='file' device='cdrom'>
>-      <driver name='qemu' type='raw'/>
>-      <target dev='sdb' bus='usb'/>
>-      <readonly/>
>-    </disk>
>     <controller type='usb' index='0'/>
>     <controller type='ide' index='0'/>
>     <controller type='pci' index='0' model='pci-root'/>
>diff --git a/tests/qemuxmlconfdata/disk-cdrom-usb-empty.x86_64-latest.err b/tests/qemuxmlconfdata/disk-cdrom-usb-empty.x86_64-latest.err
>new file mode 100644
>index 0000000000..a00cb0a4b2
>--- /dev/null
>+++ b/tests/qemuxmlconfdata/disk-cdrom-usb-empty.x86_64-latest.err
>@@ -0,0 +1 @@
>+unsupported configuration: 'usb' disk must not be empty
>diff --git a/tests/qemuxmlconfdata/disk-cdrom-usb-empty.xml b/tests/qemuxmlconfdata/disk-cdrom-usb-empty.xml
>new file mode 100644
>index 0000000000..fc1cf45085
>--- /dev/null
>+++ b/tests/qemuxmlconfdata/disk-cdrom-usb-empty.xml
>@@ -0,0 +1,29 @@
>+<domain type='qemu'>
>+  <name>QEMUGuest1</name>
>+  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
>+  <memory unit='KiB'>219100</memory>
>+  <currentMemory unit='KiB'>219100</currentMemory>
>+  <vcpu placement='static'>1</vcpu>
>+  <os>
>+    <type arch='x86_64' machine='pc'>hvm</type>
>+    <boot dev='hd'/>
>+  </os>
>+  <clock offset='utc'/>
>+  <on_poweroff>destroy</on_poweroff>
>+  <on_reboot>restart</on_reboot>
>+  <on_crash>destroy</on_crash>
>+  <devices>
>+    <emulator>/usr/bin/qemu-system-x86_64</emulator>
>+    <disk type='file' device='cdrom'>
>+      <driver name='qemu' type='raw'/>
>+      <target dev='sdb' bus='usb'/>
>+      <readonly/>
>+    </disk>
>+    <controller type='usb' index='0'/>
>+    <controller type='ide' index='0'/>
>+    <controller type='pci' index='0' model='pci-root'/>
>+    <input type='mouse' bus='ps2'/>
>+    <input type='keyboard' bus='ps2'/>
>+    <memballoon model='none'/>
>+  </devices>
>+</domain>
>diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c
>index 89292af300..436d58b977 100644
>--- a/tests/qemuxmlconftest.c
>+++ b/tests/qemuxmlconftest.c
>@@ -1536,6 +1536,7 @@ mymain(void)
>     DO_TEST_CAPS_LATEST("disk-cdrom");
>     DO_TEST_CAPS_LATEST("disk-cdrom-empty-network-invalid");
>     DO_TEST_CAPS_LATEST("disk-cdrom-bus-other");
>+    DO_TEST_CAPS_LATEST_PARSE_ERROR("disk-cdrom-usb-empty");
>     DO_TEST_CAPS_LATEST("disk-cdrom-network");
>     DO_TEST_CAPS_LATEST("disk-cdrom-tray");
>     DO_TEST_CAPS_LATEST("disk-floppy");
>-- 
>2.46.0
>
Re: [PATCH 2/2] qemu: validate: Reject empty USB disks
Posted by Peter Krempa 3 months, 1 week ago 
On Wed, Aug 21, 2024 at 14:02:50 +0200, Martin Kletzander wrote:
> On Fri, Aug 16, 2024 at 04:42:33PM +0200, Peter Krempa wrote:
> > Attempting to start qemu with or hotplug an empty 'usb-storage' based
> > disk results in the following error:
> > 
> > qemu-system-x86_64: -device {"driver":"usb-storage","bus":"usb.0","port":"2","id":"usb-disk1","removable":true}: drive property not set
> > 
> > Reject such config at validation step and adjust tests.
> > 
> 
> It's weird that we even support usb cdrom as qemu cannot even emulate
> that, at least to my knowledge.  Other than that the patch looks fine,
> but I cannot shake off the feeling of fishiness with this one.  Could
> you bring me up to speed how this even happened to be "supported"?

IIRC the change in behaviour of USB cdroms might be an oversight/fallout
of conversion from '-drive' to '-blockdev' which wasn't originally
caught for a rather long time. (Might even have been pre-existing but I
don't care digging that far back).

Now as this config "mostly works" for most media/situations we shouldn't
really just forbid it completely so that we don't break the working
situations.

AFAIK the fix for this is to switch the USB device handling to use 'uas'
instead of 'usb-storage' which becomes basically a SCSI controller and
you can then connect a proper SCSI-cdrom device to it.

That obviously requires a complete rewrite of the USB disk handling and
might not even be ABI compatible (I didn't check because I don't really
care).

As of this situation, qemu simply fails to start with taht config so
there's no harm in forbidding it for now.
Re: [PATCH 2/2] qemu: validate: Reject empty USB disks
Posted by Martin Kletzander 3 months, 1 week ago 
On Wed, Aug 21, 2024 at 02:20:22PM +0200, Peter Krempa wrote:
>On Wed, Aug 21, 2024 at 14:02:50 +0200, Martin Kletzander wrote:
>> On Fri, Aug 16, 2024 at 04:42:33PM +0200, Peter Krempa wrote:
>> > Attempting to start qemu with or hotplug an empty 'usb-storage' based
>> > disk results in the following error:
>> >
>> > qemu-system-x86_64: -device {"driver":"usb-storage","bus":"usb.0","port":"2","id":"usb-disk1","removable":true}: drive property not set
>> >
>> > Reject such config at validation step and adjust tests.
>> >
>>
>> It's weird that we even support usb cdrom as qemu cannot even emulate
>> that, at least to my knowledge.  Other than that the patch looks fine,
>> but I cannot shake off the feeling of fishiness with this one.  Could
>> you bring me up to speed how this even happened to be "supported"?
>
>IIRC the change in behaviour of USB cdroms might be an oversight/fallout
>of conversion from '-drive' to '-blockdev' which wasn't originally
>caught for a rather long time. (Might even have been pre-existing but I
>don't care digging that far back).
>
>Now as this config "mostly works" for most media/situations we shouldn't
>really just forbid it completely so that we don't break the working
>situations.
>
>AFAIK the fix for this is to switch the USB device handling to use 'uas'
>instead of 'usb-storage' which becomes basically a SCSI controller and
>you can then connect a proper SCSI-cdrom device to it.
>
>That obviously requires a complete rewrite of the USB disk handling and
>might not even be ABI compatible (I didn't check because I don't really
>care).
>
>As of this situation, qemu simply fails to start with taht config so
>there's no harm in forbidding it for now.
>

Yeah, that makes sense.  Thanks for elaborating on that.

Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.