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<kw...@redhat.com>
>> ---
>> 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
