|
From: | Anthony Liguori |
Subject: | [Qemu-devel] Re: [PATCH][RFC] Fix CVE-2008-0928 - insufficient block device address range checking |
Date: | Fri, 27 Feb 2009 11:07:58 -0600 |
User-agent: | Thunderbird 2.0.0.19 (X11/20090105) |
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 <address@hidden> 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
[Prev in Thread] | Current Thread | [Next in Thread] |