From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=49849 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OrRr3-0000Bl-S9 for qemu-devel@nongnu.org; Fri, 03 Sep 2010 04:42:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OrRr2-0007sW-K3 for qemu-devel@nongnu.org; Fri, 03 Sep 2010 04:42:37 -0400 Received: from mx1.redhat.com ([209.132.183.28]:2622) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OrRr2-0007sJ-Co for qemu-devel@nongnu.org; Fri, 03 Sep 2010 04:42:36 -0400 Message-ID: <4C80B4FD.4080703@redhat.com> Date: Fri, 03 Sep 2010 10:42:37 +0200 From: Kevin Wolf 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> <4C4F1AD4.9050905@citrix.com> <4C4F247C.8080405@codemonkey.ws> In-Reply-To: <4C4F247C.8080405@codemonkey.ws> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Anthony PERARD , "qemu-devel@nongnu.org" , Stefan Hajnoczi Am 27.07.2010 20:25, schrieb Anthony Liguori: > On 07/27/2010 12:43 PM, Anthony PERARD wrote: >> 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 ?? () > > Thanks. I see the problem. Working on a patch now. > > Regards, > > Anthony Liguori Anthony, what happened with this one? I can't see a patch applied for this and I just saw a similar report on the fedora-virt mailing list (no backtrace yet, but the same assertion triggering). Kevin