From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60675) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XlgFD-000462-5t for qemu-devel@nongnu.org; Tue, 04 Nov 2014 10:42:13 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XlgF6-0000yM-Lk for qemu-devel@nongnu.org; Tue, 04 Nov 2014 10:42:07 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60006) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XlgF6-0000yI-FU for qemu-devel@nongnu.org; Tue, 04 Nov 2014 10:42:00 -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 sA4FfxlR003289 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Tue, 4 Nov 2014 10:42:00 -0500 Date: Tue, 4 Nov 2014 15:41:58 +0000 From: Stefan Hajnoczi Message-ID: <20141104154158.GC28330@stefanha-thinkpad.redhat.com> References: <1414671976-5353-1-git-send-email-kwolf@redhat.com> <1414671976-5353-4-git-send-email-kwolf@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="c3bfwLpm8qysLVxt" Content-Disposition: inline In-Reply-To: <1414671976-5353-4-git-send-email-kwolf@redhat.com> Subject: Re: [Qemu-devel] [PATCH 3/4] 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, armbru@redhat.com --c3bfwLpm8qysLVxt Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Oct 30, 2014 at 01:26:15PM +0100, Kevin Wolf wrote: > If the user neglects to specify the image format, QEMU probes the > image to guess it automatically, for convenience. >=20 > Relying on format probing is insecure for raw images (CVE-2008-2004). > If the guest writes a suitable header to the device, the next probe > will recognize a format chosen by the guest. A malicious guest can > abuse this to gain access to host files, e.g. by crafting a QCOW2 > header with backing file /etc/shadow. >=20 > Commit 1e72d3b (April 2008) provided -drive parameter format to let > users disable probing. Commit f965509 (March 2009) extended QCOW2 to > optionally store the backing file format, to let users disable backing > file probing. QED has had a flag to suppress probing since the > beginning (2010), set whenever a raw backing file is assigned. >=20 > All of these additions that allow to avoid format probing have to be > specified explicitly. The default still allows the attack. >=20 > In order to fix this, commit 79368c8 (July 2010) put probed raw images > in a restricted mode, in which they wouldn't be able to overwrite the > first few bytes of the image so that they would identify as a different > image. If a write to the first sector would write one of the signatures > of another driver, qemu would instead zero out the first four bytes. > This patch was later reverted in commit 8b33d9e (September 2010) because > it didn't get the handling of unaligned qiov members right. >=20 > Today's block layer that is based on coroutines and has qiov utility > functions makes it much easier to get this functionality right, so this > patch implements it. >=20 > The other differences of this patch to the old one are that it doesn't > silently write something different than the guest requested by zeroing > out some bytes (it fails the request instead) and that it doesn't > maintain a list of signatures in the raw driver (it calls the usual > probe function instead). >=20 > Signed-off-by: Kevin Wolf > --- > block.c | 5 +++-- > block/raw_bsd.c | 46 +++++++++++++++++++++++++++++++++++++++++= ++++- > include/block/block_int.h | 3 +++ > 3 files changed, 51 insertions(+), 3 deletions(-) >=20 > diff --git a/block.c b/block.c > index 8006685..bd7b2b5 100644 > --- a/block.c > +++ b/block.c > @@ -655,8 +655,8 @@ BlockDriver *bdrv_find_protocol(const char *filename, > * probing score. > * Return the first block driver with the highest probing score. > */ > -static BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size, > - const char *filename) > +BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size, > + const char *filename) > { > int score_max =3D 0, score; > BlockDriver *drv =3D NULL, *d; > @@ -1482,6 +1482,7 @@ int bdrv_open(BlockDriverState **pbs, const char *f= ilename, > } > =20 > /* Image format probing */ > + bs->probed =3D !drv; > if (!drv && file) { > ret =3D find_image_format(file, filename, &drv, &local_err); > if (ret < 0) { > diff --git a/block/raw_bsd.c b/block/raw_bsd.c > index 401b967..80f3a50 100644 > --- a/block/raw_bsd.c > +++ b/block/raw_bsd.c > @@ -58,8 +58,52 @@ static int coroutine_fn raw_co_readv(BlockDriverState = *bs, int64_t sector_num, > static int coroutine_fn raw_co_writev(BlockDriverState *bs, int64_t sect= or_num, > int nb_sectors, QEMUIOVector *qiov) > { > + void *buf =3D NULL; > + BlockDriver *drv; > + QEMUIOVector local_qiov; > + int ret; > + > + if (bs->probed && sector_num =3D=3D 0) { > + /* As long as these conditions are true, we can't get partial wr= ites to > + * the probe buffer and can just directly check the request. */ > + QEMU_BUILD_BUG_ON(BLOCK_PROBE_BUF_SIZE !=3D 512); > + QEMU_BUILD_BUG_ON(BDRV_SECTOR_SIZE !=3D 512); > + > + buf =3D g_try_malloc(512); > + if (!buf) { > + ret =3D -ENOMEM; > + goto fail; > + } > + > + ret =3D qemu_iovec_to_buf(qiov, 0, buf, 512); > + if (ret !=3D 512) { > + ret =3D -EINVAL; > + goto fail; > + } > + > + drv =3D bdrv_probe_all(buf, 512, NULL); > + if (drv !=3D bs->drv) { > + ret =3D -EPERM; > + goto fail; > + } > + > + /* Use the checked buffer, a malicious guest might be overwritin= g 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, 0, qiov->size - 512); > + qiov =3D &local_qiov; > + } > + > BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO); > - return bdrv_co_writev(bs->file, sector_num, nb_sectors, qiov); > + ret =3D bdrv_co_writev(bs->file, sector_num, nb_sectors, qiov); > + > +fail: > + if (qiov =3D=3D &local_qiov) { > + qemu_iovec_destroy(&local_qiov); > + } > + g_free(buf); > + return ret; > } Looking at this patch, I'm surprised to say I actually kind of like it. Although I think it's problematic to restrict the guest from doing something that a real machine is allowed to do, it only happens in limited circumstances: 1. The image format was probed 2. The format was determined to be raw I'd be okay with this. Less invasive than I imagined. Stefan --c3bfwLpm8qysLVxt Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJUWPPGAAoJEJykq7OBq3PIflMIAIZeDeLGkE+u84PI7cVfCKia iPNzUILGgxFh9/XtbdzsLefKOfKepSif2Q/p8uqwQ0lm+TkiyM/SYYOcQKur0nTG uxthvA6+XrJjW92gzBZ0gtUkuhgPIKsWuZXfzKzdBTmEKgIUpLvtg41n4ACTcZiA kmUk3WUTMJEE1BXyImT0UvbtTLD10AGsGE+bm3cXNVh5COwTVIIRHAJ02yYeqe4X Z1z86doHCyNR3LzIAoiFYYuKAd1Mpz8HRTkcQwTMiQLsRiPu/otllBUvyc2Uu6JU szUNPNQyA62G/BoTrEh9W36WAz+mTlMSXgOsqUwKPed9r2v2vjgSId5Q+Y5owp8= =2cET -----END PGP SIGNATURE----- --c3bfwLpm8qysLVxt--