[Qemu-devel] Re: qemu unchecked block read/write vulnerability

[Qemu-devel] Re: qemu unchecked block read/write vulnerability

[Ian Jackson]

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

I was doing some merging of qemu and I noticed that the block driver
backends don't check the guest's read/write attempts against the
nominal size of the block device.

I haven't checked all of the backends but I have verified the bug with
block-cow.c, which I have in my test induced to set a bitmap bit at an
address which is not actually part of the bitmap.  In my tests I used
as my guest a Linux kernel which I'd specially modifed to allow me to
access out-of-range blocks.

I think the fix is probably to insert a couple of range checks in the
generic block dispatch layer and I attach a patch to achieve this.


andrzej zaborowski told me:
> Qemu never claimed to be secure. Of course it's better to be secure
> than not if it doesn't add a bad overhead.
...
> I'm no sure where the size check should be, doing it in the IDE driver
> would probably make more sense as some other users of bdrv_ functions
> already have such checks.  I guess also some block-* backends may
> already have such checks.  And they only make a difference for writes
> because reads will return errors.

It seems to me that the right place to make this check is in the
generic block layer, so that it applies to all block requests
regardless of source or driver.  That will avoid making this mistake
in future.

Ian.


[-- Attachment #2: qemu check block read/write extents --]
[-- Type: text/plain, Size: 3711 bytes --]

diff --git a/block.c b/block.c
index 0f8ad7b..d7f1114 100644
--- a/block.c
+++ b/block.c
@@ -123,6 +123,24 @@ void path_combine(char *dest, int dest_size,
     }
 }
 
+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;
+}
 
 static void bdrv_register(BlockDriver *bdrv)
 {
@@ -375,6 +393,7 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
     }
     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
@@ -440,6 +459,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);
@@ -505,6 +525,8 @@ int bdrv_read(BlockDriverState *bs, int64_t sector_num,
     if (!drv)
         return -ENOMEDIUM;
 
+    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++;
@@ -545,6 +567,8 @@ int bdrv_write(BlockDriverState *bs, int64_t sector_num,
         return -ENOMEDIUM;
     if (bs->read_only)
         return -EACCES;
+    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);
     }
@@ -670,6 +694,8 @@ int bdrv_pread(BlockDriverState *bs, int64_t offset,
         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);
 }
 
@@ -685,6 +711,8 @@ int bdrv_pwrite(BlockDriverState *bs, int64_t offset,
         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);
 }
 
@@ -951,6 +979,8 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
         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);
 }
 
@@ -1097,6 +1127,8 @@ BlockDriverAIOCB *bdrv_aio_read(BlockDriverState *bs, int64_t sector_num,
 
     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) {
@@ -1128,6 +1160,8 @@ BlockDriverAIOCB *bdrv_aio_write(BlockDriverState *bs, int64_t sector_num,
         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);
     }

Re: [Qemu-devel] Re: qemu unchecked block read/write vulnerability

[Daniel P. Berrange]

On Tue, Feb 19, 2008 at 04:39:07PM +0000, Ian Jackson wrote:
Content-Description: message body text
> I was doing some merging of qemu and I noticed that the block driver
> backends don't check the guest's read/write attempts against the
> nominal size of the block device.
> 
> I haven't checked all of the backends but I have verified the bug with
> block-cow.c, which I have in my test induced to set a bitmap bit at an
> address which is not actually part of the bitmap.  In my tests I used
> as my guest a Linux kernel which I'd specially modifed to allow me to
> access out-of-range blocks.
> 
> I think the fix is probably to insert a couple of range checks in the
> generic block dispatch layer and I attach a patch to achieve this.

FYI, this patch appears to cause massive unrecoverable data corruption for
qcow2 format disks. It looks like the sector range check is being applied
to the total sector count of the actual qcow datafile on disk, rather
than the total sector count of the logical disk. I suspect the same may
occur with other non-raw disk formats, so be wary....

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: [Qemu-devel] Re: qemu unchecked block read/write vulnerability

[Daniel P. Berrange]

On Tue, Feb 26, 2008 at 07:46:51PM +0000, Daniel P. Berrange wrote:
> On Tue, Feb 19, 2008 at 04:39:07PM +0000, Ian Jackson wrote:
> Content-Description: message body text
> > I was doing some merging of qemu and I noticed that the block driver
> > backends don't check the guest's read/write attempts against the
> > nominal size of the block device.
> > 
> > I haven't checked all of the backends but I have verified the bug with
> > block-cow.c, which I have in my test induced to set a bitmap bit at an
> > address which is not actually part of the bitmap.  In my tests I used
> > as my guest a Linux kernel which I'd specially modifed to allow me to
> > access out-of-range blocks.
> > 
> > I think the fix is probably to insert a couple of range checks in the
> > generic block dispatch layer and I attach a patch to achieve this.
> 
> FYI, this patch appears to cause massive unrecoverable data corruption for
> qcow2 format disks. It looks like the sector range check is being applied
> to the total sector count of the actual qcow datafile on disk, rather
> than the total sector count of the logical disk. I suspect the same may
> occur with other non-raw disk formats, so be wary....

