QEMU "drive_init()" Disk Format Security Bypass

QEMU "drive_init()" Disk Format Security Bypass

[Eren Türkay]

Hello,

An advisory about $subject was released today by secunia. The security flaw 
was fixed in QEmu SVN repository.

Kvm uses some of the old version of qemu that I can't backport patch I grabbed 
from qemu svn repository. Could you look at this issue and provide a patch?

http://secunia.com/advisories/30111/

Svn commit: 
http://svn.savannah.gnu.org/viewvc/?view=rev&root=qemu&revision=4277

Discussion: http://lists.gnu.org/archive/html/qemu-devel/2008-04/msg00675.html

Regards,
Eren

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: QEMU "drive_init()" Disk Format Security Bypass

[Daniel P. Berrange]

On Thu, May 08, 2008 at 05:02:28PM +0300, Eren T?rkay wrote:
> Hello,
> 
> An advisory about $subject was released today by secunia. The security flaw 
> was fixed in QEmu SVN repository.
> 
> Kvm uses some of the old version of qemu that I can't backport patch I grabbed 
> from qemu svn repository. Could you look at this issue and provide a patch?

KVM is synced to latest CVS version of QEMU on a regular basis.

> http://secunia.com/advisories/30111/
> 
> Svn commit: 
> http://svn.savannah.gnu.org/viewvc/?view=rev&root=qemu&revision=4277

If you look at the KVM userspace code you'll see this patch is already
included:

http://git.kernel.org/?p=virt/kvm/kvm-userspace.git;a=commit;h=ce486fc1116eb53d40635be926bfa147ad520908

Regards,
Daniel
-- 
|: Red Hat, Engineering, Boston   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: QEMU "drive_init()" Disk Format Security Bypass

[Eren Türkay]

On 08 May 2008 Thu 17:12:14 Daniel P. Berrange wrote:
> If you look at the KVM userspace code you'll see this patch is already
> included:
>
> http://git.kernel.org/?p=virt/kvm/kvm-userspace.git;a=commit;h=ce486fc1116e
>b53d40635be926bfa147ad520908

Thank you, I'll grab the patch and apply it to tarball.

> Regards,
> Daniel

Regards,
Eren

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

QEMU "drive_init()" Disk Format Security Bypass

[Eren Türkay]

Hello,

Today, a security flaw in Qemu was released at secunia [0] which was fixed in 
qemu svn repository.

Xen uses part of a qemu code including "vl.c" in which the security flaw 
appeared. I suspect that Xen is effected by this vulnerability too but I 
couldn't find same lines in vl.c and I'm not sure about it.

Could someone look at this issue and shed a light? If Xen is effected, I would 
really appreciate a patch.

[0] http://secunia.com/advisories/30111/

My best regards,
Eren

Re: QEMU "drive_init()" Disk Format Security Bypass

[Ian Jackson]

