From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Ld6CA-0005Pj-UE for qemu-devel@nongnu.org; Fri, 27 Feb 2009 12:08:19 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Ld6C9-0005Oh-B5 for qemu-devel@nongnu.org; Fri, 27 Feb 2009 12:08:18 -0500 Received: from [199.232.76.173] (port=34736 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Ld6C9-0005Oa-4t for qemu-devel@nongnu.org; Fri, 27 Feb 2009 12:08:17 -0500 Received: from e31.co.us.ibm.com ([32.97.110.149]:55850) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1Ld6C8-0005A7-K7 for qemu-devel@nongnu.org; Fri, 27 Feb 2009 12:08:16 -0500 Received: from d03relay02.boulder.ibm.com (d03relay02.boulder.ibm.com [9.17.195.227]) by e31.co.us.ibm.com (8.13.1/8.13.1) with ESMTP id n1RH5fIY019516 for ; Fri, 27 Feb 2009 10:05:41 -0700 Received: from d03av01.boulder.ibm.com (d03av01.boulder.ibm.com [9.17.195.167]) by d03relay02.boulder.ibm.com (8.13.8/8.13.8/NCO v9.2) with ESMTP id n1RH82p7178422 for ; Fri, 27 Feb 2009 10:08:05 -0700 Received: from d03av01.boulder.ibm.com (loopback [127.0.0.1]) by d03av01.boulder.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id n1RH81dD031856 for ; Fri, 27 Feb 2009 10:08:02 -0700 Message-ID: <49A81DEE.9060909@us.ibm.com> Date: Fri, 27 Feb 2009 11:07:58 -0600 From: Anthony Liguori MIME-Version: 1.0 References: <1235751623-26362-1-git-send-email-aliguori@us.ibm.com> <20090227165743.GE5000@blackpad> In-Reply-To: <20090227165743.GE5000@blackpad> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit 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: qemu-devel@nongnu.org, 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 >> >> 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