From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=48875 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OtgU4-00012L-MG for qemu-devel@nongnu.org; Thu, 09 Sep 2010 08:44:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OtgU3-0006vM-GJ for qemu-devel@nongnu.org; Thu, 09 Sep 2010 08:44:08 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44853) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OtgU3-0006vB-9a for qemu-devel@nongnu.org; Thu, 09 Sep 2010 08:44:07 -0400 Message-ID: <4C88D6A3.6050001@redhat.com> Date: Thu, 09 Sep 2010 14:44:19 +0200 From: Kevin Wolf MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] raw: Fix image header protection References: <1283861325-23785-1-git-send-email-kwolf@redhat.com> <4C88D381.5030306@codemonkey.ws> In-Reply-To: <4C88D381.5030306@codemonkey.ws> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: qemu-devel@nongnu.org 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 >> --- >> 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