Re: [Qemu-devel] [PATCH v8 13/20] qcow2: add support for LUKS encryption format

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4274 bytes --]

On 2017-06-01 19:27, Daniel P. Berrange wrote:
> This adds support for using LUKS as an encryption format
> with the qcow2 file, using the new encrypt.format parameter
> to request "luks" format. e.g.
> 
>   # qemu-img create --object secret,data=123456,id=sec0 \
>        -f qcow2 -o encrypt.format=luks,encrypt.key-secret=sec0 \
>        test.qcow2 10G
> 
> The legacy "encryption=on" parameter still results in
> creation of the old qcow2 AES format (and is equivalent
> to the new 'encryption-format=aes'). e.g. the following are
> equivalent:
> 
>   # qemu-img create --object secret,data=123456,id=sec0 \
>        -f qcow2 -o encryption=on,encrypt.key-secret=sec0 \
>        test.qcow2 10G
> 
>  # qemu-img create --object secret,data=123456,id=sec0 \
>        -f qcow2 -o encryption-format=aes,encrypt.key-secret=sec0 \
>        test.qcow2 10G
> 
> With the LUKS format it is necessary to store the LUKS
> partition header and key material in the QCow2 file. This
> data can be many MB in size, so cannot go into the QCow2
> header region directly. Thus the spec defines a FDE
> (Full Disk Encryption) header extension that specifies
> the offset of a set of clusters to hold the FDE headers,
> as well as the length of that region. The LUKS header is
> thus stored in these extra allocated clusters before the
> main image payload.
> 
> Aside from all the cryptographic differences implied by
> use of the LUKS format, there is one further key difference
> between the use of legacy AES and LUKS encryption in qcow2.
> For LUKS, the initialiazation vectors are generated using
> the host physical sector as the input, rather than the
> guest virtual sector. This guarantees unique initialization
> vectors for all sectors when qcow2 internal snapshots are
> used, thus giving stronger protection against watermarking
> attacks.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  block/qcow2-cluster.c      |   4 +-
>  block/qcow2-refcount.c     |  10 ++
>  block/qcow2.c              | 267 ++++++++++++++++++++++++++++++++++++++------
>  block/qcow2.h              |   9 ++
>  qapi/block-core.json       |   5 +-
>  tests/qemu-iotests/082.out | 270 ++++++++++++++++++++++++++++++++++++---------
>  6 files changed, 477 insertions(+), 88 deletions(-)

[...]

> diff --git a/block/qcow2.c b/block/qcow2.c
> index 38c0420..30d0343 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c

[...]

> @@ -2072,22 +2253,30 @@ static int qcow2_set_up_encryption(BlockDriverState *bs, const char *encryptfmt,
>      qdict_extract_subqdict(options, &encryptopts, "encrypt.");
>      QDECREF(options);
>  
> -    if (!g_str_equal(encryptfmt, "aes")) {
> -        error_setg(errp, "Unknown encryption format '%s', expected 'aes'",
> -                   encryptfmt);
> -        ret = -EINVAL;
> -        goto out;
> +    int fmt = qcow2_crypt_method_from_format(encryptfmt);

Because I love nit picking: Variable declarations should be at the start
of the surrounding block.

Max

> +
> +    switch (fmt) {
> +    case QCOW_CRYPT_LUKS:
> +        cryptoopts = block_crypto_create_opts_init(
> +            Q_CRYPTO_BLOCK_FORMAT_LUKS, encryptopts, errp);
> +        break;
> +    case QCOW_CRYPT_AES:
> +        cryptoopts = block_crypto_create_opts_init(
> +            Q_CRYPTO_BLOCK_FORMAT_QCOW, encryptopts, errp);
> +        break;
> +    default:
> +        error_setg(errp, "Unknown encryption format '%s'", encryptfmt);
> +        break;
>      }
> -    cryptoopts = block_crypto_create_opts_init(
> -        Q_CRYPTO_BLOCK_FORMAT_QCOW, encryptopts, errp);
>      if (!cryptoopts) {
>          ret = -EINVAL;
>          goto out;
>      }
> -    s->crypt_method_header = QCOW_CRYPT_AES;
> +    s->crypt_method_header = fmt;
>  
>      crypto = qcrypto_block_create(cryptoopts,
> -                                  NULL, NULL,
> +                                  qcow2_crypto_hdr_init_func,
> +                                  qcow2_crypto_hdr_write_func,
>                                    bs, errp);
>      if (!crypto) {
>          ret = -EINVAL;


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]