[PATCH] ioemu block device extent checks

[PATCH] ioemu block device extent checks

[Ian Jackson]

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

When a block device read or write request is made by an HVM guest,
nothing checks that the request is within the range supported by the
block backend driver in ioemu, but the code in the backend driver
typically assumes that the request is sensible.

Depending on the backend, this can allow the guest to read and write
arbitrary memory locations in qemu, and possibly gain control over the
qemu process, escaping from the virtualisation.


I have demonstrated to my own satisfaction that there is problem,
using a modified Linux kernel as a guest with an instrumented CVS head
qemu.  I haven't yet reproduced the bug with xen-unstable but I think
it's almost certainly there too.  I have prepared a patch which I have
checked prevents my test case, and adjusted it to fit and compile
against xen-unstable.  I'm subjecting it to some testing as I write.

I contacted privately five of the main qemu developers but the only
response was from Andrzej Zaborowski who didn't consider the problem
serious, saying
  Qemu never claimed to be secure.  Of course it's better to
  be secure than not if it doesn't add a bad overhead.
and suggesting that I should just post to the qemu-devel mailing list.

Ian.

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


[-- Attachment #2: ioemu check block request extents --]
[-- Type: text/plain, Size: 3740 bytes --]

diff -r 8848d9e07584 tools/ioemu/block.c
--- a/tools/ioemu/block.c	Mon Feb 18 21:26:57 2008 +0000
+++ b/tools/ioemu/block.c	Tue Feb 19 15:28:58 2008 +0000
@@ -120,6 +120,24 @@ void path_combine(char *dest, int dest_s
     }
 }
 
