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

[Anthony Liguori <anthony@codemonkey.ws>]

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?

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?

Regards,

Anthony Liguori


>
> Without the assert, the install work fine.
>
> Regards,
>