From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35096) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1d2hCN-0005eY-8C for qemu-devel@nongnu.org; Mon, 24 Apr 2017 12:50:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1d2hCM-0000wc-3W for qemu-devel@nongnu.org; Mon, 24 Apr 2017 12:50:51 -0400 Date: Mon, 24 Apr 2017 17:50:37 +0100 From: "Daniel P. Berrange" Message-ID: <20170424165037.GB453@redhat.com> Reply-To: "Daniel P. Berrange" References: <20170221115512.21918-1-berrange@redhat.com> <20170221115512.21918-12-berrange@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v5 11/18] qcow2: convert QCow2 to use QCryptoBlock for encryption List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alberto Garcia Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, Max Reitz , Kevin Wolf , Eric Blake On Tue, Feb 21, 2017 at 02:30:10PM +0100, Alberto Garcia wrote: > On Tue 21 Feb 2017 12:55:05 PM CET, Daniel P. Berrange wrote: > > + switch (s->crypt_method_header) { > > + case QCOW_CRYPT_NONE: > > + break; > > + > > + case QCOW_CRYPT_AES: > > + r->crypto_opts = block_crypto_open_opts_init( > > + Q_CRYPTO_BLOCK_FORMAT_QCOW, opts, "aes-", errp); > > + break; > > + > > + default: > > + error_setg(errp, "Unsupported encryption method %d", > > + s->crypt_method_header); > > + break; > > + } > > + if (s->crypt_method_header && !r->crypto_opts) { > > + ret = -EINVAL; > > + goto fail; > > + } > > This last condition relies on the assumption that QCOW_CRYPT_NONE == 0. > > I think it's safe to assume that its value is never going to change and > therefore this isn't too important, but I'm just pointing it out in case > you want to make it explicit. Yeah, I'll make it explicit to be kinder to future reviewers :-) > > > @@ -1122,6 +1145,24 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, > > goto fail; > > } > > > > + if (s->crypt_method_header == QCOW_CRYPT_AES) { > > + unsigned int cflags = 0; > > + if (flags & BDRV_O_NO_IO) { > > + cflags |= QCRYPTO_BLOCK_OPEN_NO_IO; > > + } > > + /* TODO how do we pass the same crypto opts down to the > > + * backing file by default, so we don't have to manually > > + * provide the same key-secret property against the full > > + * backing chain > > + */ > > + s->crypto = qcrypto_block_open(s->crypto_opts, NULL, NULL, > > + cflags, errp); > > + if (!s->crypto) { > > + ret = -EINVAL; > > + goto fail; > > + } > > + } > > Actually this has the same problem that I mentioned for patch 9: if > qcow2_open() fails then s->crypto is leaked. Yep, and the crypto_opts actually Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|