From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=53394 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OdoKc-0007U3-MG for qemu-devel@nongnu.org; Tue, 27 Jul 2010 13:52:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OdoC1-0007K6-PA for qemu-devel@nongnu.org; Tue, 27 Jul 2010 13:43:54 -0400 Received: from smtp02.citrix.com ([66.165.176.63]:8169) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OdoC1-0007K1-Fl for qemu-devel@nongnu.org; Tue, 27 Jul 2010 13:43:53 -0400 Message-ID: <4C4F1AD4.9050905@citrix.com> Date: Tue, 27 Jul 2010 18:43:48 +0100 From: Anthony PERARD MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] Make default invocation of block drivers safer (v3) References: <1279198257-23681-1-git-send-email-aliguori@us.ibm.com> <4C4F10D4.8020708@citrix.com> <4C4F147C.1000508@codemonkey.ws> In-Reply-To: <4C4F147C.1000508@codemonkey.ws> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Kevin Wolf , "qemu-devel@nongnu.org" , Stefan Hajnoczi Anthony Liguori wrote: > On 07/27/2010 12:01 PM, Anthony PERARD wrote: >> Anthony Liguori wrote: >>> CVE-2008-2004 described a vulnerability in QEMU whereas a malicious >>> user could >>> trick the block probing code into accessing arbitrary files in a >>> guest. To >>> mitigate this, we added an explicit format parameter to -drive which >>> disabling >>> block probing. >>> >>> Fast forward to today, and the vast majority of users do not use this >>> parameter. >>> libvirt does not use this by default nor does virt-manager. >>> >>> Most users want block probing so we should try to make it safer. >>> >>> This patch adds some logic to the raw device which attempts to detect >>> a write >>> operation to the beginning of a raw device. If the first 4 bytes >>> happen to >>> match an image file that has a backing file that we support, it >>> scrubs the >>> signature to all zeros. If a user specifies an explicit format >>> parameter, this >>> behavior is disabled. >>> >>> I contend that while a legitimate guest could write such a signature >>> to the >>> header, we would behave incorrectly anyway upon the next invocation >>> of QEMU. >>> This simply changes the incorrect behavior to not involve a security >>> vulnerability. >>> >>> I've tested this pretty extensively both in the positive and negative >>> case. I'm >>> not 100% confident in the block layer's ability to deal with zero >>> sized writes >>> particularly with respect to the aio functions so some additional >>> eyes would be >>> appreciated. >>> >>> Even in the case of a single sector write, we have to make sure to >>> invoked the >>> completion from a bottom half so just removing the zero sized write >>> is not an >>> option. >>> >>> Signed-off-by: Anthony Liguori >>> --- >>> v2 -> v3 >>> - add an assert to ensure the first iovec element is at least 512 bytes >>> v1 -> v2 >>> - be more paranoid about empty iovecs >>> --- >>> block.c | 4 ++ >>> block/raw.c | 130 >>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> block_int.h | 1 + >>> 3 files changed, 135 insertions(+), 0 deletions(-) >> >>> static BlockDriverAIOCB *raw_aio_writev(BlockDriverState *bs, >>> int64_t sector_num, QEMUIOVector *qiov, int nb_sectors, >>> BlockDriverCompletionFunc *cb, void *opaque) >>> { >>> + const uint8_t *first_buf; >>> + int first_buf_index = 0, i; >>> + >>> + /* This is probably being paranoid, but handle cases of zero size >>> + vectors. */ >>> + for (i = 0; i < qiov->niov; i++) { >>> + if (qiov->iov[i].iov_len) { >>> + assert(qiov->iov[i].iov_len >= 512); >>> + first_buf_index = i; >>> + break; >>> + } >>> + } >> Hi, >> >> I have try to do an installation of Windows XP SP2, with qemu fd2f659, >> and the Assertion failed when windows begin to format the disk. >> >> The command line and the error message: >> $ i386-softmmu/qemu -hda vm.img -cdrom winxpsp2.iso -boot dc >> qemu: qemu/block/raw.c:130: raw_aio_writev: Assertion >> `qiov->iov[i].iov_len >= 512' failed. >> >> And here, a little more information about the iov: >> (gdb) p *qiov >> $2 = {iov = 0x9106010, niov = 2, nalloc = 2, size = 512} >> (gdb) p qiov->iov[0] >> $3 = {iov_base = 0xaff3ce90, iov_len = 368} >> (gdb) p qiov->iov[1] >> $4 = {iov_base = 0xaff3f000, iov_len = 144} > > How can a single sector request be split between two iovs in QEMU? Are > you carrying any patches in the version of QEMU that you're testing? Is > this qemu-dm? Nop, I don't have any patch for this test. Is not qemu-dm. > To be clear, this is a discontiguous request. I'm looking at the core > now in core.c and I don't see how an IDE disk can generate a request > that looks like this. > > Can you provide a full stack trace? #0 0xb77dd424 in __kernel_vsyscall () #1 0xb7418640 in raise () from /lib/i686/cmov/libc.so.6 #2 0xb741a018 in abort () from /lib/i686/cmov/libc.so.6 #3 0xb74115be in __assert_fail () from /lib/i686/cmov/libc.so.6 #4 0x08074d30 in raw_aio_writev (bs=0xa5bcec0, sector_num=63, qiov=0xa67cf14, nb_sectors=1, cb=0x81ae8c0 , opaque=0xa67cee0) at /tmp/qemu-merge/block/raw.c:130 #5 0x0806d024 in bdrv_aio_writev (bs=0xa5bcec0, sector_num=63, qiov=0xa67cf14, nb_sectors=1, cb=0x81ae8c0 , opaque=0xa67cee0) at /tmp/qemu-merge/block.c:2004 #6 0x081aea78 in dma_bdrv_cb (opaque=0xa67cee0, ret=0) at /tmp/qemu-merge/dma-helpers.c:120 #7 0x081aebc9 in dma_bdrv_io (bs=0xa5bcec0, sg=0xa61bd48, sector_num=63, cb=0x81a9380 , opaque=0xa61c684, is_write=1) at /tmp/qemu-merge/dma-helpers.c:163 #8 0x081a9484 in ide_write_dma_cb (opaque=0xa61c684, ret=0) at /tmp/qemu-merge/hw/ide/core.c:748 #9 0x081a9eba in bmdma_cmd_writeb (opaque=0xa61c684, addr=49152, val=1) at /tmp/qemu-merge/hw/ide/pci.c:51 #10 0x080a6b7b in cpu_outb (addr=6, val=) at /tmp/qemu-merge/ioport.c:80 #11 0xb5c95609 in ?? () #12 0x0000c000 in ?? () #13 0x00000001 in ?? () #14 0xff0a0000 in ?? () #15 0xbfa41448 in ?? () #16 0x00000000 in ?? () > Regards, -- Anthony PERARD