From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Ld62f-00017Y-B9 for qemu-devel@nongnu.org; Fri, 27 Feb 2009 11:58:29 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Ld62e-00017K-CO for qemu-devel@nongnu.org; Fri, 27 Feb 2009 11:58:28 -0500 Received: from [199.232.76.173] (port=59226 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Ld62e-00017H-6S for qemu-devel@nongnu.org; Fri, 27 Feb 2009 11:58:28 -0500 Received: from caiajhbdccah.dreamhost.com ([208.97.132.207]:56337 helo=homiemail-a2.g.dreamhost.com) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Ld62d-0003UX-K0 for qemu-devel@nongnu.org; Fri, 27 Feb 2009 11:58:27 -0500 Date: Fri, 27 Feb 2009 13:57:43 -0300 From: Eduardo Habkost Message-ID: <20090227165743.GE5000@blackpad> References: <1235751623-26362-1-git-send-email-aliguori@us.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1235751623-26362-1-git-send-email-aliguori@us.ibm.com> Subject: [Qemu-devel] Re: [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: Anthony Liguori Cc: qemu-devel@nongnu.org, Aurelien Jarno , Paul Brook On Fri, Feb 27, 2009 at 10:20:23AM -0600, Anthony Liguori wrote: > 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. Agreed. > > 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; Is this really safe on all places where bdrv_file_open() is called? The original patch has added a BDRV_O_AUTOGROW and it was used on most bdrv_file_open() calls, but not on block-vpc.c. > *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; The original fix didn't allow out-of-bound read requests even on growable devices. But I think the above is safe: raw_pread() should return an error on out-of-bound read requests. > + > + len = bdrv_getlength(bs); Cool, this helps solving two problems with the original approach: - The block-based vs. byte-based range checking (https://bugzilla.redhat.com/show_bug.cgi?id=485148) - Removable devices where we can't be sure the media hasn't changed since the last time we checked the length The only thing that I am worried about is the performance impact of calling raw_getlength() on every request for raw devices such as CD-ROMs. I hope it won't trigger some expensive operation on the physical device every time we ask for the media size. > + > + 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; Uh? Where is this feature used? > + 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; > > -- Eduardo