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

[Anthony Liguori <anthony@codemonkey.ws>]

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 <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 ?? ()

Thanks.  I see the problem.  Working on a patch now.

Regards,

Anthony Liguori

>> Regards,
>