From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45442) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XrY2A-0006iV-Tp for qemu-devel@nongnu.org; Thu, 20 Nov 2014 15:09:01 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XrY24-0003kQ-Ox for qemu-devel@nongnu.org; Thu, 20 Nov 2014 15:08:54 -0500 Received: from mx1.redhat.com ([209.132.183.28]:57795) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XrY24-0003kF-HB for qemu-devel@nongnu.org; Thu, 20 Nov 2014 15:08:48 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sAKK8lEG011322 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Thu, 20 Nov 2014 15:08:47 -0500 Date: Thu, 20 Nov 2014 20:08:42 +0000 From: "Dr. David Alan Gilbert" Message-ID: <20141120200841.GK5983@work-vm> References: <1416497234-29880-1-git-send-email-kwolf@redhat.com> <1416497234-29880-8-git-send-email-kwolf@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1416497234-29880-8-git-send-email-kwolf@redhat.com> Subject: Re: [Qemu-devel] [PATCH v3 7/9] raw: Prohibit dangerous writes for probed images List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: jcody@redhat.com, mreitz@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, armbru@redhat.com * Kevin Wolf (kwolf@redhat.com) wrote: > diff --git a/block/raw_bsd.c b/block/raw_bsd.c > index 401b967..2ce5409 100644 > --- a/block/raw_bsd.c > +++ b/block/raw_bsd.c > @@ -58,8 +58,58 @@ static int coroutine_fn raw_co_readv(BlockDriverState *bs, int64_t sector_num, > static int coroutine_fn raw_co_writev(BlockDriverState *bs, int64_t sector_num, > int nb_sectors, QEMUIOVector *qiov) > { > + void *buf = NULL; > + BlockDriver *drv; > + QEMUIOVector local_qiov; > + int ret; > + > + if (bs->probed && sector_num == 0) { > + /* As long as these conditions are true, we can't get partial writes to > + * the probe buffer and can just directly check the request. */ > + QEMU_BUILD_BUG_ON(BLOCK_PROBE_BUF_SIZE != 512); > + QEMU_BUILD_BUG_ON(BDRV_SECTOR_SIZE != 512); > + > + if (nb_sectors == 0) { > + /* qemu_iovec_to_buf() would fail, but we want to return success > + * instead of -EINVAL in this case. */ > + return 0; > + } > + > + buf = qemu_try_blockalign(bs->file, 512); > + if (!buf) { > + ret = -ENOMEM; > + goto fail; > + } > + > + ret = qemu_iovec_to_buf(qiov, 0, buf, 512); > + if (ret != 512) { > + ret = -EINVAL; > + goto fail; > + } > + > + drv = bdrv_probe_all(buf, 512, NULL); > + if (drv != bs->drv) { > + ret = -EPERM; > + goto fail; > + } Two things about this worry me: 1) It allows a running guest to prod at the probing code potentially quite hard; if there is anything nasty that can be done during probing it would potentially make it easier for a guest to find it. 2) We don't log anything when this failure happens so if someone hits this by accident for some reason it'll confuse them no end. Could we add a (1 time?) error_report/printf just so that there's something to work with ? Dave > + > + /* Use the checked buffer, a malicious guest might be overwriting its > + * original buffer in the background. */ > + qemu_iovec_init(&local_qiov, qiov->niov + 1); > + qemu_iovec_add(&local_qiov, buf, 512); > + qemu_iovec_concat(&local_qiov, qiov, 512, qiov->size - 512); > + qiov = &local_qiov; > + } > + > BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO); > - return bdrv_co_writev(bs->file, sector_num, nb_sectors, qiov); > + ret = bdrv_co_writev(bs->file, sector_num, nb_sectors, qiov); > + > +fail: > + if (qiov == &local_qiov) { > + qemu_iovec_destroy(&local_qiov); > + } > + qemu_vfree(buf); > + return ret; > } > > static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs, > @@ -158,6 +208,18 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags, > Error **errp) > { > bs->sg = bs->file->sg; > + > + if (bs->probed && !bdrv_is_read_only(bs)) { > + fprintf(stderr, > + "WARNING: Image format was not specified for '%s' and probing " > + "guessed raw.\n" > + " Automatically detecting the format is dangerous for " > + "raw images, write operations on block 0 will be restricted.\n" > + " Specify the 'raw' format explicitly to remove the " > + "restrictions.\n", > + bs->file->filename); > + } > + > return 0; > } > > diff --git a/include/block/block_int.h b/include/block/block_int.h > index cd94559..192fce8 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -326,6 +326,7 @@ struct BlockDriverState { > int sg; /* if true, the device is a /dev/sg* */ > int copy_on_read; /* if true, copy read backing sectors into image > note this is a reference count */ > + bool probed; > > BlockDriver *drv; /* NULL means no media */ > void *opaque; > @@ -414,6 +415,8 @@ struct BlockDriverState { > }; > > int get_tmp_filename(char *filename, int size); > +BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size, > + const char *filename); > > void bdrv_set_io_limits(BlockDriverState *bs, > ThrottleConfig *cfg); > -- > 1.8.3.1 > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK