[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-block] [PATCH v1 10/15] qcow2: convert QCow2 to u
|
From: |
Kevin Wolf |
|
Subject: |
Re: [Qemu-devel] [Qemu-block] [PATCH v1 10/15] qcow2: convert QCow2 to use QCryptoBlock for encryption |
|
Date: |
Wed, 13 Jan 2016 19:42:20 +0100 |
|
User-agent: |
Mutt/1.5.21 (2010-09-15) |
Am 12.01.2016 um 19:56 hat Daniel P. Berrange geschrieben:
> This converts the qcow2 driver to make use of the QCryptoBlock
> APIs for encrypting image content. As well as continued support
> for the legacy QCow2 encryption format, the appealing benefit
> is that it enables support for the LUKS format inside qcow2.
>
> With the LUKS format it is neccessary 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 is 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.
>
> With this change it is now required to use the QCryptoSecret
> object for providing passwords, instead of the current block
> password APIs / interactive prompting.
>
> $QEMU \
> -object secret,id=sec0,filename=/home/berrange/encrypted.pw \
> -drive file=/home/berrange/encrypted.qcow2,key-id=sec0
>
> The new LUKS format is set as the new default format when
> creating encrypted images. ie
>
> # qemu-img create --object secret,data=123456,id=sec0 \
> -f qcow2 -o encryption,key-id=sec0 \
> test.qcow2 10G
>
> Results in creation of an image using the LUKS format.
>
> For compatibility the old qcow2 AES format can still be used
> via the 'encryption-format' parameter which accepts the
> values 'luks' or 'qcowaes'.
>
> # qemu-img create --object secret,data=123456,id=sec0 \
> -f qcow2 -o encryption,key-id=sec0,encryption-format=qcowaes \
> test.qcow2 10G
>
> Signed-off-by: Daniel P. Berrange <address@hidden>
I think for your additional pointer to some clusters you need to change
some function(s) called by qcow2_check_refcounts(). Otherwise 'qemu-img
check' won't count the reference and helpfully free the "leaked"
cluster.
> diff --git a/block/qcow2.c b/block/qcow2.c
> index c0fc259..288aada 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -34,6 +34,8 @@
> #include "qapi-event.h"
> #include "trace.h"
> #include "qemu/option_int.h"
> +#include "qapi/opts-visitor.h"
> +#include "qapi-visit.h"
>
> /*
> Differences with QCOW:
> @@ -60,6 +62,7 @@ typedef struct {
> #define QCOW2_EXT_MAGIC_END 0
> #define QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
> #define QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
> +#define QCOW2_EXT_MAGIC_FDE_HEADER 0x4c554b53
General naming comment on this series: I would prefer avoiding "FDE" in
favour of "encryption" or "crypt" in the block layer parts. With all
image formats having their own terminology, "FDE" could mean anything.
> static int qcow2_probe(const uint8_t *buf, int buf_size, const char
> *filename)
> {
> @@ -74,6 +77,83 @@ static int qcow2_probe(const uint8_t *buf, int buf_size,
> const char *filename)
> }
>
>
> +static ssize_t qcow2_fde_header_read_func(QCryptoBlock *block,
> + size_t offset,
> + uint8_t *buf,
> + size_t buflen,
> + Error **errp,
> + void *opaque)
> +{
> + BlockDriverState *bs = opaque;
> + BDRVQcow2State *s = bs->opaque;
> + ssize_t ret;
> +
> + if ((offset + buflen) > s->fde_header.length) {
> + error_setg_errno(errp, EINVAL,
> + "Request for data outside of extension header");
error_setg_errno() with a constant errno doesn't look very useful.
Better use plain error_setg() in such cases.
> + return -1;
Here returning -EINVAL could be useful, I'm not sure what your crypto
API requires. At least you seem to be returning -errno below and mixing
-1 and -errno is probably a bad idea.
> + }
> +
> + ret = bdrv_pread(bs->file->bs,
> + s->fde_header.offset + offset, buf, buflen);
> + if (ret < 0) {
> + error_setg_errno(errp, -ret, "Could not read encryption header");
> + return ret;
> + }
> + return ret;
return 0? You already processed ret in the if block and two 'return ret'
in a row look odd.
> +}
> +
> +
> +static ssize_t qcow2_fde_header_init_func(QCryptoBlock *block,
> + size_t headerlen,
> + Error **errp,
> + void *opaque)
> +{
> + BlockDriverState *bs = opaque;
> + BDRVQcow2State *s = bs->opaque;
> + int64_t ret;
> +
> + s->fde_header.length = headerlen + (headerlen % s->cluster_size);
> +
> + ret = qcow2_alloc_clusters(bs, s->fde_header.length);
> + if (ret < 0) {
> + s->fde_header.length = 0;
> + error_setg(errp, "Cannot allocate cluster for LUKS header size %zu",
> + headerlen);
I think ret is -errno on failure, so use error_setg_errno()?
> + return -1;
> + }
> +
> + s->fde_header.offset = ret;
> + return 0;
> +}
> +
> +
> +static ssize_t qcow2_fde_header_write_func(QCryptoBlock *block,
> + size_t offset,
> + const uint8_t *buf,
> + size_t buflen,
> + Error **errp,
> + void *opaque)
> +{
> + BlockDriverState *bs = opaque;
> + BDRVQcow2State *s = bs->opaque;
> + ssize_t ret;
> +
> + if ((offset + buflen) > s->fde_header.length) {
> + error_setg_errno(errp, EINVAL,
> + "Request for data outside of extension header");
error_setg(). Probably worth checking all error paths whether there is a
useful errno or not. I won't comment on additional instances.
> + return -1;
> + }
> +
> + ret = bdrv_pwrite(bs->file->bs, s->fde_header.offset + offset, buf,
> buflen);
> + if (ret < 0) {
> + error_setg_errno(errp, -ret, "Could not read encryption header");
> + return ret;
> + }
> + return ret;
> +}
Mixing -1 and -errno again.
> /*
> * read qcow2 extension and fill bs
> * start reading from start_offset
> @@ -83,12 +163,14 @@ static int qcow2_probe(const uint8_t *buf, int buf_size,
> const char *filename)
> */
> static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
> uint64_t end_offset, void **p_feature_table,
> + int flags,
> Error **errp)
> {
> BDRVQcow2State *s = bs->opaque;
> QCowExtension ext;
> uint64_t offset;
> int ret;
> + unsigned int cflags = 0;
Should we create a block for QCOW2_EXT_MAGIC_FDE_HEADER and declare it
there?
> #ifdef DEBUG_EXT
> printf("qcow2_read_extensions: start=%ld end=%ld\n", start_offset,
> end_offset);
> @@ -159,6 +241,35 @@ static int qcow2_read_extensions(BlockDriverState *bs,
> uint64_t start_offset,
> }
> break;
>
> + case QCOW2_EXT_MAGIC_FDE_HEADER:
> + if (s->crypt_method_header != QCOW_CRYPT_LUKS) {
> + error_setg(errp, "FDE header extension only "
> + "expected with LUKS encryption method");
> + return -EINVAL;
> + }
> + if (ext.len != sizeof(Qcow2FDEHeaderExtension)) {
> + error_setg(errp, "LUKS header extension size %u, "
> + "but expected size %zu", ext.len,
> + sizeof(Qcow2FDEHeaderExtension));
> + return -EINVAL;
> + }
> +
> + ret = bdrv_pread(bs->file->bs, offset, &s->fde_header, ext.len);
No error check?
> + be64_to_cpu(s->fde_header.offset);
> + be64_to_cpu(s->fde_header.length);
> +
> + if (flags & BDRV_O_NO_IO) {
> + cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
> + }
> + s->fde = qcrypto_block_open(s->fde_opts,
> + qcow2_fde_header_read_func,
> + bs,
> + cflags,
> + errp);
The surrounding code doesn't put every parameter on its own line if the
next parameter can fit on the same line. There are more instances of
this in the patch, I won't comment on each one.
> + if (!s->fde) {
> + return -EINVAL;
> + }
> + break;
> default:
> /* unknown magic - save it in case we need to rewrite the header
> */
> {
> @@ -472,6 +583,11 @@ static QemuOptsList qcow2_runtime_opts = {
> .type = QEMU_OPT_NUMBER,
> .help = "Clean unused cache entries after this time (in
> seconds)",
> },
> + {
> + .name = QCOW2_OPT_KEY_ID,
> + .type = QEMU_OPT_STRING,
> + .help = "ID of the secret that provides the encryption key",
> + },
> { /* end of list */ }
> },
> };
> @@ -589,6 +705,113 @@ static void read_cache_sizes(BlockDriverState *bs,
> QemuOpts *opts,
> }
> }
>
> +
> +static QCryptoBlockOpenOptions *
> +qcow2_fde_open_opts_init(QCryptoBlockFormat format,
> + QemuOpts *opts,
> + Error **errp)
> +{
> + OptsVisitor *ov;
> + QCryptoBlockOpenOptions *ret;
> + Error *local_err = NULL;
> +
> + ret = g_new0(QCryptoBlockOpenOptions, 1);
> + ret->format = format;
> +
> + ov = opts_visitor_new(opts);
> +
> + switch (format) {
> + case Q_CRYPTO_BLOCK_FORMAT_QCOWAES:
> + ret->u.qcowaes = g_new0(QCryptoBlockOptionsQCowAES, 1);
> + visit_type_QCryptoBlockOptionsQCowAES(opts_get_visitor(ov),
> + &ret->u.qcowaes,
> + "qcowaes", &local_err);
> + break;
> +
> + case Q_CRYPTO_BLOCK_FORMAT_LUKS:
> + ret->u.luks = g_new0(QCryptoBlockOptionsLUKS, 1);
> + visit_type_QCryptoBlockOptionsLUKS(opts_get_visitor(ov),
> + &ret->u.luks,
> + "luks", &local_err);
> + break;
> +
> + default:
> + error_setg(&local_err, "Unsupported block format %d", format);
> + break;
> + }
> +
> + if (local_err) {
> + error_propagate(errp, local_err);
> + opts_visitor_cleanup(ov);
> + qapi_free_QCryptoBlockOpenOptions(ret);
> + return NULL;
> + }
> +
> + opts_visitor_cleanup(ov);
> + return ret;
> +}
> +
> +
> +static QCryptoBlockCreateOptions *
> +qcow2_fde_create_opts_init(QCryptoBlockFormat format,
> + QemuOpts *opts,
> + Error **errp)
> +{
> + OptsVisitor *ov;
> + QCryptoBlockCreateOptions *ret;
> + Error *local_err = NULL, *end_err = NULL;
> + Visitor *v;
> +
> + ret = g_new0(QCryptoBlockCreateOptions, 1);
> + ret->format = format;
> +
> + ov = opts_visitor_new(opts);
> + v = opts_get_visitor(ov);
> + visit_start_struct(v, NULL, NULL, NULL, 0, &local_err);
> + if (local_err) {
> + goto cleanup;
> + }
> +
> + switch (format) {
> + case Q_CRYPTO_BLOCK_FORMAT_QCOWAES:
> + ret->u.qcowaes = g_new0(QCryptoBlockOptionsQCowAES, 1);
> + visit_type_QCryptoBlockOptionsQCowAES(v, &ret->u.qcowaes,
> + "qcowaes", &local_err);
> + break;
> +
> + case Q_CRYPTO_BLOCK_FORMAT_LUKS:
> + ret->u.luks = g_new0(QCryptoBlockCreateOptionsLUKS, 1);
> + visit_type_QCryptoBlockCreateOptionsLUKS(v, &ret->u.luks,
> + "luks", &local_err);
> + break;
> +
> + default:
> + error_setg(&local_err, "Unsupported block format %d", format);
> + break;
> + }
> +
> + visit_end_struct(v, &end_err);
> + if (end_err) {
> + if (!local_err) {
> + error_propagate(&local_err, end_err);
> + } else {
> + error_free(end_err);
> + }
> + }
> +
> + cleanup:
> + if (local_err) {
> + error_propagate(errp, local_err);
> + qapi_free_QCryptoBlockCreateOptions(ret);
> + ret = NULL;
> + return NULL;
> + }
> +
> + opts_visitor_cleanup(ov);
> + return ret;
> +}
> +
> +
> typedef struct Qcow2ReopenState {
> Qcow2Cache *l2_table_cache;
> Qcow2Cache *refcount_block_cache;
> @@ -596,6 +819,7 @@ typedef struct Qcow2ReopenState {
> int overlap_check;
> bool discard_passthrough[QCOW2_DISCARD_MAX];
> uint64_t cache_clean_interval;
> + QCryptoBlockOpenOptions *fde_opts; /* Disk encryption runtime options */
> } Qcow2ReopenState;
>
> static int qcow2_update_options_prepare(BlockDriverState *bs,
> @@ -754,6 +978,33 @@ static int qcow2_update_options_prepare(BlockDriverState
> *bs,
> r->discard_passthrough[QCOW2_DISCARD_OTHER] =
> qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_OTHER, false);
>
> + switch (s->crypt_method_header) {
> + case QCOW_CRYPT_NONE:
> + break;
> +
> + case QCOW_CRYPT_AES:
> + r->fde_opts = qcow2_fde_open_opts_init(
> + Q_CRYPTO_BLOCK_FORMAT_QCOWAES,
> + opts,
> + errp);
> + break;
> +
> + case QCOW_CRYPT_LUKS:
> + r->fde_opts = qcow2_fde_open_opts_init(
> + Q_CRYPTO_BLOCK_FORMAT_LUKS,
> + opts,
> + errp);
> + break;
> +
> + default:
> + g_assert_not_reached();
> + }
> + if (s->crypt_method_header &&
> + !r->fde_opts) {
> + ret = -EINVAL;
> + goto fail;
> + }
> +
> ret = 0;
> fail:
> qemu_opts_del(opts);
> @@ -788,6 +1039,9 @@ static void qcow2_update_options_commit(BlockDriverState
> *bs,
> s->cache_clean_interval = r->cache_clean_interval;
> cache_clean_timer_init(bs, bdrv_get_aio_context(bs));
> }
> +
> + qapi_free_QCryptoBlockOpenOptions(s->fde_opts);
> + s->fde_opts = r->fde_opts;
> }
>
> static void qcow2_update_options_abort(BlockDriverState *bs,
> @@ -799,6 +1053,7 @@ static void qcow2_update_options_abort(BlockDriverState
> *bs,
> if (r->refcount_block_cache) {
> qcow2_cache_destroy(bs, r->refcount_block_cache);
> }
> + qapi_free_QCryptoBlockOpenOptions(r->fde_opts);
> }
>
> static int qcow2_update_options(BlockDriverState *bs, QDict *options,
> @@ -932,7 +1187,7 @@ static int qcow2_open(BlockDriverState *bs, QDict
> *options, int flags,
> if (s->incompatible_features & ~QCOW2_INCOMPAT_MASK) {
> void *feature_table = NULL;
> qcow2_read_extensions(bs, header.header_length, ext_end,
> - &feature_table, NULL);
> + &feature_table, flags, NULL);
> report_unsupported_feature(bs, errp, feature_table,
> s->incompatible_features &
> ~QCOW2_INCOMPAT_MASK);
> @@ -964,17 +1219,6 @@ static int qcow2_open(BlockDriverState *bs, QDict
> *options, int flags,
> s->refcount_max = UINT64_C(1) << (s->refcount_bits - 1);
> s->refcount_max += s->refcount_max - 1;
>
> - if (header.crypt_method > QCOW_CRYPT_AES) {
> - error_setg(errp, "Unsupported encryption method: %" PRIu32,
> - header.crypt_method);
> - ret = -EINVAL;
> - goto fail;
> - }
> - if (!qcrypto_cipher_supports(QCRYPTO_CIPHER_ALG_AES_128)) {
> - 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;
> @@ -1104,12 +1348,40 @@ static int qcow2_open(BlockDriverState *bs, QDict
> *options, int flags,
>
> /* read qcow2 extensions */
> if (qcow2_read_extensions(bs, header.header_length, ext_end, NULL,
> - &local_err)) {
> + flags, &local_err)) {
> error_propagate(errp, local_err);
> ret = -EINVAL;
> goto fail;
> }
>
> + /* qcow2_read_extension may have setup FDE context if
> + * the crypt method needs a header region, some methods
> + * don't need header extensions, so must check here
> + */
> + if (s->crypt_method_header &&
> + !s->fde) {
Unnecessary line break.
> + if (s->crypt_method_header == QCOW_CRYPT_AES) {
> + unsigned int cflags = 0;
> + if (flags & BDRV_O_NO_IO) {
> + cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
> + }
> + s->fde = qcrypto_block_open(s->fde_opts,
> + NULL, NULL,
> + cflags,
> + errp);
> + if (!s->fde) {
> + error_setg(errp, "Could not setup encryption layer");
> + ret = -EINVAL;
> + goto fail;
> + }
> + } else if (!(flags & BDRV_O_NO_IO)) {
> + error_setg(errp, "Missing FDE header for crypt method %d",
> + s->crypt_method_header);
> + ret = -EINVAL;
> + goto fail;
> + }
> + }
> +
> /* read the backing file name */
> if (header.backing_file_offset != 0) {
> len = header.backing_file_size;
> @@ -1199,41 +1471,6 @@ static void qcow2_refresh_limits(BlockDriverState *bs,
> Error **errp)
> bs->bl.write_zeroes_alignment = s->cluster_sectors;
> }
>
> -static int qcow2_set_key(BlockDriverState *bs, const char *key)
> -{
> - BDRVQcow2State *s = bs->opaque;
> - uint8_t keybuf[16];
> - int len, i;
> - Error *err = NULL;
> -
> - memset(keybuf, 0, 16);
> - len = strlen(key);
> - if (len > 16)
> - len = 16;
> - /* XXX: we could compress the chars to 7 bits to increase
> - entropy */
> - for(i = 0;i < len;i++) {
> - keybuf[i] = key[i];
> - }
> - assert(bs->encrypted);
> -
> - qcrypto_cipher_free(s->cipher);
> - s->cipher = qcrypto_cipher_new(
> - QCRYPTO_CIPHER_ALG_AES_128,
> - QCRYPTO_CIPHER_MODE_CBC,
> - keybuf, G_N_ELEMENTS(keybuf),
> - &err);
> -
> - if (!s->cipher) {
> - /* XXX would be nice if errors in this method could
> - * be properly propagate to the caller. Would need
> - * the bdrv_set_key() API signature to be fixed. */
> - error_free(err);
> - return -1;
> - }
> - return 0;
> -}
> -
> static int qcow2_reopen_prepare(BDRVReopenState *state,
> BlockReopenQueue *queue, Error **errp)
> {
> @@ -1345,7 +1582,7 @@ static int64_t coroutine_fn
> qcow2_co_get_block_status(BlockDriverState *bs,
> }
>
> if (cluster_offset != 0 && ret != QCOW2_CLUSTER_COMPRESSED &&
> - !s->cipher) {
> + !s->fde) {
> index_in_cluster = sector_num & (s->cluster_sectors - 1);
> cluster_offset |= (index_in_cluster << BDRV_SECTOR_BITS);
> status |= BDRV_BLOCK_OFFSET_VALID | cluster_offset;
> @@ -1395,7 +1632,7 @@ static coroutine_fn int qcow2_co_readv(BlockDriverState
> *bs, int64_t sector_num,
>
> /* prepare next request */
> cur_nr_sectors = remaining_sectors;
> - if (s->cipher) {
> + if (s->fde) {
> cur_nr_sectors = MIN(cur_nr_sectors,
> QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors);
> }
> @@ -1467,7 +1704,7 @@ static coroutine_fn int qcow2_co_readv(BlockDriverState
> *bs, int64_t sector_num,
> }
>
> if (bs->encrypted) {
> - assert(s->cipher);
> + assert(s->fde);
>
> /*
> * For encrypted images, read everything into a temporary
> @@ -1501,10 +1738,12 @@ static coroutine_fn int
> qcow2_co_readv(BlockDriverState *bs, int64_t sector_num,
> goto fail;
> }
> if (bs->encrypted) {
> - assert(s->cipher);
> + assert(s->fde);
> Error *err = NULL;
> - if (qcow2_encrypt_sectors(s, sector_num, cluster_data,
> - cur_nr_sectors, false,
> + if (qcrypto_block_decrypt(s->fde,
> + sector_num,
> + cluster_data,
> + cur_nr_sectors,
> &err) < 0) {
> error_free(err);
> ret = -EIO;
> @@ -1588,7 +1827,7 @@ static coroutine_fn int
> qcow2_co_writev(BlockDriverState *bs,
>
> if (bs->encrypted) {
> Error *err = NULL;
> - assert(s->cipher);
> + assert(s->fde);
> if (!cluster_data) {
> cluster_data = qemu_try_blockalign(bs->file->bs,
> QCOW_MAX_CRYPT_CLUSTERS
> @@ -1603,8 +1842,11 @@ 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);
>
> - if (qcow2_encrypt_sectors(s, sector_num, cluster_data,
> - cur_nr_sectors, true, &err) < 0) {
> + if (qcrypto_block_encrypt(s->fde,
> + sector_num,
> + cluster_data,
> + cur_nr_sectors,
> + &err) < 0) {
> error_free(err);
> ret = -EIO;
> goto fail;
> @@ -1715,8 +1957,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);
> - s->cipher = NULL;
> + qcrypto_block_free(s->fde);
> + s->fde = NULL;
>
> g_free(s->unknown_header_fields);
> cleanup_unknown_header_ext(bs);
> @@ -1734,7 +1976,7 @@ static void qcow2_invalidate_cache(BlockDriverState
> *bs, Error **errp)
> {
> BDRVQcow2State *s = bs->opaque;
> int flags = s->flags;
> - QCryptoCipher *cipher = NULL;
> + QCryptoBlock *fde = NULL;
> QDict *options;
> Error *local_err = NULL;
> int ret;
> @@ -1744,8 +1986,8 @@ static void qcow2_invalidate_cache(BlockDriverState
> *bs, Error **errp)
> * that means we don't have to worry about reopening them here.
> */
>
> - cipher = s->cipher;
> - s->cipher = NULL;
> + fde = s->fde;
> + s->fde = NULL;
>
> qcow2_close(bs);
>
> @@ -1770,7 +2012,7 @@ static void qcow2_invalidate_cache(BlockDriverState
> *bs, Error **errp)
> return;
> }
>
> - s->cipher = cipher;
> + s->fde = fde;
> }
>
> static size_t header_ext_add(char *buf, uint32_t magic, const void *s,
> @@ -1893,6 +2135,23 @@ int qcow2_update_header(BlockDriverState *bs)
> buflen -= ret;
> }
>
> + /* Full disk encryption header pointer extension */
> + if (s->fde_header.offset != 0) {
> + cpu_to_be64(s->fde_header.offset);
> + cpu_to_be64(s->fde_header.length);
> + ret = header_ext_add(buf, QCOW2_EXT_MAGIC_FDE_HEADER,
> + &s->fde_header,
> + sizeof(s->fde_header),
> + buflen);
> + be64_to_cpu(s->fde_header.offset);
> + be64_to_cpu(s->fde_header.length);
> + if (ret < 0) {
> + goto fail;
> + }
> + buf += ret;
> + buflen -= ret;
> + }
> +
> /* Feature table */
> Qcow2Feature features[] = {
> {
> @@ -2056,6 +2315,10 @@ static int qcow2_create2(const char *filename, int64_t
> total_size,
> {
> int cluster_bits;
> QDict *options;
> + const char *fdestr;
> + QCryptoBlockCreateOptions *fdeopts = NULL;
> + QCryptoBlock *fde = NULL;
> + size_t i;
>
> /* Calculate cluster_bits */
> cluster_bits = ctz32(cluster_size);
> @@ -2179,7 +2442,48 @@ static int qcow2_create2(const char *filename, int64_t
> total_size,
> };
>
> if (flags & BLOCK_FLAG_ENCRYPT) {
> - header->crypt_method = cpu_to_be32(QCOW_CRYPT_AES);
> + /* Default to LUKS if fde-format is not set */
> + fdestr = qemu_opt_get_del(opts, QCOW2_OPT_ENC_FORMAT);
> + if (fdestr) {
> + for (i = 0; i < Q_CRYPTO_BLOCK_FORMAT__MAX; i++) {
> + if (g_str_equal(QCryptoBlockFormat_lookup[i],
> + fdestr)) {
> + fdeopts = qcow2_fde_create_opts_init(i,
> + opts,
> + errp);
> + if (!fdeopts) {
> + ret = -EINVAL;
> + goto out;
> + }
> + break;
> + }
> + }
> + if (!fdeopts) {
> + error_setg(errp, "Unknown fde format %s", fdestr);
> + ret = -EINVAL;
> + goto out;
> + }
> + } else {
> + fdeopts = qcow2_fde_create_opts_init(
> + Q_CRYPTO_BLOCK_FORMAT_LUKS,
> + opts, errp);
> + if (!fdeopts) {
> + ret = -EINVAL;
> + goto out;
> + }
> + }
> + switch (fdeopts->format) {
> + case Q_CRYPTO_BLOCK_FORMAT_QCOWAES:
> + header->crypt_method = cpu_to_be32(QCOW_CRYPT_AES);
> + break;
> + case Q_CRYPTO_BLOCK_FORMAT_LUKS:
> + header->crypt_method = cpu_to_be32(QCOW_CRYPT_LUKS);
> + break;
> + default:
> + error_setg(errp, "Unsupported fde format %s", fdestr);
> + ret = -EINVAL;
> + goto out;
> + }
> } else {
> header->crypt_method = cpu_to_be32(QCOW_CRYPT_NONE);
> }
> @@ -2213,12 +2517,15 @@ static int qcow2_create2(const char *filename,
> int64_t total_size,
> /*
> * And now open the image and make it consistent first (i.e. increase the
> * refcount of the cluster that is occupied by the header and the
> refcount
> - * table)
> + * table). Using BDRV_O_NO_IO since we've not written any encryption
> + * header yet and thus should not read/write payload data or initialize
> + * the decryption context
> */
> options = qdict_new();
> qdict_put(options, "driver", qstring_from_str("qcow2"));
> ret = bdrv_open(&bs, filename, NULL, options,
> - BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH,
> + BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH |
> + BDRV_O_NO_IO,
> &local_err);
Somehow I feel that saying BDRV_O_NO_IO, but doing I/O will bite us
sooner or later.
Why not leave header->crypt_method = 0 initially and only set up
encryption after opening the image with the qcow2 driver?
> if (ret < 0) {
> error_propagate(errp, local_err);
> @@ -2253,6 +2560,24 @@ static int qcow2_create2(const char *filename, int64_t
> total_size,
> }
> }
>
> + /* Want encryption ? There you go.*/
Move that space character from before '?' to after '.' and it will look
right. ;-)
> + if (fdeopts) {
> + fde = qcrypto_block_create(fdeopts,
> + qcow2_fde_header_init_func,
> + qcow2_fde_header_write_func,
> + bs,
> + errp);
> + if (!fde) {
> + ret = -EINVAL;
> + goto out;
> + }
> + ret = qcow2_update_header(bs);
> + if (ret < 0) {
> + error_setg_errno(errp, -ret, "Could not write encryption
> header");
> + goto out;
> + }
> + }
> +
> /* And if we're supposed to preallocate metadata, do that now */
> if (prealloc != PREALLOC_MODE_OFF) {
> BDRVQcow2State *s = bs->opaque;
> @@ -2272,7 +2597,8 @@ static int qcow2_create2(const char *filename, int64_t
> total_size,
> options = qdict_new();
> qdict_put(options, "driver", qstring_from_str("qcow2"));
> ret = bdrv_open(&bs, filename, NULL, options,
> - BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_BACKING,
> + BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_BACKING |
> + BDRV_O_NO_IO,
> &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> @@ -3047,10 +3373,10 @@ 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->cipher);
> + !!s->fde);
>
> - if (encrypt != !!s->cipher) {
> - error_report("Changing the encryption flag is not
> supported");
> + if (encrypt != !!s->fde) {
> + fprintf(stderr, "Changing the encryption flag is not
> supported");
> return -ENOTSUP;
> }
> } else if (!strcmp(desc->name, BLOCK_OPT_CLUSTER_SIZE)) {
> @@ -3285,6 +3611,41 @@ static QemuOptsList qcow2_create_opts = {
> .help = "Width of a reference count entry in bits",
> .def_value_str = "16"
> },
> + {
> + .name = QCOW2_OPT_ENC_FORMAT,
> + .type = QEMU_OPT_STRING,
> + .help = "Disk encryption format, 'luks' (default) or 'qcowaes'
> (deprecated)",
> + },
> + {
> + .name = QCOW2_OPT_KEY_ID,
> + .type = QEMU_OPT_STRING,
> + .help = "ID of the secret that provides the encryption key",
> + },
> + {
> + .name = QCOW2_OPT_CIPHER_ALG,
> + .type = QEMU_OPT_STRING,
> + .help = "Name of encryption cipher algorithm",
> + },
> + {
> + .name = QCOW2_OPT_CIPHER_MODE,
> + .type = QEMU_OPT_STRING,
> + .help = "Name of encryption cipher mode",
> + },
> + {
> + .name = QCOW2_OPT_IVGEN_ALG,
> + .type = QEMU_OPT_STRING,
> + .help = "Name of IV generator algorithm",
> + },
> + {
> + .name = QCOW2_OPT_IVGEN_HASH_ALG,
> + .type = QEMU_OPT_STRING,
> + .help = "Name of IV generator hash algorithm",
> + },
> + {
> + .name = QCOW2_OPT_HASH_ALG,
> + .type = QEMU_OPT_STRING,
> + .help = "Name of encryption hash algorithm",
> + },
> { /* end of list */ }
> }
> };
> @@ -3302,7 +3663,6 @@ BlockDriver bdrv_qcow2 = {
> .bdrv_create = qcow2_create,
> .bdrv_has_zero_init = bdrv_has_zero_init_1,
> .bdrv_co_get_block_status = qcow2_co_get_block_status,
> - .bdrv_set_key = qcow2_set_key,
>
> .bdrv_co_readv = qcow2_co_readv,
> .bdrv_co_writev = qcow2_co_writev,
> diff --git a/block/qcow2.h b/block/qcow2.h
> index ae04285..d33afb2 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -25,7 +25,7 @@
> #ifndef BLOCK_QCOW2_H
> #define BLOCK_QCOW2_H
>
> -#include "crypto/cipher.h"
> +#include "crypto/block.h"
> #include "qemu/coroutine.h"
>
> //#define DEBUG_ALLOC
> @@ -36,6 +36,7 @@
>
> #define QCOW_CRYPT_NONE 0
> #define QCOW_CRYPT_AES 1
> +#define QCOW_CRYPT_LUKS 2
>
> #define QCOW_MAX_CRYPT_CLUSTERS 32
> #define QCOW_MAX_SNAPSHOTS 65536
> @@ -98,6 +99,15 @@
> #define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size"
> #define QCOW2_OPT_CACHE_CLEAN_INTERVAL "cache-clean-interval"
>
> +#define QCOW2_OPT_ENC_FORMAT "encryption-format"
> +#define QCOW2_OPT_KEY_ID "key-id"
> +#define QCOW2_OPT_CIPHER_ALG "cipher-alg"
> +#define QCOW2_OPT_CIPHER_MODE "cipher-mode"
> +#define QCOW2_OPT_IVGEN_ALG "ivgen-alg"
> +#define QCOW2_OPT_IVGEN_HASH_ALG "ivgen-hash-alg"
> +#define QCOW2_OPT_HASH_ALG "hash-alg"
> +
> +
> typedef struct QCowHeader {
> uint32_t magic;
> uint32_t version;
> @@ -163,6 +173,11 @@ typedef struct QCowSnapshot {
> struct Qcow2Cache;
> typedef struct Qcow2Cache Qcow2Cache;
>
> +typedef struct Qcow2FDEHeaderExtension {
> + uint64_t offset;
> + uint64_t length;
> +} Qcow2FDEHeaderExtension;
Packed? It seems to be read directly from the file.
> typedef struct Qcow2UnknownHeaderExtension {
> uint32_t magic;
> uint32_t len;
> @@ -256,7 +271,9 @@ typedef struct BDRVQcow2State {
>
> CoMutex lock;
>
> - QCryptoCipher *cipher; /* current cipher, NULL if no key yet */
> + Qcow2FDEHeaderExtension fde_header; /* QCow2 header extension */
> + QCryptoBlockOpenOptions *fde_opts; /* Disk encryption runtime options */
> + QCryptoBlock *fde; /* Disk encryption format driver */
> uint32_t crypt_method_header;
> uint64_t snapshots_offset;
> int snapshots_size;
> diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
> index f236d8c..dcbe3c3 100644
> --- a/docs/specs/qcow2.txt
> +++ b/docs/specs/qcow2.txt
> @@ -45,6 +45,7 @@ The first cluster of a qcow2 image contains the file header:
> 32 - 35: crypt_method
> 0 for no encryption
> 1 for AES encryption
> + 2 for LUKS encryption
>
> 36 - 39: l1_size
> Number of entries in the active L1 table
> @@ -123,6 +124,7 @@ be stored. Each extension has a structure like the
> following:
> 0x00000000 - End of the header extension area
> 0xE2792ACA - Backing file format name
> 0x6803f857 - Feature name table
> + 0x4c554b53 - Full disk encryption header pointer
> other - Unknown header extension, can be safely
> ignored
>
> @@ -166,6 +168,75 @@ the header extension data. Each entry look like this:
> terminated if it has full length)
>
>
> +== Full disk encryption (FDE) header pointer ==
> +
> +For 'crypt_method' header values which require additional header metadata
> +to be stored (eg 'LUKS' header), the full disk encryption header pointer
> +extension is mandatory.
I think you want to make that "is present if, and only if, the
'crypt_method' header value requires metadata".
At least, we need to forbid it for the existing AES images, because old
qemu-img version would stll fix the "leaked clusters", without removing
the header extension that refers to them.
> It provides the offset at which the crypt method
> +can store its additional data, as well as the length of such data.
> +
> + Byte 0 - 7: Offset into the image file at which the encryption
> + header starts. Must be aligned to a cluster boundary.
> + Byte 8 - 16: Length of the encryption header. Must be a multiple
> + of the cluster size.
> +
> +While the header extension allocates the length as a multiple of the
> +cluster size, the encryption header may not consume the full permitted
> +allocation.
> +
> +For the LUKS crypt method, the encryption header works as follows.
> +
> +The first 592 bytes of the header clusters will contain the LUKS
> +partition header. This is then followed by the key material data areas.
> +The size of the key material data areas is determined by the number of
> +stripes in the key slot and key size. Refer to the LUKS format
> +specification ('docs/on-disk-format.pdf' in the cryptsetup source
> +package) for details of the LUKS partition header format.
> +
> +In the LUKS partition header, the "payload-offset" field does not refer
> +to the offset of the QCow2 payload. Instead it simply refers to the
> +total required length of the LUKS header plus key material regions.
> +
> +In the LUKS key slots header, the "key-material-offset" is relative
> +to the start of the LUKS header clusters in the qcow2 container,
> +not the start of the qcow2 file.
> +
> +Logically the layout looks like
> +
> + +-----------------------------+
> + | QCow2 header |
> + +-----------------------------+
> + | QCow2 header extension X |
> + +-----------------------------+
> + | QCow2 header extension FDE |
> + +-----------------------------+
> + | QCow2 header extension ... |
> + +-----------------------------+
> + | QCow2 header extension Z |
> + +-----------------------------+
> + | ....other QCow2 tables.... |
> + . .
> + . .
> + +-----------------------------+
> + | +-------------------------+ |
> + | | LUKS partition header | |
> + | +-------------------------+ |
> + | | LUKS key material 1 | |
> + | +-------------------------+ |
> + | | LUKS key material 2 | |
> + | +-------------------------+ |
> + | | LUKS key material ... | |
> + | +-------------------------+ |
> + | | LUKS key material 8 | |
> + | +-------------------------+ |
> + +-----------------------------+
> + | QCow2 cluster payload |
> + . .
> + . .
> + . .
> + | |
> + +-----------------------------+
> +
> == Host cluster management ==
>
> qcow2 manages the allocation of host clusters by maintaining a reference
> count
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 200d907..40fe3ba 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1789,6 +1789,8 @@
> # @cache-clean-interval: #optional clean unused entries in the L2 and
> refcount
> # caches. The interval is in seconds. The default
> value
> # is 0 and it disables this feature (since 2.5)
> +# @key-id: #optional the ID of a QCryptoSecret object
> providing
> +# the decryption key (since 2.6)
> #
> # Since: 1.7
> ##
> @@ -1802,8 +1804,8 @@
> '*cache-size': 'int',
> '*l2-cache-size': 'int',
> '*refcount-cache-size': 'int',
> - '*cache-clean-interval': 'int' } }
> -
> + '*cache-clean-interval': 'int',
> + '*key-id': 'str' } }
>
> ##
> # @BlockdevOptionsArchipelago
> --
> 2.5.0
>
>
Kevin
- Re: [Qemu-devel] [Qemu-block] [PATCH v1 07/15] block: add flag to indicate that no I/O will be performed, (continued)
- [Qemu-devel] [PATCH v1 11/15] qcow: make encrypt_sectors encrypt in place, Daniel P. Berrange, 2016/01/12
- [Qemu-devel] [PATCH v1 14/15] block: remove all encryption handling APIs, Daniel P. Berrange, 2016/01/12
- [Qemu-devel] [PATCH v1 12/15] qcow: convert QCow to use QCryptoBlock for encryption, Daniel P. Berrange, 2016/01/12
- [Qemu-devel] [PATCH v1 15/15] block: remove support for legecy AES qcow/qcow2 encryption, Daniel P. Berrange, 2016/01/12
- [Qemu-devel] [PATCH v1 08/15] block: add generic full disk encryption driver, Daniel P. Berrange, 2016/01/12
- [Qemu-devel] [PATCH v1 13/15] block: rip out all traces of password prompting, Daniel P. Berrange, 2016/01/12
- [Qemu-devel] [PATCH v1 10/15] qcow2: convert QCow2 to use QCryptoBlock for encryption, Daniel P. Berrange, 2016/01/12
- Re: [Qemu-devel] [Qemu-block] [PATCH v1 10/15] qcow2: convert QCow2 to use QCryptoBlock for encryption,
Kevin Wolf <=
- [Qemu-devel] [PATCH v1 04/15] crypto: add support for anti-forensic split algorithm, Daniel P. Berrange, 2016/01/12
- [Qemu-devel] [PATCH v1 06/15] crypto: implement the LUKS block encryption format, Daniel P. Berrange, 2016/01/12
- [Qemu-devel] [PATCH v1 05/15] crypto: add block encryption framework, Daniel P. Berrange, 2016/01/12