[Qemu-devel] [PATCH 0/4] CVE-2008-0928 security fix

[Qemu-devel] [PATCH 0/4] CVE-2008-0928 security fix

[Eduardo Habkost]

Hi,

This series is another try to fix CVE-2008-0928 on Qemu, a security
vulnerability that is present since a long time. The first 3 patches are
simple changes to make the way to the last patch, that is the actual fix.

This fix is similar to the previous fix tat was present on Qemu SVN
previously, but changes BlockDriverState to store total_bytes
instead of total_sectors. This should avoid problems when byte-based
reads are done on some devices, such as on qcow case. The check
based on sector range done on the previous fix caused problems for qcow,
as documented at:

https://bugzilla.redhat.com/show_bug.cgi?id=485148

The previous fix was reverted almost a year ago but no alternative
fix was committed since then. Not having a fix to the vulnerability
upstream causes pain to users of the upstream code (who have a vulnerable
Qemu) and developers of distributions including Qemu code (who have to
carry and forward-port the fix themselves).

-- 
Eduardo

[Qemu-devel] [PATCH 1/4] vmdk: check for negative sector nums also

[Eduardo Habkost]

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 block-vmdk.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/block-vmdk.c b/block-vmdk.c
index 71d7504..416fb95 100644
--- a/block-vmdk.c
+++ b/block-vmdk.c
@@ -649,7 +649,7 @@ static int vmdk_write(BlockDriverState *bs, int64_t sector_num,
     uint64_t cluster_offset;
     static int cid_update = 0;
 
-    if (sector_num > bs->total_sectors) {
+    if (sector_num < 0 || sector_num > bs->total_sectors) {
         fprintf(stderr,
                 "(VMDK) Wrong offset: sector_num=0x%" PRIx64
                 " total_sectors=0x%" PRIx64 "\n",
-- 
1.6.1

[Qemu-devel] [PATCH 2/4] hw/sd.c: remove ununsed SECTOR_SIZE define

[Eduardo Habkost]

It is not used anywhere on the file and conflicts with an existing define
on block.c, so remove it.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/sd.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/hw/sd.c b/hw/sd.c
index 0c770bc..c6307bc 100644
--- a/hw/sd.c
+++ b/hw/sd.c
@@ -243,7 +243,6 @@ static void sd_set_cid(SDState *sd)
 #define WPGROUP_SHIFT	7			/* 2 megs */
 #define CMULT_SHIFT	9			/* 512 times HWBLOCK_SIZE */
 #define BLOCK_SIZE	(1 << (HWBLOCK_SHIFT))
-#define SECTOR_SIZE	(1 << (HWBLOCK_SHIFT + SECTOR_SHIFT))
 #define WPGROUP_SIZE	(1 << (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT))
 
 static const uint8_t sd_csd_rw_mask[16] = {
-- 
1.6.1

[Qemu-devel] [PATCH 3/4] Move SECTOR_BITS/SECTOR_SIZE to block.h

[Eduardo Habkost]

We will use those defines on other parts of the Qemu code.

Remove redundant SECTOR_SIZE define from block-vmdk.c, also.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 block-vmdk.c |    1 -
 block.c      |    3 ---
 block.h      |    3 +++
 3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/block-vmdk.c b/block-vmdk.c
index 416fb95..7ac79b3 100644
--- a/block-vmdk.c
+++ b/block-vmdk.c
@@ -110,7 +110,6 @@ static int vmdk_probe(const uint8_t *buf, int buf_size, const char *filename)
 
 #define CHECK_CID 1
 
-#define SECTOR_SIZE 512
 #define DESC_SIZE 20*SECTOR_SIZE	// 20 sectors of 512 bytes each
 #define HEADER_SIZE 512   			// first sector of 512 bytes
 
diff --git a/block.c b/block.c
index 4f4bf7c..ba7fcf6 100644
--- a/block.c
+++ b/block.c
@@ -38,9 +38,6 @@
 #include <sys/disk.h>
 #endif
 
-#define SECTOR_BITS 9
-#define SECTOR_SIZE (1 << SECTOR_BITS)
-
 typedef struct BlockDriverAIOCBSync {
     BlockDriverAIOCB common;
     QEMUBH *bh;
diff --git a/block.h b/block.h
index e1927dd..ee0d403 100644
--- a/block.h
+++ b/block.h
@@ -4,6 +4,9 @@
 #include "qemu-aio.h"
 #include "qemu-common.h"
 
+#define SECTOR_BITS 9
+#define SECTOR_SIZE (1 << SECTOR_BITS)
+
 /* block.c */
 typedef struct BlockDriver BlockDriver;
 
-- 
1.6.1

[Qemu-devel] [PATCH 4/4] Fix CVE-2008-0928 - insufficient block device address range checking

[Eduardo Habkost]

From: Aurelien Jarno <aurel32>

This is based on an old patch commited by Aurelien Jarno whose commit
message was:

  Fix CVE-2008-0928 - insufficient block device address range checking

  Qemu 0.9.1 and earlier does not perform range checks for block device
  read or write requests, which allows guest host users with root
  privileges to access arbitrary memory and escape the virtual machine.

In addition to the changes done by the previous patch, this patch changes
total_sectors to total_bytes, so that the range checking works for
backing devices that are not sector-based (for example, when block-qcow
is reading the backing file). This was done to avoid bugs such as:

https://bugzilla.redhat.com/show_bug.cgi?id=485148

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 block-bochs.c     |    4 +-
 block-cloop.c     |    2 +-
 block-cow.c       |    6 +++-
 block-parallels.c |    2 +-
 block-qcow.c      |    4 +-
 block-qcow2.c     |   11 ++++----
 block-raw-posix.c |   10 ++++----
 block-vmdk.c      |   16 +++++++----
 block-vpc.c       |    7 ++++-
 block-vvfat.c     |    4 ++-
 block.c           |   70 +++++++++++++++++++++++++++++++++++++++++++++++++---
 block.h           |    1 +
 block_int.h       |    5 ++-
 qemu-nbd.c        |    2 +-
 14 files changed, 110 insertions(+), 34 deletions(-)

diff --git a/block-bochs.c b/block-bochs.c
index 593cf69..eb36f87 100644
--- a/block-bochs.c
+++ b/block-bochs.c
@@ -140,9 +140,9 @@ static int bochs_open(BlockDriverState *bs, const char *filename, int flags)
 
     if (le32_to_cpu(bochs.version) == HEADER_V1) {
       memcpy(&header_v1, &bochs, sizeof(bochs));
-      bs->total_sectors = le64_to_cpu(header_v1.extra.redolog.disk) / 512;
+      bs->total_bytes = le64_to_cpu(header_v1.extra.redolog.disk);
     } else {
-      bs->total_sectors = le64_to_cpu(bochs.extra.redolog.disk) / 512;
+      bs->total_bytes = le64_to_cpu(bochs.extra.redolog.disk);
     }
 
     lseek(s->fd, le32_to_cpu(bochs.header), SEEK_SET);
diff --git a/block-cloop.c b/block-cloop.c
index 6985084..a88e8fd 100644
--- a/block-cloop.c
+++ b/block-cloop.c
@@ -95,7 +95,7 @@ cloop_close:
     s->current_block=s->n_blocks;
 
     s->sectors_per_block = s->block_size/512;
-    bs->total_sectors = s->n_blocks*s->sectors_per_block;
+    bs->total_bytes = (s->n_blocks*s->sectors_per_block) * SECTOR_SIZE;
     return 0;
 }
 
diff --git a/block-cow.c b/block-cow.c
index 9e7b646..b863dd4 100644
--- a/block-cow.c
+++ b/block-cow.c
@@ -68,6 +68,7 @@ static int cow_open(BlockDriverState *bs, const char *filename, int flags)
     int fd;
     struct cow_header_v2 cow_header;
     int64_t size;
+    int64_t sectors;
 
     fd = open(filename, O_RDWR | O_BINARY | O_LARGEFILE);
     if (fd < 0) {
@@ -88,13 +89,14 @@ static int cow_open(BlockDriverState *bs, const char *filename, int flags)
 
     /* cow image found */
     size = be64_to_cpu(cow_header.size);
-    bs->total_sectors = size / 512;
+    sectors = size / 512;
+    bs->total_bytes = size;
 
     pstrcpy(bs->backing_file, sizeof(bs->backing_file),
             cow_header.backing_file);
 
     /* mmap the bitmap */
-    s->cow_bitmap_size = ((bs->total_sectors + 7) >> 3) + sizeof(cow_header);
+    s->cow_bitmap_size = ((sectors + 7) >> 3) + sizeof(cow_header);
     s->cow_bitmap_addr = mmap(get_mmap_addr(s->cow_bitmap_size),
                               s->cow_bitmap_size,
                               PROT_READ | PROT_WRITE,
diff --git a/block-parallels.c b/block-parallels.c
index 8a8ec63..67d7ab4 100644
--- a/block-parallels.c
+++ b/block-parallels.c
@@ -92,7 +92,7 @@ static int parallels_open(BlockDriverState *bs, const char *filename, int flags)
         goto fail;
     }
 
-    bs->total_sectors = le32_to_cpu(ph.nb_sectors);
+    bs->total_bytes = le32_to_cpu(ph.nb_sectors) * SECTOR_SIZE;
 
     if (lseek(s->fd, 64, SEEK_SET) != 64)
 	goto fail;
diff --git a/block-qcow.c b/block-qcow.c
index 91c53b1..899e237 100644
--- a/block-qcow.c
+++ b/block-qcow.c
@@ -95,7 +95,7 @@ static int qcow_open(BlockDriverState *bs, const char *filename, int flags)
     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))
@@ -123,7 +123,7 @@ static int qcow_open(BlockDriverState *bs, const char *filename, int flags)
     s->cluster_sectors = 1 << (s->cluster_bits - 9);
     s->l2_bits = header.l2_bits;
     s->l2_size = 1 << s->l2_bits;
-    bs->total_sectors = header.size / 512;
+    bs->total_bytes = header.size;
     s->cluster_offset_mask = (1LL << (63 - s->cluster_bits)) - 1;
 
     /* read the level 1 table */
diff --git a/block-qcow2.c b/block-qcow2.c
index 465dcd6..858f4ac 100644
--- a/block-qcow2.c
+++ b/block-qcow2.c
@@ -203,7 +203,7 @@ static int qcow_open(BlockDriverState *bs, const char *filename, int flags)
         flags |= BDRV_O_CACHE_WB;
         flags &= ~BDRV_O_CACHE_DEF;
     }
-    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))
@@ -238,7 +238,7 @@ static int qcow_open(BlockDriverState *bs, const char *filename, int flags)
     s->cluster_sectors = 1 << (s->cluster_bits - 9);
     s->l2_bits = s->cluster_bits - 3; /* L2 is always one cluster */
     s->l2_size = 1 << s->l2_bits;
-    bs->total_sectors = header.size / 512;
+    bs->total_bytes = header.size;
     s->csize_shift = (62 - (s->cluster_bits - 8));
     s->csize_mask = (1 << (s->cluster_bits - 8)) - 1;
     s->cluster_offset_mask = (1LL << s->csize_shift) - 1;
@@ -1081,12 +1081,13 @@ static int backing_read1(BlockDriverState *bs,
                          int64_t sector_num, uint8_t *buf, int nb_sectors)
 {
     int n1;
-    if ((sector_num + nb_sectors) <= bs->total_sectors)
+    uint64_t total_sectors = bs->total_bytes >> SECTOR_BITS;
+    if ((sector_num + nb_sectors) <= total_sectors)
         return nb_sectors;
-    if (sector_num >= bs->total_sectors)
+    if (sector_num >= total_sectors)
         n1 = 0;
     else
-        n1 = bs->total_sectors - sector_num;
+        n1 = total_sectors - sector_num;
     memset(buf + n1 * 512, 0, 512 * (nb_sectors - n1));
     return n1;
 }
diff --git a/block-raw-posix.c b/block-raw-posix.c
index 620791b..4e59bf7 100644
--- a/block-raw-posix.c
+++ b/block-raw-posix.c
@@ -200,7 +200,7 @@ static int raw_pread_aligned(BlockDriverState *bs, int64_t offset,
             DEBUG_BLOCK_PRINT("raw_pread(%d:%s, %" PRId64 ", %p, %d) [%" PRId64
                               "] lseek failed : %d = %s\n",
                               s->fd, bs->filename, offset, buf, count,
-                              bs->total_sectors, errno, strerror(errno));
+                              bs->total_bytes, errno, strerror(errno));
         }
         return -1;
     }
