[Xen-devel] [PATCH] ioemu block device extent checks

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxx>

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

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