From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50307) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YzT2z-0003pF-4W for qemu-devel@nongnu.org; Mon, 01 Jun 2015 12:58:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YzT2t-0001C4-20 for qemu-devel@nongnu.org; Mon, 01 Jun 2015 12:58:45 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41483) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YzT2s-0001Be-R5 for qemu-devel@nongnu.org; Mon, 01 Jun 2015 12:58:38 -0400 Date: Mon, 1 Jun 2015 17:58:33 +0100 From: "Daniel P. Berrange" Message-ID: <20150601165833.GE17374@redhat.com> References: <1432205817-16414-1-git-send-email-berrange@redhat.com> <1432205817-16414-10-git-send-email-berrange@redhat.com> <55681261.5020907@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <55681261.5020907@huawei.com> Subject: Re: [Qemu-devel] [PATCH 09/10] block: convert qcow/qcow2 to use generic cipher API Reply-To: "Daniel P. Berrange" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gonglei Cc: Kevin Wolf , Paolo Bonzini , qemu-devel@nongnu.org, Gerd Hoffmann On Fri, May 29, 2015 at 03:16:49PM +0800, Gonglei wrote: > On 2015/5/21 18:56, Daniel P. Berrange wrote: > > Switch the qcow/qcow2 block driver over to use the generic cipher > > API, this allows it to use the pluggable AES implementations, > > instead of being hardcoded to use QEMU's built-in impl. > > > > Signed-off-by: Daniel P. Berrange > > --- > > block/qcow.c | 100 ++++++++++++++++++++++++++++++++++++-------------- > > block/qcow2-cluster.c | 46 ++++++++++++++++++----- > > block/qcow2.c | 94 ++++++++++++++++++++++++----------------------- > > block/qcow2.h | 13 +++---- > > 4 files changed, 162 insertions(+), 91 deletions(-) > > > > diff --git a/block/qcow.c b/block/qcow.c > > index 50dbcee..7338d1d 100644 > > --- a/block/qcow.c > > +++ b/block/qcow.c > > @@ -25,7 +25,7 @@ > > #include "block/block_int.h" > > #include "qemu/module.h" > > #include > > -#include "crypto/aes.h" > > +#include "crypto/cipher.h" > > #include "migration/migration.h" > > > > /**************************************************************/ > > @@ -71,10 +71,8 @@ typedef struct BDRVQcowState { > > uint8_t *cluster_cache; > > uint8_t *cluster_data; > > uint64_t cluster_cache_offset; > > - uint32_t crypt_method; /* current crypt method, 0 if no key yet */ > > + QCryptoCipher *cipher; /* NULL if no key yet */ > > uint32_t crypt_method_header; > > - AES_KEY aes_encrypt_key; > > - AES_KEY aes_decrypt_key; > > CoMutex lock; > > Error *migration_blocker; > > } BDRVQcowState; > > @@ -153,6 +151,11 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, > > ret = -EINVAL; > > goto fail; > > } > > + if (!qcrypto_cipher_supports(QCRYPTO_CIPHER_ALG_AES)) { > > + error_setg(errp, "AES cipher not available"); > > + ret = -EINVAL; > > + goto fail; > > + } > > s->crypt_method_header = header.crypt_method; > > if (s->crypt_method_header) { > > bs->encrypted = 1; > > @@ -259,6 +262,7 @@ static int qcow_set_key(BlockDriverState *bs, const char *key) > > BDRVQcowState *s = bs->opaque; > > uint8_t keybuf[16]; > > int len, i; > > + Error *err; > > > > memset(keybuf, 0, 16); > > len = strlen(key); > > @@ -269,38 +273,66 @@ static int qcow_set_key(BlockDriverState *bs, const char *key) > > for(i = 0;i < len;i++) { > > keybuf[i] = key[i]; > > } > > - s->crypt_method = s->crypt_method_header; > > > > - if (AES_set_encrypt_key(keybuf, 128, &s->aes_encrypt_key) != 0) > > - return -1; > > - if (AES_set_decrypt_key(keybuf, 128, &s->aes_decrypt_key) != 0) > > + if (s->cipher) { > > This above check is superfluous. Hmm, yes, the free method accepts NULL just fine. > > > + qcrypto_cipher_free(s->cipher); > > + } > > + s->cipher = qcrypto_cipher_new( > > + QCRYPTO_CIPHER_ALG_AES, > > + QCRYPTO_CIPHER_MODE_CBC, > > + keybuf, G_N_ELEMENTS(keybuf), > > + &err); > > + > > + if (!s->cipher) { > > + error_free(err); > > Maybe we should report the error message before free it. > It's the same for below error handling. We're limited by the error code abilities of this code - basically there is no where to propagate the error back up to. As & when this code is updated to properly propagate errors we can do this here. > > @@ -1455,6 +1463,8 @@ static void qcow2_close(BlockDriverState *bs) > > qcow2_cache_destroy(bs, s->l2_table_cache); > > qcow2_cache_destroy(bs, s->refcount_block_cache); > > > > + qcrypto_cipher_free(s->cipher); > > + > > Do we need to set s->cipher = NULL ? Yes, probably worth while as a sanity check. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|