[libvirt] [PATCH] qemu: Put format=raw onto cmd line for SCSI passthrough

Michal Privoznik posted 1 patch 5 years, 5 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/27d2fab3e79edc82b180a1ba679f784d08f818d3.1539342883.git.mprivozn@redhat.com
Test syntax-check passed
src/qemu/qemu_command.c                              | 2 +-
tests/qemuxml2argvdata/hostdev-scsi-lsi.args         | 2 +-
tests/qemuxml2argvdata/hostdev-scsi-readonly.args    | 2 +-
tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.args | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
[libvirt] [PATCH] qemu: Put format=raw onto cmd line for SCSI passthrough
Posted by Michal Privoznik 5 years, 5 months ago
https://bugzilla.redhat.com/show_bug.cgi?id=1632833

When doing a SCSI passthrough we don't put format= onto the
command line. This causes qemu to probe the format automatically
which ends up in a warning in the domain log and possible qemu
disabling writes to the first block (according to the warning
message).

Based-on-work-of: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_command.c                              | 2 +-
 tests/qemuxml2argvdata/hostdev-scsi-lsi.args         | 2 +-
 tests/qemuxml2argvdata/hostdev-scsi-readonly.args    | 2 +-
 tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.args | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 269276f2f9..1ff593c657 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4841,7 +4841,7 @@ qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev,
     } else {
         if (!(source = qemuBuildSCSIHostHostdevDrvStr(dev)))
             goto error;
-        virBufferAsprintf(&buf, "file=/dev/%s,if=none", source);
+        virBufferAsprintf(&buf, "file=/dev/%s,if=none,format=raw", source);
     }
     VIR_FREE(source);
 
diff --git a/tests/qemuxml2argvdata/hostdev-scsi-lsi.args b/tests/qemuxml2argvdata/hostdev-scsi-lsi.args
index d05e2a8bf8..f2048fe920 100644
--- a/tests/qemuxml2argvdata/hostdev-scsi-lsi.args
+++ b/tests/qemuxml2argvdata/hostdev-scsi-lsi.args
@@ -25,6 +25,6 @@ server,nowait \
 -drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-ide0-0-0 \
 -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,\
 bootindex=1 \
--drive file=/dev/sg0,if=none,id=drive-hostdev0 \
+-drive file=/dev/sg0,if=none,format=raw,id=drive-hostdev0 \
 -device scsi-generic,bus=scsi0.0,scsi-id=7,drive=drive-hostdev0,id=hostdev0 \
 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
diff --git a/tests/qemuxml2argvdata/hostdev-scsi-readonly.args b/tests/qemuxml2argvdata/hostdev-scsi-readonly.args
index c6336ca441..0d5a0d327d 100644
--- a/tests/qemuxml2argvdata/hostdev-scsi-readonly.args
+++ b/tests/qemuxml2argvdata/hostdev-scsi-readonly.args
@@ -25,7 +25,7 @@ server,nowait \
 -drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-ide0-0-0 \
 -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,\
 bootindex=1 \
--drive file=/dev/sg0,if=none,id=drive-hostdev0,readonly=on \
+-drive file=/dev/sg0,if=none,format=raw,id=drive-hostdev0,readonly=on \
 -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=8,\
 drive=drive-hostdev0,id=hostdev0 \
 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
diff --git a/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.args b/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.args
index 4bf4ce7f82..13a1e9fe95 100644
--- a/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.args
+++ b/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.args
@@ -25,7 +25,7 @@ server,nowait \
 -drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-ide0-0-0 \
 -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,\
 bootindex=1 \
--drive file=/dev/sg0,if=none,id=drive-hostdev0 \
+-drive file=/dev/sg0,if=none,format=raw,id=drive-hostdev0 \
 -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=8,\
 drive=drive-hostdev0,id=hostdev0 \
 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
-- 
2.18.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Put format=raw onto cmd line for SCSI passthrough
Posted by Daniel P. Berrangé 5 years, 5 months ago
On Fri, Oct 12, 2018 at 01:14:51PM +0200, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1632833
> 
> When doing a SCSI passthrough we don't put format= onto the
> command line. This causes qemu to probe the format automatically
> which ends up in a warning in the domain log and possible qemu
> disabling writes to the first block (according to the warning
> message).

