[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH] Make default invocation of block drivers safer
From: |
Kevin Wolf |
Subject: |
[Qemu-devel] Re: [PATCH] Make default invocation of block drivers safer (v2) |
Date: |
Thu, 15 Jul 2010 10:13:11 +0200 |
User-agent: |
Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.9) Gecko/20100430 Fedora/3.0.4-2.fc12 Thunderbird/3.0.4 |
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 <address@hidden>
> ---
> 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