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

[Kevin Wolf]

Am 27.07.2010 20:25, schrieb Anthony Liguori:
> On 07/27/2010 12:43 PM, Anthony PERARD wrote:
>> 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 <address@hidden>
>>>>> ---
>>>>> 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 ?? ()
> 
> Thanks.  I see the problem.  Working on a patch now.
> 
> Regards,
> 
> Anthony Liguori

Anthony, what happened with this one? I can't see a patch applied for
this and I just saw a similar report on the fedora-virt mailing list (no
backtrace yet, but the same assertion triggering).

Kevin