@@ -213,7 +213,7 @@ static int raw_pread_aligned(BlockDriverState *bs, int64_t offset,
     DEBUG_BLOCK_PRINT("raw_pread(%d:%s, %" PRId64 ", %p, %d) [%" PRId64
                       "] read failed %d : %d = %s\n",
                       s->fd, bs->filename, offset, buf, count,
-                      bs->total_sectors, ret, errno, strerror(errno));
+                      bs->total_bytes, ret, errno, strerror(errno));
 
     /* Try harder for CDrom. */
     if (bs->type == BDRV_TYPE_CDROM) {
@@ -229,7 +229,7 @@ static int raw_pread_aligned(BlockDriverState *bs, int64_t offset,
         DEBUG_BLOCK_PRINT("raw_pread(%d:%s, %" PRId64 ", %p, %d) [%" PRId64
                           "] retry read failed %d : %d = %s\n",
                           s->fd, bs->filename, offset, buf, count,
-                          bs->total_sectors, ret, errno, strerror(errno));
+                          bs->total_bytes, ret, errno, strerror(errno));
     }
 
 label__raw_read__success:
@@ -260,7 +260,7 @@ static int raw_pwrite_aligned(BlockDriverState *bs, int64_t offset,
             DEBUG_BLOCK_PRINT("raw_pwrite(%d:%s, %" PRId64 ", %p, %d) [%"
                               PRId64 "] lseek failed : %d = %s\n",
                               s->fd, bs->filename, offset, buf, count,
-                              bs->total_sectors, errno, strerror(errno));
+                              bs->total_bytes, errno, strerror(errno));
         }
         return -EIO;
     }