Eren Türkay writes ("[Xen-devel] QEMU "drive_init()" Disk Format
> Security Bypass"): Today, a security flaw in Qemu was released at
> secunia [0] which was fixed in qemu svn repository.
>
> Xen uses part of a qemu code including "vl.c" in which the security
> flaw appeared. I suspect that Xen is effected by this vulnerability
> too but I couldn't find same lines in vl.c and I'm not sure about
> it.

I've looked into it and I'm afraid that yes, Xen is vulnerable.  We
use the same code in qemu, but via a different path.  The patch used
to fix the situation in qemu upstream in inapplicable to the current
ioemu.  As far as I can see the problem is with HVM guests where a
file which is supposed to be a raw image is specified in the
configuration.

If the object mentioned in the configuration is a block device all is
well, as qemu forces the format to raw in that case.  If the file is
actually a non-raw image format qemu will determine the type
correctly.  For PV guests, the tap driver is used instead - although I
haven't checked that for a similar problem.

There is a problem constructing a proper fix, unfortunately.  If you
write   file:/path/to/some/file   in your configuration, it is
ambiguous: did you mean that /path/to/some/file was a raw disk image
or a cow format with separate backing file ?  (The cow formats contain
the filename of the backing file.)

As far as I can tell there is not currently any way to specify the
format explicitly.  qemu-dm always autoguesses.

Should we break all old installations by requiring everyone to specify
a format ?  Or should we break only some old installations by
retaining the current syntax to mean one thing or the other ?  Perhaps
we should attempt to guess according to the _filename_, which is
controlled by the host and thus safe.  Do users typically choose
filenames for cow images which are enough of a giveaway ?

We can add a safety catch so that if what is supposedly a raw image
looks like a cow disk, we fail, unless the rawness was explicitly
specified.  So we can avoid data corruption although as far as I can
see at the moment we have to at least break some existing
deployments.

Ian.

Re: QEMU "drive_init()" Disk Format Security Bypass

[Eren Türkay]

On 08 May 2008 Thu 19:58:04 Ian Jackson wrote:
> We can add a safety catch so that if what is supposedly a raw image
> looks like a cow disk, we fail, unless the rawness was explicitly
> specified.  So we can avoid data corruption although as far as I can
> see at the moment we have to at least break some existing
> deployments.

Thank you for reply.

Should I file a bug about this situation? I'm looking forward to security fix. 
Btw, KVM also fixed this vulnerability (they just pulled latest qemu code).

Re: QEMU "drive_init()" Disk Format Security Bypass

[Daniel P. Berrange]

On Thu, May 08, 2008 at 05:58:04PM +0100, Ian Jackson wrote:
> Eren Türkay writes ("[Xen-devel] QEMU "drive_init()" Disk Format
> > Security Bypass"): Today, a security flaw in Qemu was released at
> > secunia [0] which was fixed in qemu svn repository.
> >
> > Xen uses part of a qemu code including "vl.c" in which the security
> > flaw appeared. I suspect that Xen is effected by this vulnerability
> > too but I couldn't find same lines in vl.c and I'm not sure about
> > it.
> 
> I've looked into it and I'm afraid that yes, Xen is vulnerable.  We
> use the same code in qemu, but via a different path.  The patch used
> to fix the situation in qemu upstream in inapplicable to the current
> ioemu.  As far as I can see the problem is with HVM guests where a
> file which is supposed to be a raw image is specified in the
> configuration.
> 
> If the object mentioned in the configuration is a block device all is
> well, as qemu forces the format to raw in that case.  If the file is
> actually a non-raw image format qemu will determine the type
> correctly.  For PV guests, the tap driver is used instead - although I
> haven't checked that for a similar problem.
> 
> There is a problem constructing a proper fix, unfortunately.  If you
> write   file:/path/to/some/file   in your configuration, it is
> ambiguous: did you mean that /path/to/some/file was a raw disk image
> or a cow format with separate backing file ?  (The cow formats contain
> the filename of the backing file.)
> 
> As far as I can tell there is not currently any way to specify the
> format explicitly.  qemu-dm always autoguesses.
> 
> Should we break all old installations by requiring everyone to specify
> a format ?  Or should we break only some old installations by
> retaining the current syntax to mean one thing or the other ?  Perhaps
> we should attempt to guess according to the _filename_, which is
> controlled by the host and thus safe.  Do users typically choose
> filenames for cow images which are enough of a giveaway ?

Well, tap:XXX: style URLS already encode the format explicitly. So if
we made QEMU understand that syntax too, then that gives admins the 
option to be secure, while keeping file: fas a legacy (unsecure) mode
for compatability. This has the added advantage that it'd be the same
syntax used for PV-on-HVM drivers, and avoids nasty guessing based on
filename.

Dan.
-- 
|: Red Hat, Engineering, Boston   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: QEMU "drive_init()" Disk Format Security Bypass

[Keir Fraser]

On 8/5/08 18:12, "Daniel P. Berrange" <berrange@redhat.com> wrote:

>> Should we break all old installations by requiring everyone to specify
>> a format ?  Or should we break only some old installations by
>> retaining the current syntax to mean one thing or the other ?  Perhaps
>> we should attempt to guess according to the _filename_, which is
>> controlled by the host and thus safe.  Do users typically choose
>> filenames for cow images which are enough of a giveaway ?
> 
> Well, tap:XXX: style URLS already encode the format explicitly. So if
> we made QEMU understand that syntax too, then that gives admins the
> option to be secure, while keeping file: fas a legacy (unsecure) mode
> for compatability. This has the added advantage that it'd be the same
> syntax used for PV-on-HVM drivers, and avoids nasty guessing based on
> filename.

Yes, I think we should keep the existing syntax's existing semantics. Just
as qemu/kvm have done.

 -- Keir

Re: QEMU "drive_init()" Disk Format Security Bypass

[Ian Jackson]

Daniel P. Berrange writes ("Re: [Xen-devel] QEMU "drive_init()" Disk Format Security Bypass"):
> Well, tap:XXX: style URLS already encode the format explicitly. So if
> we made QEMU understand that syntax too, then that gives admins the 
> option to be secure, while keeping file: fas a legacy (unsecure) mode
> for compatability. This has the added advantage that it'd be the same
> syntax used for PV-on-HVM drivers, and avoids nasty guessing based on
> filename.

Yes, encoding the format explicit is definitely the way forward.

The question is what to do for existing deployments.  Would the users
prefer to have their system break now or to get rooted in a month or
two ?

Ian.

Re: QEMU "drive_init()" Disk Format Security Bypass

[Daniel P. Berrange]

On Thu, May 08, 2008 at 06:19:30PM +0100, Ian Jackson wrote:
> Daniel P. Berrange writes ("Re: [Xen-devel] QEMU "drive_init()" Disk Format Security Bypass"):
> > Well, tap:XXX: style URLS already encode the format explicitly. So if
> > we made QEMU understand that syntax too, then that gives admins the 
> > option to be secure, while keeping file: fas a legacy (unsecure) mode
> > for compatability. This has the added advantage that it'd be the same
> > syntax used for PV-on-HVM drivers, and avoids nasty guessing based on
> > filename.
> 
> Yes, encoding the format explicit is definitely the way forward.
> 
> The question is what to do for existing deployments.  Would the users
> prefer to have their system break now or to get rooted in a month or
> two ?

Then disable all format guessing with file: for HVM guests and make it
only use RAW format - this matches semantics of file: with PV guests.
And let them use tap:XXX: if they want QCow with HVM

Dan.
-- 
|: Red Hat, Engineering, Boston   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: QEMU "drive_init()" Disk Format Security Bypass

[Ian Jackson]

Daniel P. Berrange writes ("Re: [Xen-devel] QEMU "drive_init()" Disk Format Security Bypass"):
> Then disable all format guessing with file: for HVM guests and make it
> only use RAW format - this matches semantics of file: with PV guests.
> And let them use tap:XXX: if they want QCow with HVM

If most people who use file: with HVM are using raw images, then that
is I think the best an interpretation for existing configs.  Users who
want non-blktap cow can say  file:qcow:/path/to/image

Ian.

Re: QEMU "drive_init()" Disk Format Security Bypass

[Daniel P. Berrange]

On Thu, May 08, 2008 at 06:27:10PM +0100, Ian Jackson wrote:
> Daniel P. Berrange writes ("Re: [Xen-devel] QEMU "drive_init()" Disk Format Security Bypass"):
> > Then disable all format guessing with file: for HVM guests and make it
> > only use RAW format - this matches semantics of file: with PV guests.
> > And let them use tap:XXX: if they want QCow with HVM
> 
> If most people who use file: with HVM are using raw images, then that
> is I think the best an interpretation for existing configs.  Users who
> want non-blktap cow can say  file:qcow:/path/to/image

No, that's changing the semantics of file:  That should be done with separate
syntax like tap:qcow:.  There are too many tools which see a disk config
file  file:/path/to/image and expect that everything after the leading
'file:' is the real path. Adding a further qcow: will break those tools.

Dan.
-- 
|: Red Hat, Engineering, Boston   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

[PATCH] QEMU "drive_init()" Disk Format Security Bypass

[Ian Jackson]

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 2481 bytes --]

Thanks to everyone for their comments.  It turns out that the
 file:/path/to/image
syntax doesn't work properly at the moment unless the image is a raw
image: although qemu-dm will treat it as a cow image, if it is in a
format it understands, blktap will not.  This means that the guest
would see the processed cow version via the emulated IDE controller
but the raw cow differences file, header and all, via the PV block
interface.

So it is fine for us to make  file:/path/to/image  always expect raw
images.   Non-raw images are done with  tap:qcow:/path/to/image
and that currently works.

Below is a patch, with suggested commit message.

The resulting code in xenstore.c is sadly not very simple.  This is
because it is untangling a mess made (entirely pointlessly) by xend,
as well as translating between the different naming schemes used by
xenstore and qemu for image formats.  I hope to remove this block
driver special case machinery from qemu as part of or shortly after I
complete my merge with qemu upstream.


ioemu: fix disk format security vulnerability

* make the xenstore reader in qemu-dm's startup determine which
  of qemu's block drivers to use according to the xenstore
  backend `type' field.  This `type' field typically comes from
  the front of the drive mapping string in ioemu.  The
  supported cases are:
    xm config file string      `type'  image format    qemu driver
     phy:[/dev/]<device>	phy	raw image	bdrv_raw
     file:<filename>		file	raw image	bdrv_raw
     tap:aio:<filename>		tap	raw image	bdrv_raw
     tap:qcow:<image>		tap	not raw		autoprobe
     tap:<cow-fmt>:<image>	tap	named format	bdrv_<cow-fmt>
  It is still necessary to autoprobe when the image is specified as
  `tap:qcow:<image>', because qemu distinguishes `qcow' and `qcow2'
  whereas blktap doesn't; `qcow' in xenstore typically means what
  qemu calls qcow2.  This is OK because qemu can safely distinguish
  the different cow formats provided we know it's not a raw image.

* Make the format autoprobing machinery never return `raw'.  This has
  two purposes: firstly, it arranges that the `tap:qcow:...' case
  above can be handled without accidentally falling back to raw
  format.  Secondly it prevents accidents in case the code changes in
  future: autoprobing will now always fail on supposed cow files which
  actually contain junk, rather than giving the guest access to the
  underlying file.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>


Ian.


[-- Attachment #2: make file:/ in hvm guest configs never autoprobe --]
[-- Type: text/plain, Size: 3489 bytes --]

diff -r c99a88623eda tools/ioemu/block.c
--- a/tools/ioemu/block.c	Thu May 08 14:33:31 2008 +0100
+++ b/tools/ioemu/block.c	Fri May 09 15:31:46 2008 +0100
@@ -254,7 +254,7 @@ static BlockDriver *find_protocol(const 
 #endif
     p = strchr(filename, ':');
     if (!p)
-        return &bdrv_raw;
+        return NULL; /* do not ever guess raw, it is a security problem! */
     len = p - filename;
     if (len > sizeof(protocol) - 1)
         len = sizeof(protocol) - 1;
diff -r c99a88623eda tools/ioemu/xenstore.c
--- a/tools/ioemu/xenstore.c	Thu May 08 14:33:31 2008 +0100
+++ b/tools/ioemu/xenstore.c	Fri May 09 16:32:43 2008 +0100
@@ -90,6 +90,7 @@ void xenstore_parse_domain_config(int hv
     int i, is_scsi, is_hdN = 0;
     unsigned int len, num, hd_index, pci_devid = 0;
     BlockDriverState *bs;
+    BlockDriver *format;
 
     for(i = 0; i < MAX_DISKS + MAX_SCSI_DISKS; i++)
         media_filename[i] = NULL;
@@ -135,6 +136,8 @@ void xenstore_parse_domain_config(int hv
     }
         
     for (i = 0; i < num; i++) {
+	format = NULL; /* don't know what the format is yet */
+
         /* read the backend path */
         if (pasprintf(&buf, "%s/device/vbd/%s/backend", path, e[i]) == -1)
             continue;
@@ -181,13 +184,20 @@ void xenstore_parse_domain_config(int hv
         drv = xs_read(xsh, XBT_NULL, buf, &len);
         if (drv == NULL)
             continue;
-        /* Strip off blktap sub-type prefix aio: - QEMU can autodetect this */
+        /* Obtain blktap sub-type prefix */
         if (!strcmp(drv, "tap") && params[0]) {
             char *offset = strchr(params, ':'); 
             if (!offset)
                 continue ;
+	    free(drv);
+	    drv = malloc(offset - params + 1);
+	    memcpy(drv, params, offset - params);
+	    drv[offset - params] = '\0';
+	    if (!strcmp(drv, "aio"))
+		/* qemu does aio anyway if it can */
+		format = &bdrv_raw;
             memmove(params, offset+1, strlen(offset+1)+1 );
-            fprintf(logfile, "Strip off blktap sub-type prefix to %s\n", params); 
+            fprintf(logfile, "Strip off blktap sub-type prefix to %s (drv '%s')\n", params, drv); 
         }
         /* Prefix with /dev/ if needed */
         if (!strcmp(drv, "phy") && params[0] != '/') {
@@ -195,6 +205,7 @@ void xenstore_parse_domain_config(int hv
             sprintf(newparams, "/dev/%s", params);
             free(params);
             params = newparams;
+	    format = &bdrv_raw;
         }
 
         /* 
@@ -240,8 +251,25 @@ void xenstore_parse_domain_config(int hv
 #endif
 
         if (params[0]) {
-            if (bdrv_open(bs, params, 0 /* snapshot */) < 0)
-                fprintf(stderr, "qemu: could not open vbd '%s' or hard disk image '%s'\n", buf, params);
+	    if (!format) {
+		if (!drv) {
+		    fprintf(stderr, "qemu: type (image format) not specified for vbd '%s' or image '%s'\n", buf, params);
+		    continue;
+		}
+		if (!strcmp(drv,"qcow")) {
+		    /* autoguess qcow vs qcow2 */
+		} else if (!strcmp(drv,"file")) {
+		    format = &bdrv_raw;
+		} else {
+		    format = bdrv_find_format(drv);
+		    if (!format) {
+			fprintf(stderr, "qemu: type (image format) '%s' unknown for vbd '%s' or image '%s'\n", drv, buf, params);
+			continue;
+		    }
+		}
+	    }
+            if (bdrv_open2(bs, params, 0 /* snapshot */, format) < 0)
+                fprintf(stderr, "qemu: could not open vbd '%s' or hard disk image '%s' (drv '%s')\n", buf, params, drv ? drv : "?");
         }
     }
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] QEMU "drive_init()" Disk Format Security Bypass

[Ian Jackson]

> Below is a patch, with suggested commit message.

I'm afraid I didn't test this thoroughly enough and it broke
phy:... disks (!)

This change, on top of my previous patch, should fix it:

diff -r 53195719f762 tools/ioemu/xenstore.c
--- a/tools/ioemu/xenstore.c	Tue May 13 15:08:17 2008 +0100
+++ b/tools/ioemu/xenstore.c	Tue May 13 17:52:35 2008 +0100
@@ -260,6 +260,8 @@ void xenstore_parse_domain_config(int hv
 		    /* autoguess qcow vs qcow2 */
 		} else if (!strcmp(drv,"file")) {
 		    format = &bdrv_raw;
+		} else if (!strcmp(drv,"phy")) {
+		    format = &bdrv_raw;
 		} else {
 		    format = bdrv_find_format(drv);
 		    if (!format) {

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

Sorry!

Ian.

Re: [PATCH] QEMU "drive_init()" Disk Format Security Bypass

[Markus Armbruster]

I'm looking at xen-unstable cset 17606 and 17646.  If I understand
your patches correctly, you attack the security problem in two places:

(1) make format probing never return raw, and

(2) provide means to specify the format explicitly, bypassing probing.

You put (2) in xenstore_parse_domain_config().  I can see how that
works for block devices defined in the domain configuration.  But what
about USB disks?  I created a guest with the following settings:

    usb = 1
    usbdevice = "disk:/var/lib/xen/images/usbkey.img"

This duly started qemu with arguments

    -usb -usbdevice disk:/var/lib/xen/images/usbkey.img

The -usbdevice argument is ultimately processed by usb_device_add(),
which calls usb_msd_init() to do the real work.  I think we get (1),
but not (2) there, i.e. your change breaks raw format USB disks.

Monitor command "usb_add" also runs usb_device_add(), so it should
have the same problem.

I suspect monitor command "change" has the same problem, too.

Re: [PATCH] QEMU "drive_init()" Disk Format Security Bypass

[Ian Jackson]

Markus Armbruster writes ("Re: [Xen-devel] [PATCH] QEMU "drive_init()" Disk Format Security Bypass"):
> I'm looking at xen-unstable cset 17606 and 17646.  If I understand
> your patches correctly, you attack the security problem in two places:
> 
> (1) make format probing never return raw, and

Right.  That's a safety catch so that there's no vulnerability in any
cases I missed, of which I was definitely expecting some.

> (2) provide means to specify the format explicitly, bypassing probing.
> 
> You put (2) in xenstore_parse_domain_config().  I can see how that
> works for block devices defined in the domain configuration.  But what
> about USB disks?  I created a guest with the following settings:
...
> The -usbdevice argument is ultimately processed by usb_device_add(),
> which calls usb_msd_init() to do the real work.  I think we get (1),
> but not (2) there, i.e. your change breaks raw format USB disks.

That's quite likely.  I hadn't spotted that separate arrangement.  The
best thing to do would be probably be to cross-port the format
parameter code which upstream have introduced in this area to (mostly)
fix the bug in their version.  I'll look into it.

Ian.

Re: [PATCH] QEMU "drive_init()" Disk Format Security Bypass

[Ian Jackson]

I wrote:
> Markus Armbruster writes ("Re: [Xen-devel] [PATCH] QEMU "drive_init()" Disk Format Security Bypass"):
> > The -usbdevice argument is ultimately processed by usb_device_add(),
> > which calls usb_msd_init() to do the real work.  I think we get (1),
> > but not (2) there, i.e. your change breaks raw format USB disks.
> 
> That's quite likely.  I hadn't spotted that separate arrangement.  The
> best thing to do would be probably be to cross-port the format
> parameter code which upstream have introduced in this area to (mostly)
> fix the bug in their version.  I'll look into it.

The code in current qemu and in ioemu are very different in this area.
The machinery to which qemu added the format=... parameter doesn't
exist in ioemu and I don't think we want to backport that.

Instead below is a batch which is intended to make
   usbdevice = "disk:<filename>"
expect a raw device (as this probably is the most usual case) and
   usbdevice = "disk-qcow:<filename>"
expect a COW image (autodetected, probably qcow2).

This latter will eventually have to change to bring things into line
with recent qemu, but we can probably provide backwards compatibility
at that time.

Markus and Eren: could you please try this and let me know if it
solves the problem for you ?  I don't have a handy test setup here
right now.  If you can't conveniently test it let me know and I'll do
it.

Regards,
Ian.

diff -r a88e19526770 tools/ioemu/hw/usb-msd.c
--- a/tools/ioemu/hw/usb-msd.c	Fri Jun 13 15:31:35 2008 +0100
+++ b/tools/ioemu/hw/usb-msd.c	Fri Jun 13 16:08:34 2008 +0100
@@ -510,7 +510,7 @@ static void usb_msd_handle_destroy(USBDe
     qemu_free(s);
 }
 
-USBDevice *usb_msd_init(const char *filename)
+USBDevice *usb_msd_init(const char *filename, BlockDriver *drv)
 {
     MSDState *s;
     BlockDriverState *bdrv;
@@ -520,7 +520,7 @@ USBDevice *usb_msd_init(const char *file
         return NULL;
 
     bdrv = bdrv_new("usb");
-    if (bdrv_open(bdrv, filename, 0) < 0)
+    if (bdrv_open2(bdrv, filename, 0, drv) < 0)
         goto fail;
     s->bs = bdrv;
 
diff -r a88e19526770 tools/ioemu/hw/usb.h
--- a/tools/ioemu/hw/usb.h	Fri Jun 13 15:31:35 2008 +0100
+++ b/tools/ioemu/hw/usb.h	Fri Jun 13 16:08:05 2008 +0100
@@ -217,7 +217,7 @@ USBDevice *usb_tablet_init(void);
 USBDevice *usb_tablet_init(void);
 
 /* usb-msd.c */
-USBDevice *usb_msd_init(const char *filename);
+USBDevice *usb_msd_init(const char *filename, BlockDriver *drv);
 
 /* usb.c */
 void generic_usb_save(QEMUFile* f, void *opaque);
diff -r a88e19526770 tools/ioemu/vl.c
--- a/tools/ioemu/vl.c	Fri Jun 13 15:31:35 2008 +0100
+++ b/tools/ioemu/vl.c	Fri Jun 13 16:08:51 2008 +0100
@@ -4260,7 +4260,9 @@ static int usb_device_add(const char *de
     } else if (!strcmp(devname, "tablet")) {
 	dev = usb_tablet_init();
     } else if (strstart(devname, "disk:", &p)) {
-        dev = usb_msd_init(p);
+        dev = usb_msd_init(p, &bdrv_raw);
+    } else if (strstart(devname, "disk-qcow:", &p)) {
+        dev = usb_msd_init(p, 0);
     } else {
         return -1;
     }

Re: [PATCH] QEMU "drive_init()" Disk Format Security Bypass

[Ian Jackson]

Markus Armbruster writes ("Re: [Xen-devel] [PATCH] QEMU "drive_init()" Disk Format Security Bypass"):
> Monitor command "usb_add" also runs usb_device_add(), so it should
> have the same problem.
> 
> I suspect monitor command "change" has the same problem, too.

I hadn't been aware that people were using the qemu monitor in Xen
deployments.  You're right that monitor change does have the same
problem and the patch I've just sent doesn't fix monitor usb_add.

If the monitor is supposed to work then we need a different
arrangement.

Ian.

Re: [PATCH] QEMU "drive_init()" Disk Format Security Bypass

[Markus Armbruster]

Ian Jackson <Ian.Jackson@eu.citrix.com> writes:

> I wrote:
>> Markus Armbruster writes ("Re: [Xen-devel] [PATCH] QEMU "drive_init()" Disk Format Security Bypass"):
>> > The -usbdevice argument is ultimately processed by usb_device_add(),
>> > which calls usb_msd_init() to do the real work.  I think we get (1),
>> > but not (2) there, i.e. your change breaks raw format USB disks.
>> 
>> That's quite likely.  I hadn't spotted that separate arrangement.  The
>> best thing to do would be probably be to cross-port the format
>> parameter code which upstream have introduced in this area to (mostly)
>> fix the bug in their version.  I'll look into it.
>
> The code in current qemu and in ioemu are very different in this area.
> The machinery to which qemu added the format=... parameter doesn't
> exist in ioemu and I don't think we want to backport that.
>
> Instead below is a batch which is intended to make
>    usbdevice = "disk:<filename>"
> expect a raw device (as this probably is the most usual case) and
>    usbdevice = "disk-qcow:<filename>"
> expect a COW image (autodetected, probably qcow2).
>
> This latter will eventually have to change to bring things into line
> with recent qemu, but we can probably provide backwards compatibility
> at that time.
>
> Markus and Eren: could you please try this and let me know if it
> solves the problem for you ?  I don't have a handy test setup here
> right now.  If you can't conveniently test it let me know and I'll do
> it.
>
> Regards,
> Ian.
[...]

Patch looks sane.  I backported it to F-8 and verified that:

1. usbdevice = "disk:IMG" opens the image IMG raw regardless of file
   contents.  Same for monitor command usb_add disk:IMG.

2. usbdevice = "disk-qcow:IMG" opens the qcow image IMG correctly.
   Same for monitor command usb_add disk-qcow:IMG.

I believe monitor command change is still broken.  I tried "change fda
IMG", with a qcow image IMG, and it was opened qcow.  But changing to
a raw image failed; I think that feature was broken by by your
security fix.

Re: [PATCH] QEMU "drive_init()" Disk Format Security Bypass

[Ian Jackson]

Markus Armbruster writes ("Re: [Xen-devel] [PATCH] QEMU "drive_init()" Disk Format Security Bypass"):
> Patch looks sane.  I backported it to F-8 and verified that:
> 
> 1. usbdevice = "disk:IMG" opens the image IMG raw regardless of file
>    contents.  Same for monitor command usb_add disk:IMG.
> 
> 2. usbdevice = "disk-qcow:IMG" opens the qcow image IMG correctly.
>    Same for monitor command usb_add disk-qcow:IMG.

Good, thanks.

> I believe monitor command change is still broken.  I tried "change fda
> IMG", with a qcow image IMG, and it was opened qcow.  But changing to
> a raw image failed; I think that feature was broken by by your
> security fix.

Yes, this is expected.  If this is a problem then we need a more
sophisticated solution.  NB that hopefully xen-unstable will acquire a
much more recent qemu shortly so there is no need to fix it right now
for xen-unstable unless it's a big problem which I think it probably
isn't given how long it's been like this now ...

Ian.

Re: [PATCH] QEMU "drive_init()" Disk Format Security Bypass

[Markus Armbruster]

Ian Jackson <Ian.Jackson@eu.citrix.com> writes:

> Markus Armbruster writes ("Re: [Xen-devel] [PATCH] QEMU "drive_init()" Disk Format Security Bypass"):
>> Patch looks sane.  I backported it to F-8 and verified that:
>> 
>> 1. usbdevice = "disk:IMG" opens the image IMG raw regardless of file
>>    contents.  Same for monitor command usb_add disk:IMG.
>> 
>> 2. usbdevice = "disk-qcow:IMG" opens the qcow image IMG correctly.
>>    Same for monitor command usb_add disk-qcow:IMG.
>
> Good, thanks.
>
>> I believe monitor command change is still broken.  I tried "change fda
>> IMG", with a qcow image IMG, and it was opened qcow.  But changing to
>> a raw image failed; I think that feature was broken by by your
>> security fix.
>
> Yes, this is expected.  If this is a problem then we need a more
> sophisticated solution.  NB that hopefully xen-unstable will acquire a
> much more recent qemu shortly so there is no need to fix it right now
> for xen-unstable unless it's a big problem which I think it probably
> isn't given how long it's been like this now ...
>
> Ian.

We could plug the hole by forcing raw in do_change_block().
One-liner, minor loss of functionality.  What do you think?

Re: [PATCH] QEMU "drive_init()" Disk Format Security Bypass

[Ian Jackson]

Markus Armbruster writes ("Re: [Xen-devel] [PATCH] QEMU "drive_init()" Disk Format Security Bypass"):
> We could plug the hole by forcing raw in do_change_block().
> One-liner, minor loss of functionality.  What do you think?

Certainly I think that would be an improvement over the current
situation.  Media change of raw images is more the kind of thing
people might want to do than media change between different cows.

If no-one else objects that this kills their use case I think we
should do it.

Ian.

Re: [PATCH] QEMU "drive_init()" Disk Format Security Bypass

[Markus Armbruster]

Ian Jackson <Ian.Jackson@eu.citrix.com> writes:

> Markus Armbruster writes ("Re: [Xen-devel] [PATCH] QEMU "drive_init()" Disk Format Security Bypass"):
>> We could plug the hole by forcing raw in do_change_block().
>> One-liner, minor loss of functionality.  What do you think?
>
> Certainly I think that would be an improvement over the current
> situation.  Media change of raw images is more the kind of thing
> people might want to do than media change between different cows.
>
> If no-one else objects that this kills their use case I think we
> should do it.
>
> Ian.

Okay, I'll post the obvious patch.

[PATCH] ioemu: Disable format auto-probing in monitor command change

[Markus Armbruster]

Format auto-probing of writable images is a security hole.  The last
known remaining instance is monitor command change.  Disable probing
there and use raw.  This breaks change for images in all other
formats.

Signed-off-by: Markus Armbruster <armbru@redhat.com>

diff -r ac745ad5f018 tools/ioemu/monitor.c
--- a/tools/ioemu/monitor.c	Fri Jun 13 16:10:50 2008 +0100
+++ b/tools/ioemu/monitor.c	Tue Jun 17 18:43:25 2008 +0200
@@ -387,7 +387,7 @@ static void do_change_block(const char *
     }
     if (eject_device(bs, 0) < 0)
         return;
-    bdrv_open(bs, filename, 0);
+    bdrv_open2(bs, filename, 0, &bdrv_raw);
     if (bdrv_is_encrypted(bs)) {
         term_printf("%s is encrypted.\n", device);
         for(i = 0; i < 3; i++) {

Re: [PATCH] ioemu: Disable format auto-probing in monitor command change

[Ian Jackson]

Markus Armbruster writes ("[Xen-devel] [PATCH] ioemu: Disable format auto-probing in monitor command change"):
> Format auto-probing of writable images is a security hole.  The last
> known remaining instance is monitor command change.  Disable probing
> there and use raw.  This breaks change for images in all other
> formats.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

For the avoidance of any doubt, re Markus's patch:
 Acked-By: Ian Jackson <ian.jackson@eu.citrix.com>

But this should be applied together with the patch I posted earlier,
which I assume Markus is also now happy with.  So here is that one
again for your comfort and convenience.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ian.

diff -r a88e19526770 tools/ioemu/hw/usb-msd.c
--- a/tools/ioemu/hw/usb-msd.c	Fri Jun 13 15:31:35 2008 +0100
+++ b/tools/ioemu/hw/usb-msd.c	Fri Jun 13 16:08:34 2008 +0100
@@ -510,7 +510,7 @@ static void usb_msd_handle_destroy(USBDe
     qemu_free(s);
 }
 
-USBDevice *usb_msd_init(const char *filename)
+USBDevice *usb_msd_init(const char *filename, BlockDriver *drv)
 {
     MSDState *s;
     BlockDriverState *bdrv;
@@ -520,7 +520,7 @@ USBDevice *usb_msd_init(const char *file
         return NULL;
 
     bdrv = bdrv_new("usb");
-    if (bdrv_open(bdrv, filename, 0) < 0)
+    if (bdrv_open2(bdrv, filename, 0, drv) < 0)
         goto fail;
     s->bs = bdrv;
 
diff -r a88e19526770 tools/ioemu/hw/usb.h
--- a/tools/ioemu/hw/usb.h	Fri Jun 13 15:31:35 2008 +0100
+++ b/tools/ioemu/hw/usb.h	Fri Jun 13 16:08:05 2008 +0100
@@ -217,7 +217,7 @@ USBDevice *usb_tablet_init(void);
 USBDevice *usb_tablet_init(void);
 
 /* usb-msd.c */
-USBDevice *usb_msd_init(const char *filename);
+USBDevice *usb_msd_init(const char *filename, BlockDriver *drv);
 
 /* usb.c */
 void generic_usb_save(QEMUFile* f, void *opaque);
diff -r a88e19526770 tools/ioemu/vl.c
--- a/tools/ioemu/vl.c	Fri Jun 13 15:31:35 2008 +0100
+++ b/tools/ioemu/vl.c	Fri Jun 13 16:08:51 2008 +0100
@@ -4260,7 +4260,9 @@ static int usb_device_add(const char *de
     } else if (!strcmp(devname, "tablet")) {
 	dev = usb_tablet_init();
     } else if (strstart(devname, "disk:", &p)) {
-        dev = usb_msd_init(p);
+        dev = usb_msd_init(p, &bdrv_raw);
+    } else if (strstart(devname, "disk-qcow:", &p)) {
+        dev = usb_msd_init(p, 0);
     } else {
         return -1;
     }

Re: [PATCH] ioemu: Disable format auto-probing in monitor command change

[Markus Armbruster]

Ian Jackson <Ian.Jackson@eu.citrix.com> writes:

> Markus Armbruster writes ("[Xen-devel] [PATCH] ioemu: Disable format auto-probing in monitor command change"):
>> Format auto-probing of writable images is a security hole.  The last
>> known remaining instance is monitor command change.  Disable probing
>> there and use raw.  This breaks change for images in all other
>> formats.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> For the avoidance of any doubt, re Markus's patch:
>  Acked-By: Ian Jackson <ian.jackson@eu.citrix.com>
>
> But this should be applied together with the patch I posted earlier,

Yes.

> which I assume Markus is also now happy with.  So here is that one

Yes.

> again for your comfort and convenience.
>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

Acked-by: Markus Armbruster <armbru@redhat.com>