The original patch adds checks to the main  bdrv_XXX apis to validate that
the I/O operation does not exceed the bounds of the disk - ie beyond the
total_sectors count. This works correctly for bdrv_XXX calls from the IDE
driver.  With disk formats like QCow though,  bdrv_XXX is re-entrant, 
because the QCow driver uses the block APIs for dealing with its underlying
file.  The problem is that QCow files are grow-on-demand, so writes will
*explicitly* be beyond the end of the file. The original patch blocks any
I/O operation which would cause the QCow file to grow, resulting it more
or less catasatrophic data loss.  

Basically the bounds checking needs to distinguish between checking for
the logical disk extents, vs the physical disk extents. For raw files
these are the same so initial tests showed no problems, but for QCow
format disks they are different & thus we see a problem

What follows is a revised patch which introduces a flag BDRV_O_AUTOGROW
which can be passed to bdrv_open to indicate that the files can be allowed
to automatically extend their extents. This flag should only be used by
internal block drivers such as block-qcow2.c, block-vmdk.c  In my testing
this has fixed the qcow corruption, and still maintains the goal of Ian's
original patch which was to prevent the guest VM writing beyond the logical
disk extents.

 block-qcow.c  |    2 -
 block-qcow2.c |    2 -
 block-vmdk.c  |    2 -
 block.c       |   74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 block.h       |    1 
 block_int.h   |    1 
 6 files changed, 79 insertions(+), 3 deletions(-)

Dan.

Index: block-qcow.c
===================================================================
RCS file: /sources/qemu/qemu/block-qcow.c,v
retrieving revision 1.15
diff -u -p -r1.15 block-qcow.c
--- block-qcow.c	11 Nov 2007 02:51:16 -0000	1.15
+++ block-qcow.c	26 Feb 2008 23:46:41 -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_AUTOGROW);
     if (ret < 0)
         return ret;
     if (bdrv_pread(s->hd, 0, &header, sizeof(header)) != sizeof(header))
Index: block-qcow2.c
===================================================================
RCS file: /sources/qemu/qemu/block-qcow2.c,v
retrieving revision 1.10
diff -u -p -r1.10 block-qcow2.c
--- block-qcow2.c	11 Nov 2007 02:51:16 -0000	1.10
+++ block-qcow2.c	26 Feb 2008 23:46:41 -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_AUTOGROW);
     if (ret < 0)
         return ret;
     if (bdrv_pread(s->hd, 0, &header, sizeof(header)) != sizeof(header))
Index: block-vmdk.c
===================================================================
RCS file: /sources/qemu/qemu/block-vmdk.c,v
retrieving revision 1.19
diff -u -p -r1.19 block-vmdk.c
--- block-vmdk.c	14 Jan 2008 03:48:37 -0000	1.19
+++ block-vmdk.c	26 Feb 2008 23:46:41 -0000
@@ -378,7 +378,7 @@ static int vmdk_open(BlockDriverState *b
         flags = BDRV_O_RDONLY;
     fprintf(stderr, "(VMDK) image open: flags=0x%x filename=%s\n", flags, bs->filename);
 
-    ret = bdrv_file_open(&s->hd, filename, flags);
+    ret = bdrv_file_open(&s->hd, filename, flags | BDRV_O_AUTOGROW);
     if (ret < 0)
         return ret;
     if (bdrv_pread(s->hd, 0, &magic, sizeof(magic)) != sizeof(magic))
Index: block.c
===================================================================
RCS file: /sources/qemu/qemu/block.c,v
retrieving revision 1.53
diff -u -p -r1.53 block.c
--- block.c	24 Dec 2007 16:10:43 -0000	1.53
+++ block.c	26 Feb 2008 23:46:41 -0000
@@ -123,6 +123,60 @@ void path_combine(char *dest, int dest_s
     }
 }
 
+static int bdrv_rd_badreq_sectors(BlockDriverState *bs,
+                                  int64_t sector_num, int nb_sectors)
+{
+    return
+        nb_sectors < 0 ||
+        sector_num < 0 ||
+        nb_sectors > bs->total_sectors ||
+        sector_num > bs->total_sectors - nb_sectors;
+}
+
+static int bdrv_rd_badreq_bytes(BlockDriverState *bs,
+                                int64_t offset, int count)
+{
+    int64_t size = bs->total_sectors << SECTOR_BITS;
+    return
+        count < 0 ||
+        size < 0 ||
+        count > size ||
+        offset > size - count;
+}
+
+static int bdrv_wr_badreq_sectors(BlockDriverState *bs,
+                                  int64_t sector_num, int nb_sectors)
+{
+    if (sector_num < 0 ||
+        nb_sectors < 0)
+        return 1;
+
+    if (sector_num > bs->total_sectors - nb_sectors) {
+        if (bs->autogrow)
+            bs->total_sectors = sector_num + nb_sectors;
+        else
+            return 1;
+    }
+    return 0;
+}
+
+static int bdrv_wr_badreq_bytes(BlockDriverState *bs,
+                                int64_t offset, int count)
+{
+    int64_t size = bs->total_sectors << SECTOR_BITS;
+    if (count < 0 ||
+        offset < 0)
+        return 1;
+
+    if (offset > size - count) {
+        if (bs->autogrow)
+            bs->total_sectors = (offset + count + SECTOR_SIZE - 1) >> SECTOR_BITS;
+        else
+            return 1;
+    }
+    return 0;
+}
+
 
 static void bdrv_register(BlockDriver *bdrv)
 {
@@ -331,6 +385,10 @@ int bdrv_open2(BlockDriverState *bs, con
     bs->read_only = 0;
     bs->is_temporary = 0;
     bs->encrypted = 0;
+    bs->autogrow = 0;
+
+    if (flags & BDRV_O_AUTOGROW)
+        bs->autogrow = 1;
 
     if (flags & BDRV_O_SNAPSHOT) {
         BlockDriverState *bs1;
@@ -375,6 +433,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
@@ -440,6 +499,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);
@@ -505,6 +565,8 @@ int bdrv_read(BlockDriverState *bs, int6
     if (!drv)
         return -ENOMEDIUM;
 
+    if (bdrv_rd_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++;
@@ -545,6 +607,8 @@ int bdrv_write(BlockDriverState *bs, int
         return -ENOMEDIUM;
     if (bs->read_only)
         return -EACCES;
+    if (bdrv_wr_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);
     }
@@ -670,6 +734,8 @@ int bdrv_pread(BlockDriverState *bs, int
         return -ENOMEDIUM;
     if (!drv->bdrv_pread)
         return bdrv_pread_em(bs, offset, buf1, count1);
+    if (bdrv_rd_badreq_bytes(bs, offset, count1))
+        return -EDOM;
     return drv->bdrv_pread(bs, offset, buf1, count1);
 }
 
@@ -685,6 +751,8 @@ int bdrv_pwrite(BlockDriverState *bs, in
         return -ENOMEDIUM;
     if (!drv->bdrv_pwrite)
         return bdrv_pwrite_em(bs, offset, buf1, count1);
+    if (bdrv_wr_badreq_bytes(bs, offset, count1))
+        return -EDOM;
     return drv->bdrv_pwrite(bs, offset, buf1, count1);
 }
 
@@ -951,6 +1019,8 @@ int bdrv_write_compressed(BlockDriverSta
         return -ENOMEDIUM;
     if (!drv->bdrv_write_compressed)
         return -ENOTSUP;
+    if (bdrv_wr_badreq_sectors(bs, sector_num, nb_sectors))
+        return -EDOM;
     return drv->bdrv_write_compressed(bs, sector_num, buf, nb_sectors);
 }
 
@@ -1097,6 +1167,8 @@ BlockDriverAIOCB *bdrv_aio_read(BlockDri
 
     if (!drv)
         return NULL;
+    if (bdrv_rd_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) {
@@ -1128,6 +1200,8 @@ BlockDriverAIOCB *bdrv_aio_write(BlockDr
         return NULL;
     if (bs->read_only)
         return NULL;
+    if (bdrv_wr_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);
     }
Index: block.h
===================================================================
RCS file: /sources/qemu/qemu/block.h,v
retrieving revision 1.6
diff -u -p -r1.6 block.h
--- block.h	24 Dec 2007 16:10:43 -0000	1.6
+++ block.h	26 Feb 2008 23:46:41 -0000
@@ -45,6 +45,7 @@ typedef struct QEMUSnapshotInfo {
                                      it (default for
                                      bdrv_file_open()) */
 #define BDRV_O_DIRECT      0x0020
+#define BDRV_O_AUTOGROW    0x0040 /* Allow backing file to extend when writing past end of file */
 
 #ifndef QEMU_IMG
 void bdrv_info(void);
Index: block_int.h
===================================================================
RCS file: /sources/qemu/qemu/block_int.h,v
retrieving revision 1.16
diff -u -p -r1.16 block_int.h
--- block_int.h	24 Dec 2007 16:10:43 -0000	1.16
+++ block_int.h	26 Feb 2008 23:46:41 -0000
@@ -97,6 +97,7 @@ struct BlockDriverState {
     int locked;    /* if true, the media cannot temporarily be ejected */
     int encrypted; /* if true, the media is encrypted */
     int sg;        /* if true, the device is a /dev/sg* */
+    int autogrow;  /* if true, the backing store can auto-extend to allocate new extents */
     /* event callback when inserting/removing */
     void (*change_cb)(void *opaque);
     void *change_opaque;


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