@@ -273,7 +273,7 @@ static int raw_pwrite_aligned(BlockDriverState *bs, int64_t offset,
     DEBUG_BLOCK_PRINT("raw_pwrite(%d:%s, %" PRId64 ", %p, %d) [%" PRId64
                       "] write failed %d : %d = %s\n",
                       s->fd, bs->filename, offset, buf, count,
-                      bs->total_sectors, ret, errno, strerror(errno));
+                      bs->total_bytes, ret, errno, strerror(errno));
 
 label__raw_write__success:
 
diff --git a/block-vmdk.c b/block-vmdk.c
index 7ac79b3..0a52127 100644
--- a/block-vmdk.c
+++ b/block-vmdk.c
@@ -368,12 +368,13 @@ static int vmdk_open(BlockDriverState *bs, const char *filename, int flags)
     BDRVVmdkState *s = bs->opaque;
     uint32_t magic;
     int l1_size, i, ret;
+    int64_t sectors;
 
     if (parent_open)
         // Parent must be opened as RO.
         flags = BDRV_O_RDONLY;
 
-    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))
@@ -388,7 +389,8 @@ static int vmdk_open(BlockDriverState *bs, const char *filename, int flags)
         s->cluster_sectors = le32_to_cpu(header.granularity);
         s->l2_size = 1 << 9;
         s->l1_size = 1 << 6;
