Xen devel - May 2008 - QEMU "drive_init()" Disk Format Security Bypass

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

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

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.

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

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).

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

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 :|

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

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



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

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.

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

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 :|

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

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.

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

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 :|

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

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.



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

> 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.

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

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.

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

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.

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

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;
     }

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

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.

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

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.

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

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.

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

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?

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

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.

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

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.

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

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++) {

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

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;
     }


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

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>

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