* [Qemu-devel] [PATCH][RFC] Fix CVE-2008-0928 - insufficient block device address range checking
@ 2009-02-27 16:20 Anthony Liguori
2009-02-27 16:57 ` [Qemu-devel] " Eduardo Habkost
0 siblings, 1 reply; 3+ messages in thread
From: Anthony Liguori @ 2009-02-27 16:20 UTC (permalink / raw)
To: qemu-devel; +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 <aliguori@us.ibm.com>
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;
^ permalink raw reply related [flat|nested] 3+ messages in thread* [Qemu-devel] Re: [PATCH][RFC] Fix CVE-2008-0928 - insufficient block device address range checking
2009-02-27 16:20 [Qemu-devel] [PATCH][RFC] Fix CVE-2008-0928 - insufficient block device address range checking Anthony Liguori
@ 2009-02-27 16:57 ` Eduardo Habkost
2009-02-27 17:07 ` Anthony Liguori
0 siblings, 1 reply; 3+ messages in thread
From: Eduardo Habkost @ 2009-02-27 16:57 UTC (permalink / raw)
To: Anthony Liguori; +Cc: qemu-devel, 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 <aliguori@us.ibm.com>
>
> 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
^ permalink raw reply [flat|nested] 3+ messages in thread* [Qemu-devel] Re: [PATCH][RFC] Fix CVE-2008-0928 - insufficient block device address range checking
2009-02-27 16:57 ` [Qemu-devel] " Eduardo Habkost
@ 2009-02-27 17:07 ` Anthony Liguori
0 siblings, 0 replies; 3+ messages in thread
From: Anthony Liguori @ 2009-02-27 17:07 UTC (permalink / raw)
To: qemu-devel, Aurelien Jarno, Paul Brook
Eduardo Habkost wrote:
>> 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 <aliguori@us.ibm.com>
>>
>> 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.
>
Yes. This is what bdrv_file_open() means as opposed to bdrv_open().
FWIW, I didn't write the original patch, but my guess is that
block-vpc.c write support is relatively new. Before write support was
added (less than a month ago), there was no reason to allow a VPC to
grow. But this definitely demonstrates a problem with the old patch. It
would have broken block-vpc write support.
>
> 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.
>
An out of bound read request will return an error, so, yeah, I'm
perfectly happy with it.
>> +
>> + 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 it does, then we'll fix that properly. bdrv_getlength() needs to be
fast to be useful.
>> +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?
>
SCSI generic pass through. Avi has patches to eliminate this an
introduce a new API but they haven't been merged yet.
Regards,
Anthony Liguori
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2009-02-27 17:08 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-02-27 16:20 [Qemu-devel] [PATCH][RFC] Fix CVE-2008-0928 - insufficient block device address range checking Anthony Liguori
2009-02-27 16:57 ` [Qemu-devel] " Eduardo Habkost
2009-02-27 17:07 ` Anthony Liguori
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).