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

[Eric Blake]

On 04/25/2017 10:38 AM, 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 \

s/encrypt.-format/encrypt.format/

>        test.qcow2 10G
> 

> 
> 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.
> 
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---

> @@ -165,6 +246,47 @@ static int qcow2_read_extensions(BlockDriverState *bs, 
> uint64_t start_offset,
>              }
>              break;
>  
> +        case QCOW2_EXT_MAGIC_CRYPTO_HEADER: {
> +            unsigned int cflags = 0;
> +            if (s->crypt_method_header != QCOW_CRYPT_LUKS) {
> +                error_setg(errp, "CRYPTO header extension only "
> +                           "expected with LUKS encryption method");
> +                return -EINVAL;
> +            }
> +            if (ext.len != sizeof(Qcow2CryptoHeaderExtension)) {
> +                error_setg(errp, "CRYPTO header extension size %u, "
> +                           "but expected size %zu", ext.len,
> +                           sizeof(Qcow2CryptoHeaderExtension));
> +                return -EINVAL;
> +            }
> +
> +            ret = bdrv_pread(bs->file, offset, &s->crypto_header, ext.len);
> +            if (ret < 0) {
> +                error_setg_errno(errp, -ret,
> +                                 "Unable to read CRYPTO header extension");
> +                return ret;
> +            }
> +            be64_to_cpus(&s->crypto_header.offset);
> +            be64_to_cpus(&s->crypto_header.length);
> +
> +            if ((s->crypto_header.offset % s->cluster_size) != 0) {
> +                error_setg(errp, "Encryption header offset '%" PRIu64 "' is "
> +                           "not a multiple of cluster size '%u'",
> +                           s->crypto_header.offset, s->cluster_size);
> +                return -EINVAL;
> +            }

Do we need to sanity check that crypto_header.length is not bogus?

> +
> +            if (flags & BDRV_O_NO_IO) {
> +                cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
> +            }
> +            s->crypto = qcrypto_block_open(s->crypto_opts,
> +                                           qcow2_crypto_hdr_read_func,
> +                                           bs, cflags, errp);
> +            if (!s->crypto) {
> +                return -EINVAL;
> +            }
> +        }   break;
> +
>          default:
>              /* unknown magic - save it in case we need to rewrite the header 
> */
>              {
> @@ -464,7 +586,8 @@ static QemuOptsList qcow2_runtime_opts = {
>              .type = QEMU_OPT_NUMBER,
>              .help = "Clean unused cache entries after this time (in 
> seconds)",
>          },
> -        BLOCK_CRYPTO_OPT_DEF_QCOW_KEY_SECRET("encrypt."),
> +        BLOCK_CRYPTO_OPT_DEF_KEY_SECRET("encrypt.",
> +            "ID of secret providing qcow AES key or LUKS passphrase"),

s/qcow/qcow2/ ?

Minor enough that I'm okay giving:
Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature