Xen devel - Apr 2009 - [PATCH]ioemu:fix up error when using qemu-img-xen to create img

If find_protocol() can not find the the drv. The caller should give
a default value.

Signed-off-by: Yang Zhang <yang.zhang@intel.com>

diff --git a/block.c b/block.c
index 8a0e8b2..3e84707 100644
--- a/block.c
+++ b/block.c
@@ -306,6 +306,8 @@ static BlockDriver *find_image_format(const char *filename)
     /* no need to test disk image format if the filename told us */
     if (drv != NULL)
         return drv;
+    else 
+        drv = &bdrv_raw;
 
     ret = bdrv_file_open(&bs, filename, BDRV_O_RDONLY);
     if (ret < 0)

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

Hi Ian,
Any comments on this patch? it''s a block issue to our QA test.
Thanks!
-Xin
>-----Original Message-----
>From: xen-devel-bounces@lists.xensource.com
>[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Zhang, Yang
>Sent: Tuesday, April 21, 2009 9:24 AM
>To: Ian Jackson
>Cc: xen-devel@lists.xensource.com
>Subject: [Xen-devel][PATCH]ioemu:fix up error when using qemu-img-xen to
>create img
>
>If find_protocol() can not find the the drv. The caller should give
>a default value.
>
>Signed-off-by: Yang Zhang <yang.zhang@intel.com>
>
>diff --git a/block.c b/block.c
>index 8a0e8b2..3e84707 100644
>--- a/block.c
>+++ b/block.c
>@@ -306,6 +306,8 @@ static BlockDriver *find_image_format(const char
>*filename)
>     /* no need to test disk image format if the filename told us */
>     if (drv != NULL)
>         return drv;
>+    else
>+        drv = &bdrv_raw;
>
>     ret = bdrv_file_open(&bs, filename, BDRV_O_RDONLY);
>     if (ret < 0)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Li, Xin writes ("RE: [Xen-devel][PATCH]ioemu:fix up error when using
qemu-img-xen to create img"):
> Any comments on this patch? it''s a block issue to our QA test.
Sorry to delay replying to this.
> >+    else
> >+        drv = &bdrv_raw;
This appears to reintroduce the image format vulnerability.  Are you
sure it''s right ?

The effect of your patch seems to be in this case:
  * the filename does not refer to a block device
  * the format is not specified in the filename
  * the actual contents of the image is not autodetected as any
    image format (eg, qcow or qcow2)
In that case, without your patch, the open fails.  With your patch,
the file is opened with bdrv_raw.

Ian.

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

Ian, saw your back, how do you think of this patch?
Thanks!
-Xin
>-----Original Message-----
>From: xen-devel-bounces@lists.xensource.com
>[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Li, Xin
>Sent: Monday, April 27, 2009 11:13 AM
>To: Zhang, Yang; Ian Jackson
>Cc: xen-devel@lists.xensource.com
>Subject: RE: [Xen-devel][PATCH]ioemu:fix up error when using qemu-img-xen to
>create img
>
>Hi Ian,
>Any comments on this patch? it''s a block issue to our QA test.
>Thanks!
>-Xin
>
>>-----Original Message-----
>>From: xen-devel-bounces@lists.xensource.com
>>[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Zhang, Yang
>>Sent: Tuesday, April 21, 2009 9:24 AM
>>To: Ian Jackson
>>Cc: xen-devel@lists.xensource.com
>>Subject: [Xen-devel][PATCH]ioemu:fix up error when using qemu-img-xen to
>>create img
>>
>>If find_protocol() can not find the the drv. The caller should give
>>a default value.
>>
>>Signed-off-by: Yang Zhang <yang.zhang@intel.com>
>>
>>diff --git a/block.c b/block.c
>>index 8a0e8b2..3e84707 100644
>>--- a/block.c
>>+++ b/block.c
>>@@ -306,6 +306,8 @@ static BlockDriver *find_image_format(const char
>>*filename)
>>     /* no need to test disk image format if the filename told us */
>>     if (drv != NULL)
>>         return drv;
>>+    else
>>+        drv = &bdrv_raw;
>>
>>     ret = bdrv_file_open(&bs, filename, BDRV_O_RDONLY);
>>     if (ret < 0)
>
>_______________________________________________
>Xen-devel mailing list
>Xen-devel@lists.xensource.com
>http://lists.xensource.com/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Li, Xin writes ("RE: [Xen-devel][PATCH]ioemu:fix up error when using
qemu-img-xen to create img"):
> Ian, saw [you''re] back, how do you think of this patch?
I replied to this yesterday.  Did you see my mail ?  Here it is again:

   > >+    else
   > >+        drv = &bdrv_raw;

   This appears to reintroduce the image format vulnerability.  Are you
   sure it''s right ?

   The effect of your patch seems to be in this case:
     * the filename does not refer to a block device
     * the format is not specified in the filename
     * the actual contents of the image is not autodetected as any
       image format (eg, qcow or qcow2)
   In that case, without your patch, the open fails.  With your patch,
   the file is opened with bdrv_raw.

Ian.

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

Hi, Ian, 
	Thanks for your ack. We think Yang's patch is right in this case. When
creating image by command: "qemu-img-xen create -b base_image filename -f
fmt", if the base_image is in the following case as you mentioned:
1)  The filename does not refer to a block device.
2)  The format is not specified in the filename.
3)  The actual contents of the image is not auto-probed as any image format (eg,
qcow, qcow2)
	This command could fail without Yang's patch. As we know, raw image could
not be auto-probed. So if all the other file-type probing functions could not
recognize the image format, then we should treat it as a raw file image. And if
someday there are some other new "specific" image types, we should add
"specific" auto-probe methods for them, and always reserve default
format for raw type. Also this command is a typical usage model, and execution
failure is not so friendly to end user, so we think that this default value is
needed. Thanks!

Best Regards, 
-- Dongxiao

-----Original Message-----
From: xen-devel-bounces@lists.xensource.com
[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Ian Jackson
Sent: 2009年5月1日 17:39
To: Li, Xin
Cc: xen-devel@lists.xensource.com; Zhang, Yang
Subject: RE: [Xen-devel][PATCH]ioemu:fix up error when using qemu-img-xen to
create img

Li, Xin writes ("RE: [Xen-devel][PATCH]ioemu:fix up error when using
qemu-img-xen to create img"):
> Ian, saw [you're] back, how do you think of this patch?
I replied to this yesterday.  Did you see my mail ?  Here it is again:

   > >+    else
   > >+        drv = &bdrv_raw;

   This appears to reintroduce the image format vulnerability.  Are you
   sure it's right ?

   The effect of your patch seems to be in this case:
     * the filename does not refer to a block device
     * the format is not specified in the filename
     * the actual contents of the image is not autodetected as any
       image format (eg, qcow or qcow2)
   In that case, without your patch, the open fails.  With your patch,
   the file is opened with bdrv_raw.

Ian.

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

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

Ian, could you also help to explain the meaning of "reintroduce the image
format vulnerability"? Thanks!

Best Regards, 
-- Dongxiao
-----Original Message-----
From: xen-devel-bounces@lists.xensource.com
[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Xu, Dongxiao
Sent: 2009年5月4日 20:51
To: Ian Jackson; Li, Xin
Cc: xen-devel@lists.xensource.com; Zhang, Yang
Subject: RE: [Xen-devel][PATCH]ioemu:fix up error when using qemu-img-xen to
create img

Hi, Ian, 
	Thanks for your ack. We think Yang's patch is right in this case. When
creating image by command: "qemu-img-xen create -b base_image filename -f
fmt", if the base_image is in the following case as you mentioned:
1)  The filename does not refer to a block device.
2)  The format is not specified in the filename.
3)  The actual contents of the image is not auto-probed as any image format (eg,
qcow, qcow2)
	This command could fail without Yang's patch. As we know, raw image could
not be auto-probed. So if all the other file-type probing functions could not
recognize the image format, then we should treat it as a raw file image. And if
someday there are some other new "specific" image types, we should add
"specific" auto-probe methods for them, and always reserve default
format for raw type. Also this command is a typical usage model, and execution
failure is not so friendly to end user, so we think that this default value is
needed. Thanks!

Best Regards, 
-- Dongxiao

-----Original Message-----
From: xen-devel-bounces@lists.xensource.com
[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Ian Jackson
Sent: 2009年5月1日 17:39
To: Li, Xin
Cc: xen-devel@lists.xensource.com; Zhang, Yang
Subject: RE: [Xen-devel][PATCH]ioemu:fix up error when using qemu-img-xen to
create img

Li, Xin writes ("RE: [Xen-devel][PATCH]ioemu:fix up error when using
qemu-img-xen to create img"):
> Ian, saw [you're] back, how do you think of this patch?
I replied to this yesterday.  Did you see my mail ?  Here it is again:

   > >+    else
   > >+        drv = &bdrv_raw;

   This appears to reintroduce the image format vulnerability.  Are you
   sure it's right ?

   The effect of your patch seems to be in this case:
     * the filename does not refer to a block device
     * the format is not specified in the filename
     * the actual contents of the image is not autodetected as any
       image format (eg, qcow or qcow2)
   In that case, without your patch, the open fails.  With your patch,
   the file is opened with bdrv_raw.

Ian.

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

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

Xu, Dongxiao writes ("RE: [Xen-devel][PATCH]ioemu:fix up error when
using qemu-img-xen to create img"):
> 	Thanks for your ack. We think Yang''s patch is right in this
> case. When creating image by command: "qemu-img-xen create -b
> base_image filename -f fmt", if the base_image is in the following
> case as you mentioned:
Thanks for reporting the qemu-img-xen command you were using.  I''ve
reproduced the problem, and I''m about to commit a fix.

The fix makes it impossible to use our qemu-img-xen to create deeper
`nested'' cow formats, where the base file is itself a cow image.
Unfortunately this is needed because the way that qemu-img (upstream)
handles its command line arguments doesn''t permit the specification of
the image format and (as I explain below) it is wrong to have a
situation where the image format is autoprobed but defaults to raw.
I don''t want to carry in our tree the changes necessary to plumb a
format through from the command line.

Xu, Dongxiao writes ("RE: [Xen-devel][PATCH]ioemu:fix up error when
using qemu-img-xen to create img"):
> Ian, could you also help to explain the meaning of "reintroduce the
> image format vulnerability"? Thanks!
Of course.  The problem is as follows:

Consider a raw disk image file which is writeable by a guest.  (This
is of course one very common usage model.)  The guest can write
anything it likes to the image file, including anything to the start
of the file - where the cow header would be if it were a cow file.

So it can, if it likes, write a cow header (qcow2 for example) to the
start of its `virtual disk image''.  Qemu''s cow headers contain
the
pathname of the backing file, and the guest can of course name any
file it likes.

If this image, which is supposedly a raw image, is then opened by any
tool which autoguesses the format, that tool will then spot the cow
header written by the guest and access the backing file (in the
context of the host) specified by the guest.

Depending on the exact circumstances this can allow the guest to get
copies of or even complete read access to any data of its choice in
the host.

Upstream qemu have fixed this problem in a half-hearted way and
evidently their qemu-img is still vulnerable.  We have changed the
format-determination code in block.c so that any attempt to autodetect
a format never returns `raw''; that means that any vulnerable code
anywhere is instantly fixed although it may break some existing usages
in cases where we haven''t properly plumbed through a specification of
the image format.

Ian.

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

On Tue, May 05, 2009 at 03:17:34PM +0100, Ian Jackson
wrote:
> 
> Consider a raw disk image file which is writeable by a guest.  (This
> is of course one very common usage model.)  The guest can write
> anything it likes to the image file, including anything to the start
> of the file - where the cow header would be if it were a cow file.
> 
> So it can, if it likes, write a cow header (qcow2 for example) to the
> start of its `virtual disk image''.  Qemu''s cow headers
contain the
> pathname of the backing file, and the guest can of course name any
> file it likes.
> 
> If this image, which is supposedly a raw image, is then opened by any
> tool which autoguesses the format, that tool will then spot the cow
> header written by the guest and access the backing file (in the
> context of the host) specified by the guest.
> 
> Depending on the exact circumstances this can allow the guest to get
> copies of or even complete read access to any data of its choice in
> the host.
> 
> Upstream qemu have fixed this problem in a half-hearted way and
> evidently their qemu-img is still vulnerable.  We have changed the
> format-determination code in block.c so that any attempt to autodetect
> a format never returns `raw''; that means that any vulnerable code
> anywhere is instantly fixed although it may break some existing usages
> in cases where we haven''t properly plumbed through a specification
of
> the image format.
Wasn''t the upstream change to add a ''-F
baseimage_format'' enough to
allow the flaw to be avoided when creating new images ? Or are you
attempting to prevent the issue, even when -F is not used ?

Daniel
-- 
|: Red Hat, Engineering, London   -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][PATCH]ioemu:fix up error when
using qemu-img-xen to create img"):
> On Tue, May 05, 2009 at 03:17:34PM +0100, Ian Jackson wrote:
> > Upstream qemu have fixed this problem in a half-hearted way and
> > evidently their qemu-img is still vulnerable.  We have changed the
> > format-determination code in block.c so that any attempt to autodetect
> > a format never returns `raw''; that means that any vulnerable
code
> > anywhere is instantly fixed although it may break some existing usages
> > in cases where we haven''t properly plumbed through a
specification of
> > the image format.
> 
> Wasn''t the upstream change to add a ''-F
baseimage_format'' enough to
> allow the flaw to be avoided when creating new images ? Or are you
> attempting to prevent the issue, even when -F is not used ?
Yes, upstream have introduced -F but only in the main branch, and not
the stable branch we''re basing Xen 3.4 on.  So yes, this problem will
go away when I pull after the Xen 3.4 release.

Ian.

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