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 <berra...@redhat.com>
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