If the warning message is correct, this should have been reported
as a security bug to libvirt and given a CVE.

On the other hand if the warning from QEMU isn't correct, then
QEMU shouldn't have printed the warning about it being dangerous.

So something is missing here either way.

> 
> Based-on-work-of: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_command.c                              | 2 +-
>  tests/qemuxml2argvdata/hostdev-scsi-lsi.args         | 2 +-
>  tests/qemuxml2argvdata/hostdev-scsi-readonly.args    | 2 +-
>  tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.args | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)



> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 269276f2f9..1ff593c657 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -4841,7 +4841,7 @@ qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev,
>      } else {
>          if (!(source = qemuBuildSCSIHostHostdevDrvStr(dev)))
>              goto error;
> -        virBufferAsprintf(&buf, "file=/dev/%s,if=none", source);
> +        virBufferAsprintf(&buf, "file=/dev/%s,if=none,format=raw", source);
>      }
>      VIR_FREE(source);
>  
> diff --git a/tests/qemuxml2argvdata/hostdev-scsi-lsi.args b/tests/qemuxml2argvdata/hostdev-scsi-lsi.args
> index d05e2a8bf8..f2048fe920 100644
> --- a/tests/qemuxml2argvdata/hostdev-scsi-lsi.args
> +++ b/tests/qemuxml2argvdata/hostdev-scsi-lsi.args
> @@ -25,6 +25,6 @@ server,nowait \
>  -drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-ide0-0-0 \
>  -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,\
>  bootindex=1 \
> --drive file=/dev/sg0,if=none,id=drive-hostdev0 \
> +-drive file=/dev/sg0,if=none,format=raw,id=drive-hostdev0 \
>  -device scsi-generic,bus=scsi0.0,scsi-id=7,drive=drive-hostdev0,id=hostdev0 \
>  -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
> diff --git a/tests/qemuxml2argvdata/hostdev-scsi-readonly.args b/tests/qemuxml2argvdata/hostdev-scsi-readonly.args
> index c6336ca441..0d5a0d327d 100644
> --- a/tests/qemuxml2argvdata/hostdev-scsi-readonly.args
> +++ b/tests/qemuxml2argvdata/hostdev-scsi-readonly.args
> @@ -25,7 +25,7 @@ server,nowait \
>  -drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-ide0-0-0 \
>  -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,\
>  bootindex=1 \
> --drive file=/dev/sg0,if=none,id=drive-hostdev0,readonly=on \
> +-drive file=/dev/sg0,if=none,format=raw,id=drive-hostdev0,readonly=on \
>  -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=8,\
>  drive=drive-hostdev0,id=hostdev0 \
>  -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
> diff --git a/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.args b/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.args
> index 4bf4ce7f82..13a1e9fe95 100644
> --- a/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.args
> +++ b/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.args
> @@ -25,7 +25,7 @@ server,nowait \
>  -drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-ide0-0-0 \
>  -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,\
>  bootindex=1 \
> --drive file=/dev/sg0,if=none,id=drive-hostdev0 \
> +-drive file=/dev/sg0,if=none,format=raw,id=drive-hostdev0 \
>  -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=8,\
>  drive=drive-hostdev0,id=hostdev0 \
>  -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
> -- 
> 2.18.1
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Put format=raw onto cmd line for SCSI passthrough
Posted by Michal Privoznik 5 years, 5 months ago
On 10/12/2018 02:17 PM, Daniel P. Berrangé wrote:
> On Fri, Oct 12, 2018 at 01:14:51PM +0200, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1632833
>>
>> When doing a SCSI passthrough we don't put format= onto the
>> command line. This causes qemu to probe the format automatically
>> which ends up in a warning in the domain log and possible qemu
>> disabling writes to the first block (according to the warning
>> message).
> 
> If the warning message is correct, this should have been reported
> as a security bug to libvirt and given a CVE.

Why is that? It the message is correct, qemu would prevent from writing
to the first block. No harm there.

