From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=60841 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Odnlo-0005Xh-HT for qemu-devel@nongnu.org; Tue, 27 Jul 2010 13:16:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1Odnln-0003RK-5V for qemu-devel@nongnu.org; Tue, 27 Jul 2010 13:16:48 -0400 Received: from mail-gy0-f173.google.com ([209.85.160.173]:59885) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Odnln-0003RC-1i for qemu-devel@nongnu.org; Tue, 27 Jul 2010 13:16:47 -0400 Received: by gyd10 with SMTP id 10so1513940gyd.4 for ; Tue, 27 Jul 2010 10:16:46 -0700 (PDT) Message-ID: <4C4F147C.1000508@codemonkey.ws> Date: Tue, 27 Jul 2010 12:16:44 -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> In-Reply-To: <4C4F10D4.8020708@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: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? 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? Regards, Anthony Liguori > > Without the assert, the install work fine. > > Regards, >