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

[Ian Jackson]

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.

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