> 
> On the other hand if the warning from QEMU isn't correct, then
> QEMU shouldn't have printed the warning about it being dangerous.

In my testing I was able to write to the first block. Therefore, IMO
qemu is throwing incorrect warning message.

> 
> So something is missing here either way.

Sure, but that doesn't invalidate my patch, does it?

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Put format=raw onto cmd line for SCSI passthrough
Posted by Daniel P. Berrangé 5 years, 5 months ago
On Fri, Oct 12, 2018 at 02:27:26PM +0200, Michal Privoznik wrote:
> On 10/12/2018 02:17 PM, Daniel P. Berrangé wrote:
> > On Fri, Oct 12, 2018 at 01:14:51PM +0200, Michal Privoznik wrote:
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1632833
> >>
> >> When doing a SCSI passthrough we don't put format= onto the
> >> command line. This causes qemu to probe the format automatically
> >> which ends up in a warning in the domain log and possible qemu
> >> disabling writes to the first block (according to the warning
> >> message).
> > 
> > If the warning message is correct, this should have been reported
> > as a security bug to libvirt and given a CVE.
> 
> Why is that? It the message is correct, qemu would prevent from writing
> to the first block. No harm there.

Only QEMU >= 2.3.0 has that protection, so this is not
something we can rely to avoid calling it a CVE. It just
means distros when QEMU >=2.3.0 would not be affected by
the CVE.

> > On the other hand if the warning from QEMU isn't correct, then
> > QEMU shouldn't have printed the warning about it being dangerous.
> 
> In my testing I was able to write to the first block. Therefore, IMO
> qemu is throwing incorrect warning message.
> 
> > 
> > So something is missing here either way.
> 
> Sure, but that doesn't invalidate my patch, does it?

Only the commit message - if this is a security flaw, we must be more
explicit about it in the commit.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Put format=raw onto cmd line for SCSI passthrough
Posted by Daniel P. Berrangé 5 years, 5 months ago
On Fri, Oct 12, 2018 at 01:17:42PM +0100, Daniel P. Berrangé wrote:
> On Fri, Oct 12, 2018 at 01:14:51PM +0200, Michal Privoznik wrote:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1632833
> > 
> > When doing a SCSI passthrough we don't put format= onto the
> > command line. This causes qemu to probe the format automatically
> > which ends up in a warning in the domain log and possible qemu
> > disabling writes to the first block (according to the warning
> > message).
> 
> If the warning message is correct, this should have been reported
> as a security bug to libvirt and given a CVE.
> 
> On the other hand if the warning from QEMU isn't correct, then
> QEMU shouldn't have printed the warning about it being dangerous.
> 
> So something is missing here either way.

I used 'modprobe scsi_debug' to create a fake SCSI device
which appears as "scsi_host6" in my host OS, and has a
/dev/sg3 and /dev/sdd device nodes.

When I use this XML:

    <hostdev mode='subsystem' type='scsi' managed='no' sgio='filtered' rawio='yes'>
      <source>
        <adapter name='scsi_host6'/>
        <address bus='0' target='0' unit='0'/>
      </source>
      <alias name='hostdev0'/>
      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
    </hostdev>

libvirt spawns QEMU pointing to /dev/sg3

  -drive file=/dev/sg3,if=none,id=drive-hostdev0

Inside the guest, I can successfully run

   qemu-img create -f qcow2 /dev/sda 4M

and on the host OS this now appears visible in the host

  # qemu-img info /dev/sdd 
  image: /dev/sdd
  file format: qcow2
  virtual size: 6.0M (6291456 bytes)
  disk size: 0
  cluster_size: 65536
  Format specific information:
      compat: 1.1
      lazy refcounts: false
      refcount bits: 16
      corrupt: false


this will *not* have an effect on this QEMU binary if
it reboots, however, because QEMU does not appear to
actually trigger format probing when used via the
"scsi-generic" device type.

Attempting to run qemu-img info against the /dev/sg3
device in the host fails as you can't read from an
sg device directly. Presumably this is why QEMU won't
do format probing either.

Thus I don't think there is any security flaw here.

The QEMU warning appears bogus.

So to  shut QEMU up:

  Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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