From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33827) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XrlQk-0001zX-HU for qemu-devel@nongnu.org; Fri, 21 Nov 2014 05:27:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XrlQd-0004uz-Mv for qemu-devel@nongnu.org; Fri, 21 Nov 2014 05:27:10 -0500 Received: from mx1.redhat.com ([209.132.183.28]:33354) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XrlQd-0004us-FS for qemu-devel@nongnu.org; Fri, 21 Nov 2014 05:27:03 -0500 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sALAR11P031504 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Fri, 21 Nov 2014 05:27:02 -0500 Date: Fri, 21 Nov 2014 10:26:57 +0000 From: "Dr. David Alan Gilbert" Message-ID: <20141121102656.GF3165@work-vm> References: <1416497234-29880-1-git-send-email-kwolf@redhat.com> <1416497234-29880-8-git-send-email-kwolf@redhat.com> <20141120200841.GK5983@work-vm> <20141121101543.GB3956@noname.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20141121101543.GB3956@noname.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: > Am 20.11.2014 um 21:08 hat Dr. David Alan Gilbert geschrieben: > > * 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. > > The probing functions are trivial. You can audit them in no time even > with no previous block layer experience. They just do a few tests on the > passed buffer. Hmm, OK. > > 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 ? > > We already log a warning on bdrv_open(). Don't you think that should be > enough? Imagine a place that's got lots of QEMUs running already, they'll install the QEMU that has this fix in it and carry on; they won't look at their logs. Then if they hit a problem with a write failure they'll get confused; the logs in bdrv_open just tell them they should be doing something safely, they don't tell them they've actually hit a case fo something trying to do the write. Dave > Kevin -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK