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 <berra...@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 :|
