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

[Anthony PERARD <anthony.perard@citrix.com>]

Anthony Liguori wrote:
> On 07/27/2010 12:01 PM, Anthony PERARD wrote:
>> Anthony Liguori wrote:
>>> 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 <aliguori@us.ibm.com>
>>> ---
>>> v2 -> v3
>>>  - add an assert to ensure the first iovec element is at least 512 bytes
>>> v1 -> v2
>>>  - be more paranoid about empty iovecs
>>> ---
>>>  block.c     |    4 ++
>>>  block/raw.c |  130 
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  block_int.h |    1 +
>>>  3 files changed, 135 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) {
>>> +            assert(qiov->iov[i].iov_len >= 512);
>>> +            first_buf_index = i;
>>> +            break;
>>> +        }
>>> +    }
>> Hi,
>>
>> I have try to do an installation of Windows XP SP2, with qemu fd2f659,
>> and the Assertion failed when windows begin to format the disk.
>>
>> The command line and the error message:
>> $ i386-softmmu/qemu -hda vm.img -cdrom winxpsp2.iso -boot dc
>> qemu: qemu/block/raw.c:130: raw_aio_writev: Assertion 
>> `qiov->iov[i].iov_len >= 512' failed.
>>
>> And here, a little more information about the iov:
>> (gdb) p *qiov
>> $2 = {iov = 0x9106010, niov = 2, nalloc = 2, size = 512}
>> (gdb) p qiov->iov[0]
>> $3 = {iov_base = 0xaff3ce90, iov_len = 368}
>> (gdb) p qiov->iov[1]
>> $4 = {iov_base = 0xaff3f000, iov_len = 144}
> 
> How can a single sector request be split between two iovs in QEMU?  Are 
> you carrying any patches in the version of QEMU that you're testing?  Is 
> this qemu-dm?

Nop, I don't have any patch for this test. Is not qemu-dm.

> To be clear, this is a discontiguous request. I'm looking at the core 
> now in core.c and I don't see how an IDE disk can generate a request 
> that looks like this.
> 
> Can you provide a full stack trace?

#0  0xb77dd424 in __kernel_vsyscall ()
#1  0xb7418640 in raise () from /lib/i686/cmov/libc.so.6
#2  0xb741a018 in abort () from /lib/i686/cmov/libc.so.6
#3  0xb74115be in __assert_fail () from /lib/i686/cmov/libc.so.6
#4  0x08074d30 in raw_aio_writev (bs=0xa5bcec0, sector_num=63, qiov=0xa67cf14, nb_sectors=1, cb=0x81ae8c0 <dma_bdrv_cb>,
     opaque=0xa67cee0) at /tmp/qemu-merge/block/raw.c:130
#5  0x0806d024 in bdrv_aio_writev (bs=0xa5bcec0, sector_num=63, qiov=0xa67cf14, nb_sectors=1, cb=0x81ae8c0 <dma_bdrv_cb>,
     opaque=0xa67cee0) at /tmp/qemu-merge/block.c:2004
#6  0x081aea78 in dma_bdrv_cb (opaque=0xa67cee0, ret=0) at /tmp/qemu-merge/dma-helpers.c:120
#7  0x081aebc9 in dma_bdrv_io (bs=0xa5bcec0, sg=0xa61bd48, sector_num=63, cb=0x81a9380 <ide_write_dma_cb>, opaque=0xa61c684,
     is_write=1) at /tmp/qemu-merge/dma-helpers.c:163
#8  0x081a9484 in ide_write_dma_cb (opaque=0xa61c684, ret=0) at /tmp/qemu-merge/hw/ide/core.c:748
#9  0x081a9eba in bmdma_cmd_writeb (opaque=0xa61c684, addr=49152, val=1) at /tmp/qemu-merge/hw/ide/pci.c:51
#10 0x080a6b7b in cpu_outb (addr=6, val=<value optimized out>) at /tmp/qemu-merge/ioport.c:80
#11 0xb5c95609 in ?? ()
#12 0x0000c000 in ?? ()
#13 0x00000001 in ?? ()
#14 0xff0a0000 in ?? ()
#15 0xbfa41448 in ?? ()
#16 0x00000000 in ?? ()

> Regards,

-- 
Anthony PERARD