From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Ld5Rt-0004Ve-W7 for qemu-devel@nongnu.org; Fri, 27 Feb 2009 11:20:30 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Ld5Rs-0004VS-EZ for qemu-devel@nongnu.org; Fri, 27 Feb 2009 11:20:28 -0500 Received: from [199.232.76.173] (port=59242 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Ld5Rs-0004VP-7B for qemu-devel@nongnu.org; Fri, 27 Feb 2009 11:20:28 -0500 Received: from e3.ny.us.ibm.com ([32.97.182.143]:47615) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1Ld5Rr-0006Ol-TO for qemu-devel@nongnu.org; Fri, 27 Feb 2009 11:20:28 -0500 Received: from d01relay02.pok.ibm.com (d01relay02.pok.ibm.com [9.56.227.234]) by e3.ny.us.ibm.com (8.13.1/8.13.1) with ESMTP id n1RGHtvp003259 for ; Fri, 27 Feb 2009 11:17:55 -0500 Received: from d01av03.pok.ibm.com (d01av03.pok.ibm.com [9.56.224.217]) by d01relay02.pok.ibm.com (8.13.8/8.13.8/NCO v9.2) with ESMTP id n1RGKQYJ195312 for ; Fri, 27 Feb 2009 11:20:26 -0500 Received: from d01av03.pok.ibm.com (loopback [127.0.0.1]) by d01av03.pok.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id n1RGKPlN029218 for ; Fri, 27 Feb 2009 11:20:26 -0500 From: Anthony Liguori Date: Fri, 27 Feb 2009 10:20:23 -0600 Message-Id: <1235751623-26362-1-git-send-email-aliguori@us.ibm.com> Subject: [Qemu-devel] [PATCH][RFC] Fix CVE-2008-0928 - insufficient block device address range checking Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: Anthony Liguori , Eduardo Habkost , Paul Brook , Aurelien Jarno 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 diff --git a/block.c b/block.c index 4f4bf7c..ab88d05 100644 --- a/block.c +++ b/block.c @@ -318,6 +318,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags) bdrv_delete(bs); return ret; } + bs->growable = 1; *pbs = bs; return 0; } @@ -519,6 +520,39 @@ int bdrv_commit(BlockDriverState *bs) 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 @@ int bdrv_read(BlockDriverState *bs, int64_t sector_num, 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 @@ int bdrv_write(BlockDriverState *bs, int64_t sector_num, 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 @@ int bdrv_pread(BlockDriverState *bs, int64_t offset, 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 @@ int bdrv_pwrite(BlockDriverState *bs, int64_t offset, 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 @@ BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num, 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 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num, 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 @@ BlockDriverAIOCB *bdrv_aio_read(BlockDriverState *bs, int64_t sector_num, 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 @@ BlockDriverAIOCB *bdrv_aio_write(BlockDriverState *bs, int64_t sector_num, 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); diff --git a/block_int.h b/block_int.h index 781789c..e1943aa 100644 --- a/block_int.h +++ b/block_int.h @@ -121,6 +121,9 @@ struct BlockDriverState { 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;