From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=40537 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OdoqM-0006YU-EV for qemu-devel@nongnu.org; Tue, 27 Jul 2010 14:25:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OdoqB-0004fO-Cw for qemu-devel@nongnu.org; Tue, 27 Jul 2010 14:25:24 -0400 Received: from mail-yx0-f173.google.com ([209.85.213.173]:62337) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OdoqB-0004fK-61 for qemu-devel@nongnu.org; Tue, 27 Jul 2010 14:25:23 -0400 Received: by yxn35 with SMTP id 35so640163yxn.4 for ; Tue, 27 Jul 2010 11:25:22 -0700 (PDT) Message-ID: <4C4F247C.8080405@codemonkey.ws> Date: Tue, 27 Jul 2010 13:25:00 -0500 From: Anthony Liguori 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> In-Reply-To: <4C4F1AD4.9050905@citrix.com> 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 PERARD Cc: Kevin Wolf , "qemu-devel@nongnu.org" , Stefan Hajnoczi 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 >> Regards, >