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

[Gonglei]

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 <address@hidden>
> ---
>  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.

> +        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.

>          return -1;
> +    }
>      return 0;
>  }
>  
>  /* The crypt function is compatible with the linux cryptoloop
>     algorithm for < 4 GB images. NOTE: out_buf == in_buf is
>     supported */
> -static void encrypt_sectors(BDRVQcowState *s, int64_t sector_num,
> -                            uint8_t *out_buf, const uint8_t *in_buf,
> -                            int nb_sectors, int enc,
> -                            const AES_KEY *key)
> +static int encrypt_sectors(BDRVQcowState *s, int64_t sector_num,
> +                           uint8_t *out_buf, const uint8_t *in_buf,
> +                           int nb_sectors, bool enc, Error **errp)
>  {
>      union {
>          uint64_t ll[2];
>          uint8_t b[16];
>      } ivec;
>      int i;
> +    int ret;
>  
>      for(i = 0; i < nb_sectors; i++) {
>          ivec.ll[0] = cpu_to_le64(sector_num);
>          ivec.ll[1] = 0;
> -        AES_cbc_encrypt(in_buf, out_buf, 512, key,
> -                        ivec.b, enc);
> +        if (qcrypto_cipher_setiv(s->cipher,
> +                                 ivec.b, G_N_ELEMENTS(ivec.b),
> +                                 errp) < 0) {
> +            return -1;
> +        }
> +        if (enc) {
> +            ret = qcrypto_cipher_encrypt(s->cipher,
> +                                         in_buf,
> +                                         out_buf,
> +                                         512,
> +                                         errp);
> +        } else {
> +            ret = qcrypto_cipher_decrypt(s->cipher,
> +                                         in_buf,
> +                                         out_buf,
> +                                         512,
> +                                         errp);
> +        }
> +        if (ret < 0) {
> +            return -1;
> +        }
>          sector_num++;
>          in_buf += 512;
>          out_buf += 512;
>      }
> +    return 0;
>  }
>  
>  /* 'allocate' is:
> @@ -411,17 +443,22 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
>                  bdrv_truncate(bs->file, cluster_offset + s->cluster_size);
>                  /* if encrypted, we must initialize the cluster
>                     content which won't be written */
> -                if (s->crypt_method &&
> +                if (s->cipher &&
>                      (n_end - n_start) < s->cluster_sectors) {
>                      uint64_t start_sect;
>                      start_sect = (offset & ~(s->cluster_size - 1)) >> 9;
>                      memset(s->cluster_data + 512, 0x00, 512);
>                      for(i = 0; i < s->cluster_sectors; i++) {
>                          if (i < n_start || i >= n_end) {
> -                            encrypt_sectors(s, start_sect + i,
> -                                            s->cluster_data,
> -                                            s->cluster_data + 512, 1, 1,
> -                                            &s->aes_encrypt_key);
> +                            Error *err = NULL;
> +                            if (encrypt_sectors(s, start_sect + i,
> +                                                s->cluster_data,
> +                                                s->cluster_data + 512, 1,
> +                                                true, &err) < 0) {
> +                                error_free(err);
> +                                errno = EIO;
> +                                return -1;
> +                            }
>                              if (bdrv_pwrite(bs->file, cluster_offset + i * 
> 512,
>                                              s->cluster_data, 512) != 512)
>                                  return -1;
> @@ -461,7 +498,7 @@ static int64_t coroutine_fn 
> qcow_co_get_block_status(BlockDriverState *bs,
>      if (!cluster_offset) {
>          return 0;
>      }
> -    if ((cluster_offset & QCOW_OFLAG_COMPRESSED) || s->crypt_method) {
> +    if ((cluster_offset & QCOW_OFLAG_COMPRESSED) || s->cipher) {
>          return BDRV_BLOCK_DATA;
>      }
>      cluster_offset |= (index_in_cluster << BDRV_SECTOR_BITS);
> @@ -528,6 +565,7 @@ static coroutine_fn int qcow_co_readv(BlockDriverState 
> *bs, int64_t sector_num,
>      QEMUIOVector hd_qiov;
>      uint8_t *buf;
>      void *orig_buf;
> +    Error *err = NULL;
>  
>      if (qiov->niov > 1) {
>          buf = orig_buf = qemu_try_blockalign(bs, qiov->size);
> @@ -590,10 +628,11 @@ static coroutine_fn int qcow_co_readv(BlockDriverState 
> *bs, int64_t sector_num,
>              if (ret < 0) {
>                  break;
>              }
> -            if (s->crypt_method) {
> -                encrypt_sectors(s, sector_num, buf, buf,
> -                                n, 0,
> -                                &s->aes_decrypt_key);
> +            if (s->cipher) {
> +                if (encrypt_sectors(s, sector_num, buf, buf,
> +                                    n, false, &err) < 0) {
> +                    goto fail;
> +                }
>              }
>          }
>          ret = 0;
> @@ -614,6 +653,7 @@ done:
>      return ret;
>  
>  fail:
> +    error_free(err);
>      ret = -EIO;
>      goto done;
>  }
> @@ -661,12 +701,17 @@ static coroutine_fn int qcow_co_writev(BlockDriverState 
> *bs, int64_t sector_num,
>              ret = -EIO;
>              break;
>          }
> -        if (s->crypt_method) {
> +        if (s->cipher) {
> +            Error *err = NULL;
>              if (!cluster_data) {
>                  cluster_data = g_malloc0(s->cluster_size);
>              }
> -            encrypt_sectors(s, sector_num, cluster_data, buf,
> -                            n, 1, &s->aes_encrypt_key);
> +            if (encrypt_sectors(s, sector_num, cluster_data, buf,
> +                                n, true, &err) < 0) {
> +                error_free(err);
> +                ret = -EIO;
> +                break;
> +            }
>              src_buf = cluster_data;
>          } else {
>              src_buf = buf;
> @@ -703,6 +748,7 @@ static void qcow_close(BlockDriverState *bs)
>  {
>      BDRVQcowState *s = bs->opaque;
>  
> +    qcrypto_cipher_free(s->cipher);
>      g_free(s->l1_table);
>      qemu_vfree(s->l2_cache);
>      g_free(s->cluster_cache);
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index ed2b44d..afc2d90 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -342,26 +342,47 @@ static int count_contiguous_free_clusters(uint64_t 
> nb_clusters, uint64_t *l2_tab
>  /* The crypt function is compatible with the linux cryptoloop
>     algorithm for < 4 GB images. NOTE: out_buf == in_buf is
>     supported */
> -void qcow2_encrypt_sectors(BDRVQcowState *s, int64_t sector_num,
> -                           uint8_t *out_buf, const uint8_t *in_buf,
> -                           int nb_sectors, int enc,
> -                           const AES_KEY *key)
> +int qcow2_encrypt_sectors(BDRVQcowState *s, int64_t sector_num,
> +                          uint8_t *out_buf, const uint8_t *in_buf,
> +                          int nb_sectors, bool enc,
> +                          Error **errp)
>  {
>      union {
>          uint64_t ll[2];
>          uint8_t b[16];
>      } ivec;
>      int i;
> +    int ret;
>  
>      for(i = 0; i < nb_sectors; i++) {
>          ivec.ll[0] = cpu_to_le64(sector_num);
>          ivec.ll[1] = 0;
> -        AES_cbc_encrypt(in_buf, out_buf, 512, key,
> -                        ivec.b, enc);
> +        if (qcrypto_cipher_setiv(s->cipher,
> +                                 ivec.b, G_N_ELEMENTS(ivec.b),
> +                                 errp) < 0) {
> +            return -1;
> +        }
> +        if (enc) {
> +            ret = qcrypto_cipher_encrypt(s->cipher,
> +                                         in_buf,
> +                                         out_buf,
> +                                         512,
> +                                         errp);
> +        } else {
> +            ret = qcrypto_cipher_decrypt(s->cipher,
> +                                         in_buf,
> +                                         out_buf,
> +                                         512,
> +                                         errp);
> +        }
> +        if (ret < 0) {
> +            return -1;
> +        }
>          sector_num++;
>          in_buf += 512;
>          out_buf += 512;
>      }
> +    return 0;
>  }
>  
>  static int coroutine_fn copy_sectors(BlockDriverState *bs,
> @@ -403,10 +424,15 @@ static int coroutine_fn copy_sectors(BlockDriverState 
> *bs,
>          goto out;
>      }
>  
> -    if (s->crypt_method) {
> -        qcow2_encrypt_sectors(s, start_sect + n_start,
> -                        iov.iov_base, iov.iov_base, n, 1,
> -                        &s->aes_encrypt_key);
> +    if (s->cipher) {
> +        Error *err = NULL;
> +        if (qcow2_encrypt_sectors(s, start_sect + n_start,
> +                                  iov.iov_base, iov.iov_base, n,
> +                                  true, &err) < 0) {
> +            ret = -EIO;
> +            error_free(err);
> +            goto out;
> +        }
>      }
>  
>      ret = qcow2_pre_write_overlap_check(bs, 0,
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 83e5ec9..2d1100d 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -694,6 +694,11 @@ static int qcow2_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;
> @@ -1026,6 +1031,7 @@ static int qcow2_set_key(BlockDriverState *bs, const 
> char *key)
>      BDRVQcowState *s = bs->opaque;
>      uint8_t keybuf[16];
>      int len, i;
> +    Error *err = NULL;
>  
>      memset(keybuf, 0, 16);
>      len = strlen(key);
> @@ -1036,30 +1042,20 @@ static int qcow2_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) {
> +        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);
>          return -1;
> -#if 0
> -    /* test */
> -    {
> -        uint8_t in[16];
> -        uint8_t out[16];
> -        uint8_t tmp[16];
> -        for(i=0;i<16;i++)
> -            in[i] = i;
> -        AES_encrypt(in, tmp, &s->aes_encrypt_key);
> -        AES_decrypt(tmp, out, &s->aes_decrypt_key);
> -        for(i = 0; i < 16; i++)
> -            printf(" %02x", tmp[i]);
> -        printf("\n");
> -        for(i = 0; i < 16; i++)
> -            printf(" %02x", out[i]);
> -        printf("\n");
>      }
> -#endif
>      return 0;
>  }
>  
> @@ -1102,7 +1098,7 @@ static int64_t coroutine_fn 
> qcow2_co_get_block_status(BlockDriverState *bs,
>      }
>  
>      if (cluster_offset != 0 && ret != QCOW2_CLUSTER_COMPRESSED &&
> -        !s->crypt_method) {
> +        !s->cipher) {
>          index_in_cluster = sector_num & (s->cluster_sectors - 1);
>          cluster_offset |= (index_in_cluster << BDRV_SECTOR_BITS);
>          status |= BDRV_BLOCK_OFFSET_VALID | cluster_offset;
> @@ -1152,7 +1148,7 @@ static coroutine_fn int qcow2_co_readv(BlockDriverState 
> *bs, int64_t sector_num,
>  
>          /* prepare next request */
>          cur_nr_sectors = remaining_sectors;
> -        if (s->crypt_method) {
> +        if (s->cipher) {
>              cur_nr_sectors = MIN(cur_nr_sectors,
>                  QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors);
>          }
> @@ -1223,7 +1219,7 @@ static coroutine_fn int qcow2_co_readv(BlockDriverState 
> *bs, int64_t sector_num,
>                  goto fail;
>              }
>  
> -            if (s->crypt_method) {
> +            if (s->cipher) {
>                  /*
>                   * For encrypted images, read everything into a temporary
>                   * contiguous buffer on which the AES functions can work.
> @@ -1254,9 +1250,15 @@ static coroutine_fn int 
> qcow2_co_readv(BlockDriverState *bs, int64_t sector_num,
>              if (ret < 0) {
>                  goto fail;
>              }
> -            if (s->crypt_method) {
> -                qcow2_encrypt_sectors(s, sector_num,  cluster_data,
> -                    cluster_data, cur_nr_sectors, 0, &s->aes_decrypt_key);
> +            if (s->cipher) {
> +                Error *err = NULL;
> +                if (qcow2_encrypt_sectors(s, sector_num,  cluster_data,
> +                                          cluster_data, cur_nr_sectors, 
> false,
> +                                          &err) < 0) {
> +                    error_free(err);
> +                    ret = -EIO;
> +                    goto fail;
> +                }
>                  qemu_iovec_from_buf(qiov, bytes_done,
>                      cluster_data, 512 * cur_nr_sectors);
>              }
> @@ -1314,7 +1316,7 @@ static coroutine_fn int 
> qcow2_co_writev(BlockDriverState *bs,
>          trace_qcow2_writev_start_part(qemu_coroutine_self());
>          index_in_cluster = sector_num & (s->cluster_sectors - 1);
>          cur_nr_sectors = remaining_sectors;
> -        if (s->crypt_method &&
> +        if (s->cipher &&
>              cur_nr_sectors >
>              QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors - index_in_cluster) 
> {
>              cur_nr_sectors =
> @@ -1333,7 +1335,8 @@ static coroutine_fn int 
> qcow2_co_writev(BlockDriverState *bs,
>          qemu_iovec_concat(&hd_qiov, qiov, bytes_done,
>              cur_nr_sectors * 512);
>  
> -        if (s->crypt_method) {
> +        if (s->cipher) {
> +            Error *err = NULL;
>              if (!cluster_data) {
>                  cluster_data = qemu_try_blockalign(bs->file,
>                                                     QCOW_MAX_CRYPT_CLUSTERS
> @@ -1348,8 +1351,13 @@ static coroutine_fn int 
> qcow2_co_writev(BlockDriverState *bs,
>                     QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size);
>              qemu_iovec_to_buf(&hd_qiov, 0, cluster_data, hd_qiov.size);
>  
> -            qcow2_encrypt_sectors(s, sector_num, cluster_data,
> -                cluster_data, cur_nr_sectors, 1, &s->aes_encrypt_key);
> +            if (qcow2_encrypt_sectors(s, sector_num, cluster_data,
> +                                      cluster_data, cur_nr_sectors,
> +                                      true, &err) < 0) {
> +                error_free(err);
> +                ret = -EIO;
> +                goto fail;
> +            }
>  
>              qemu_iovec_reset(&hd_qiov);
>              qemu_iovec_add(&hd_qiov, cluster_data,
> @@ -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 ?

Regards,
-Gonglei

>      g_free(s->unknown_header_fields);
>      cleanup_unknown_header_ext(bs);
>  
> @@ -1471,9 +1481,7 @@ static void qcow2_invalidate_cache(BlockDriverState 
> *bs, Error **errp)
>  {
>      BDRVQcowState *s = bs->opaque;
>      int flags = s->flags;
> -    AES_KEY aes_encrypt_key;
> -    AES_KEY aes_decrypt_key;
> -    uint32_t crypt_method = 0;
> +    QCryptoCipher *cipher = NULL;
>      QDict *options;
>      Error *local_err = NULL;
>      int ret;
> @@ -1483,11 +1491,8 @@ static void qcow2_invalidate_cache(BlockDriverState 
> *bs, Error **errp)
>       * that means we don't have to worry about reopening them here.
>       */
>  
> -    if (s->crypt_method) {
> -        crypt_method = s->crypt_method;
> -        memcpy(&aes_encrypt_key, &s->aes_encrypt_key, 
> sizeof(aes_encrypt_key));
> -        memcpy(&aes_decrypt_key, &s->aes_decrypt_key, 
> sizeof(aes_decrypt_key));
> -    }
> +    cipher = s->cipher;
> +    s->cipher = NULL;
>  
>      qcow2_close(bs);
>  
> @@ -1512,11 +1517,7 @@ static void qcow2_invalidate_cache(BlockDriverState 
> *bs, Error **errp)
>          return;
>      }
>  
> -    if (crypt_method) {
> -        s->crypt_method = crypt_method;
> -        memcpy(&s->aes_encrypt_key, &aes_encrypt_key, 
> sizeof(aes_encrypt_key));
> -        memcpy(&s->aes_decrypt_key, &aes_decrypt_key, 
> sizeof(aes_decrypt_key));
> -    }
> +    s->cipher = cipher;
>  }
>  
>  static size_t header_ext_add(char *buf, uint32_t magic, const void *s,
> @@ -2717,8 +2718,9 @@ static int qcow2_amend_options(BlockDriverState *bs, 
> QemuOpts *opts,
>              backing_format = qemu_opt_get(opts, BLOCK_OPT_BACKING_FMT);
>          } else if (!strcmp(desc->name, BLOCK_OPT_ENCRYPT)) {
>              encrypt = qemu_opt_get_bool(opts, BLOCK_OPT_ENCRYPT,
> -                                        s->crypt_method);
> -            if (encrypt != !!s->crypt_method) {
> +                                        !!s->cipher);
> +
> +            if (encrypt != !!s->cipher) {
>                  fprintf(stderr, "Changing the encryption flag is not "
>                          "supported.\n");
>                  return -ENOTSUP;
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 3cf5318..4af1471 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -25,7 +25,7 @@
>  #ifndef BLOCK_QCOW2_H
>  #define BLOCK_QCOW2_H
>  
> -#include "crypto/aes.h"
> +#include "crypto/cipher.h"
>  #include "block/coroutine.h"
>  
>  //#define DEBUG_ALLOC
> @@ -250,10 +250,8 @@ typedef struct BDRVQcowState {
>  
>      CoMutex lock;
>  
> -    uint32_t crypt_method; /* current crypt method, 0 if no key yet */
> +    QCryptoCipher *cipher; /* current cipher, NULL if no key yet */
>      uint32_t crypt_method_header;
> -    AES_KEY aes_encrypt_key;
> -    AES_KEY aes_decrypt_key;
>      uint64_t snapshots_offset;
>      int snapshots_size;
>      unsigned int nb_snapshots;
> @@ -533,10 +531,9 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t 
> min_size,
>  int qcow2_write_l1_entry(BlockDriverState *bs, int l1_index);
>  void qcow2_l2_cache_reset(BlockDriverState *bs);
>  int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset);
> -void qcow2_encrypt_sectors(BDRVQcowState *s, int64_t sector_num,
> -                     uint8_t *out_buf, const uint8_t *in_buf,
> -                     int nb_sectors, int enc,
> -                     const AES_KEY *key);
> +int qcow2_encrypt_sectors(BDRVQcowState *s, int64_t sector_num,
> +                          uint8_t *out_buf, const uint8_t *in_buf,
> +                          int nb_sectors, bool enc, Error **errp);
>  
>  int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
>      int *num, uint64_t *cluster_offset);
>