Re: [Qemu-devel] [PATCH] raw: Fix image header protection

[Kevin Wolf]

Am 09.09.2010 14:30, schrieb Anthony Liguori:
> On 09/07/2010 07:08 AM, Kevin Wolf wrote:
>> Recenty a patch was committed to protect the first four bytes of an image to
>> avoid "converting" a probed raw image to a different format when a malicious
>> guest writes e.g. a qcow2 header to it.
>>
>> This check relies on the assumption that all qiov entries are multiples of 
>> 512,
>> which isn't true in practice. At least the installers of some Windows 
>> versions
>> are reported to fail the corresponding assertion.
>>
>> This patch removes the wrong assumption and fixes Win 2003 installation for 
>> me
>> (other Windows versions not tested, should be the same)
>>
>> Signed-off-by: Kevin Wolf<address@hidden>
>> ---
>>   block/raw.c   |   57 
>> ++++++++++++++++++++++++---------------------------------
>>   cutils.c      |   16 ++++++++++++----
>>   qemu-common.h |    1 +
>>   3 files changed, 37 insertions(+), 37 deletions(-)
>>
>> diff --git a/block/raw.c b/block/raw.c
>> index 61e6748..3286550 100644
>> --- a/block/raw.c
>> +++ b/block/raw.c
>> @@ -99,6 +99,7 @@ typedef struct RawScrubberBounce
>>   {
>>       BlockDriverCompletionFunc *cb;
>>       void *opaque;
>> +    uint8_t *buf;
>>       QEMUIOVector qiov;
>>   } RawScrubberBounce;
>>
>> @@ -113,6 +114,7 @@ static void raw_aio_writev_scrubbed(void *opaque, int 
>> ret)
>>       }
>>
>>       qemu_iovec_destroy(&b->qiov);
>> +    qemu_free(b->buf);
>>       qemu_free(b);
>>   }
>>
>> @@ -120,46 +122,35 @@ 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;
>> -        }
>> -    }
>> +    if (bs->probed) {
>> +        uint8_t first_buf[512];
>> +        qemu_iovec_part_to_buffer(qiov, first_buf, 512);
>>
>> -    first_buf = qiov->iov[first_buf_index].iov_base;
>> +        if (check_write_unsafe(bs, sector_num, first_buf, nb_sectors)) {
>> +            RawScrubberBounce *b;
>> +            int ret;
>>
>> -    if (check_write_unsafe(bs, sector_num, first_buf, nb_sectors)) {
>> -        RawScrubberBounce *b;
>> -        int ret;
>> +            /* write the first sector using sync I/O */
>> +            ret = raw_write_scrubbed_bootsect(bs, first_buf);
>> +            if (ret<  0) {
>> +                return NULL;
>> +            }
>>
>> -        /* write the first sector using sync I/O */
>> -        ret = raw_write_scrubbed_bootsect(bs, first_buf);
>> -        if (ret<  0) {
>> -            return NULL;
>> -        }
>> -
>> -        /* adjust request to be everything but first sector */
>> +            /* adjust request to be everything but first sector */
>>
>> -        b = qemu_malloc(sizeof(*b));
>> -        b->cb = cb;
>> -        b->opaque = opaque;
>> +            b = qemu_malloc(sizeof(*b));
>> +            b->cb = cb;
>> +            b->opaque = opaque;
>>
>> -        qemu_iovec_init(&b->qiov, qiov->nalloc);
>> -        qemu_iovec_concat(&b->qiov, qiov, qiov->size);
>> +            b->buf = qemu_malloc(qiov->size);
>> +            qemu_iovec_to_buffer(qiov, b->buf);
>>    
> 
> Isn't this an unbounded, guest controlled, malloc?  IOW, a guest could 
> do a request of 4GB and on a 32-bit system crash the qemu instance.

If you're concerned about that, we need to ban qemu_iovec_to_buffer()
completely. Currently we do the same thing for every write request for
every format but raw. Or instead of completely removing it, we could add
a size limit, though I suspect that would mean violating some specs.

If I was a guest though and wanted to crash qemu, I would just mess up
the virtio ring a bit so that qemu would exit() voluntarily. ;-)

Kevin