From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=33872 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OZJZG-0001K9-UL for qemu-devel@nongnu.org; Thu, 15 Jul 2010 04:13:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OZJZF-0006pV-Kw for qemu-devel@nongnu.org; Thu, 15 Jul 2010 04:13:18 -0400 Received: from mx1.redhat.com ([209.132.183.28]:29241) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OZJZF-0006pP-EF for qemu-devel@nongnu.org; Thu, 15 Jul 2010 04:13:17 -0400 Message-ID: <4C3EC317.3020706@redhat.com> Date: Thu, 15 Jul 2010 10:13:11 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1279130069-5331-1-git-send-email-aliguori@us.ibm.com> In-Reply-To: <1279130069-5331-1-git-send-email-aliguori@us.ibm.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH] Make default invocation of block drivers safer (v2) List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: qemu-devel@nongnu.org, Stefan Hajnoczi Am 14.07.2010 19:54, schrieb Anthony Liguori: > 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 > --- > v1 -> v2 > - be more paranoid about empty iovecs > --- > block.c | 4 ++ > block/raw.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > block_int.h | 1 + > 3 files changed, 134 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) { > + first_buf_index = i; > + break; > + } > + } > + > + first_buf = qiov->iov[first_buf_index].iov_base; It's still not paranoid enough for the case where the magic is spread over multiple buffers. We should probably have a qemu_iovec_to_buffer() with limited size so that you can just get 4 bytes into a temporary buffer. Kevin