-        bs->total_sectors = le32_to_cpu(header.disk_sectors);
+        sectors = le32_to_cpu(header.disk_sectors);
+        bs->total_bytes = sectors * SECTOR_SIZE;
         s->l1_table_offset = le32_to_cpu(header.l1dir_offset) << 9;
         s->l1_backup_table_offset = 0;
         s->l1_entry_sectors = s->l2_size * s->cluster_sectors;
@@ -397,13 +399,14 @@ static int vmdk_open(BlockDriverState *bs, const char *filename, int flags)
 
         if (bdrv_pread(s->hd, sizeof(magic), &header, sizeof(header)) != sizeof(header))
             goto fail;
-        bs->total_sectors = le64_to_cpu(header.capacity);
+        sectors = le64_to_cpu(header.capacity);
+        bs->total_bytes = sectors * SECTOR_SIZE;
         s->cluster_sectors = le64_to_cpu(header.granularity);
         s->l2_size = le32_to_cpu(header.num_gtes_per_gte);
         s->l1_entry_sectors = s->l2_size * s->cluster_sectors;
         if (s->l1_entry_sectors <= 0)
             goto fail;
-        s->l1_size = (bs->total_sectors + s->l1_entry_sectors - 1)
+        s->l1_size = (sectors + s->l1_entry_sectors - 1)
             / s->l1_entry_sectors;
         s->l1_table_offset = le64_to_cpu(header.rgd_offset) << 9;
         s->l1_backup_table_offset = le64_to_cpu(header.gd_offset) << 9;
