| Message ID | 1452624982-19332-11-git-send-email-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <berrange@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
On Wed, Jan 13, 2016 at 07:42:20PM +0100, Kevin Wolf wrote: > 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 <berrange@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. Opps, yes, having those garbage collected would be a very bad thing for your ability to decrypt your data :-) > > @@ -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. Ok, will rename this - wasn't too happy with FDE myself either. > > 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. I wasn't too sure - I figured since the block layer seems to propagate errno's around alot, that I ought to report an errno here, but will happiyl drop it. > > + 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. The crypto API doesn't deal with errno's at all - it uses the Error object exclusively, so yeah, I can drop it from the place below. > > > + } > > + > > + 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()? Ok. > > > + 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. Will fix as per the read_func() above. > > @@ -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? Yep, I can do that. > > #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. Ok, that's just my habit from libvirt - where we either put everything on one line, or everything on a separate line and don't mix. > > @@ -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. Indeed, once I add the assertions you suggested in the previous patch this will probably break horribly. > Why not leave header->crypt_method = 0 initially and only set up > encryption after opening the image with the qcow2 driver? Oh yes, good idea. > > 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. Yes > > @@ -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. Yes, we shouldn't be using the header with the AES crypt method, only the LUKS method for now. Regards, Daniel
Am 14.01.2016 um 13:14 hat Daniel P. Berrange geschrieben: > On Wed, Jan 13, 2016 at 07:42:20PM +0100, Kevin Wolf wrote: > > Am 12.01.2016 um 19:56 hat Daniel P. Berrange geschrieben: > > > +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. > > I wasn't too sure - I figured since the block layer seems to > propagate errno's around alot, that I ought to report an > errno here, but will happiyl drop it. error_setg_errno() doesn't keep the error code around for the callers to inspect, but just adds the error string to the message. And you already have a much more useful error message than "Invalid argument". > > > + 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. > > The crypto API doesn't deal with errno's at all - it uses the > Error object exclusively, so yeah, I can drop it from the > place below. Then you could probably just make the function void. I genereally prefer to use only one mechanism to return errors instead of both an int return value and an Error** argument, but there are many places in qemu which use both. So whatever feels right to you. Kevin
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index f5bc4f2..33297f8 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -341,45 +341,6 @@ static int count_contiguous_clusters_by_type(int nb_clusters, return i; } -int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t sector_num, - uint8_t *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; - 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, - buf, buf, - 512, - errp); - } else { - ret = qcrypto_cipher_decrypt(s->cipher, - buf, buf, - 512, - errp); - } - if (ret < 0) { - return -1; - } - sector_num++; - buf += 512; - } - return 0; -} - static int coroutine_fn copy_sectors(BlockDriverState *bs, uint64_t start_sect, uint64_t cluster_offset, @@ -421,10 +382,11 @@ static int coroutine_fn copy_sectors(BlockDriverState *bs, if (bs->encrypted) { Error *err = NULL; - assert(s->cipher); - if (qcow2_encrypt_sectors(s, start_sect + n_start, + assert(s->fde); + if (qcrypto_block_encrypt(s->fde, + start_sect + n_start, iov.iov_base, n, - true, &err) < 0) { + &err) < 0) { ret = -EIO; error_free(err); goto out; 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 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"); + return -1; + } + + 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; +} + + +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); + 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"); + 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; +} + + /* * 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; #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); + 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); + 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) { + 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); 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.*/ + 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; + 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. 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
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 <berrange@redhat.com> --- block/qcow2-cluster.c | 46 +---- block/qcow2.c | 502 +++++++++++++++++++++++++++++++++++++++++++------- block/qcow2.h | 21 ++- docs/specs/qcow2.txt | 71 +++++++ qapi/block-core.json | 6 +- 5 files changed, 529 insertions(+), 117 deletions(-)