From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=51228 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OZNpL-0004dM-UE for qemu-devel@nongnu.org; Thu, 15 Jul 2010 08:46:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OZNpG-0003L9-2Q for qemu-devel@nongnu.org; Thu, 15 Jul 2010 08:46:11 -0400 Received: from e32.co.us.ibm.com ([32.97.110.150]:54397) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OZNpF-0003Ke-TS for qemu-devel@nongnu.org; Thu, 15 Jul 2010 08:46:06 -0400 Received: from d03relay02.boulder.ibm.com (d03relay02.boulder.ibm.com [9.17.195.227]) by e32.co.us.ibm.com (8.14.4/8.13.1) with ESMTP id o6FCc7Zn008810 for ; Thu, 15 Jul 2010 06:38:07 -0600 Received: from d03av01.boulder.ibm.com (d03av01.boulder.ibm.com [9.17.195.167]) by d03relay02.boulder.ibm.com (8.13.8/8.13.8/NCO v9.1) with ESMTP id o6FCjtlx113190 for ; Thu, 15 Jul 2010 06:45:57 -0600 Received: from d03av01.boulder.ibm.com (loopback [127.0.0.1]) by d03av01.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id o6FCjj2u031223 for ; Thu, 15 Jul 2010 06:45:46 -0600 Message-ID: <4C3F02FC.3090003@linux.vnet.ibm.com> Date: Thu, 15 Jul 2010 07:45:48 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1279130069-5331-1-git-send-email-aliguori@us.ibm.com> <4C3EC317.3020706@redhat.com> <4C3EFF05.9010100@linux.vnet.ibm.com> <4C3F0296.4050300@redhat.com> In-Reply-To: <4C3F0296.4050300@redhat.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed 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: Kevin Wolf Cc: qemu-devel@nongnu.org, Stefan Hajnoczi On 07/15/2010 07:44 AM, Kevin Wolf wrote: > Am 15.07.2010 14:28, schrieb Anthony Liguori: > >> On 07/15/2010 03:13 AM, Kevin Wolf wrote: >> >>> 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. >>> >>> >> I'm quite confident that iovec buffers are never less than 512 in size. >> While it could be more paranoid, I don't think the added complexity helps. >> > We rely on that anyway, we'd overflow iov_len otherwise. Maybe adding an > assert there wouldn't hurt. But I'm fine either way. > An assert's a good idea. Regards, Anthony Liguori > Kevin >