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<aligu...@us.ibm.com>
>>> ---
>>> 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.
Kevin