+static int bdrv_rw_badreq_sectors(BlockDriverState *bs,
+				int64_t sector_num, int nb_sectors)
+{
+    return
+	nb_sectors < 0 ||
+	nb_sectors > bs->total_sectors ||
+	sector_num > bs->total_sectors - nb_sectors;
+}
+
+static int bdrv_rw_badreq_bytes(BlockDriverState *bs,
+				  int64_t offset, int count)
+{
+    int64_t size = bs->total_sectors << SECTOR_BITS;
+    return
+	count < 0 ||
+	count > size ||
+	offset > size - count;
+}
 
 void bdrv_register(BlockDriver *bdrv)
 {
@@ -372,6 +390,7 @@ int bdrv_open2(BlockDriverState *bs, con
     }
     bs->drv = drv;
     bs->opaque = qemu_mallocz(drv->instance_size);
+    bs->total_sectors = 0; /* driver will set if it does not do getlength */
     if (bs->opaque == NULL && drv->instance_size > 0)
         return -1;
     /* Note: for compatibility, we open disk image files as RDWR, and
@@ -437,6 +456,7 @@ void bdrv_close(BlockDriverState *bs)
         bs->drv = NULL;
 
         /* call the change callback */
+	bs->total_sectors = 0;
         bs->media_changed = 1;
         if (bs->change_cb)
             bs->change_cb(bs->change_opaque);
@@ -502,9 +522,8 @@ int bdrv_read(BlockDriverState *bs, int6
     if (!drv)
         return -ENOMEDIUM;
 
-    if (sector_num < 0)
-	return -EINVAL;
-
+    if (bdrv_rw_badreq_sectors(bs, sector_num, nb_sectors))
+	return -EDOM;
     if (sector_num == 0 && bs->boot_sector_enabled && nb_sectors > 0) {
             memcpy(buf, bs->boot_sector_data, 512);
         sector_num++;
@@ -542,8 +561,8 @@ int bdrv_write(BlockDriverState *bs, int
         return -ENOMEDIUM;
     if (bs->read_only)
         return -EACCES;
-    if (sector_num < 0)
-	return -EINVAL;
+    if (bdrv_rw_badreq_sectors(bs, sector_num, nb_sectors))
+	return -EDOM;
     if (sector_num == 0 && bs->boot_sector_enabled && nb_sectors > 0) {
         memcpy(bs->boot_sector_data, buf, 512);   
     }
@@ -666,6 +685,8 @@ int bdrv_pread(BlockDriverState *bs, int
         return -ENOMEDIUM;
     if (!drv->bdrv_pread)
         return bdrv_pread_em(bs, offset, buf1, count1);
+    if (bdrv_rw_badreq_bytes(bs, offset, count1))
+	return -EDOM;
     return drv->bdrv_pread(bs, offset, buf1, count1);
 }
 
@@ -681,6 +702,8 @@ int bdrv_pwrite(BlockDriverState *bs, in
         return -ENOMEDIUM;
     if (!drv->bdrv_pwrite)
         return bdrv_pwrite_em(bs, offset, buf1, count1);
+    if (bdrv_rw_badreq_bytes(bs, offset, count1))
+	return -EDOM;
     return drv->bdrv_pwrite(bs, offset, buf1, count1);
 }
 
@@ -922,6 +945,8 @@ int bdrv_write_compressed(BlockDriverSta
         return -ENOMEDIUM;
     if (!drv->bdrv_write_compressed)
         return -ENOTSUP;
+    if (bdrv_rw_badreq_sectors(bs, sector_num, nb_sectors))
+	return -EDOM;
     return drv->bdrv_write_compressed(bs, sector_num, buf, nb_sectors);
 }
     
@@ -1067,7 +1092,9 @@ BlockDriverAIOCB *bdrv_aio_read(BlockDri
 
     if (!drv)
         return NULL;
-    
+    if (bdrv_rw_badreq_sectors(bs, sector_num, nb_sectors))
+	return NULL;
+
     /* XXX: we assume that nb_sectors == 0 is suppored by the async read */
     if (sector_num == 0 && bs->boot_sector_enabled && nb_sectors > 0) {
         memcpy(buf, bs->boot_sector_data, 512);
@@ -1089,6 +1116,8 @@ BlockDriverAIOCB *bdrv_aio_write(BlockDr
         return NULL;
     if (bs->read_only)
         return NULL;
+    if (bdrv_rw_badreq_sectors(bs, sector_num, nb_sectors))
+	return NULL;
     if (sector_num == 0 && bs->boot_sector_enabled && nb_sectors > 0) {
         memcpy(bs->boot_sector_data, buf, 512);   
     }

[-- 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] ioemu block device extent checks

[Daniel P. Berrange]

On Tue, Feb 19, 2008 at 04:38:24PM +0000, Ian Jackson wrote:
Content-Description: message body text
> When a block device read or write request is made by an HVM guest,
> nothing checks that the request is within the range supported by the
> block backend driver in ioemu, but the code in the backend driver
> typically assumes that the request is sensible.
> 
> Depending on the backend, this can allow the guest to read and write
> arbitrary memory locations in qemu, and possibly gain control over the
> qemu process, escaping from the virtualisation.
> 
> 
> I have demonstrated to my own satisfaction that there is problem,
> using a modified Linux kernel as a guest with an instrumented CVS head
> qemu.  I haven't yet reproduced the bug with xen-unstable but I think
> it's almost certainly there too.  I have prepared a patch which I have
> checked prevents my test case, and adjusted it to fit and compile
> against xen-unstable.  I'm subjecting it to some testing as I write.

FYI, this patch causes massive unrecoverable data loss / corruption on
QCow2 files. The checks themselves are OK in terms of the first level
of bdrv_* calls from the guest. The qcow driver though calls back into
the raw driver for performing I/O on its underlying file. The qcow 
driver relies on this file being grow-on-demand for purposes of allocating
new qcow sectors. The safety checks cause this allocation to fail and
it all goes downhill from there :-(  

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

Re: [PATCH] ioemu block device extent checks

[Ian Jackson]

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

Daniel P. Berrange writes ("Re: [Xen-devel] [PATCH] ioemu block device extent checks"):
>    The qcow driver though calls back into
> the raw driver for performing I/O on its underlying file. The qcow 
> driver relies on this file being grow-on-demand for purposes of allocating
> new qcow sectors. The safety checks cause this allocation to fail and
> it all goes downhill from there :-(  

Oh dear.  (I'm a bit surprised that it's taken this long to spot!)
Here is a patch for xen-unstable which I think will fix it.  Could you
give it a quick spin, if you have a suitable test setup ?

Sadly it's rather more intrusive than ideal, since it needs all of the
drivers which are going to extend files via their parents to announce
this, and a couple of bits of necessary infrastructure needed adding.

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


[-- Attachment #2: block range checking, fix for cow extension --]
[-- Type: text/plain, Size: 4782 bytes --]

diff -r 757cd7bb5e35 tools/ioemu/block-qcow.c
--- a/tools/ioemu/block-qcow.c	Fri Feb 22 16:49:56 2008 +0000
+++ b/tools/ioemu/block-qcow.c	Wed Feb 27 11:09:56 2008 +0000
@@ -95,7 +95,7 @@ static int qcow_open(BlockDriverState *b
     int len, i, shift, ret;
     QCowHeader header;
 
-    ret = bdrv_file_open(&s->hd, filename, flags);
+    ret = bdrv_file_open(&s->hd, filename, flags | BDRV_O_EXTENDABLE);
     if (ret < 0)
         return ret;
     if (bdrv_pread(s->hd, 0, &header, sizeof(header)) != sizeof(header))
diff -r 757cd7bb5e35 tools/ioemu/block-qcow2.c
--- a/tools/ioemu/block-qcow2.c	Fri Feb 22 16:49:56 2008 +0000
+++ b/tools/ioemu/block-qcow2.c	Wed Feb 27 11:09:56 2008 +0000
@@ -191,7 +191,7 @@ static int qcow_open(BlockDriverState *b
     int len, i, shift, ret;
     QCowHeader header;
 
-    ret = bdrv_file_open(&s->hd, filename, flags);
+    ret = bdrv_file_open(&s->hd, filename, flags | BDRV_O_EXTENDABLE);
     if (ret < 0)
         return ret;
     if (bdrv_pread(s->hd, 0, &header, sizeof(header)) != sizeof(header))
diff -r 757cd7bb5e35 tools/ioemu/block-raw.c
--- a/tools/ioemu/block-raw.c	Fri Feb 22 16:49:56 2008 +0000
+++ b/tools/ioemu/block-raw.c	Wed Feb 27 11:12:28 2008 +0000
@@ -1489,5 +1489,7 @@ BlockDriver bdrv_host_device = {
     .bdrv_pread = raw_pread,
     .bdrv_pwrite = raw_pwrite,
     .bdrv_getlength = raw_getlength,
+
+    .bdrv_flags = BLOCK_DRIVER_FLAG_EXTENDABLE
 };
 #endif /* _WIN32 */
diff -r 757cd7bb5e35 tools/ioemu/block-vmdk.c
--- a/tools/ioemu/block-vmdk.c	Fri Feb 22 16:49:56 2008 +0000
+++ b/tools/ioemu/block-vmdk.c	Wed Feb 27 11:10:02 2008 +0000
@@ -352,7 +352,7 @@ static int vmdk_open(BlockDriverState *b
     uint32_t magic;
     int l1_size, i, ret;
 
-    ret = bdrv_file_open(&s->hd, filename, flags);
+    ret = bdrv_file_open(&s->hd, filename, flags | BDRV_O_EXTENDABLE);
     if (ret < 0)
         return ret;
     if (bdrv_pread(s->hd, 0, &magic, sizeof(magic)) != sizeof(magic))
diff -r 757cd7bb5e35 tools/ioemu/block.c
--- a/tools/ioemu/block.c	Fri Feb 22 16:49:56 2008 +0000
+++ b/tools/ioemu/block.c	Wed Feb 27 11:13:57 2008 +0000
@@ -123,20 +123,23 @@ static int bdrv_rw_badreq_sectors(BlockD
 static int bdrv_rw_badreq_sectors(BlockDriverState *bs,
 				int64_t sector_num, int nb_sectors)
 {
-    return
+    return (
 	nb_sectors < 0 ||
 	nb_sectors > bs->total_sectors ||
-	sector_num > bs->total_sectors - nb_sectors;
+	sector_num > bs->total_sectors - nb_sectors
+	) && !bs->extendable;
 }
 
 static int bdrv_rw_badreq_bytes(BlockDriverState *bs,
 				  int64_t offset, int count)
 {
     int64_t size = bs->total_sectors << SECTOR_BITS;
-    return
+    return (
 	count < 0 ||
 	count > size ||
-	offset > size - count;
+	offset > size - count
+	) && !bs->extendable;
+    
 }
 
 void bdrv_register(BlockDriver *bdrv)
@@ -347,6 +350,12 @@ int bdrv_open2(BlockDriverState *bs, con
     bs->is_temporary = 0;
     bs->encrypted = 0;
 
+    if (flags & BDRV_O_EXTENDABLE) {
+	if (!(drv->bdrv_flags & BLOCK_DRIVER_FLAG_EXTENDABLE))
+	    return -ENOSYS;
+	bs->extendable = 1;
+    }
+
     if (flags & BDRV_O_SNAPSHOT) {
         BlockDriverState *bs1;
         int64_t total_size;
diff -r 757cd7bb5e35 tools/ioemu/block_int.h
--- a/tools/ioemu/block_int.h	Fri Feb 22 16:49:56 2008 +0000
+++ b/tools/ioemu/block_int.h	Wed Feb 27 11:11:39 2008 +0000
@@ -23,6 +23,8 @@
  */
 #ifndef BLOCK_INT_H
 #define BLOCK_INT_H
+
+#define BLOCK_DRIVER_FLAG_EXTENDABLE  0x0001u
 
 struct BlockDriver {
     const char *format_name;
@@ -76,6 +78,7 @@ struct BlockDriver {
     int (*bdrv_eject)(BlockDriverState *bs, int eject_flag);
     int (*bdrv_set_locked)(BlockDriverState *bs, int locked);
     
+    unsigned bdrv_flags;
     BlockDriverAIOCB *free_aiocb;
     struct BlockDriver *next;
 };
@@ -87,6 +90,7 @@ struct BlockDriverState {
     int removable; /* if true, the media can be removed */
     int locked;    /* if true, the media cannot temporarily be ejected */
     int encrypted; /* if true, the media is encrypted */
+    int extendable;/* if true, we may write out of original range */
     /* event callback when inserting/removing */
     void (*change_cb)(void *opaque);
     void *change_opaque;
diff -r 757cd7bb5e35 tools/ioemu/vl.h
--- a/tools/ioemu/vl.h	Fri Feb 22 16:49:56 2008 +0000
+++ b/tools/ioemu/vl.h	Wed Feb 27 11:12:03 2008 +0000
@@ -614,6 +614,8 @@ typedef struct QEMUSnapshotInfo {
                                      use a disk image format on top of
                                      it (default for
                                      bdrv_file_open()) */
+#define BDRV_O_EXTENDABLE  0x0080 /* allow writes out of original size range;
+				     only effective for some drivers */
 
 void bdrv_init(void);
 BlockDriver *bdrv_find_format(const char *format_name);

[-- 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] ioemu block device extent checks

[Daniel P. Berrange]

On Wed, Feb 27, 2008 at 11:28:05AM +0000, Ian Jackson wrote:
Content-Description: message body text
> Daniel P. Berrange writes ("Re: [Xen-devel] [PATCH] ioemu block device extent checks"):
> >    The qcow driver though calls back into
> > the raw driver for performing I/O on its underlying file. The qcow 
> > driver relies on this file being grow-on-demand for purposes of allocating
> > new qcow sectors. The safety checks cause this allocation to fail and
> > it all goes downhill from there :-(  
> 
> Oh dear.  (I'm a bit surprised that it's taken this long to spot!)
> Here is a patch for xen-unstable which I think will fix it.  Could you
> give it a quick spin, if you have a suitable test setup ?
> 
> Sadly it's rather more intrusive than ideal, since it needs all of the
> drivers which are going to extend files via their parents to announce
> this, and a couple of bits of necessary infrastructure needed adding.

I don't think this is correct - it allows a -ve  size / nb_sectors
value when autoextenable is set, and allows out of bounds reads.

I sent a patch to qemu-devel yuesterday which also uses the auto-extend
flag, but has separate checks for read vs writes. When doing a write that
would extend the device it increases the total_sectors count so that the
subsequent reads can be validated to be within the written bounds.

http://lists.gnu.org/archive/html/qemu-devel/2008-02/msg00497.html

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

Re: [PATCH] ioemu block device extent checks

[Ian Jackson]

Daniel P. Berrange writes ("Re: [Xen-devel] [PATCH] ioemu block device extent checks"):
> I don't think this is correct - it allows a -ve  size / nb_sectors
> value when autoextenable is set, and allows out of bounds reads.

That's fine because it's only called like that as a the parent block
driver.  Perhaps a different name would have been better.

Out of bounds reads have to be permitted because the block size in the
header may remain un-updated, so reads of newly-cloned blocks may
fail.

> I sent a patch to qemu-devel yuesterday which also uses the auto-extend
> flag, but has separate checks for read vs writes. When doing a write that
> would extend the device it increases the total_sectors count so that the
> subsequent reads can be validated to be within the written bounds.

Well, that seems like makework to me but fine if upstream accept it.

Ian.

Re: [PATCH] ioemu block device extent checks

[Daniel P. Berrange]

On Wed, Feb 27, 2008 at 01:14:16PM +0000, Ian Jackson wrote:
> Daniel P. Berrange writes ("Re: [Xen-devel] [PATCH] ioemu block device extent checks"):
> > I don't think this is correct - it allows a -ve  size / nb_sectors
> > value when autoextenable is set, and allows out of bounds reads.
> 
> That's fine because it's only called like that as a the parent block
> driver.  Perhaps a different name would have been better.
> 
> Out of bounds reads have to be permitted because the block size in the
> header may remain un-updated, so reads of newly-cloned blocks may
> fail.

Which is why I updated the the total_sectors count during writes...

> 
> > I sent a patch to qemu-devel yuesterday which also uses the auto-extend
> > flag, but has separate checks for read vs writes. When doing a write that
> > would extend the device it increases the total_sectors count so that the
> > subsequent reads can be validated to be within the written bounds.
> 
> Well, that seems like makework to me but fine if upstream accept it.

It could help prevent future bugs if some new code is written which is
expecting the total_sectors value to be correct - admitedly the existing
code has incorrect total_sectors count already, but I figure its worth
fixing this to avoid unexpected surprises in the future.

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

Re: [PATCH] ioemu block device extent checks

[Keir Fraser]

On 4/3/08 10:16, "Kevin Wolf" <kwolf@suse.de> wrote:

> Daniel P. Berrange schrieb:
>> FYI, this patch causes massive unrecoverable data loss / corruption on
>> QCow2 files. The checks themselves are OK in terms of the first level
>> of bdrv_* calls from the guest. The qcow driver though calls back into
>> the raw driver for performing I/O on its underlying file. The qcow
>> driver relies on this file being grow-on-demand for purposes of allocating
>> new qcow sectors. The safety checks cause this allocation to fail and
>> it all goes downhill from there :-(
> 
> I just hit this problem. What's the status?  If I understand correctly,
> you discussed two different solutions and as a result no fix at all got
> committed.

Current xen-unstable tip should have it fixed (by a combination of
changesets 17133 and 17177).

 -- Keir

Re: [PATCH] ioemu block device extent checks

[Kevin Wolf]

Daniel P. Berrange schrieb:
> FYI, this patch causes massive unrecoverable data loss / corruption on
> QCow2 files. The checks themselves are OK in terms of the first level
> of bdrv_* calls from the guest. The qcow driver though calls back into
> the raw driver for performing I/O on its underlying file. The qcow 
> driver relies on this file being grow-on-demand for purposes of allocating
> new qcow sectors. The safety checks cause this allocation to fail and
> it all goes downhill from there :-(  

I just hit this problem. What's the status?  If I understand correctly,
you discussed two different solutions and as a result no fix at all got
committed.

Kevin

Re: [PATCH] ioemu block device extent checks

[Kevin Wolf]

Keir Fraser schrieb:
> On 4/3/08 10:16, "Kevin Wolf" <kwolf@suse.de> wrote:
>> I just hit this problem. What's the status?  If I understand correctly,
>> you discussed two different solutions and as a result no fix at all got
>> committed.
> 
> Current xen-unstable tip should have it fixed (by a combination of
> changesets 17133 and 17177).
> 
>  -- Keir

Oh, right... Sorry for the noise.

Kevin