Re: [Qemu-devel] [PATCH 09/10] block: convert qcow/qcow2 to use generic cipher API

["Daniel P. Berrange" <berrange@redhat.com>]

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 <berrange@redhat.com>
> > ---
> >  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 <zlib.h>
> > -#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 :|