@@ -647,12 +650,13 @@ static int vmdk_write(BlockDriverState *bs, int64_t sector_num,
     int index_in_cluster, n;
     uint64_t cluster_offset;
     static int cid_update = 0;
+    int64_t total_sectors = bs->total_bytes >> SECTOR_BITS;
 
-    if (sector_num < 0 || sector_num > bs->total_sectors) {
+    if (sector_num < 0 || sector_num > total_sectors) {
         fprintf(stderr,
                 "(VMDK) Wrong offset: sector_num=0x%" PRIx64
                 " total_sectors=0x%" PRIx64 "\n",
-                sector_num, bs->total_sectors);
+                sector_num, total_sectors);
         return -1;
     }
 
diff --git a/block-vpc.c b/block-vpc.c
index e353e31..e3eaf75 100644
--- a/block-vpc.c
+++ b/block-vpc.c
@@ -157,6 +157,7 @@ static int vpc_open(BlockDriverState *bs, const char *filename, int flags)
     struct vhd_dyndisk_header* dyndisk_header;
     uint8_t buf[HEADER_SIZE];
     uint32_t checksum;
+    int64_t sectors;
 
     ret = bdrv_file_open(&s->hd, filename, flags);
     if (ret < 0)
@@ -178,8 +179,9 @@ static int vpc_open(BlockDriverState *bs, const char *filename, int flags)
     // The visible size of a image in Virtual PC depends on the geometry
     // rather than on the size stored in the footer (the size in the footer
     // is too large usually)
-    bs->total_sectors = (int64_t)
+    sectors = (int64_t)
         be16_to_cpu(footer->cyls) * footer->heads * footer->secs_per_cyl;
+    bs->total_bytes = sectors * SECTOR_SIZE;
 
     if (bdrv_pread(s->hd, be64_to_cpu(footer->data_offset), buf, HEADER_SIZE)
             != HEADER_SIZE)
@@ -336,9 +338,10 @@ static int64_t alloc_block(BlockDriverState* bs, int64_t sector_num)
     uint32_t index, bat_value;
     int ret;
     uint8_t bitmap[s->bitmap_size];
+    uint64_t total_sectors = bs->total_bytes >> SECTOR_BITS;
 
     // Check if sector_num is valid
-    if ((sector_num < 0) || (sector_num > bs->total_sectors))
+    if ((sector_num < 0) || (sector_num > total_sectors))
         return -1;
 
     // Write entry into in-memory BAT
diff --git a/block-vvfat.c b/block-vvfat.c
index 623ba98..e8ba445 100644
--- a/block-vvfat.c
+++ b/block-vvfat.c
@@ -995,6 +995,7 @@ static int vvfat_open(BlockDriverState *bs, const char* dirname, int flags)
     BDRVVVFATState *s = bs->opaque;
     int floppy = 0;
     int i;
+    int64_t sectors;
 
 #ifdef DEBUG
     vvv = s;
@@ -1060,7 +1061,8 @@ DLOG(if (stderr == NULL) {
     else
 	dirname += i+1;
 
-    bs->total_sectors=bs->cyls*bs->heads*bs->secs;
+    sectors = bs->cyls*bs->heads*bs->secs;
+    bs->total_bytes = sectors << SECTOR_BITS;
 
     if(init_directories(s, dirname))
 	return -1;
diff --git a/block.c b/block.c
index ba7fcf6..b7e2004 100644
--- a/block.c
+++ b/block.c
@@ -124,6 +124,47 @@ void path_combine(char *dest, int dest_size,
     }
 }
 
+static int bdrv_rd_badreq_bytes(BlockDriverState *bs,
+                                int64_t offset, int count)
+{
+    int64_t size = bs->total_bytes;
+    return
+        count < 0 ||
+        size < 0 ||
+        count > size ||
+        offset > size - count;
+}
+
+static int bdrv_rd_badreq_sectors(BlockDriverState *bs,
+                                  int64_t sector_num, int nb_sectors)
+{
+    return bdrv_rd_badreq_bytes(bs, sector_num << SECTOR_BITS, nb_sectors << SECTOR_BITS);
+}
+
+
+static int bdrv_wr_badreq_bytes(BlockDriverState *bs,
+                                int64_t offset, int count)
+{
+    int64_t size = bs->total_bytes;
+    if (count < 0 ||
+        offset < 0)
+        return 1;
+
+    if (offset > size - count) {
+        if (bs->autogrow)
+            bs->total_bytes = (offset + count);
+        else
+            return 1;
+    }
+    return 0;
+}
+
+
+static int bdrv_wr_badreq_sectors(BlockDriverState *bs,
+                                  int64_t sector_num, int nb_sectors)
+{
+    return bdrv_wr_badreq_bytes(bs, sector_num << SECTOR_BITS, nb_sectors << SECTOR_BITS);
+}
 
 static void bdrv_register(BlockDriver *bdrv)
 {
@@ -334,6 +375,10 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
     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;
@@ -390,6 +435,7 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
     }
     bs->drv = drv;
     bs->opaque = qemu_mallocz(drv->instance_size);
+    bs->total_bytes = 0; /* driver will set if it does not do getlength */
     /* Note: for compatibility, we open disk image files as RDWR, and
        RDONLY as fallback */
     if (!(flags & BDRV_O_FILE))
@@ -408,7 +454,7 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
         return ret;
     }
     if (drv->bdrv_getlength) {
-        bs->total_sectors = bdrv_getlength(bs) >> SECTOR_BITS;
+        bs->total_bytes = bdrv_getlength(bs);
     }
 #ifndef _WIN32
     if (bs->is_temporary) {
@@ -453,6 +499,7 @@ void bdrv_close(BlockDriverState *bs)
         bs->drv = NULL;
 
         /* call the change callback */
+        bs->total_bytes = 0;
         bs->media_changed = 1;
         if (bs->change_cb)
             bs->change_cb(bs->change_opaque);
@@ -525,6 +572,8 @@ int bdrv_read(BlockDriverState *bs, int64_t sector_num,
     if (!drv)
         return -ENOMEDIUM;
 
+    if (bdrv_rd_badreq_sectors(bs, sector_num, nb_sectors))
+        return -EDOM;
     if (drv->bdrv_pread) {
         int ret, len;
         len = nb_sectors * 512;
@@ -557,6 +606,8 @@ int bdrv_write(BlockDriverState *bs, int64_t sector_num,
         return -ENOMEDIUM;
     if (bs->read_only)
         return -EACCES;
+    if (bdrv_wr_badreq_sectors(bs, sector_num, nb_sectors))
+        return -EDOM;
     if (drv->bdrv_pwrite) {
         int ret, len, count = 0;
         len = nb_sectors * 512;
@@ -680,6 +731,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_rd_badreq_bytes(bs, offset, count1))
+        return -EDOM;
     return drv->bdrv_pread(bs, offset, buf1, count1);
 }
 
@@ -695,6 +748,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_wr_badreq_bytes(bs, offset, count1))
+        return -EDOM;
     return drv->bdrv_pwrite(bs, offset, buf1, count1);
 }
 
@@ -721,7 +776,7 @@ int64_t bdrv_getlength(BlockDriverState *bs)
         return -ENOMEDIUM;
     if (!drv->bdrv_getlength) {
         /* legacy mode */
-        return bs->total_sectors * SECTOR_SIZE;
+        return bs->total_bytes;
     }
     return drv->bdrv_getlength(bs);
 }
@@ -1015,11 +1070,12 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
 {
     int64_t n;
     if (!bs->drv->bdrv_is_allocated) {
-        if (sector_num >= bs->total_sectors) {
+        int64_t total_sectors = bs->total_bytes >> SECTOR_BITS;
+        if (sector_num >= total_sectors) {
             *pnum = 0;
             return 0;
         }
-        n = bs->total_sectors - sector_num;
+        n = total_sectors - sector_num;
         *pnum = (n < nb_sectors) ? (n) : (nb_sectors);
         return 1;
     }
@@ -1108,6 +1164,8 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
         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);
 }
 
@@ -1317,6 +1375,8 @@ BlockDriverAIOCB *bdrv_aio_read(BlockDriverState *bs, int64_t sector_num,
 
     if (!drv)
         return NULL;
+    if (bdrv_rd_badreq_sectors(bs, sector_num, nb_sectors))
+        return NULL;
 
     ret = drv->bdrv_aio_read(bs, sector_num, buf, nb_sectors, cb, opaque);
 
@@ -1340,6 +1400,8 @@ BlockDriverAIOCB *bdrv_aio_write(BlockDriverState *bs, int64_t sector_num,
         return NULL;
     if (bs->read_only)
         return NULL;
+    if (bdrv_wr_badreq_sectors(bs, sector_num, nb_sectors))
+        return NULL;
 
     ret = drv->bdrv_aio_write(bs, sector_num, buf, nb_sectors, cb, opaque);
 
diff --git a/block.h b/block.h
index ee0d403..0b3ab6b 100644
--- a/block.h
+++ b/block.h
@@ -56,6 +56,7 @@ typedef struct QEMUSnapshotInfo {
 #define BDRV_O_NOCACHE     0x0020 /* do not use the host page cache */
 #define BDRV_O_CACHE_WB    0x0040 /* use write-back caching */
 #define BDRV_O_CACHE_DEF   0x0080 /* use default caching */
+#define BDRV_O_AUTOGROW    0x0100 /* Allow backing file to extend when writing past end of file */
 
 #define BDRV_O_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_CACHE_WB | BDRV_O_CACHE_DEF)
 
diff --git a/block_int.h b/block_int.h
index 781789c..ebbb057 100644
--- a/block_int.h
+++ b/block_int.h
@@ -90,13 +90,14 @@ struct BlockDriver {
 };
 
 struct BlockDriverState {
-    int64_t total_sectors; /* if we are reading a disk image, give its
-                              size in sectors */
+    int64_t total_bytes; /* if we are reading a disk image, give its
+                              size in bytes */
     int read_only; /* if true, the media is read only */
     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 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;
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 0af97ca..e811fa1 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -335,7 +335,7 @@ int main(int argc, char **argv)
     if (bdrv_open(bs, argv[optind], flags) == -1)
         return 1;
 
-    fd_size = bs->total_sectors * 512;
+    fd_size = bs->total_bytes;
 
     if (partition != -1 &&
         find_partition(bs, partition, &dev_offset, &fd_size))
-- 
1.6.1

Re: [Qemu-devel] [PATCH 4/4] Fix CVE-2008-0928 - insufficient block device address range checking

[Eduardo Habkost]

Excerpts from Eduardo Pereira Habkost's message of Qui Fev 19 18:19:36 -0300 2009:
> From: Aurelien Jarno <aurel32>

Oops. The line above wasn't supposed to be there. Author info on my git
repository got messed when I've squashed two patches.

> 
> This is based on an old patch commited by Aurelien Jarno whose commit
> message was:
> 
>   Fix CVE-2008-0928 - insufficient block device address range checking
> 
>   Qemu 0.9.1 and earlier does not perform range checks for block device
>   read or write requests, which allows guest host users with root
>   privileges to access arbitrary memory and escape the virtual machine.
> 
> In addition to the changes done by the previous patch, this patch changes
> total_sectors to total_bytes, so that the range checking works for
> backing devices that are not sector-based (for example, when block-qcow
> is reading the backing file). This was done to avoid bugs such as:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=485148
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/4] vmdk: check for negative sector nums also

[Stefan Weil]

Eduardo Habkost schrieb:
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  block-vmdk.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/block-vmdk.c b/block-vmdk.c
> index 71d7504..416fb95 100644
> --- a/block-vmdk.c
> +++ b/block-vmdk.c
> @@ -649,7 +649,7 @@ static int vmdk_write(BlockDriverState *bs, int64_t sector_num,
>   

Why is sector_num signed? An unsigned quantity would simplify the code below
(no need to check for < 0).

>      uint64_t cluster_offset;
>      static int cid_update = 0;
>  
> -    if (sector_num > bs->total_sectors) {
> +    if (sector_num < 0 || sector_num > bs->total_sectors) {
>          fprintf(stderr,
>                  "(VMDK) Wrong offset: sector_num=0x%" PRIx64
>                  " total_sectors=0x%" PRIx64 "\n",
>   

Regards
Stefan Weil

Re: [Qemu-devel] [PATCH 1/4] vmdk: check for negative sector nums also

[Eduardo Habkost]

Excerpts from Stefan Weil's message of Qui Fev 19 18:44:40 -0300 2009:
> Eduardo Habkost schrieb:
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  block-vmdk.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/block-vmdk.c b/block-vmdk.c
> > index 71d7504..416fb95 100644
> > --- a/block-vmdk.c
> > +++ b/block-vmdk.c
> > @@ -649,7 +649,7 @@ static int vmdk_write(BlockDriverState *bs, int64_t sector_num,
> >   
> 
> Why is sector_num signed? An unsigned quantity would simplify the code below
> (no need to check for < 0).

It's part of the block device interface:

    int (*bdrv_write)(BlockDriverState *bs, int64_t sector_num,
                          const uint8_t *buf, int nb_sectors);

Changing it would require changing every other block driver. Shall we do
that?


> 
> >      uint64_t cluster_offset;
> >      static int cid_update = 0;
> >  
> > -    if (sector_num > bs->total_sectors) {
> > +    if (sector_num < 0 || sector_num > bs->total_sectors) {
> >          fprintf(stderr,
> >                  "(VMDK) Wrong offset: sector_num=0x%" PRIx64
> >                  " total_sectors=0x%" PRIx64 "\n",
> >   
> 
> Regards
> Stefan Weil
-- 
Eduardo

Re: [Qemu-devel] [PATCH 4/4] Fix CVE-2008-0928 - insufficient block device address range checking

[Aurelien Jarno]

On Thu, Feb 19, 2009 at 06:19:36PM -0300, Eduardo Habkost wrote:
> From: Aurelien Jarno <aurel32>
> 
> This is based on an old patch commited by Aurelien Jarno whose commit
> message was:
> 
>   Fix CVE-2008-0928 - insufficient block device address range checking
> 
>   Qemu 0.9.1 and earlier does not perform range checks for block device
>   read or write requests, which allows guest host users with root
>   privileges to access arbitrary memory and escape the virtual machine.
> 
> In addition to the changes done by the previous patch, this patch changes
> total_sectors to total_bytes, so that the range checking works for
> backing devices that are not sector-based (for example, when block-qcow
> is reading the backing file). This was done to avoid bugs such as:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=485148
> 

I don't think it addresses comments from Fabrice Bellard [1], that was
the primarily reason why this patch has been reverted [2]. He asked
that the tests are done in block-{qcow,qcow2,vmdk}.c.

[1] http://lists.gnu.org/archive/html/qemu-devel/2008-03/msg00128.html
[2] http://lists.gnu.org/archive/html/qemu-devel/2008-03/msg00132.html

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net