[Qemu-devel] [6677] Fix CVE-2008-0928 - insufficient block device address range checking ( Anthony Liguori)

[Anthony Liguori]

Revision: 6677
          http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=6677
Author:   aliguori
Date:     2009-03-03 17:37:16 +0000 (Tue, 03 Mar 2009)
Log Message:
-----------
Fix CVE-2008-0928 - insufficient block device address range checking (Anthony 
Liguori)

Introduce a growable flag that's set by bdrv_file_open().  Block devices should
never be growable, only files that are being used by block devices.

I went through Fabrice's early comments about the patch that was first applied.
While I disagree with that patch, I also disagree with Fabrice's suggestion.

There's no good reason to do the checks in the block drivers themselves.  It
just increases the possibility that this bug could show up again.  Since we're
calling bdrv_getlength() to determine the length, we're giving the block drivers
a chance to chime in and let us know what range is valid.

Basically, this patch makes the BlockDriver API guarantee that all requests are
within 0..bdrv_getlength() which to me seems like a Good Thing.

What do others think?

Signed-off-by: Anthony Liguori <address@hidden>
Signed-off-by: Anthony Liguori <address@hidden>

Modified Paths:
--------------
    trunk/block.c
    trunk/block_int.h

Modified: trunk/block.c
===================================================================
--- trunk/block.c       2009-03-03 09:14:10 UTC (rev 6676)
+++ trunk/block.c       2009-03-03 17:37:16 UTC (rev 6677)
@@ -318,6 +318,7 @@
         bdrv_delete(bs);
         return ret;
     }
+    bs->growable = 1;
     *pbs = bs;
     return 0;
 }
@@ -519,6 +520,39 @@
     return 0;
 }
 
+static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
+                                   size_t size)
+{
+    int64_t len;
+
+    if (!bdrv_is_inserted(bs))
+        return -ENOMEDIUM;
+
+    if (bs->growable)
+        return 0;
+
+    len = bdrv_getlength(bs);
+
+    if ((offset + size) > len)
+        return -EIO;
+
+    return 0;
+}
+
+static int bdrv_check_request(BlockDriverState *bs, int64_t sector_num,
+                              int nb_sectors)
+{
+    int64_t offset;
+
+    /* Deal with byte accesses */
+    if (sector_num < 0)
+        offset = -sector_num;
+    else
+        offset = sector_num * 512;
+
+    return bdrv_check_byte_request(bs, offset, nb_sectors * 512);
+}
+
 /* return < 0 if error. See bdrv_write() for the return codes */
 int bdrv_read(BlockDriverState *bs, int64_t sector_num,
               uint8_t *buf, int nb_sectors)
@@ -527,6 +561,8 @@
 
     if (!drv)
         return -ENOMEDIUM;
+    if (bdrv_check_request(bs, sector_num, nb_sectors))
+        return -EIO;
 
     if (drv->bdrv_pread) {
         int ret, len;
@@ -560,6 +596,9 @@
         return -ENOMEDIUM;
     if (bs->read_only)
         return -EACCES;
+    if (bdrv_check_request(bs, sector_num, nb_sectors))
+        return -EIO;
+
     if (drv->bdrv_pwrite) {
         int ret, len, count = 0;
         len = nb_sectors * 512;
@@ -681,6 +720,9 @@
 
     if (!drv)
         return -ENOMEDIUM;
+    if (bdrv_check_byte_request(bs, offset, count1))
+        return -EIO;
+
     if (!drv->bdrv_pread)
         return bdrv_pread_em(bs, offset, buf1, count1);
     return drv->bdrv_pread(bs, offset, buf1, count1);
@@ -696,6 +738,9 @@
 
     if (!drv)
         return -ENOMEDIUM;
+    if (bdrv_check_byte_request(bs, offset, count1))
+        return -EIO;
+
     if (!drv->bdrv_pwrite)
         return bdrv_pwrite_em(bs, offset, buf1, count1);
     return drv->bdrv_pwrite(bs, offset, buf1, count1);
@@ -1299,6 +1344,9 @@
                                  QEMUIOVector *iov, int nb_sectors,
                                  BlockDriverCompletionFunc *cb, void *opaque)
 {
+    if (bdrv_check_request(bs, sector_num, nb_sectors))
+        return NULL;
+
     return bdrv_aio_rw_vector(bs, sector_num, iov, nb_sectors,
                               cb, opaque, 0);
 }
@@ -1307,6 +1355,9 @@
                                   QEMUIOVector *iov, int nb_sectors,
                                   BlockDriverCompletionFunc *cb, void *opaque)
 {
+    if (bdrv_check_request(bs, sector_num, nb_sectors))
+        return NULL;
+
     return bdrv_aio_rw_vector(bs, sector_num, iov, nb_sectors,
                               cb, opaque, 1);
 }
@@ -1320,6 +1371,8 @@
 
     if (!drv)
         return NULL;
+    if (bdrv_check_request(bs, sector_num, nb_sectors))
+        return NULL;
 
     ret = drv->bdrv_aio_read(bs, sector_num, buf, nb_sectors, cb, opaque);
 
@@ -1343,6 +1396,8 @@
         return NULL;
     if (bs->read_only)
         return NULL;
+    if (bdrv_check_request(bs, sector_num, nb_sectors))
+        return NULL;
 
     ret = drv->bdrv_aio_write(bs, sector_num, buf, nb_sectors, cb, opaque);
 

Modified: trunk/block_int.h
===================================================================
--- trunk/block_int.h   2009-03-03 09:14:10 UTC (rev 6676)
+++ trunk/block_int.h   2009-03-03 17:37:16 UTC (rev 6677)
@@ -121,6 +121,9 @@
     uint64_t rd_ops;
     uint64_t wr_ops;
 
+    /* Whether the disk can expand beyond total_sectors */
+    int growable;
+
     /* NOTE: the following infos are only hints for real hardware
        drivers. They are not used by the block driver */
     int cyls, heads, secs, translation;