Xen devel - May 2008 - VMX status report. Xen: #17630 & Xen0: #540 -- blocked

Hi, all

Today''s nightly testing is still blocked for both PAE and 32E, c/s
17606
introduced this issue.

Previous blocker issue (#1249) got fixed.

New issue:
[Bug 1250] HVM guest can not boot with Qcow image
http://bugzilla.xensource.com/bugzilla/show_bug.cgi?id=1250

Fixed issue:
[Bug 1249] failed to create HVM guest on both 32E and PAE
http://bugzilla.xensource.com/bugzilla/show_bug.cgi?id=1249
 
-- haicheng

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

Li, Haicheng writes ("[Xen-devel] VMX status report. Xen: #17630 &
Xen0: #540 -- blocked"):
> Today''s nightly testing is still blocked for both PAE and 32E, c/s
17606
> introduced this issue.
...
> New issue:
> [Bug 1250] HVM guest can not boot with Qcow image
> http://bugzilla.xensource.com/bugzilla/show_bug.cgi?id=1250
I''m afraid you are using the wrong syntax.
file:... is for raw images.

Prior to changeset 17606, the emulated IDE controller would, if the
specified file looked like a qcow image, interpret it that way - but
the PV presentation of the same device (via blktap) to the same guest
would treat it as a raw image.  This (and other related scenarios)
allows a malicious guest to read any file on the host (including for
example the host''s underlying disk devices), because the qcow image
format contains the (host) pathname of the backing file.

In changeset 17606 the behaviour was changed so that file:... always
treats the specified file as a raw disk image.  To use a file
containing a qcow image, you have to say tap:qcow:...

This was discussed here on the list quite recently; feel free to
comment further if you think the fix is the wrong one.

Ian.

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

Ian Jackson wrote:
> Li, Haicheng writes ("[Xen-devel] VMX status report. Xen: #17630 &
> Xen0: #540 -- blocked"): 
>> Today''s nightly testing is still blocked for both PAE and 32E,
c/s
>> 17606 introduced this issue.
> ...
>> New issue:
>> [Bug 1250] HVM guest can not boot with Qcow image
>> http://bugzilla.xensource.com/bugzilla/show_bug.cgi?id=1250
> 
> I''m afraid you are using the wrong syntax.
> file:... is for raw images.
> 
> Prior to changeset 17606, the emulated IDE controller would, if the
> specified file looked like a qcow image, interpret it that way - but
> the PV presentation of the same device (via blktap) to the same guest
> would treat it as a raw image.  This (and other related scenarios)
> allows a malicious guest to read any file on the host (including for
> example the host''s underlying disk devices), because the qcow
image
> format contains the (host) pathname of the backing file.
> 
> In changeset 17606 the behaviour was changed so that file:... always
> treats the specified file as a raw disk image.  To use a file
> containing a qcow image, you have to say tap:qcow:...
> 
> This was discussed here on the list quite recently; feel free to
> comment further if you think the fix is the wrong one.
> 
> Ian.
Ian, 

even with syntax like "tap:qcow:" as following, it still
doesn''t work,
guest bios shows the size of this disk is 0 MB. 
	disk = [
''tap:qcow:/mnt/sda5/hli22/xen-test/manual-test/xpsp2_smp_ia32.qcow,hda,w
'']



Qemu log is pasted here:

domid: 5
qemu: the number of cpus is 4
Strip off blktap sub-type prefix to
/mnt/sda5/hli22/xen-test/manual-test/xpsp2_smp_ia32.qcow (drv
''qcow'')
qemu: could not open vbd
''/local/domain/5/device/vbd/768/phantom_vbd'' or
hard disk image
''/mnt/sda5/hli22/xen-test/manual-test/xpsp2_smp_ia32.qcow''
(drv ''qcow'')
Watching /local/domain/0/device-model/5/logdirty/next-active
Watching /local/domain/0/device-model/5/command
char device redirected to /dev/pts/5
qemu_map_cache_init nr_buckets = 4000 size 196608
shared page at pfn 3fffe
buffered io page at pfn 3fffc
Time offset set 0
Register xen platform.
Done register platform.
I/O request not ready: 0, ptr: 0, port: 0, data: 0, count: 0, size: 0
I/O request not ready: 0, ptr: 0, port: 0, data: 0, count: 0, size: 0
I/O request not ready: 0, ptr: 0, port: 0, data: 0, count: 0, size: 0
I/O request not ready: 0, ptr: 0, port: 0, data: 0, count: 0, size: 0

-- haicheng

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

I just looked into this issue, and I was confused by the recursive function call
while opening a qcow file
(bdrv_open2->qcow_open->bdrv_file_open->bdrv_open2->...).
    Remove the modification in block.c in C/S 17606 could work around this
issue, but it is not a good way.

Best regards,
-- Dongxiao

-----Original Message-----
From: xen-devel-bounces@lists.xensource.com
[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Li, Haicheng
Sent: 2008年5月15日 15:40
To: Ian Jackson
Cc: xen-devel@lists.xensource.com
Subject: RE: [Xen-devel] VMX status report. Xen: #17630 & Xen0: #540 --
blocked

Ian Jackson wrote:
> Li, Haicheng writes ("[Xen-devel] VMX status report. Xen: #17630 &
> Xen0: #540 -- blocked"): 
>> Today''s nightly testing is still blocked for both PAE and 32E,
c/s
>> 17606 introduced this issue.
> ...
>> New issue:
>> [Bug 1250] HVM guest can not boot with Qcow image
>> http://bugzilla.xensource.com/bugzilla/show_bug.cgi?id=1250
> 
> I''m afraid you are using the wrong syntax.
> file:... is for raw images.
> 
> Prior to changeset 17606, the emulated IDE controller would, if the
> specified file looked like a qcow image, interpret it that way - but
> the PV presentation of the same device (via blktap) to the same guest
> would treat it as a raw image.  This (and other related scenarios)
> allows a malicious guest to read any file on the host (including for
> example the host''s underlying disk devices), because the qcow
image
> format contains the (host) pathname of the backing file.
> 
> In changeset 17606 the behaviour was changed so that file:... always
> treats the specified file as a raw disk image.  To use a file
> containing a qcow image, you have to say tap:qcow:...
> 
> This was discussed here on the list quite recently; feel free to
> comment further if you think the fix is the wrong one.
> 
> Ian.
Ian, 

even with syntax like "tap:qcow:" as following, it still
doesn''t work,
guest bios shows the size of this disk is 0 MB. 
	disk = [
''tap:qcow:/mnt/sda5/hli22/xen-test/manual-test/xpsp2_smp_ia32.qcow,hda,w
'']



Qemu log is pasted here:

domid: 5
qemu: the number of cpus is 4
Strip off blktap sub-type prefix to
/mnt/sda5/hli22/xen-test/manual-test/xpsp2_smp_ia32.qcow (drv
''qcow'')
qemu: could not open vbd
''/local/domain/5/device/vbd/768/phantom_vbd'' or
hard disk image
''/mnt/sda5/hli22/xen-test/manual-test/xpsp2_smp_ia32.qcow''
(drv ''qcow'')
Watching /local/domain/0/device-model/5/logdirty/next-active
Watching /local/domain/0/device-model/5/command
char device redirected to /dev/pts/5
qemu_map_cache_init nr_buckets = 4000 size 196608
shared page at pfn 3fffe
buffered io page at pfn 3fffc
Time offset set 0
Register xen platform.
Done register platform.
I/O request not ready: 0, ptr: 0, port: 0, data: 0, count: 0, size: 0
I/O request not ready: 0, ptr: 0, port: 0, data: 0, count: 0, size: 0
I/O request not ready: 0, ptr: 0, port: 0, data: 0, count: 0, size: 0
I/O request not ready: 0, ptr: 0, port: 0, data: 0, count: 0, size: 0

-- haicheng

_______________________________________________
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 schrieb:
> I just looked into this issue, and I was confused by the recursive function
call while opening a qcow file
(bdrv_open2->qcow_open->bdrv_file_open->bdrv_open2->...).
>     Remove the modification in block.c in C/S 17606 could work around this
issue, but it is not a good way.
I think the right solution is to remove bdrv_open altogether and force
the callers to provide the driver as parameter to bdrv_open2. For qcow
itself the following patch could be enough (compiles, but completely
untested).

But then, I don''t even understand why find_protocol has been crippled -
isn''t this one harmless and we should rather have removed the actual
guessing in find_image_format?

Kevin


diff -r 0beee5c839ea tools/ioemu/block.c
--- a/tools/ioemu/block.c       Tue May 13 12:17:08 2008
+++ b/tools/ioemu/block.c       Thu May 15 11:51:15 2008
@@ -329,7 +329,7 @@
     bs = bdrv_new("");
     if (!bs)
         return -ENOMEM;
-    ret = bdrv_open2(bs, filename, flags | BDRV_O_FILE, NULL);
+    ret = bdrv_open2(bs, filename, flags | BDRV_O_FILE, &bdrv_raw);
     if (ret < 0) {
         bdrv_delete(bs);
         return ret;

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

I just tried your patch, and seems that it doesn''t work.
Actually, find_protocol() could not find out the qcow protocol, and for qcow
image, that function will always return NULL after C/S 17606. I think this is
the root cause why qcow image could not be booted.

-- Dongxiao

-----Original Message-----
From: Kevin Wolf [mailto:kwolf@suse.de] 
Sent: 2008年5月15日 17:01
To: Xu, Dongxiao
Cc: Li, Haicheng; Ian Jackson; xen-devel@lists.xensource.com
Subject: Re: [Xen-devel] VMX status report. Xen: #17630 & Xen0: #540 --
blocked

Xu, Dongxiao schrieb:
> I just looked into this issue, and I was confused by the recursive function
call while opening a qcow file
(bdrv_open2->qcow_open->bdrv_file_open->bdrv_open2->...).
>     Remove the modification in block.c in C/S 17606 could work around this
issue, but it is not a good way.
I think the right solution is to remove bdrv_open altogether and force
the callers to provide the driver as parameter to bdrv_open2. For qcow
itself the following patch could be enough (compiles, but completely
untested).

But then, I don''t even understand why find_protocol has been crippled -
isn''t this one harmless and we should rather have removed the actual
guessing in find_image_format?

Kevin


diff -r 0beee5c839ea tools/ioemu/block.c
--- a/tools/ioemu/block.c       Tue May 13 12:17:08 2008
+++ b/tools/ioemu/block.c       Thu May 15 11:51:15 2008
@@ -329,7 +329,7 @@
     bs = bdrv_new("");
     if (!bs)
         return -ENOMEM;
-    ret = bdrv_open2(bs, filename, flags | BDRV_O_FILE, NULL);
+    ret = bdrv_open2(bs, filename, flags | BDRV_O_FILE, &bdrv_raw);
     if (ret < 0) {
         bdrv_delete(bs);
         return ret;

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

Xu, Dongxiao schrieb:
> I just tried your patch, and seems that it doesn''t work.
> Actually, find_protocol() could not find out the qcow protocol, and for
qcow image, that function will always return NULL after C/S 17606. I think this
is the root cause why qcow image could not be booted.
find_protocol isn''t even responsible for finding out that it''s
qcow.
There is a difference between image format and protocol (IIRC, the
latter is used in qemu for things like vvfat which maps a host directory
to a virtual FAT floppy/harddisk). I suspect the problem arises from the
bdrv_file_open call in find_image_format which won''t correctly get
&bdrv_raw but NULL as driver.

And you''re right, the patch can''t work. I missed that in
bdrv_open2 for
BDRV_O_FILE drv is ignored and always overwritten.

Kevin

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

Could those of you having this problem please try the attached patch ?
I have tested this one much more thoroughly :-/.

Ian.



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

Ian Jackson schrieb:
 > Could those of you having this problem please try the attached patch ?
 > I have tested this one much more thoroughly :-/.

It works for me, I don''t like it though. Maybe I don''t
understand
correctly what the intent of the whole protocol stuff was. But if I do, 
I think you''re destroying this with your patch. This is not necessarily
bad as we don''t use it anyway. But then it would be much cleaner to 
remove the functionality altogether.

To be a bit more concrete, I think the following change is wrong (even 
if there is no user anyway):

-    /* no need to test disk image formats for vvfat */
-    if (drv == &bdrv_vvfat)
+    /* no need to test disk image format if the filename told us */
+    if (drv != NULL)
          return drv;

find_protocol doesn''t tell you the image format of a file, it tells you
a protocol through which you should obtain the image. And the image you 
get could be a qcow image then. I think the reason why they are using 
bdrv functions for all file accesses is that e.g. the qcow driver could 
use the protocol driver then (if there was any protocol driver...)

Currently xend won''t let you specify a filename with a protocol 
specifier anyway, so removing the whole thing is certainly an option.

Kevin

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

Kevin Wolf writes ("Re: [Xen-devel] VMX status report. Xen: #17630 &
Xen0: #540 -- blocked"):
> Ian Jackson schrieb:
>  > Could those of you having this problem please try the attached patch
?
>  > I have tested this one much more thoroughly :-/.
> 
> It works for me, I don''t like it though. Maybe I don''t
understand
> correctly what the intent of the whole protocol stuff was. But if I do, 
> I think you''re destroying this with your patch. This is not
necessarily
> bad as we don''t use it anyway. But then it would be much cleaner
to
> remove the functionality altogether.
I think you''ve misunderstood, but I could be wrong.  To me it seems
that Qemu uses the terms `protocol'' and `format'' almost
interchangeably - I don''t think they refer to different things.

find_protocol, find_format and find_image_format all select from the
same set of bdrv_xxx drivers: find_format take a name and returns the
driver with that name; find_protocol takes a filename and looks for a
`:'' in it, and if so it assumes that the part before the `:''
is the
name and finds the driver with that name.  (Just for extra confusion
the two namespaces are different and some drivers don''t have a name
that can be put in filenames, but some drivers have both names.)
find_image_format reads the start of the file and passes it to drivers
until one of them recognises it.
> To be a bit more concrete, I think the following change is wrong (even 
> if there is no user anyway):
> 
> -    /* no need to test disk image formats for vvfat */
> -    if (drv == &bdrv_vvfat)
> +    /* no need to test disk image format if the filename told us */
> +    if (drv != NULL)
>           return drv;
> 
> find_protocol doesn''t tell you the image format of a file, it
tells you
> a protocol through which you should obtain the image. And the image you 
> get could be a qcow image then.
These `protocols'' aren''t network protocols, they''re
disk image
formats.  The only ones which aren''t a disk image format are vvfat and
(in ioemu) vbd.

vvfat is a crazy thing which you can''t currently sanely layer anything
on top of (even though you might want to) and for which layering
a block driver underneath makes no sense.

vbd is a mistake; it should be minios''s block_raw.
>   I think the reason why they are using 
> bdrv functions for all file accesses is that e.g. the qcow driver could 
> use the protocol driver then (if there was any protocol driver...)
The reason they''re using bdrv functions is so that they can go through
the bdrv_raw driver for backing file and image file accesses; this is
necessary to preserve the properties of the block IO abstraction
through the various layers.

Ian.

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

Ian Jackson schrieb:
> Kevin Wolf writes ("Re: [Xen-devel] VMX status report. Xen: #17630
& Xen0: #540 -- blocked"):
>> Ian Jackson schrieb:
>>  > Could those of you having this problem please try the attached
patch ?
>>  > I have tested this one much more thoroughly :-/.
>>
>> It works for me, I don''t like it though. Maybe I
don''t understand
>> correctly what the intent of the whole protocol stuff was. But if I do,
>> I think you''re destroying this with your patch. This is not
necessarily
>> bad as we don''t use it anyway. But then it would be much
cleaner to
>> remove the functionality altogether.
> 
> I think you''ve misunderstood, but I could be wrong.  To me it
seems
> that Qemu uses the terms `protocol'' and `format'' almost
> interchangeably - I don''t think they refer to different things.
I certainly don''t want to exclude that I''ve misunderstood. But
if it was
meant to select the image format, the code looks overcomplicated to me.

And now that upstream qemu has a -drive parameter with a format option, 
we could get rid of this filename parsing in both ioemu and upstream 
qemu then. In Xen we don''t use it anyway.
> find_protocol, find_format and find_image_format all select from the
> same set of bdrv_xxx drivers: find_format take a name and returns the
> driver with that name; find_protocol takes a filename and looks for a
> `:'' in it, and if so it assumes that the part before the
`:'' is the
> name and finds the driver with that name.  (Just for extra confusion
> the two namespaces are different and some drivers don''t have a
name
> that can be put in filenames, but some drivers have both names.)
> find_image_format reads the start of the file and passes it to drivers
> until one of them recognises it.
I think I understand quite well how it''s processed in the code. ;-) And
I also think that the code allows both of our understandings. If you 
wanted, I think you could add a "protocol" which in fact acts like a 
network protocol.

And the different namespaces still don''t make too much sense to me if 
formats and protocols are really meant to be the same...
>> To be a bit more concrete, I think the following change is wrong (even 
>> if there is no user anyway):
>>
>> -    /* no need to test disk image formats for vvfat */
>> -    if (drv == &bdrv_vvfat)
>> +    /* no need to test disk image format if the filename told us */
>> +    if (drv != NULL)
>>           return drv;
>>
>> find_protocol doesn''t tell you the image format of a file, it
tells you
>> a protocol through which you should obtain the image. And the image you
>> get could be a qcow image then.
> 
> These `protocols'' aren''t network protocols,
they''re disk image
> formats.  The only ones which aren''t a disk image format are vvfat
and
> (in ioemu) vbd.
> 
> vvfat is a crazy thing which you can''t currently sanely layer
anything
> on top of (even though you might want to) and for which layering
> a block driver underneath makes no sense.
I agree that vvfat isn''t really a protocol as I understand them.
It''s
only another block driver and it''s using the protocol thing only
because
there was no format option qemu until recently. Maybe you''re right and 
the "protocol" was only introduced as a hack to allow overriding the 
image format.

Kevin

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

Hi, Ian,
    In your new patch, if find_protocol() function returns NULL, then you will
let drv = &bdrv_raw, it is just guessing the image to be raw, I remember you
mentioned it as a security problem. So I don''t find any difference as
the following one, it just reverts the change in block.c in C/S 17606.

diff -r 86587698116d tools/ioemu/block.c
--- a/tools/ioemu/block.c       Wed May 14 14:12:53 2008 +0100
+++ b/tools/ioemu/block.c       Fri May 16 09:24:44 2008 +0800
@@ -254,7 +254,7 @@ static BlockDriver *find_protocol(const
 #endif
     p = strchr(filename, '':'');
     if (!p)
-        return NULL; /* do not ever guess raw, it is a security problem! */
+        return &bdrv_raw;
     len = p - filename;
     if (len > sizeof(protocol) - 1)
         len = sizeof(protocol) - 1;



-----Original Message-----
From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com] 
Sent: 2008年5月15日 19:08
To: Kevin Wolf
Cc: Xu, Dongxiao; Li, Haicheng; xen-devel@lists.xensource.com
Subject: Re: [Xen-devel] VMX status report. Xen: #17630 & Xen0: #540 --
blocked

Could those of you having this problem please try the attached patch ?
I have tested this one much more thoroughly :-/.

Ian.


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

Xu, Dongxiao writes ("RE: [Xen-devel] VMX status report. Xen: #17630
&
> Xen0: #540 -- blocked"):
>     In your new patch, if find_protocol() function returns NULL,
> then you will let drv = &bdrv_raw, it is just guessing the image to
> be raw, I remember you mentioned it as a security problem. So I
> don''t find any difference as the following one, it just reverts
the
> change in block.c in C/S 17606.
Yes, I did revert that specific change; it''s dealt with by
find_protocol''s caller, instead.

I''ve also tested that  tap:qcow:/path/to/raw/image.iso  does not work.

Ian.

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