[Qemu-devel] Re: [PATCH] Make default invocation of block drivers safer (v2)

[Kevin Wolf]

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<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.
>>    
> 
> 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