Convert QCow[2] to QCryptoBlock & add LUKS support
Subsystems (2)
v5 cover Convert QCow[2] to QCryptoBlock & add LUKS support
Previously posted:
v1: https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg00201.html
v2: https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05147.html
v3: https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05671.html
This series is a continuation of previous work to support LUKS in QEMU. The existing merged code supports LUKS as a standalone driver which can be layered over/under any other QEMU block device driver. This works well when using LUKS over protocol drivers (file, rbd, iscsi, etc, etc), but has some downsides when combined with format drivers like qcow2.
If you layer LUKS under qcow2 (eg qcow2 -> luks -> file) then you cannot get any information about the qcow2 file without first decrypting it, as both the header and payload are encrypted.
If you layer LUKS over qcow2 (eg luks -> qcow2 -> file) then you cannot distinguish between a qcow2 file where the guest has done LUKS encryption from a qcow2 file which qemu has done encryption. More seriously, when encrypting sectors the guest virtual sector is used as the input for deriving the initialization vectors. When internal snapshots are used, this means that multiple sectors in the qcow2 file may be encrypted with the same initialization vector. This is a security weakness when combined with certain cryptographic modes.
Integrating LUKS natively into qcow2 allows us to combine the best aspects of both layering strategies above. In particular the header remains unecrypted, but initialization vectors are generated using physical sector numbers preserving security when snapshots are used. This is a change from previous postings of this work, where the IVs were (incorrectly) generated based on the virtual disk sector.
In a previous posting of this work, Fam had suggested that we do integration by layering luks over qcow2, but having QEMU block layer automatically create the luks driver above qcow2 based on the qcow2 header crypt_method field. This is not possible though, because such a scheme would suffer from the problem of IVs being generated from the virtual disk sector instead of physical disk sector. So having LUKS specific code in the qcow2 block driver is unavoidable. In comparison to the previous posting though, the amount of code in qcow2.c has been reduced by allowing re-use of code from block/crypto.c for handling QemuOpts -> QAPI conversion. So extra lines of code in qcow2 to support LUKS is < 200.
I have also split the changes to qcow2 up into 2 patches. The first patch simply introduces use of the QCryptoBlock framework to qcow2 for the existing (deprecated) AES-CBC encryption method. The second patch wires up the LUKS support for qcow2. This makes it clearer which parts of the changes are related to plain code refactoring, vs enabling the new features. Specifically we can now see that the LUKS enablement in qcow2 has this footprint:
Changed in v5:
- Remove accidental use of tabs in spec (Alberto)
- Clarify payload-offset position semantics (Alberto)
- Fix leak of QemuOpts in qcow v1 (Alberto)
- Avoid overwriting existing Error ** (Alberto)
- Reuse string -> enum convertor for qcow2 encryption format (Alberto)
- Fix iotests to deal with recent changes on git master
Changed in v4:
- Change qcow size check to == 0, instead of <= 1 (Alberto/Eric)
- Fix crash if qcow2 header contains unexpected crypt method (Alberto)
- Formatting tweaks in qcow2 spec additions (Max)
- Remove badly merged comment in qapi schema (Max)
- Don't add iotest number 173 (Max)
- Fix test-qcrypto-block.c (Max)
Changed in v3:
- Modify qemu-img to check for 'encryption-format' option too and reject it when combined with compression
- Add check to prevent 'qemu-img amend' changing encryption format
- Ensure crypto layer is able to report correct option names in errors. ie luks-key-secret rather than just key-secret
- Use read -P 0 in test case 174
Changed in v2:
- Split qcow2 LUKS tests into separate patch
- Split qcow2 LUKS spec addition into separate patch
- Use strstart instead of g_str_has_prefix + pointer manipulation
- Use -1 instead of 1 for error condition when iterating over opts
- Fix formatting of qemu-img manpage for qcow2 AES flaws list
- Fix writing zeros in qcow when encrypting sector
- Don't overwrite input data buffer in qcow2 when encrypting data
- Use TODO instead of XXX markers
- Rename qcow2_change_encryption to qcow2_set_up_encryption
- Add missing QEMU_IO_OPTIONS_NO_FMT variable to iotests
- Explicitly fill crypto header unused space with zeros
- Fix byte-swapping of crypto header in qcow2
- Enforce crypto header offset is multiple of cluster size
- Move setting of crypt_physical_offset flag
- Fix docs for 'encryption-format' option
- Deprecate legacy 'encryption' option
- Drop redundant test scenarios
- Use small file sizes for iotests
- Drop pbkdf iteration time to 10ms during iotests
- Use separate passphrase for top vs backing file in iotests
- Mark 'encryption_key_missing@ field as deprecated
Daniel P. Berrange (18):
block: expose crypto option names / defs to other drivers
block: add ability to set a prefix for opt names
qcow: document another weakness of qcow AES encryption
qcow: require image size to be > 1 for new images
iotests: skip 042 with qcow which dosn't support zero sized images
iotests: skip 048 with qcow which doesn't support resize
iotests: fix 097 when run with qcow
qcow: make encrypt_sectors encrypt in place
qcow: convert QCow to use QCryptoBlock for encryption
qcow2: make qcow2_encrypt_sectors encrypt in place
qcow2: convert QCow2 to use QCryptoBlock for encryption
qcow2: extend specification to cover LUKS encryption
qcow2: add support for LUKS encryption format
qcow2: add iotests to cover LUKS encryption support
iotests: enable tests 134 and 158 to work with qcow (v1)
block: rip out all traces of password prompting
block: remove all encryption handling APIs
block: pass option prefix down to crypto layer
block.c | 77 +------- block/crypto.c | 171 ++++++++++++------ block/crypto.h | 102 +++++++++++ block/qapi.c | 2 +- block/qcow.c | 216 ++++++++++------------ block/qcow2-cluster.c | 56 +----- block/qcow2-refcount.c | 10 + block/qcow2.c | 442 ++++++++++++++++++++++++++++++++++++--------- block/qcow2.h | 17 +- blockdev.c | 37 +--- crypto/block-luks.c | 8 +- crypto/block-qcow.c | 8 +- crypto/block.c | 6 +- crypto/blockpriv.h | 2 + docs/specs/qcow2.txt | 96 ++++++++++ hmp.c | 31 ---- include/block/block.h | 3 - include/block/block_int.h | 2 +- include/crypto/block.h | 6 +- include/monitor/monitor.h | 7 - include/qapi/error.h | 1 - include/qemu/osdep.h | 2 - monitor.c | 68 ------- qapi-schema.json | 10 +- qapi/block-core.json | 33 +++- qapi/common.json | 5 +- qemu-img.c | 35 +--- qemu-img.texi | 19 +- qemu-io.c | 20 -- qmp.c | 12 +- tests/qemu-iotests/042 | 2 +- tests/qemu-iotests/048 | 2 +- tests/qemu-iotests/049 | 2 +- tests/qemu-iotests/049.out | 4 +- tests/qemu-iotests/082.out | 297 +++++++++++++++++++++++++++--- tests/qemu-iotests/087 | 37 +++- tests/qemu-iotests/087.out | 16 +- tests/qemu-iotests/097 | 10 +- tests/qemu-iotests/097.out | 125 +------------ tests/qemu-iotests/134 | 20 +- tests/qemu-iotests/134.out | 10 +- tests/qemu-iotests/158 | 21 ++- tests/qemu-iotests/158.out | 14 +- tests/qemu-iotests/175 | 126 +++++++++++++ tests/qemu-iotests/175.out | 119 ++++++++++++ tests/qemu-iotests/176 | 76 ++++++++ tests/qemu-iotests/176.out | 18 ++ tests/qemu-iotests/177 | 86 +++++++++ tests/qemu-iotests/177.out | 26 +++ tests/qemu-iotests/common | 10 +- tests/qemu-iotests/group | 3 + tests/test-crypto-block.c | 8 +- util/oslib-posix.c | 66 ------- util/oslib-win32.c | 24 --- 54 files changed, 1686 insertions(+), 940 deletions(-) create mode 100644 block/crypto.h create mode 100755 tests/qemu-iotests/175 create mode 100644 tests/qemu-iotests/175.out create mode 100755 tests/qemu-iotests/176 create mode 100644 tests/qemu-iotests/176.out create mode 100755 tests/qemu-iotests/177 create mode 100644 tests/qemu-iotests/177.out
Signature · 1 line
2.9.3
PATCH v5 1/18 block: expose crypto option names / defs to other drivers
The block/crypto.c defines a set of QemuOpts that provide parameters for encryption. This will also be needed by the qcow/qcow2 integration, so expose the relevant pieces in a new block/crypto.h header.
--- block/crypto.c | 61 +++++++-------------------------------- block/crypto.h | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+), 50 deletions(-) create mode 100644 block/crypto.h
block/crypto.c +11 -50
@@ -26,14 +26,7 @@#include "qapi/opts-visitor.h" #include "qapi-visit.h" #include "qapi/error.h" - -#define BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET "key-secret" -#define BLOCK_CRYPTO_OPT_LUKS_CIPHER_ALG "cipher-alg" -#define BLOCK_CRYPTO_OPT_LUKS_CIPHER_MODE "cipher-mode" -#define BLOCK_CRYPTO_OPT_LUKS_IVGEN_ALG "ivgen-alg" -#define BLOCK_CRYPTO_OPT_LUKS_IVGEN_HASH_ALG "ivgen-hash-alg" -#define BLOCK_CRYPTO_OPT_LUKS_HASH_ALG "hash-alg" -#define BLOCK_CRYPTO_OPT_LUKS_ITER_TIME "iter-time" +#include "block/crypto.h" typedef struct BlockCrypto BlockCrypto ;
@@ -135,11 +128,7 @@ static QemuOptsList block_crypto_runtime_opts_luks = { .name = "crypto", .head = QTAILQ_HEAD_INITIALIZER(block_crypto_runtime_opts_luks.head), .desc = { - { - .name = BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET, - .type = QEMU_OPT_STRING, - .help = "ID of the secret that provides the encryption key", - }, + BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET, {/* end of list */ } }, };
@@ -154,47 +143,19 @@ static QemuOptsList block_crypto_create_opts_luks = { .type = QEMU_OPT_SIZE, .help = "Virtual disk size" }, - { - .name = BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET, - .type = QEMU_OPT_STRING, - .help = "ID of the secret that provides the encryption key", - }, - { - .name = BLOCK_CRYPTO_OPT_LUKS_CIPHER_ALG, - .type = QEMU_OPT_STRING, - .help = "Name of encryption cipher algorithm", - }, - { - .name = BLOCK_CRYPTO_OPT_LUKS_CIPHER_MODE, - .type = QEMU_OPT_STRING, - .help = "Name of encryption cipher mode", - }, - { - .name = BLOCK_CRYPTO_OPT_LUKS_IVGEN_ALG, - .type = QEMU_OPT_STRING, - .help = "Name of IV generator algorithm", - }, - { - .name = BLOCK_CRYPTO_OPT_LUKS_IVGEN_HASH_ALG, - .type = QEMU_OPT_STRING, - .help = "Name of IV generator hash algorithm", - }, - { - .name = BLOCK_CRYPTO_OPT_LUKS_HASH_ALG, - .type = QEMU_OPT_STRING, - .help = "Name of encryption hash algorithm", - }, - { - .name = BLOCK_CRYPTO_OPT_LUKS_ITER_TIME, - .type = QEMU_OPT_NUMBER, - .help = "Time to spend in PBKDF in milliseconds", - }, + BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET, +BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_ALG , +BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_MODE , +BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_ALG , +BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_HASH_ALG , +BLOCK_CRYPTO_OPT_DEF_LUKS_HASH_ALG , + BLOCK_CRYPTO_OPT_DEF_LUKS_ITER_TIME, { /* end of list */ } }, }; -static QCryptoBlockOpenOptions * +QCryptoBlockOpenOptions *block_crypto_open_opts_init (QCryptoBlockFormat format,QemuOpts *opts,Error **errp)
@@ -240,7 +201,7 @@ block_crypto_open_opts_init(QCryptoBlockFormat format, } -static QCryptoBlockCreateOptions * +QCryptoBlockCreateOptions *block_crypto_create_opts_init (QCryptoBlockFormat format,QemuOpts *opts,Error **errp)
block/crypto.h new file +91 -0
91 lines not on this page · raw message
Signature · 1 line
2.9.3
PATCH v5 2/18 block: add ability to set a prefix for opt names
In reply to Daniel P. BerrangeWhen integrating the crypto support with qcow/qcow2, we don't want to use the bare LUKS option names "hash-alg", "key-secret", etc. We want to namespace them "luks-hash-alg", "luks-key-secret" so that they don't clash with any general qcow options at a later date.
--- block/crypto.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++++-------- block/crypto.h | 42 +++++++++++----------- 2 files changed, 119 insertions(+), 34 deletions(-)
block/crypto.c +97 -14
@@ -27,6 +27,7 @@#include "qapi-visit.h" #include "qapi/error.h" #include "block/crypto.h" +#include "qemu/cutils.h" typedef struct BlockCrypto BlockCrypto ;
@@ -128,7 +129,7 @@ static QemuOptsList block_crypto_runtime_opts_luks = { .name = "crypto", .head = QTAILQ_HEAD_INITIALIZER(block_crypto_runtime_opts_luks.head), .desc = { - BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET, + BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET(""), {/* end of list */ } }, };
@@ -143,31 +144,101 @@ static QemuOptsList block_crypto_create_opts_luks = { .type = QEMU_OPT_SIZE, .help = "Virtual disk size" }, - BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET, -BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_ALG , -BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_MODE , -BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_ALG , -BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_HASH_ALG , -BLOCK_CRYPTO_OPT_DEF_LUKS_HASH_ALG , - BLOCK_CRYPTO_OPT_DEF_LUKS_ITER_TIME, + BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET(""), + BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_ALG(""), + BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_MODE(""), + BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_ALG(""), + BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_HASH_ALG(""), + BLOCK_CRYPTO_OPT_DEF_LUKS_HASH_ALG(""), + BLOCK_CRYPTO_OPT_DEF_LUKS_ITER_TIME(""), {/* end of list */ } }, }; +static QemuOptsList empty_opts = { + .name ="crypto-empty" , + .merge_lists = false, + .head =QTAILQ_HEAD_INITIALIZER (empty_opts.head), + .desc = { +/* no elements => accept any params */ + {/* end of list */ } + }, +}; + + +struct BlockCryptoCopyData { +QemuOpts *opts; +const char *prefix; +}; + +static int block_crypto_copy_value (void *opaque,const char *name, +const char *value,Error **errp) +{ +struct BlockCryptoCopyData *data = opaque; +const char *newname; + +if (strstart (name, data->prefix, &newname)) { +Error *local_err =NULL ; + +qemu_opt_set (data->opts, newname, value, &local_err); +if (local_err) { +error_propagate (errp, local_err); +return -1 ; + } + } + +return 0 ; +} + +/* +* Create a copy of @opts containing only the fields with +* a prefix of @prefix, stripping the prefix in the returned +* opts +*/ +static QemuOpts * +block_crypto_copy_opts (QemuOpts *opts, +const char *prefix, +Error **errp) +{ +struct BlockCryptoCopyData data = { + .opts =qemu_opts_create (&empty_opts,NULL , false, errp), + .prefix = prefix + }; +if (!data.opts) { +return NULL ; + } + +if (qemu_opt_foreach (opts, block_crypto_copy_value, &data, errp) <0 ) { +qemu_opts_del (data.opts); +return NULL ; + } + +return data.opts; +}QCryptoBlockOpenOptions *block_crypto_open_opts_init (QCryptoBlockFormat format,QemuOpts *opts, +const char *prefix,Error **errp) { -Visitor *v; +Visitor *v =NULL ;QCryptoBlockOpenOptions *ret =NULL ;Error *local_err =NULL ; +QemuOpts *newopts =NULL ; ret =g_new0 (QCryptoBlockOpenOptions,1 ); ret->format = format; - v =opts_visitor_new (opts); +if (prefix !=NULL ) { + newopts =block_crypto_copy_opts (opts, prefix, &local_err); +if (local_err) { + goto out; + } + v =opts_visitor_new (newopts); + }else { + v =opts_visitor_new (opts); + }visit_start_struct (v,NULL ,NULL ,0 , &local_err);if (local_err) {
@@ -196,6 +267,7 @@ block_crypto_open_opts_init(QCryptoBlockFormat format,qapi_free_QCryptoBlockOpenOptions (ret); ret =NULL ; } +qemu_opts_del (newopts);visit_free (v);return ret; }
@@ -204,16 +276,26 @@ block_crypto_open_opts_init(QCryptoBlockFormat format,QCryptoBlockCreateOptions *block_crypto_create_opts_init (QCryptoBlockFormat format,QemuOpts *opts, +const char *prefix,Error **errp) { -Visitor *v; +Visitor *v =NULL ;QCryptoBlockCreateOptions *ret =NULL ;Error *local_err =NULL ; +QemuOpts *newopts =NULL ; ret =g_new0 (QCryptoBlockCreateOptions,1 ); ret->format = format; - v =opts_visitor_new (opts); +if (prefix !=NULL ) { + newopts =block_crypto_copy_opts (opts, prefix, &local_err); +if (local_err) { + goto out; + } + v =opts_visitor_new (newopts); + }else { + v =opts_visitor_new (opts); + }visit_start_struct (v,NULL ,NULL ,0 , &local_err);if (local_err) {
@@ -242,6 +324,7 @@ block_crypto_create_opts_init(QCryptoBlockFormat format,qapi_free_QCryptoBlockCreateOptions (ret); ret =NULL ; } +qemu_opts_del (newopts);visit_free (v);return ret; }
@@ -268,7 +351,7 @@ static int block_crypto_open_generic(QCryptoBlockFormat format, goto cleanup; } - open_opts =block_crypto_open_opts_init (format, opts, errp); + open_opts =block_crypto_open_opts_init (format, opts,NULL , errp);if (!open_opts) { goto cleanup; }
@@ -312,7 +395,7 @@ static int block_crypto_create_generic(QCryptoBlockFormat format, .filename = filename, }; - create_opts =block_crypto_create_opts_init (format, opts, errp); + create_opts =block_crypto_create_opts_init (format, opts,NULL , errp);if (!create_opts) {return -1 ; }
block/crypto.h +22 -20
84 lines not on this page · raw message
Signature · 1 line
2.9.3
Re: [Qemu-devel] [PATCH v5 02/18] block: add ability to set a prefix for opt names
Am 21.02.2017 um 12:54 hat Daniel P. Berrange geschrieben:
Quoted 5 lines
When integrating the crypto support with qcow/qcow2, we don't want to use the bare LUKS option names "hash-alg", "key-secret", etc. We want to namespace them "luks-hash-alg", "luks-key-secret" so that they don't clash with any general qcow options at a later date.
Or maybe "luks.key-secret", i.e. actually embed the LUKS options QAPI type into the qcow2 one? In that case, maybe qdict_extract_subqdict() can even be used before calling into this, so that we don't have to write a QemuOpts version of the function.
However, the only option I can see at the end of this series in BlockdevOptionsQcow2 is luks-key-secret, so what happened with this plan?
And if we really have only luks-key-secret (and that not in a separate sub-dict), I don't really see the need to have separate aes-key-secret and luks-key-secret options.
Quoted 3 lines
Reviewed-by Max Reitz <[email protected]>Reviewed-by Alberto Garcia <[email protected]>Signed-off-by Daniel P. Berrange <[email protected]>
Having said all that, while I'm not sure if the goal of the patch is completely right, it does seem to correctly implement what it promises.
Kevin
Re: [Qemu-devel] [PATCH v5 02/18] block: add ability to set a prefix for opt names
Quoted 11 lines · Kevin Wolf
Am 21.02.2017 um 12:54 hat Daniel P. Berrange geschrieben:
Quoted 5 lines
When integrating the crypto support with qcow/qcow2, we don't want to use the bare LUKS option names "hash-alg", "key-secret", etc. We want to namespace them "luks-hash-alg", "luks-key-secret" so that they don't clash with any general qcow options at a later date.
Or maybe "luks.key-secret", i.e. actually embed the LUKS options QAPI type into the qcow2 one? In that case, maybe
qdict_extract_subqdict()can even be used before calling into this, so that we don't have to write a QemuOpts version of the function.
Yeah, the use of '-' vs '.' is a significant decision point from a QAPI modelling POV. Currently as you see from earlier patch, I just add 'luks-key-secret' straight into BlockdevOptionsQcow2. ie I duplicate fields from QCryptoBlockOptionsLUKS straight into BlockdevOptionsQcow2. Likewise for fields from QCryptoBlockOptionsQCow.
If we used '.', we could potentially have BlockdevOptionsQcow2 directly reference the QCryptoBlockOptionsLUKS & QCryptoBlockOptionsQCow structs.
As a point of reference though, we didn't do this for the standalone 'luks' block driver - BlockdevOptionsLUKS contains a copy of the fields from QCryptoBlockOptionsLUKS because it needs to add inheritance from BlockdevOptionsGenericFormat.
Both approaches have pluses & minuses and I don't have a particularly strong opinion either way, so interested to hear what others thing.
Quoted 7 lines
However, the only option I can see at the end of this series in BlockdevOptionsQcow2 is luks-key-secret, so what happened with this plan?
And if we really have only luks-key-secret (and that not in a separate sub-dict), I don't really see the need to have separate aes-key-secret and luks-key-secret options.
Currently, all except the key-secret are create time options, not runtime and blockdev hasn't qapi'ified create time options. So the bits like luks-cipher-alg, luks-cipher-mode, etc don't appear in the QAPI schema, only the QemuOpts runtime_opts variable.
That both the legacy AES & LUKS modes only have a single runtime option is just a co-incidence that is likely to be a temporary state of affairs. It is quite possible that we'll gain more runtime LUKS options in the future. For example, options to specify which precise keyslot to load, or to provide the name of a separate image containing a standalone luks header, etc.
Hence, I wanted to have separation between the legacy AES & LUKS namespaces, to make it clear what applies to what scenario.
Quoted 6 lines
Quoted 3 lines
Reviewed-by Max Reitz <[email protected]>Reviewed-by Alberto Garcia <[email protected]>Signed-off-by Daniel P. Berrange <[email protected]>Having said all that, while I'm not sure if the goal of the patch is completely right, it does seem to correctly implement what it promises.
Regards,
Daniel
Signature · 3 lines
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
Re: [Qemu-devel] [PATCH v5 02/18] block: add ability to set a prefix for opt names
Quoted 13 lines · Daniel P. Berrange
Quoted 11 lines · Kevin Wolf
Am 21.02.2017 um 12:54 hat Daniel P. Berrange geschrieben:
Quoted 5 lines
When integrating the crypto support with qcow/qcow2, we don't want to use the bare LUKS option names "hash-alg", "key-secret", etc. We want to namespace them "luks-hash-alg", "luks-key-secret" so that they don't clash with any general qcow options at a later date.
Or maybe "luks.key-secret", i.e. actually embed the LUKS options QAPI type into the qcow2 one? In that case, maybe
qdict_extract_subqdict()can even be used before calling into this, so that we don't have to write a QemuOpts version of the function.
Quoted 8 lines
Yeah, the use of '-' vs '.' is a significant decision point from a QAPI modelling POV. Currently as you see from earlier patch, I just add 'luks-key-secret' straight into BlockdevOptionsQcow2. ie I duplicate fields from QCryptoBlockOptionsLUKS straight into BlockdevOptionsQcow2. Likewise for fields from QCryptoBlockOptionsQCow.
If we used '.', we could potentially have BlockdevOptionsQcow2 directly reference the QCryptoBlockOptionsLUKS & QCryptoBlockOptionsQCow structs.
Using '.' would mean a layer of {} nesting on the wire, maybe as in:
{ "driver": "qcow2", ..., "luks" : { "hash-alg": ... } }
but conceptually, I like that a bit better, as it consolidates all the luks-related options in one place, and may indeed make it possible to reuse the type rather than having two variants (one prefixed, one not, depending on whether it is standalone or qcow2).
I'm also looking later in your series (13/18), where you have:
@@ -2344,7 +2348,8 @@
'*l2-cache-size': 'int',
'*refcount-cache-size': 'int',
'*cache-clean-interval': 'int',
- '*aes-key-secret': 'str' } }
+ '*aes-key-secret': 'str',
+ '*luks-key-secret': 'str' } }Uggh - we have two optional parameters, that must not both be present at once. I'm wondering if we can instead do this (hmm, my patches for anonymous base/branches in a flat union haven't been taken yet, but you get the idea):
...
'*cache-clean-interval': 'int',
'*encryption': 'Qcow2Encryption' } }
{ 'enum': 'Qcow2EncryptionType': [ 'aes', 'luks' ] }
{ 'union': 'Qcow2Encryption', 'base': { 'type': 'Qcow2EncryptionType' },
'discriminator': 'type', 'data': {
'aes': { 'key-secret': 'str' },
'luks': { 'key-secret': 'str', '*hash-alg': ..., '*slot': 'int' } } }so that you can only provide one encryption type, but once you have that type, you can then provide all the associated fields for that type. So the QMP would look like:
{ "driver": "qcow2", ..., "encryption" : { "type": "luks", "hash-alg":
... } }
Quoted 8 lines
As a point of reference though, we didn't do this for the standalone 'luks' block driver - BlockdevOptionsLUKS contains a copy of the fields from QCryptoBlockOptionsLUKS because it needs to add inheritance from BlockdevOptionsGenericFormat.
Both approaches have pluses & minuses and I don't have a particularly strong opinion either way, so interested to hear what others thing.
I think there are enough potentials for alternative designs, especially where we can reuse code/structs without having to do prefixes, that it is worth exploring some of those possibilities to see if they turn out nicer.
Quoted 14 lines
Quoted 8 lines
However, the only option I can see at the end of this series in BlockdevOptionsQcow2 is luks-key-secret, so what happened with this plan?
And if we really have only luks-key-secret (and that not in a separate sub-dict), I don't really see the need to have separate aes-key-secret and luks-key-secret options.
Currently, all except the key-secret are create time options, not runtime and blockdev hasn't qapi'ified create time options. So the bits like luks-cipher-alg, luks-cipher-mode, etc don't appear in the QAPI schema, only the QemuOpts
runtime_optsvariable.
Quoted 6 lines
That both the legacy AES & LUKS modes only have a single runtime option is just a co-incidence that is likely to be a temporary state of affairs. It is quite possible that we'll gain more runtime LUKS options in the future. For example, options to specify which precise keyslot to load, or to provide the name of a separate image containing a standalone luks header, etc.
See my idea above about sticking a '*slot':'int' into the schema at the right place; made easier if the qcow2-level member 'encryption' is a nested flat union.
Quoted 3 lines
Hence, I wanted to have separation between the legacy AES & LUKS namespaces, to make it clear what applies to what scenario.
Again, I think the idea of a flat union, with the discriminator, makes it easier to enforce mutual exclusion, rather than having two top-level optional fields that cannot both be specified at once. Maybe we should also consider making qapi flat unions support a default discriminator, so that the "type":"luks" can be omitted, but that's sugar.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH v5 02/18] block: add ability to set a prefix for opt names
Am 22.02.2017 um 19:28 hat Eric Blake geschrieben:
Quoted 8 lines
Using '.' would mean a layer of {} nesting on the wire, maybe as in:
{ "driver": "qcow2", ..., "luks" : { "hash-alg": ... } }
but conceptually, I like that a bit better, as it consolidates all the luks-related options in one place, and may indeed make it possible to reuse the type rather than having two variants (one prefixed, one not, depending on whether it is standalone or qcow2).
Right, and this extra nesting to keep everything luks related in one place is exactly what I wanted to achieve with it.
Quoted 27 lines
I'm also looking later in your series (13/18), where you have:
(quoted diff) +2
-1@@ -2344,7 +2348,8 @@ '*l2-cache-size': 'int', '*refcount-cache-size': 'int', '*cache-clean-interval': 'int', - '*aes-key-secret': 'str' } } + '*aes-key-secret': 'str', + '*luks-key-secret': 'str' } }Uggh - we have two optional parameters, that must not both be present at once. I'm wondering if we can instead do this (hmm, my patches for anonymous base/branches in a flat union haven't been taken yet, but you get the idea):
...
'*cache-clean-interval': 'int',
'*encryption': 'Qcow2Encryption' } }{ 'enum': 'Qcow2EncryptionType': [ 'aes', 'luks' ] }
{ 'union': 'Qcow2Encryption', 'base': { 'type': 'Qcow2EncryptionType' },'discriminator': 'type', 'data': { 'aes': { 'key-secret': 'str' }, 'luks': { 'key-secret': 'str', '*hash-alg': ..., '*slot': 'int' } } }
Quoted 6 lines
so that you can only provide one encryption type, but once you have that type, you can then provide all the associated fields for that type. So the QMP would look like:
{ "driver": "qcow2", ..., "encryption" : { "type": "luks", "hash-alg":
... } }
That's actually even better, a more accurate description of the options on the QAPI level. I like it.
Quoted 8 lines
Quoted 2 lines
Hence, I wanted to have separation between the legacy AES & LUKS namespaces, to make it clear what applies to what scenario.
Again, I think the idea of a flat union, with the discriminator, makes it easier to enforce mutual exclusion, rather than having two top-level optional fields that cannot both be specified at once. Maybe we should also consider making qapi flat unions support a default discriminator, so that the "type":"luks" can be omitted, but that's sugar.
With the default discriminator, I'm not sure if that's still "sugar" or already "too much magic".
Kevin
Re: [Qemu-devel] [PATCH v5 02/18] block: add ability to set a prefix for opt names
Quoted 44 lines · Kevin Wolf
Am 22.02.2017 um 19:28 hat Eric Blake geschrieben:
Quoted 8 lines
Using '.' would mean a layer of {} nesting on the wire, maybe as in:
{ "driver": "qcow2", ..., "luks" : { "hash-alg": ... } }
but conceptually, I like that a bit better, as it consolidates all the luks-related options in one place, and may indeed make it possible to reuse the type rather than having two variants (one prefixed, one not, depending on whether it is standalone or qcow2).
Right, and this extra nesting to keep everything luks related in one place is exactly what I wanted to achieve with it.
Quoted 31 lines
I'm also looking later in your series (13/18), where you have:
(quoted diff) +2
-1@@ -2344,7 +2348,8 @@ '*l2-cache-size': 'int', '*refcount-cache-size': 'int', '*cache-clean-interval': 'int', - '*aes-key-secret': 'str' } } + '*aes-key-secret': 'str', + '*luks-key-secret': 'str' } }Uggh - we have two optional parameters, that must not both be present at once. I'm wondering if we can instead do this (hmm, my patches for anonymous base/branches in a flat union haven't been taken yet, but you get the idea):
...
'*cache-clean-interval': 'int',
'*encryption': 'Qcow2Encryption' } }{ 'enum': 'Qcow2EncryptionType': [ 'aes', 'luks' ] }
{ 'union': 'Qcow2Encryption', 'base': { 'type': 'Qcow2EncryptionType' },'discriminator': 'type', 'data': { 'aes': { 'key-secret': 'str' }, 'luks': { 'key-secret': 'str', '*hash-alg': ..., '*slot': 'int' } } }so that you can only provide one encryption type, but once you have that type, you can then provide all the associated fields for that type. So the QMP would look like:
Quoted 5 lines
Quoted 2 lines
{ "driver": "qcow2", ..., "encryption" : { "type": "luks", "hash-alg":
... } }That's actually even better, a more accurate description of the options on the QAPI level. I like it.
Ok, I'll try implementing this approach, though I won't have time to do this before soft-freeze, so the qcow2/luks feature is definitely 2.10 material now.
Regards,
Daniel
Signature · 3 lines
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
PATCH v5 3/18 qcow: document another weakness of qcow AES encryption
In reply to Daniel P. BerrangeDocument that use of guest virtual sector numbers as the basis for the initialization vectors is a potential weakness, when combined with internal snapshots or multiple images using the same passphrase. This fixes the formatting of the itemized list too.
--- qemu-img.texi | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)
qemu-img.texi +16 -3
@@ -544,16 +544,29 @@ The use of encryption in qcow and qcow2 images is considered to be flawed by modern cryptography standards, suffering from a number of design problems: @itemize @minus -@item The AES-CBC cipher is used with predictable initialization vectors based +@item +The AES-CBC cipher is used with predictable initialization vectors based on the sector number. This makes it vulnerable to chosen plaintext attacks which can reveal the existence of encrypted data. -@item The user passphrase is directly used as the encryption key. A poorly +@item +The user passphrase is directly used as the encryption key. A poorly chosen or short passphrase will compromise the security of the encryption. -@item In the event of the passphrase being compromised there is no way to +@item +In the event of the passphrase being compromised there is no way to change the passphrase to protect data in any qcow images. The files must be cloned, using a different encryption passphrase in the new file. The original file must then be securely erased using a program like shred, though even this is ineffective with many modern storage technologies. +@item +Initialization vectors used to encrypt sectors are based on the +guest virtual sector number, instead of the host physical sector. When +a disk image has multiple internal snapshots this means that data in +multiple physical sectors is encrypted with the same initialization +vector. With the CBC mode, this opens the possibility of watermarking +attacks if the attack can collect multiple sectors encrypted with the +same IV and some predictable data. Having multiple qcow2 images with +the same passphrase also exposes this weakness since the passphrase +is directly used as the key. @end itemize Use of qcow / qcow2 encryption is thus strongly discouraged. Users are
Signature · 1 line
2.9.3
PATCH v5 4/18 qcow: require image size to be > 1 for new images
In reply to Daniel P. BerrangeThe qcow driver refuses to open images which are less than 2 bytes in size, but will happily create such images. Add a check in the create path to avoid this discrepancy.
--- block/qcow.c | 6 ++++++ 1 file changed, 6 insertions(+)
block/qcow.c +6 -0
@@ -805,6 +805,12 @@ static int qcow_create(const char *filename, QemuOpts *opts, Error **errp)/* Read out options */ total_size =ROUND_UP (qemu_opt_get_size_del (opts,BLOCK_OPT_SIZE ,0 ),BDRV_SECTOR_SIZE ); +if (total_size ==0 ) { +error_setg (errp,"Image size is too small, cannot be zero length" ); + ret = -EINVAL ; + goto cleanup; + } + backing_file =qemu_opt_get_del (opts,BLOCK_OPT_BACKING_FILE );if (qemu_opt_get_bool_del (opts,BLOCK_OPT_ENCRYPT , false)) { flags |=BLOCK_FLAG_ENCRYPT ;
Signature · 1 line
2.9.3
PATCH v5 5/18 iotests: skip 042 with qcow which dosn't support zero sized images
In reply to Daniel P. BerrangeTest 042 is designed to verify operation with zero sized images. Such images are not supported with qcow (v1), so this test has always failed.
--- tests/qemu-iotests/042 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
tests/qemu-iotests/042 +1 -1
@@ -37,7 +37,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 . ./common.rc . ./common.filter -_supported_fmt qcow2 qcow qed +_supported_fmt qcow2 qed _supported_proto file _supported_os Linux
Signature · 1 line
2.9.3
PATCH v5 6/18 iotests: skip 048 with qcow which doesn't support resize
In reply to Daniel P. BerrangeTest 048 is designed to verify data preservation during an image resize. The qcow (v1) format impl has never supported resize so always fails.
--- tests/qemu-iotests/048 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
tests/qemu-iotests/048 +1 -1
@@ -46,7 +46,7 @@ _compare() . ./common.filter . ./common.pattern -_supported_fmt raw qcow qcow2 qed luks +_supported_fmt raw qcow2 qed luks _supported_proto file _supported_os Linux
Signature · 1 line
2.9.3
PATCH v5 7/18 iotests: fix 097 when run with qcow
In reply to Daniel P. BerrangeThe previous commit:
commit a3e1505daec31ef56f0489f8c8fff1b8e4ca92bd
Author: Eric Blake <[email protected]>
Date: Mon Dec 5 09:49:34 2016 -0600
qcow2: Don't strand clusters near 2G intervals during commit
extended the 097 test case so that it did two passes, once with an internal snapshot, once without.
qcow (v1) does not support internal snapshots, so this change broke test 097 when run against qcow.
This splits 097 in two, creating a new 173 that tests the internal snapshot codepath, effectively putting 097 back to its content before the above commit.
--- tests/qemu-iotests/097 | 10 +--- tests/qemu-iotests/097.out | 125 ++------------------------------------------ tests/qemu-iotests/175 | 126 +++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/175.out | 119 ++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/group | 1 + 5 files changed, 251 insertions(+), 130 deletions(-) create mode 100755 tests/qemu-iotests/175 create mode 100644 tests/qemu-iotests/175.out
tests/qemu-iotests/097 +1 -9
@@ -56,26 +56,19 @@ _supported_os Linux # 3: Two-layer backing chain, commit to lower backing file # (in this case, the top image will implicitly stay unchanged) # -# Each pass is run twice, since qcow2 has different code paths for cleaning -# an image depending on whether it has a snapshot. -# # 020 already tests committing, so this only tests whether image chains are # working properly and that all images above the base are emptied; therefore, # no complicated patterns are necessary. Check near the 2G mark, as qcow2 # has been buggy at that boundary in the past. for i in 0 1 2 3; do -for j in 0 1; do echo -echo "=== Test pass $i.$j ===" +echo "=== Test pass $i ===" echo TEST_IMG="$TEST_IMG.base" _make_test_img 2100M TEST_IMG="$TEST_IMG.itmd" _make_test_img -b "$TEST_IMG.base" 2100M _make_test_img -b "$TEST_IMG.itmd" 2100M -if [ $j -eq 0 ]; then - $QEMU_IMG snapshot -c snap "$TEST_IMG" -fi $QEMU_IO -c 'write -P 1 0x7ffd0000 192k' "$TEST_IMG.base" | _filter_qemu_io $QEMU_IO -c 'write -P 2 0x7ffe0000 128k' "$TEST_IMG.itmd" | _filter_qemu_io
@@ -121,7 +114,6 @@ $QEMU_IMG map "$TEST_IMG.itmd" | _filter_qemu_img_map $QEMU_IMG map "$TEST_IMG" | _filter_qemu_img_map done -done # success, all done
tests/qemu-iotests/097.out +4 -121
148 lines not on this page · raw message
tests/qemu-iotests/175 new file +126 -0
126 lines not on this page · raw message
tests/qemu-iotests/175.out new file +119 -0
119 lines not on this page · raw message
tests/qemu-iotests/group +1 -0
@@ -167,3 +167,4 @@ 172 auto 173 rw auto 174 auto +175 rw auto backing
Signature · 1 line
2.9.3
Re: [Qemu-devel] [PATCH v5 07/18] iotests: fix 097 when run with qcow
Quoted 20 lines · Daniel P. Berrange
The previous commit:
commit
a3e1505daec31ef56f0489f8c8fff1b8e4ca92bd
Author: Eric Blake <[email protected]>
Date: Mon Dec 5 09:49:34 2016 -0600qcow2: Don't strand clusters near 2G intervals during commit
extended the 097 test case so that it did two passes, once with an internal snapshot, once without.
qcow (v1) does not support internal snapshots, so this change broke test 097 when run against qcow.
This splits 097 in two, creating a new 173 that tests the internal snapshot codepath, effectively putting 097 back to its content before the above commit.
Reviewed-by Max Reitz <[email protected]>Signed-off-by Daniel P. Berrange <[email protected]>
Quoted 9 lines
--- tests/qemu-iotests/097 | 10 +--- tests/qemu-iotests/097.out | 125 ++------------------------------------------ tests/qemu-iotests/175 | 126 +++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/175.out | 119 ++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/group | 1 + 5 files changed, 251 insertions(+), 130 deletions(-) create mode 100755 tests/qemu-iotests/175 create mode 100644 tests/qemu-iotests/175.out
tests/qemu-iotests/group line 170
Quoted 6 lines
+++ b/tests/qemu-iotests/group
tests/qemu-iotests/group +1
-0@@ -167,3 +167,4 @@ 172 auto 173 rw auto 174 auto +175 rw auto backing
I used 175 in v5 of my blkdebug series, but Kevin had comments that are worth me respinning, so I'm rebasing mine to use 176.
https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg03049.html
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH v5 07/18] iotests: fix 097 when run with qcow
Quoted 9 lines · Eric Blake
Quoted 8 lines · Daniel P. Berrange
The previous commit:
commit
a3e1505daec31ef56f0489f8c8fff1b8e4ca92bd
Author: Eric Blake <[email protected]>
Date: Mon Dec 5 09:49:34 2016 -0600qcow2: Don't strand clusters near 2G intervals during commit
Quoted 7 lines
Quoted 7 lines
extended the 097 test case so that it did two passes, once with an internal snapshot, once without.
qcow (v1) does not support internal snapshots, so this change broke test 097 when run against qcow.
This splits 097 in two, creating a new 173 that tests the
173 is wrong, and 175 is now taken.
Quoted 10 lines
Quoted 10 lines
internal snapshot codepath, effectively putting 097 back to its content before the above commit.
Reviewed-by Max Reitz <[email protected]>Signed-off-by Daniel P. Berrange <[email protected]>--- tests/qemu-iotests/097 | 10 +--- tests/qemu-iotests/097.out | 125 ++------------------------------------------ tests/qemu-iotests/175 | 126 +++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/175.out | 119 ++++++++++++++++++++++++++++++++++++++++++
I'll be incorporating this patch into my series on blkdebug improvements, as I have a further enhancement (to both 97 and what is now 176) to cover an image that is not cluster-aligned. You may want to rebase the rest of your series (which adds two more tests) on top of that posting.
Quoted 3 lines
I used 175 in v5 of my blkdebug series, but Kevin had comments that are worth me respinning, so I'm rebasing mine to use 176.
https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg03049.html
Particularly since my blkdebug series will now be using 177.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH v5 07/18] iotests: fix 097 when run with qcow
Quoted 37 lines · Eric Blake
Quoted 9 lines · Eric Blake
Quoted 8 lines · Daniel P. Berrange
The previous commit:
commit
a3e1505daec31ef56f0489f8c8fff1b8e4ca92bd
Author: Eric Blake <[email protected]>
Date: Mon Dec 5 09:49:34 2016 -0600qcow2: Don't strand clusters near 2G intervals during commit
Quoted 7 lines
Quoted 7 lines
extended the 097 test case so that it did two passes, once with an internal snapshot, once without.
qcow (v1) does not support internal snapshots, so this change broke test 097 when run against qcow.
This splits 097 in two, creating a new 173 that tests the
173 is wrong, and 175 is now taken.
Quoted 10 lines
Quoted 10 lines
internal snapshot codepath, effectively putting 097 back to its content before the above commit.
Reviewed-by Max Reitz <[email protected]>Signed-off-by Daniel P. Berrange <[email protected]>--- tests/qemu-iotests/097 | 10 +--- tests/qemu-iotests/097.out | 125 ++------------------------------------------ tests/qemu-iotests/175 | 126 +++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/175.out | 119 ++++++++++++++++++++++++++++++++++++++++++I'll be incorporating this patch into my series on blkdebug improvements, as I have a further enhancement (to both 97 and what is now 176) to cover an image that is not cluster-aligned. You may want to rebase the rest of your series (which adds two more tests) on top of that posting.
Quoted 5 lines
Quoted 3 lines
I used 175 in v5 of my blkdebug series, but Kevin had comments that are worth me respinning, so I'm rebasing mine to use 176.
https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg03049.htmlParticularly since my blkdebug series will now be using 177.
/me can't help thinking that we should being giving tests names instead of numbers so we avoid the frequent clashes between people's series :-)
Regards,
Daniel
Signature · 3 lines
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
PATCH v5 8/18 qcow: make encrypt_sectors encrypt in place
In reply to Daniel P. BerrangeInstead of requiring separate input/output buffers for encrypting data, change encrypt_sectors() to assume use of a single buffer, encrypting in place. One current caller uses the same buffer for input/output already and the other two callers are easily converted to do so.
--- block/qcow.c | 44 +++++++++++++++----------------------------- 1 file changed, 15 insertions(+), 29 deletions(-)
block/qcow.c +15 -29
@@ -316,11 +316,10 @@ static int qcow_set_key(BlockDriverState *bs, const char *key) }/* The crypt function is compatible with the linux cryptoloop -algorithm for < 4 GB images. NOTE: out_buf == in_buf is -supported */ +algorithm for < 4 GB images. */ static int encrypt_sectors (BDRVQcowState *s,int64_t sector_num, -uint8_t *out_buf,const uint8_t *in_buf, -int nb_sectors,bool enc,Error **errp) +uint8_t *buf,int nb_sectors,bool enc, +Error **errp) {union {uint64_t ll[2 ];
@@ -339,14 +338,12 @@ static int encrypt_sectors(BDRVQcowState *s, int64_t sector_num, }if (enc) { ret =qcrypto_cipher_encrypt (s->cipher, - in_buf, - out_buf, + buf, buf,512 , errp); }else { ret =qcrypto_cipher_decrypt (s->cipher, - in_buf, - out_buf, + buf, buf,512 , errp); }
@@ -354,8 +351,7 @@ static int encrypt_sectors(BDRVQcowState *s, int64_t sector_num,return -1 ; } sector_num++; - in_buf +=512 ; - out_buf +=512 ; + buf +=512 ; }return 0 ; }
@@ -475,13 +471,12 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,uint64_t start_sect;assert (s->cipher); start_sect = (offset & ~(s->cluster_size -1 )) >>9 ; -memset (s->cluster_data +512 ,0x00 ,512 );for (i =0 ; i < s->cluster_sectors; i++) {if (i < n_start || i >= n_end) {Error *err =NULL ; +memset (s->cluster_data,0x00 ,512 );if (encrypt_sectors (s, start_sect + i, - s->cluster_data, - s->cluster_data +512 ,1 , + s->cluster_data,1 , true, &err) <0 ) {error_free (err); errno =EIO ;
@@ -659,7 +654,7 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num, }if (bs->encrypted) {assert (s->cipher); -if (encrypt_sectors (s, sector_num, buf, buf, +if (encrypt_sectors (s, sector_num, buf, n, false, &err) <0 ) { goto fail; }
@@ -694,9 +689,7 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,BDRVQcowState *s = bs->opaque;int index_in_cluster;uint64_t cluster_offset; -const uint8_t *src_buf;int ret =0 , n; -uint8_t *cluster_data =NULL ;struct iovec hd_iov;QEMUIOVector hd_qiov;uint8_t *buf;
@@ -704,7 +697,9 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num, s->cluster_cache_offset =-1 ;/* disable compressed cache */ -if (qiov->niov >1 ) { +/* We must always copy the iov when encrypting, so we +* don't modify the original data buffer during encryption */ +if (bs->encrypted || qiov->niov >1 ) { buf = orig_buf =qemu_try_blockalign (bs, qiov->size);if (buf ==NULL ) {return -ENOMEM ;
@@ -734,21 +729,15 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,if (bs->encrypted) {Error *err =NULL ;assert (s->cipher); -if (!cluster_data) { - cluster_data =g_malloc0 (s->cluster_size); - } -if (encrypt_sectors (s, sector_num, cluster_data, buf, +if (encrypt_sectors (s, sector_num, buf, n, true, &err) <0 ) {error_free (err); ret = -EIO ;break ; } - src_buf = cluster_data; - }else { - src_buf = buf; } - hd_iov.iov_base = (void *)src_buf; + hd_iov.iov_base = (void *)buf; hd_iov.iov_len = n *512 ;qemu_iovec_init_external (&hd_qiov, &hd_iov,1 );qemu_co_mutex_unlock (&s->lock);
@@ -767,10 +756,7 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num, }qemu_co_mutex_unlock (&s->lock); -if (qiov->niov >1 ) { -qemu_vfree (orig_buf); - } -g_free (cluster_data); +qemu_vfree (orig_buf);return ret; }
Signature · 1 line
2.9.3
Re: [Qemu-devel] [PATCH v5 08/18] qcow: make encrypt_sectors encrypt in place
Am 21.02.2017 um 12:55 hat Daniel P. Berrange geschrieben:
Quoted 12 lines
Instead of requiring separate input/output buffers for encrypting data, change
encrypt_sectors()to assume use of a single buffer, encrypting in place. One current caller uses the same buffer for input/output already and the other two callers are easily converted to do so.Reviewed-by Alberto Garcia <[email protected]>Reviewed-by Eric Blake <[email protected]>Reviewed-by Max Reitz <[email protected]>Signed-off-by Daniel P. Berrange <[email protected]>
block/qcow.c static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num, · line 733
Quoted 9 lines
block/qcow.c +1
-4@@ -734,21 +729,15 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,if (bs->encrypted) {Error *err =NULL ;assert (s->cipher); -if (!cluster_data) { - cluster_data =g_malloc0 (s->cluster_size); - } -if (encrypt_sectors (s, sector_num, cluster_data, buf, +if (encrypt_sectors (s, sector_num, buf, n, true, &err) <0 ) {
This fits in a single line now.
Other than that:
PATCH v5 9/18 qcow: convert QCow to use QCryptoBlock for encryption
In reply to Daniel P. BerrangeThis converts the qcow driver to make use of the QCryptoBlock APIs for encrypting image content. This is only wired up to permit use of the legacy QCow encryption format. Users who wish to have the strong LUKS format should switch to qcow2 instead.
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.qcow,aes-key-secret=sec0--- block/crypto.c | 10 +++ block/crypto.h | 9 +++ block/qcow.c | 184 +++++++++++++++++++++++---------------------------- qapi/block-core.json | 17 ++++- 4 files changed, 117 insertions(+), 103 deletions(-)
block/crypto.c +10 -0
@@ -251,6 +251,11 @@ block_crypto_open_opts_init(QCryptoBlockFormat format, v, &ret->u.luks, &local_err);break ; +case Q_CRYPTO_BLOCK_FORMAT_QCOW : +visit_type_QCryptoBlockOptionsQCow_members ( + v, &ret->u.qcow, &local_err); +break ; +default :error_setg (&local_err,"Unsupported block format %d" , format);break ;
@@ -308,6 +313,11 @@ block_crypto_create_opts_init(QCryptoBlockFormat format, v, &ret->u.luks, &local_err);break ; +case Q_CRYPTO_BLOCK_FORMAT_QCOW : +visit_type_QCryptoBlockOptionsQCow_members ( + v, &ret->u.qcow, &local_err); +break ; +default :error_setg (&local_err,"Unsupported block format %d" , format);break ;
block/crypto.h +9 -0
@@ -21,6 +21,15 @@#ifndef BLOCK_CRYPTO_H__ #define BLOCK_CRYPTO_H__ +#define BLOCK_CRYPTO_OPT_QCOW_KEY_SECRET "key-secret" + +#define BLOCK_CRYPTO_OPT_DEF_QCOW_KEY_SECRET (prefix) \ + { \ + .name = prefix BLOCK_CRYPTO_OPT_QCOW_KEY_SECRET, \ + .type = QEMU_OPT_STRING, \ + .help = "ID of the secret that provides the AES encryption key", \ + } +#define BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET "key-secret"#define BLOCK_CRYPTO_OPT_LUKS_CIPHER_ALG "cipher-alg"#define BLOCK_CRYPTO_OPT_LUKS_CIPHER_MODE "cipher-mode"
block/qcow.c +82 -102
309 lines not on this page · raw message
qapi/block-core.json +16 -1
@@ -2278,6 +2278,21 @@ 'mode': 'Qcow2OverlapCheckMode' } } ## +# @BlockdevOptionsQcow: +# +# Driver specific block device options for qcow. +# +# @aes-key-secret: #optional ID of the "secret" object providing the +# AES decryption key. +# +# Since: 2.9 +## +{ 'struct': 'BlockdevOptionsQcow', + 'base': 'BlockdevOptionsGenericCOWFormat', + 'data': { '*aes-key-secret': 'str' } } + + +## # @BlockdevOptionsQcow2: # # Driver specific block device options for qcow2.
@@ -2794,7 +2809,7 @@ 'null-co': 'BlockdevOptionsNull', 'parallels': 'BlockdevOptionsGenericFormat', 'qcow2': 'BlockdevOptionsQcow2', - 'qcow': 'BlockdevOptionsGenericCOWFormat', + 'qcow': 'BlockdevOptionsQcow', 'qed': 'BlockdevOptionsGenericCOWFormat', 'quorum': 'BlockdevOptionsQuorum', 'raw': 'BlockdevOptionsRaw',
Signature · 1 line
2.9.3
Re: [Qemu-devel] [PATCH v5 09/18] qcow: convert QCow to use QCryptoBlock for encryption
Quoted 8 lines · Daniel P. Berrange
block/qcow.c +4
-0@@ -175,8 +185,31 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, ret = -ENOSYS ; goto fail; } +if (s->crypt_method_header ==QCOW_CRYPT_AES ) { + crypto_opts =block_crypto_open_opts_init ( +Q_CRYPTO_BLOCK_FORMAT_QCOW , opts,"aes-" , &local_err); +if (local_err) {
block/qcow.c static int qcow_open(BlockDriverState *bs, QDict *options, int flags, · line 201
Quoted 10 lines
block/qcow.c +9
-0+error_propagate (errp, local_err); + ret = -EINVAL ; + goto fail; + } +if (flags &BDRV_O_NO_IO ) { + cflags |=QCRYPTO_BLOCK_OPEN_NO_IO ; + } + s->crypto =qcrypto_block_open (crypto_opts,NULL ,NULL , + cflags, errp);
You don't call qcrypto_block_free() if qcow_open() eventually fails. Although qcow_close() takes care of that, a failure to open the image sets bs->drv = NULL in bdrv_open_common(), preventing qcow_close() from being called.
block/qcow.c static int qcow_open(BlockDriverState *bs, QDict *options, int flags, · line 301
Quoted 10 lines
block/qcow.c +2
-0@@ -260,14 +293,17 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, goto fail; } +qapi_free_QCryptoBlockOpenOptions (crypto_opts);qemu_co_mutex_init (&s->lock);return 0 ; fail: +qemu_opts_del (opts);
You need to delete opts as well if this function succeeds, don't you?
Berto
Re: [Qemu-devel] [PATCH v5 09/18] qcow: convert QCow to use QCryptoBlock for encryption
Quoted 14 lines · Alberto Garcia
Quoted 13 lines · Daniel P. Berrange
block/qcow.c +8
-0@@ -175,8 +185,31 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, ret = -ENOSYS ; goto fail; } +if (s->crypt_method_header ==QCOW_CRYPT_AES ) { + crypto_opts =block_crypto_open_opts_init ( +Q_CRYPTO_BLOCK_FORMAT_QCOW , opts,"aes-" , &local_err); +if (local_err) { +error_propagate (errp, local_err); + ret = -EINVAL ; + goto fail; + }
Quoted 10 lines
block/qcow.c static int qcow_open(BlockDriverState *bs, QDict *options, int flags, · line 201
Quoted 5 lines
block/qcow.c +5
-0+if (flags &BDRV_O_NO_IO ) { + cflags |=QCRYPTO_BLOCK_OPEN_NO_IO ; + } + s->crypto =qcrypto_block_open (crypto_opts,NULL ,NULL , + cflags, errp);You don't call
qcrypto_block_free()ifqcow_open()eventually fails. Althoughqcow_close()takes care of that, a failure to open the image setsbs->drv= NULL inbdrv_open_common(), preventingqcow_close()from being called.
Yes, I'll fix that leak.
Quoted 5 lines
Quoted 5 lines
block/qcow.c +0
-0@@ -260,14 +293,17 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, goto fail; }
Quoted 8 lines
block/qcow.c static int qcow_open(BlockDriverState *bs, QDict *options, int flags, · line 301
Quoted 6 lines
block/qcow.c +2
-0+qapi_free_QCryptoBlockOpenOptions (crypto_opts);qemu_co_mutex_init (&s->lock);return 0 ; fail: +qemu_opts_del (opts);You need to delete opts as well if this function succeeds, don't you?
New version will avoid the use of QemuOpts entirely - using the QDict APIs instead, but I'll have equiv cleanup todo.
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
PATCH v5 10/18 qcow2: make qcow2_encrypt_sectors encrypt in place
In reply to Daniel P. BerrangeInstead of requiring separate input/output buffers for encrypting data, change qcow2_encrypt_sectors() to assume use of a single buffer, encrypting in place. The current callers all used the same buffer for input/output already.
--- block/qcow2-cluster.c | 17 ++++++----------- block/qcow2.c | 4 ++-- block/qcow2.h | 3 +-- 3 files changed, 9 insertions(+), 15 deletions(-)
block/qcow2-cluster.c +6 -11
@@ -346,11 +346,9 @@ static int count_contiguous_clusters_by_type(int nb_clusters, }/* The crypt function is compatible with the linux cryptoloop -algorithm for < 4 GB images. NOTE: out_buf == in_buf is -supported */ +algorithm for < 4 GB images. */ int qcow2_encrypt_sectors (BDRVQcow2State *s,int64_t sector_num, -uint8_t *out_buf,const uint8_t *in_buf, -int nb_sectors,bool enc, +uint8_t *buf,int nb_sectors,bool enc,Error **errp) {union {
@@ -370,14 +368,12 @@ int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t sector_num, }if (enc) { ret =qcrypto_cipher_encrypt (s->cipher, - in_buf, - out_buf, + buf, buf,512 , errp); }else { ret =qcrypto_cipher_decrypt (s->cipher, - in_buf, - out_buf, + buf, buf,512 , errp); }
@@ -385,8 +381,7 @@ int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t sector_num,return -1 ; } sector_num++; - in_buf +=512 ; - out_buf +=512 ; + buf +=512 ; }return 0 ; }
@@ -434,7 +429,7 @@ static int coroutine_fn do_perform_cow(BlockDriverState *bs,assert (s->cipher);assert ((offset_in_cluster & ~BDRV_SECTOR_MASK ) ==0 );assert ((bytes & ~BDRV_SECTOR_MASK ) ==0 ); -if (qcow2_encrypt_sectors (s, sector, iov.iov_base, iov.iov_base, +if (qcow2_encrypt_sectors (s, sector, iov.iov_base, bytes >>BDRV_SECTOR_BITS , true, &err) <0 ) { ret = -EIO ;error_free (err);
block/qcow2.c +2 -2
@@ -1530,7 +1530,7 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,assert ((cur_bytes & (BDRV_SECTOR_SIZE -1 )) ==0 );Error *err =NULL ;if (qcow2_encrypt_sectors (s, offset >>BDRV_SECTOR_BITS , - cluster_data, cluster_data, + cluster_data, cur_bytes >>BDRV_SECTOR_BITS , false, &err) <0 ) {error_free (err);
@@ -1626,7 +1626,7 @@ static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,qemu_iovec_to_buf (&hd_qiov,0 , cluster_data, hd_qiov.size);if (qcow2_encrypt_sectors (s, offset >>BDRV_SECTOR_BITS , - cluster_data, cluster_data, + cluster_data, cur_bytes >>BDRV_SECTOR_BITS , true, &err) <0 ) {error_free (err);
block/qcow2.h +1 -2
@@ -531,8 +531,7 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,int qcow2_write_l1_entry (BlockDriverState *bs,int l1_index);int qcow2_decompress_cluster (BlockDriverState *bs,uint64_t cluster_offset);int qcow2_encrypt_sectors (BDRVQcow2State *s,int64_t sector_num, -uint8_t *out_buf,const uint8_t *in_buf, -int nb_sectors,bool enc,Error **errp); +uint8_t *buf,int nb_sectors,bool enc,Error **errp);int qcow2_get_cluster_offset (BlockDriverState *bs,uint64_t offset,unsigned int *bytes,uint64_t *cluster_offset);
Signature · 1 line
2.9.3
PATCH v5 11/18 qcow2: convert QCow2 to use QCryptoBlock for encryption
In reply to Daniel P. BerrangeThis converts the qcow2 driver to make use of the QCryptoBlock APIs for encrypting image content, using the legacyy QCow2 AES scheme.
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,aes-key-secret=sec0The test 087 could be simplified since there is no longer a difference in behaviour when using blockdev_add with encrypted images for the running vs stopped CPU state.
--- block/qcow2-cluster.c | 47 +---------- block/qcow2.c | 191 +++++++++++++++++++++++++++++---------------- block/qcow2.h | 5 +- qapi/block-core.json | 8 +- tests/qemu-iotests/049 | 2 +- tests/qemu-iotests/049.out | 4 +- tests/qemu-iotests/082.out | 27 +++++++ tests/qemu-iotests/087 | 27 +++---- tests/qemu-iotests/087.out | 12 +-- tests/qemu-iotests/134 | 18 +++-- tests/qemu-iotests/134.out | 10 +-- tests/qemu-iotests/158 | 19 +++-- tests/qemu-iotests/158.out | 14 +--- tests/qemu-iotests/common | 10 ++- 14 files changed, 213 insertions(+), 181 deletions(-)
block/qcow2-cluster.c +3 -44
@@ -345,47 +345,6 @@ static int count_contiguous_clusters_by_type(int nb_clusters,return i; } -/* The crypt function is compatible with the linux cryptoloop -algorithm for < 4 GB images. */ -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 do_perform_cow(BlockDriverState *bs, uint64_t src_cluster_offset, uint64_t cluster_offset,
@@ -426,11 +385,11 @@ static int coroutine_fn do_perform_cow(BlockDriverState *bs,Error *err =NULL ;int64_t sector = (src_cluster_offset + offset_in_cluster) >>BDRV_SECTOR_BITS ; -assert (s->cipher);assert ((offset_in_cluster & ~BDRV_SECTOR_MASK ) ==0 );assert ((bytes & ~BDRV_SECTOR_MASK ) ==0 ); -if (qcow2_encrypt_sectors (s, sector, iov.iov_base, - bytes >>BDRV_SECTOR_BITS , true, &err) <0 ) { +assert (s->crypto); +if (qcrypto_block_encrypt (s->crypto, sector, iov.iov_base, + bytes, &err) <0 ) { ret = -EIO ;error_free (err); goto out;
block/qcow2.c +123 -68
362 lines not on this page · raw message
block/qcow2.h +3 -2
@@ -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
@@ -257,7 +257,8 @@ typedef struct BDRVQcow2State {CoMutex lock; -QCryptoCipher *cipher;/* current cipher, NULL if no key yet */ +QCryptoBlockOpenOptions *crypto_opts;/* Disk encryption runtime options */ +QCryptoBlock *crypto;/* Disk encryption format driver */ uint32_t crypt_method_header;uint64_t snapshots_offset;int snapshots_size;
qapi/block-core.json +6 -2
@@ -2326,6 +2326,10 @@ # @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) +# @aes-key-secret: #optional the ID of a QCryptoSecret object providing +# the AES decryption key (since 2.9). Mandatory for +# encrypted images, except when doing a metadata-only +# probe of the image. # # Since: 1.7 ##
@@ -2339,8 +2343,8 @@ '*cache-size': 'int', '*l2-cache-size': 'int', '*refcount-cache-size': 'int', - '*cache-clean-interval': 'int' } } - + '*cache-clean-interval': 'int', + '*aes-key-secret': 'str' } } ## # @BlockdevOptionsArchipelago:
tests/qemu-iotests/049 +1 -1
@@ -106,7 +106,7 @@ test_qemu_img create -f $IMGFMT -o preallocation=1234 "$TEST_IMG" 64M echo "== Check encryption option ==" echo test_qemu_img create -f $IMGFMT -o encryption=off "$TEST_IMG" 64M -test_qemu_img create -f $IMGFMT -o encryption=on "$TEST_IMG" 64M +test_qemu_img create -f $IMGFMT --object secret,id=sec0,data=123456 -o encryption=on,aes-key-secret=sec0 "$TEST_IMG" 64M echo "== Check lazy_refcounts option (only with v3) ==" echo
tests/qemu-iotests/049.out +2 -2
@@ -186,8 +186,8 @@ Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 encryption=off cluster_si qemu-img create -f qcow2 -o encryption=off TEST_DIR/t.qcow2 64M Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16 -qemu-img create -f qcow2 -o encryption=on TEST_DIR/t.qcow2 64M -Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 encryption=on cluster_size=65536 lazy_refcounts=off refcount_bits=16 +qemu-img create -f qcow2 --object secret,id=sec0,data=123456 -o encryption=on,aes-key-secret=sec0 TEST_DIR/t.qcow2 64M +Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 encryption=on cluster_size=65536 lazy_refcounts=off refcount_bits=16 aes-key-secret=sec0 == Check lazy_refcounts option (only with v3) ==
tests/qemu-iotests/082.out +27 -0
@@ -53,6 +53,7 @@ cluster_size qcow2 cluster size preallocation Preallocation mode (allowed values: off, metadata, falloc, full) lazy_refcounts Postpone refcount updates refcount_bits Width of a reference count entry in bits +aes-key-secret ID of the secret that provides the AES encryption key nocow Turn off copy-on-write (valid only on btrfs) Testing: create -f qcow2 -o ? TEST_DIR/t.qcow2 128M
@@ -66,6 +67,7 @@ cluster_size qcow2 cluster size preallocation Preallocation mode (allowed values: off, metadata, falloc, full) lazy_refcounts Postpone refcount updates refcount_bits Width of a reference count entry in bits +aes-key-secret ID of the secret that provides the AES encryption key nocow Turn off copy-on-write (valid only on btrfs) Testing: create -f qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2 128M
@@ -79,6 +81,7 @@ cluster_size qcow2 cluster size preallocation Preallocation mode (allowed values: off, metadata, falloc, full) lazy_refcounts Postpone refcount updates refcount_bits Width of a reference count entry in bits +aes-key-secret ID of the secret that provides the AES encryption key nocow Turn off copy-on-write (valid only on btrfs) Testing: create -f qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2 128M
@@ -92,6 +95,7 @@ cluster_size qcow2 cluster size preallocation Preallocation mode (allowed values: off, metadata, falloc, full) lazy_refcounts Postpone refcount updates refcount_bits Width of a reference count entry in bits +aes-key-secret ID of the secret that provides the AES encryption key nocow Turn off copy-on-write (valid only on btrfs) Testing: create -f qcow2 -o help,cluster_size=4k TEST_DIR/t.qcow2 128M
@@ -105,6 +109,7 @@ cluster_size qcow2 cluster size preallocation Preallocation mode (allowed values: off, metadata, falloc, full) lazy_refcounts Postpone refcount updates refcount_bits Width of a reference count entry in bits +aes-key-secret ID of the secret that provides the AES encryption key nocow Turn off copy-on-write (valid only on btrfs) Testing: create -f qcow2 -o ?,cluster_size=4k TEST_DIR/t.qcow2 128M
@@ -118,6 +123,7 @@ cluster_size qcow2 cluster size preallocation Preallocation mode (allowed values: off, metadata, falloc, full) lazy_refcounts Postpone refcount updates refcount_bits Width of a reference count entry in bits +aes-key-secret ID of the secret that provides the AES encryption key nocow Turn off copy-on-write (valid only on btrfs) Testing: create -f qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2 128M
@@ -131,6 +137,7 @@ cluster_size qcow2 cluster size preallocation Preallocation mode (allowed values: off, metadata, falloc, full) lazy_refcounts Postpone refcount updates refcount_bits Width of a reference count entry in bits +aes-key-secret ID of the secret that provides the AES encryption key nocow Turn off copy-on-write (valid only on btrfs) Testing: create -f qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2 128M
@@ -144,6 +151,7 @@ cluster_size qcow2 cluster size preallocation Preallocation mode (allowed values: off, metadata, falloc, full) lazy_refcounts Postpone refcount updates refcount_bits Width of a reference count entry in bits +aes-key-secret ID of the secret that provides the AES encryption key nocow Turn off copy-on-write (valid only on btrfs) Testing: create -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2 128M
@@ -172,6 +180,7 @@ cluster_size qcow2 cluster size preallocation Preallocation mode (allowed values: off, metadata, falloc, full) lazy_refcounts Postpone refcount updates refcount_bits Width of a reference count entry in bits +aes-key-secret ID of the secret that provides the AES encryption key Testing: create -o help Supported options:
@@ -234,6 +243,7 @@ cluster_size qcow2 cluster size preallocation Preallocation mode (allowed values: off, metadata, falloc, full) lazy_refcounts Postpone refcount updates refcount_bits Width of a reference count entry in bits +aes-key-secret ID of the secret that provides the AES encryption key nocow Turn off copy-on-write (valid only on btrfs) Testing: convert -O qcow2 -o ? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
@@ -247,6 +257,7 @@ cluster_size qcow2 cluster size preallocation Preallocation mode (allowed values: off, metadata, falloc, full) lazy_refcounts Postpone refcount updates refcount_bits Width of a reference count entry in bits +aes-key-secret ID of the secret that provides the AES encryption key nocow Turn off copy-on-write (valid only on btrfs) Testing: convert -O qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
@@ -260,6 +271,7 @@ cluster_size qcow2 cluster size preallocation Preallocation mode (allowed values: off, metadata, falloc, full) lazy_refcounts Postpone refcount updates refcount_bits Width of a reference count entry in bits +aes-key-secret ID of the secret that provides the AES encryption key nocow Turn off copy-on-write (valid only on btrfs) Testing: convert -O qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
@@ -273,6 +285,7 @@ cluster_size qcow2 cluster size preallocation Preallocation mode (allowed values: off, metadata, falloc, full) lazy_refcounts Postpone refcount updates refcount_bits Width of a reference count entry in bits +aes-key-secret ID of the secret that provides the AES encryption key nocow Turn off copy-on-write (valid only on btrfs) Testing: convert -O qcow2 -o help,cluster_size=4k TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
@@ -286,6 +299,7 @@ cluster_size qcow2 cluster size preallocation Preallocation mode (allowed values: off, metadata, falloc, full) lazy_refcounts Postpone refcount updates refcount_bits Width of a reference count entry in bits +aes-key-secret ID of the secret that provides the AES encryption key nocow Turn off copy-on-write (valid only on btrfs) Testing: convert -O qcow2 -o ?,cluster_size=4k TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
@@ -299,6 +313,7 @@ cluster_size qcow2 cluster size preallocation Preallocation mode (allowed values: off, metadata, falloc, full) lazy_refcounts Postpone refcount updates refcount_bits Width of a reference count entry in bits +aes-key-secret ID of the secret that provides the AES encryption key nocow Turn off copy-on-write (valid only on btrfs) Testing: convert -O qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
@@ -312,6 +327,7 @@ cluster_size qcow2 cluster size preallocation Preallocation mode (allowed values: off, metadata, falloc, full) lazy_refcounts Postpone refcount updates refcount_bits Width of a reference count entry in bits +aes-key-secret ID of the secret that provides the AES encryption key nocow Turn off copy-on-write (valid only on btrfs) Testing: convert -O qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
@@ -325,6 +341,7 @@ cluster_size qcow2 cluster size preallocation Preallocation mode (allowed values: off, metadata, falloc, full) lazy_refcounts Postpone refcount updates refcount_bits Width of a reference count entry in bits +aes-key-secret ID of the secret that provides the AES encryption key nocow Turn off copy-on-write (valid only on btrfs) Testing: convert -O qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
@@ -353,6 +370,7 @@ cluster_size qcow2 cluster size preallocation Preallocation mode (allowed values: off, metadata, falloc, full) lazy_refcounts Postpone refcount updates refcount_bits Width of a reference count entry in bits +aes-key-secret ID of the secret that provides the AES encryption key Testing: convert -o help Supported options:
@@ -412,6 +430,7 @@ cluster_size qcow2 cluster size preallocation Preallocation mode (allowed values: off, metadata, falloc, full) lazy_refcounts Postpone refcount updates refcount_bits Width of a reference count entry in bits +aes-key-secret ID of the secret that provides the AES encryption key nocow Turn off copy-on-write (valid only on btrfs) Testing: amend -f qcow2 -o ? TEST_DIR/t.qcow2
@@ -425,6 +444,7 @@ cluster_size qcow2 cluster size preallocation Preallocation mode (allowed values: off, metadata, falloc, full) lazy_refcounts Postpone refcount updates refcount_bits Width of a reference count entry in bits +aes-key-secret ID of the secret that provides the AES encryption key nocow Turn off copy-on-write (valid only on btrfs) Testing: amend -f qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2
@@ -438,6 +458,7 @@ cluster_size qcow2 cluster size preallocation Preallocation mode (allowed values: off, metadata, falloc, full) lazy_refcounts Postpone refcount updates refcount_bits Width of a reference count entry in bits +aes-key-secret ID of the secret that provides the AES encryption key nocow Turn off copy-on-write (valid only on btrfs) Testing: amend -f qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2
@@ -451,6 +472,7 @@ cluster_size qcow2 cluster size preallocation Preallocation mode (allowed values: off, metadata, falloc, full) lazy_refcounts Postpone refcount updates refcount_bits Width of a reference count entry in bits +aes-key-secret ID of the secret that provides the AES encryption key nocow Turn off copy-on-write (valid only on btrfs) Testing: amend -f qcow2 -o help,cluster_size=4k TEST_DIR/t.qcow2
@@ -464,6 +486,7 @@ cluster_size qcow2 cluster size preallocation Preallocation mode (allowed values: off, metadata, falloc, full) lazy_refcounts Postpone refcount updates refcount_bits Width of a reference count entry in bits +aes-key-secret ID of the secret that provides the AES encryption key nocow Turn off copy-on-write (valid only on btrfs) Testing: amend -f qcow2 -o ?,cluster_size=4k TEST_DIR/t.qcow2
@@ -477,6 +500,7 @@ cluster_size qcow2 cluster size preallocation Preallocation mode (allowed values: off, metadata, falloc, full) lazy_refcounts Postpone refcount updates refcount_bits Width of a reference count entry in bits +aes-key-secret ID of the secret that provides the AES encryption key nocow Turn off copy-on-write (valid only on btrfs) Testing: amend -f qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2
@@ -490,6 +514,7 @@ cluster_size qcow2 cluster size preallocation Preallocation mode (allowed values: off, metadata, falloc, full) lazy_refcounts Postpone refcount updates refcount_bits Width of a reference count entry in bits +aes-key-secret ID of the secret that provides the AES encryption key nocow Turn off copy-on-write (valid only on btrfs) Testing: amend -f qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2
@@ -503,6 +528,7 @@ cluster_size qcow2 cluster size preallocation Preallocation mode (allowed values: off, metadata, falloc, full) lazy_refcounts Postpone refcount updates refcount_bits Width of a reference count entry in bits +aes-key-secret ID of the secret that provides the AES encryption key nocow Turn off copy-on-write (valid only on btrfs) Testing: amend -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2
@@ -533,6 +559,7 @@ cluster_size qcow2 cluster size preallocation Preallocation mode (allowed values: off, metadata, falloc, full) lazy_refcounts Postpone refcount updates refcount_bits Width of a reference count entry in bits +aes-key-secret ID of the secret that provides the AES encryption key Testing: convert -o help Supported options:
tests/qemu-iotests/087 +11 -16
@@ -124,24 +124,18 @@ echo echo === Encrypted image === echo -_make_test_img -o encryption=on $size -run_qemu -S <<EOF +_make_test_img --object secret,id=sec0,data=123456 -o encryption=on,aes-key-secret=sec0 $size +run_qemu <<EOF { "execute": "qmp_capabilities" } -{ "execute": "blockdev-add", +{ "execute": "object-add", "arguments": { - "driver": "$IMGFMT", - "node-name": "disk", - "file": { - "driver": "file", - "filename": "$TEST_IMG" + "qom-type": "secret", + "id": "sec0", + "props": { + "data": "123456" } - } } -{ "execute": "quit" } -EOF - -run_qemu <<EOF -{ "execute": "qmp_capabilities" } +} { "execute": "blockdev-add", "arguments": { "driver": "$IMGFMT",
@@ -149,7 +143,8 @@ run_qemu <<EOF "file": { "driver": "file", "filename": "$TEST_IMG" - } + }, + "aes-key-secret": "sec0" } } { "execute": "quit" }
@@ -159,7 +154,7 @@ echo echo === Missing driver === echo -_make_test_img -o encryption=on $size +_make_test_img --object secret,id=sec0,data=123456 -o encryption=on,aes-key-secret=sec0 $size run_qemu -S <<EOF { "execute": "qmp_capabilities" } { "execute": "blockdev-add",
tests/qemu-iotests/087.out +3 -9
@@ -34,17 +34,11 @@ QMP_VERSION === Encrypted image === -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on -Testing: -S -QMP_VERSION -{"return": {}} -{"error": {"class": "GenericError", "desc": "Use of AES-CBC encrypted IMGFMT images is no longer supported in system emulators"}} -{"return": {}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"} - +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on aes-key-secret=sec0 Testing: QMP_VERSION {"return": {}} +{"return": {}} {"error": {"class": "GenericError", "desc": "Use of AES-CBC encrypted IMGFMT images is no longer supported in system emulators"}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"}
@@ -52,7 +46,7 @@ QMP_VERSION === Missing driver === -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on aes-key-secret=sec0 Testing: -S QMP_VERSION {"return": {}}
tests/qemu-iotests/134 +13 -5
@@ -43,23 +43,31 @@ _supported_os Linux size=128M -IMGOPTS="encryption=on" _make_test_img $size + +SECRET="secret,id=sec0,data=astrochicken" +SECRETALT="secret,id=sec0,data=platypus" + +_make_test_img --object $SECRET -o "encryption=on,aes-key-secret=sec0" $size + +IMGSPEC="driver=$IMGFMT,file.filename=$TEST_IMG,aes-key-secret=sec0" + +QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT echo echo "== reading whole image ==" -echo "astrochicken" | $QEMU_IO -c "read 0 $size" "$TEST_IMG" | _filter_qemu_io | _filter_testdir +$QEMU_IO --object $SECRET -c "read 0 $size" --image-opts $IMGSPEC | _filter_qemu_io | _filter_testdir echo echo "== rewriting whole image ==" -echo "astrochicken" | $QEMU_IO -c "write -P 0xa 0 $size" "$TEST_IMG" | _filter_qemu_io | _filter_testdir +$QEMU_IO --object $SECRET -c "write -P 0xa 0 $size" --image-opts $IMGSPEC | _filter_qemu_io | _filter_testdir echo echo "== verify pattern ==" -echo "astrochicken" | $QEMU_IO -c "read -P 0xa 0 $size" "$TEST_IMG" | _filter_qemu_io | _filter_testdir +$QEMU_IO --object $SECRET -c "read -P 0xa 0 $size" --image-opts $IMGSPEC | _filter_qemu_io | _filter_testdir echo echo "== verify pattern failure with wrong password ==" -echo "platypus" | $QEMU_IO -c "read -P 0xa 0 $size" "$TEST_IMG" | _filter_qemu_io | _filter_testdir +$QEMU_IO --object $SECRETALT -c "read -P 0xa 0 $size" --image-opts $IMGSPEC | _filter_qemu_io | _filter_testdir # success, all done
tests/qemu-iotests/134.out +1 -9
@@ -1,27 +1,19 @@ QA output created by 134 -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on aes-key-secret=sec0 == reading whole image == -Disk image 'TEST_DIR/t.qcow2' is encrypted. -password: read 134217728/134217728 bytes at offset 0 128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) == rewriting whole image == -Disk image 'TEST_DIR/t.qcow2' is encrypted. -password: wrote 134217728/134217728 bytes at offset 0 128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) == verify pattern == -Disk image 'TEST_DIR/t.qcow2' is encrypted. -password: read 134217728/134217728 bytes at offset 0 128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) == verify pattern failure with wrong password == -Disk image 'TEST_DIR/t.qcow2' is encrypted. -password: Pattern verification failed at offset 0, 134217728 bytes read 134217728/134217728 bytes at offset 0 128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
tests/qemu-iotests/158 +12 -7
@@ -44,34 +44,39 @@ _supported_os Linux size=128M TEST_IMG_BASE=$TEST_IMG.base +SECRET="secret,id=sec0,data=astrochicken" TEST_IMG_SAVE=$TEST_IMG TEST_IMG=$TEST_IMG_BASE echo "== create base ==" -IMGOPTS="encryption=on" _make_test_img $size +_make_test_img --object $SECRET -o "encryption=on,aes-key-secret=sec0" $size TEST_IMG=$TEST_IMG_SAVE +IMGSPECBASE="driver=$IMGFMT,file.filename=$TEST_IMG_BASE,aes-key-secret=sec0" +IMGSPEC="driver=$IMGFMT,file.filename=$TEST_IMG,backing.driver=$IMGFMT,backing.file.filename=$TEST_IMG_BASE,backing.aes-key-secret=sec0,aes-key-secret=sec0" +QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT + echo echo "== writing whole image ==" -echo "astrochicken" | $QEMU_IO -c "write -P 0xa 0 $size" "$TEST_IMG_BASE" | _filter_qemu_io | _filter_testdir +$QEMU_IO --object $SECRET -c "write -P 0xa 0 $size" --image-opts $IMGSPECBASE | _filter_qemu_io | _filter_testdir echo echo "== verify pattern ==" -echo "astrochicken" | $QEMU_IO -c "read -P 0xa 0 $size" "$TEST_IMG_BASE" | _filter_qemu_io | _filter_testdir +$QEMU_IO --object $SECRET -c "read -P 0xa 0 $size" --image-opts $IMGSPECBASE | _filter_qemu_io | _filter_testdir echo "== create overlay ==" -IMGOPTS="encryption=on" _make_test_img -b "$TEST_IMG_BASE" $size +_make_test_img --object $SECRET -o "encryption=on,aes-key-secret=sec0" -b "$TEST_IMG_BASE" $size echo echo "== writing part of a cluster ==" -echo "astrochicken" | $QEMU_IO -c "write -P 0xe 0 1024" "$TEST_IMG" | _filter_qemu_io | _filter_testdir +$QEMU_IO --object $SECRET -c "write -P 0xe 0 1024" --image-opts $IMGSPEC | _filter_qemu_io | _filter_testdir echo echo "== verify pattern ==" -echo "astrochicken" | $QEMU_IO -c "read -P 0xe 0 1024" "$TEST_IMG" | _filter_qemu_io | _filter_testdir +$QEMU_IO --object $SECRET -c "read -P 0xe 0 1024" --image-opts $IMGSPEC | _filter_qemu_io | _filter_testdir echo echo "== verify pattern ==" -echo "astrochicken" | $QEMU_IO -c "read -P 0xa 1024 64512" "$TEST_IMG" | _filter_qemu_io | _filter_testdir +$QEMU_IO --object $SECRET -c "read -P 0xa 1024 64512" --image-opts $IMGSPEC | _filter_qemu_io | _filter_testdir # success, all done
tests/qemu-iotests/158.out +2 -12
@@ -1,36 +1,26 @@ QA output created by 158 == create base == -Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=134217728 encryption=on +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=134217728 encryption=on aes-key-secret=sec0 == writing whole image == -Disk image 'TEST_DIR/t.qcow2.base' is encrypted. -password: wrote 134217728/134217728 bytes at offset 0 128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) == verify pattern == -Disk image 'TEST_DIR/t.qcow2.base' is encrypted. -password: read 134217728/134217728 bytes at offset 0 128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) == create overlay == -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/t.IMGFMT.base encryption=on +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/t.IMGFMT.base encryption=on aes-key-secret=sec0 == writing part of a cluster == -Disk image 'TEST_DIR/t.qcow2' is encrypted. -password: wrote 1024/1024 bytes at offset 0 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) == verify pattern == -Disk image 'TEST_DIR/t.qcow2' is encrypted. -password: read 1024/1024 bytes at offset 0 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) == verify pattern == -Disk image 'TEST_DIR/t.qcow2' is encrypted. -password: read 64512/64512 bytes at offset 1024 63 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) *** done
tests/qemu-iotests/common +6 -4
@@ -50,6 +50,7 @@ export IMGPROTO=file export IMGOPTS="" export CACHEMODE="writeback" export QEMU_IO_OPTIONS="" +export QEMU_IO_OPTIONS_NO_FMT="" export CACHEMODE_IS_DEFAULT=true export QEMU_OPTIONS="-nodefaults -machine accel=qtest" export VALGRIND_QEMU=
@@ -408,10 +409,11 @@ BEGIN { for (t='$start'; t<='$end'; t++) printf "%03d\n",t }' \ done # Set qemu-io cache mode with $CACHEMODE we have -if [ "$IMGOPTSSYNTAX" = "true" ]; then - QEMU_IO_OPTIONS="$QEMU_IO_OPTIONS --cache $CACHEMODE" -else - QEMU_IO_OPTIONS="$QEMU_IO_OPTIONS -f $IMGFMT --cache $CACHEMODE" +QEMU_IO_OPTIONS="$QEMU_IO_OPTIONS --cache $CACHEMODE" + +QEMU_IO_OPTIONS_NO_FMT="$QEMU_IO_OPTIONS" +if [ "$IMGOPTSSYNTAX" != "true" ]; then + QEMU_IO_OPTIONS="$QEMU_IO_OPTIONS -f $IMGFMT" fi # Set default options for qemu-img create -o if they were not specified
Signature · 1 line
2.9.3
Re: [Qemu-devel] [PATCH v5 11/18] qcow2: convert QCow2 to use QCryptoBlock for encryption
Quoted 8 lines · Daniel P. Berrange
block/qcow2.c +8
-0+switch (s->crypt_method_header) { +case QCOW_CRYPT_NONE : +break ; + +case QCOW_CRYPT_AES : + r->crypto_opts =block_crypto_open_opts_init ( +Q_CRYPTO_BLOCK_FORMAT_QCOW , opts,"aes-" , errp); +break ;
block/qcow2.c static int qcow2_update_options_prepare(BlockDriverState *bs, · line 776
Quoted 10 lines
block/qcow2.c +10
-0+ +default : +error_setg (errp,"Unsupported encryption method %d" , + s->crypt_method_header); +break ; + } +if (s->crypt_method_header && !r->crypto_opts) { + ret = -EINVAL ; + goto fail; + }
This last condition relies on the assumption that QCOW_CRYPT_NONE == 0.
I think it's safe to assume that its value is never going to change and therefore this isn't too important, but I'm just pointing it out in case you want to make it explicit.
Quoted 11 lines
block/qcow2.c +7
-0@@ -1122,6 +1145,24 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, goto fail; } +if (s->crypt_method_header ==QCOW_CRYPT_AES ) { +unsigned int cflags =0 ; +if (flags &BDRV_O_NO_IO ) { + cflags |=QCRYPTO_BLOCK_OPEN_NO_IO ; + } +/* TODO how do we pass the same crypto opts down to the +* backing file by default, so we don't have to manually
block/qcow2.c static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, · line 1164
Quoted 10 lines
block/qcow2.c +10
-0+* provide the same key-secret property against the full +* backing chain +*/ + s->crypto =qcrypto_block_open (s->crypto_opts,NULL ,NULL , + cflags, errp); +if (!s->crypto) { + ret = -EINVAL ; + goto fail; + } + }
Actually this has the same problem that I mentioned for patch 9: if qcow2_open() fails then s->crypto is leaked.
Berto
Re: [Qemu-devel] [PATCH v5 11/18] qcow2: convert QCow2 to use QCryptoBlock for encryption
Quoted 22 lines · Alberto Garcia
Quoted 8 lines · Daniel P. Berrange
block/qcow2.c +8
-0+switch (s->crypt_method_header) { +case QCOW_CRYPT_NONE : +break ; + +case QCOW_CRYPT_AES : + r->crypto_opts =block_crypto_open_opts_init ( +Q_CRYPTO_BLOCK_FORMAT_QCOW , opts,"aes-" , errp); +break ;block/qcow2.c static int qcow2_update_options_prepare(BlockDriverState *bs, · line 776
Quoted 10 lines
block/qcow2.c +10
-0+ +default : +error_setg (errp,"Unsupported encryption method %d" , + s->crypt_method_header); +break ; + } +if (s->crypt_method_header && !r->crypto_opts) { + ret = -EINVAL ; + goto fail; + }This last condition relies on the assumption that
QCOW_CRYPT_NONE== 0.
Quoted 3 lines
I think it's safe to assume that its value is never going to change and therefore this isn't too important, but I'm just pointing it out in case you want to make it explicit.
Yeah, I'll make it explicit to be kinder to future reviewers :-)
Quoted 17 lines
Quoted 17 lines
block/qcow2.c +12
-0@@ -1122,6 +1145,24 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, goto fail; } +if (s->crypt_method_header ==QCOW_CRYPT_AES ) { +unsigned int cflags =0 ; +if (flags &BDRV_O_NO_IO ) { + cflags |=QCRYPTO_BLOCK_OPEN_NO_IO ; + } +/* TODO how do we pass the same crypto opts down to the +* backing file by default, so we don't have to manually +* provide the same key-secret property against the full +* backing chain +*/ + s->crypto =qcrypto_block_open (s->crypto_opts,NULL ,NULL , + cflags, errp);
Quoted 8 lines
block/qcow2.c static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, · line 1164
Quoted 5 lines
block/qcow2.c +5
-0+if (!s->crypto) { + ret = -EINVAL ; + goto fail; + } + }Actually this has the same problem that I mentioned for patch 9: if
qcow2_open()fails thens->cryptois leaked.
Yep, and the crypto_opts actually
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
PATCH v5 12/18 qcow2: extend specification to cover LUKS encryption
In reply to Daniel P. BerrangeUpdate the qcow2 specification to describe how the LUKS header is placed inside a qcow2 file, when using LUKS encryption for the qcow2 payload instead of the legacy AES-CBC encryption
--- docs/specs/qcow2.txt | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 96 insertions(+)
docs/specs/qcow2.txt +96 -0
@@ -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
@@ -135,6 +136,7 @@ be stored. Each extension has a structure like the following: 0xE2792ACA - Backing file format name 0x6803f857 - Feature name table 0x23852875 - Bitmaps extension + 0x0537be77 - Full disk encryption header pointer other - Unknown header extension, can be safely ignored
@@ -207,6 +209,100 @@ The fields of the bitmaps extension are: Offset into the image file at which the bitmap directory starts. Must be aligned to a cluster boundary. +== Full disk encryption header pointer == + +The full disk encryption header must be present if, and only if, the +'crypt_method' header requires metadata. Currently this is only true +of the 'LUKS' crypt method. The header extension must be absent for +other methods. + +This header 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 in bytes. Must be aligned to a cluster + boundary. + Byte 8 - 15: Length of the written encryption header in bytes. + Note actual space allocated in the qcow2 file may + be larger than this value, since it will be rounded + to the nearest multiple of the cluster size. Any + unused bytes in the allocated space will be initialized + to 0. + +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 will be +calculated as normal for the LUKS spec. ie the size of the LUKS +header, plus key material regions, plus padding, relative to the +start of the LUKS header. Its value is never used in the context +of qcow2, however, since the qcow2 file format itself defines where +the real payload offset is. + +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 | + . . + . . + . . + | | + +-----------------------------+ + +== Data encryption == + +When an encryption method is requested in the header, the image payload +data must be encrypted/decrypted on every write/read. The image headers +and metadata is never encrypted. + +The algorithms used for encryption vary depending on the method + + - AES: + + The AES cipher, in CBC mode, with 256 bit keys. + + Initialization vectors generated using plain64 method, with + the virtual disk sector as the input tweak. + + - LUKS: + + The algorithms are specified in the LUKS header. + + Initialization vectors generated using the method specified + in the LUKS header, with the physical disk sector as the + input tweak. == Host cluster management ==
Signature · 1 line
2.9.3
Re: [Qemu-devel] [PATCH v5 12/18] qcow2: extend specification to cover LUKS encryption
Quoted 6 lines · Daniel P. Berrange
Update the qcow2 specification to describe how the LUKS header is placed inside a qcow2 file, when using LUKS encryption for the qcow2 payload instead of the legacy AES-CBC encryption
Reviewed-by Max Reitz <[email protected]>Signed-off-by Daniel P. Berrange <[email protected]>
Berto
PATCH v5 13/18 qcow2: add support for LUKS encryption format
In reply to Daniel P. BerrangeThis adds support for using LUKS as an encryption format with the qcow2 file. The use of the existing 'encryption=on' parameter is replaced by a new parameter 'encryption-format' which takes the values 'aes' or 'luks'. e.g.
# qemu-img create --object secret,data=123456,id=sec0 \
-f qcow2 -o encryption-format=luks,luks-key-secret=sec0 \
test.qcow2 10Gresults in the creation of an image using the LUKS format. Use of the legacy 'encryption=on' parameter still results in creation of the old qcow2 AES format, and is equivalent to the new 'encryption-format=aes'. e.g. the following are equivalent:
# qemu-img create --object secret,data=123456,id=sec0 \
-f qcow2 -o encryption=on,aes-key-secret=sec0 \
test.qcow2 10G # qemu-img create --object secret,data=123456,id=sec0 \
-f qcow2 -o encryption-format=aes,aes-key-secret=sec0 \
test.qcow2 10GWith the LUKS format it is necessary to store the LUKS partition header and key material in the QCow2 file. This data can be many MB in size, so cannot go into the QCow2 header region directly. Thus the spec defines a FDE (Full Disk Encryption) header extension that specifies the offset of a set of clusters to hold the FDE headers, as well as the length of that region. The LUKS header is thus stored in these extra allocated clusters before the main image payload.
Aside from all the cryptographic differences implied by use of the LUKS format, there is one further key difference between the use of legacy AES and LUKS encryption in qcow2. For LUKS, the initialiazation vectors are generated using the host physical sector as the input, rather than the guest virtual sector. This guarantees unique initialization vectors for all sectors when qcow2 internal snapshots are used, thus giving stronger protection against watermarking attacks.
--- block/qcow2-cluster.c | 4 +- block/qcow2-refcount.c | 10 ++ block/qcow2.c | 289 +++++++++++++++++++++++++++++++++++++++------ block/qcow2.h | 9 ++ include/block/block_int.h | 1 + qapi/block-core.json | 9 +- qemu-img.c | 4 +- tests/qemu-iotests/082.out | 270 +++++++++++++++++++++++++++++++++++++----- 8 files changed, 530 insertions(+), 66 deletions(-)
block/qcow2-cluster.c +3 -1
@@ -383,7 +383,9 @@ static int coroutine_fn do_perform_cow(BlockDriverState *bs,if (bs->encrypted) {Error *err =NULL ; -int64_t sector = (src_cluster_offset + offset_in_cluster) +int64_t sector = (s->crypt_physical_offset ? + (cluster_offset + offset_in_cluster) : + (src_cluster_offset + offset_in_cluster)) >>BDRV_SECTOR_BITS ;assert ((offset_in_cluster & ~BDRV_SECTOR_MASK ) ==0 );assert ((bytes & ~BDRV_SECTOR_MASK ) ==0 );
block/qcow2-refcount.c +10 -0
@@ -1859,6 +1859,16 @@ static int calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res,return ret; } +/* encryption */ +if (s->crypto_header.length) { + ret =inc_refcounts (bs, res, refcount_table, nb_clusters, + s->crypto_header.offset, + s->crypto_header.length); +if (ret <0 ) { +return ret; + } + } +return check_refblocks (bs, res, fix, rebuild, refcount_table, nb_clusters); }
block/qcow2.c +254 -35
435 lines not on this page · raw message
block/qcow2.h +9 -0
@@ -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
@@ -163,6 +164,11 @@ typedef struct QCowSnapshot {struct Qcow2Cache ;typedef struct Qcow2Cache Qcow2Cache ; +typedef struct Qcow2CryptoHeaderExtension { +uint64_t offset; +uint64_t length; +} QEMU_PACKED Qcow2CryptoHeaderExtension; + typedef struct Qcow2UnknownHeaderExtension { uint32_t magic; uint32_t len;
@@ -257,8 +263,11 @@ typedef struct BDRVQcow2State {CoMutex lock; +Qcow2CryptoHeaderExtension crypto_header;/* QCow2 header extension */ QCryptoBlockOpenOptions *crypto_opts;/* Disk encryption runtime options */ QCryptoBlock *crypto;/* Disk encryption format driver */ +bool crypt_physical_offset;/* Whether to use virtual or physical offset +for encryption initialization vector tweak */ uint32_t crypt_method_header;uint64_t snapshots_offset;int snapshots_size;
include/block/block_int.h +1 -0
@@ -41,6 +41,7 @@#define BLOCK_OPT_SIZE "size"#define BLOCK_OPT_ENCRYPT "encryption" +#define BLOCK_OPT_ENCRYPTION_FORMAT "encryption-format"#define BLOCK_OPT_COMPAT6 "compat6"#define BLOCK_OPT_HWVERSION "hwversion"#define BLOCK_OPT_BACKING_FILE "backing_file"
qapi/block-core.json +7 -2
@@ -2327,9 +2327,13 @@ # caches. The interval is in seconds. The default value # is 0 and it disables this feature (since 2.5) # @aes-key-secret: #optional the ID of a QCryptoSecret object providing -# the AES decryption key (since 2.9). Mandatory for +# the AES decryption key (since 2.9). Mandatory for AES # encrypted images, except when doing a metadata-only # probe of the image. +# @luks-key-secret: #optional the ID of a QCryptoSecret object providing +# the LUKS keyslot passphrase (since 2.9). Mandatory for +# LUKS encrypted images, except when doing a metadata- +# only probe of the image. # # Since: 1.7 ##
@@ -2344,7 +2348,8 @@ '*l2-cache-size': 'int', '*refcount-cache-size': 'int', '*cache-clean-interval': 'int', - '*aes-key-secret': 'str' } } + '*aes-key-secret': 'str', + '*luks-key-secret': 'str' } } ## # @BlockdevOptionsArchipelago:
qemu-img.c +3 -1
@@ -2048,6 +2048,8 @@ static int img_convert(int argc, char **argv)if (compress) {bool encryption =qemu_opt_get_bool (opts,BLOCK_OPT_ENCRYPT , false); +const char *encryption_format = +qemu_opt_get (opts,BLOCK_OPT_ENCRYPTION_FORMAT );const char *preallocation =qemu_opt_get (opts,BLOCK_OPT_PREALLOC );
@@ -2057,7 +2059,7 @@ static int img_convert(int argc, char **argv) goto out; } -if (encryption) { +if (encryption || encryption_format) {error_report ("Compression and encryption not supported at " "the same time" ); ret =-1 ;
tests/qemu-iotests/082.out +243 -27
567 lines not on this page · raw message
Signature · 1 line
2.9.3
Re: [Qemu-devel] [PATCH v5 13/18] qcow2: add support for LUKS encryption format
Quoted 20 lines · Daniel P. Berrange
static int qcow2_set_up_encryption(BlockDriverState *bs, QemuOpts *opts,
block/qcow2.c +10
-3- Error **errp) + const char *fmtstr, Error **errp) { BDRVQcow2State *s = bs->opaque;QCryptoBlockCreateOptions *cryptoopts =NULL ;QCryptoBlock *crypto =NULL ;int ret = -EINVAL ; +int fmt =qcow2_crypt_method_from_format (fmtstr); - cryptoopts =block_crypto_create_opts_init ( -Q_CRYPTO_BLOCK_FORMAT_QCOW , opts,"aes-" , errp); +switch (fmt) { +case QCOW_CRYPT_LUKS : + cryptoopts =block_crypto_create_opts_init ( +Q_CRYPTO_BLOCK_FORMAT_LUKS , opts,"luks-" , errp); +break ; +case QCOW_CRYPT_AES : + cryptoopts =block_crypto_create_opts_init ( +Q_CRYPTO_BLOCK_FORMAT_QCOW , opts,"aes-" , errp);
block/qcow2.c static int qcow2_change_backing_file(BlockDriverState *bs, · line 2236
Quoted 10 lines
block/qcow2.c +6
-0+break ; +default : +error_setg (errp,"Unknown encryption format %s" , fmtstr); +break ; + } + s->crypt_method_header = fmt;if (!cryptoopts) { ret = -EINVAL ; goto out; }
I don't believe it has any practical effect in this case, but I think it's cleaner to set s->crypt_method_header = fmt after checking that everything went well. Otherwise an incorrect format string will result in s->crypt_method_header = -EINVAL, which doesn't make sense (it's not even the correct type: the variable is unsigned).
Other than that, the patch looks good to me.
Berto
Re: [Qemu-devel] [PATCH v5 13/18] qcow2: add support for LUKS encryption format
Quoted 32 lines · Alberto Garcia
Quoted 20 lines · Daniel P. Berrange
static int qcow2_set_up_encryption(BlockDriverState *bs, QemuOpts *opts,
block/qcow2.c +10
-3- Error **errp) + const char *fmtstr, Error **errp) { BDRVQcow2State *s = bs->opaque;QCryptoBlockCreateOptions *cryptoopts =NULL ;QCryptoBlock *crypto =NULL ;int ret = -EINVAL ; +int fmt =qcow2_crypt_method_from_format (fmtstr); - cryptoopts =block_crypto_create_opts_init ( -Q_CRYPTO_BLOCK_FORMAT_QCOW , opts,"aes-" , errp); +switch (fmt) { +case QCOW_CRYPT_LUKS : + cryptoopts =block_crypto_create_opts_init ( +Q_CRYPTO_BLOCK_FORMAT_LUKS , opts,"luks-" , errp); +break ; +case QCOW_CRYPT_AES : + cryptoopts =block_crypto_create_opts_init ( +Q_CRYPTO_BLOCK_FORMAT_QCOW , opts,"aes-" , errp);block/qcow2.c static int qcow2_change_backing_file(BlockDriverState *bs, · line 2236
Quoted 10 lines
block/qcow2.c +6
-0+break ; +default : +error_setg (errp,"Unknown encryption format %s" , fmtstr); +break ; + } + s->crypt_method_header = fmt;if (!cryptoopts) { ret = -EINVAL ; goto out; }
Quoted 5 lines
I don't believe it has any practical effect in this case, but I think it's cleaner to set
s->crypt_method_header= fmt after checking that everything went well. Otherwise an incorrect format string will result ins->crypt_method_header=-EINVAL, which doesn't make sense (it's not even the correct type: the variable is unsigned).
Yes, makes sense.
Quoted 3 lines
Other than that, the patch looks good to me.
Reviewed-by Alberto Garcia <[email protected]>
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
PATCH v5 14/18 qcow2: add iotests to cover LUKS encryption support
In reply to Daniel P. BerrangeThis extends the 087 iotest to cover LUKS encryption when doing blockdev-add.
Two further tests are added to validate read/write of LUKS encrypted images with a single file and with a backing file.
--- tests/qemu-iotests/087 | 32 ++++++++++++++++- tests/qemu-iotests/087.out | 14 +++++++- tests/qemu-iotests/176 | 76 ++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/176.out | 18 ++++++++++ tests/qemu-iotests/177 | 86 ++++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/177.out | 26 ++++++++++++++ tests/qemu-iotests/group | 2 ++ 7 files changed, 252 insertions(+), 2 deletions(-) create mode 100755 tests/qemu-iotests/176 create mode 100644 tests/qemu-iotests/176.out create mode 100755 tests/qemu-iotests/177 create mode 100644 tests/qemu-iotests/177.out
tests/qemu-iotests/087 +31 -1
@@ -121,7 +121,7 @@ run_qemu <<EOF EOF echo -echo === Encrypted image === +echo === Encrypted image QCow === echo _make_test_img --object secret,id=sec0,data=123456 -o encryption=on,aes-key-secret=sec0 $size
@@ -151,6 +151,36 @@ run_qemu <<EOF EOF echo +echo === Encrypted image LUKS === +echo + +_make_test_img --object secret,id=sec0,data=123456 -o encryption-format=luks,luks-key-secret=sec0 $size +run_qemu <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "object-add", + "arguments": { + "qom-type": "secret", + "id": "sec0", + "props": { + "data": "123456" + } + } +} +{ "execute": "blockdev-add", + "arguments": { + "driver": "$IMGFMT", + "node-name": "disk", + "file": { + "driver": "file", + "filename": "$TEST_IMG" + }, + "luks-key-secret": "sec0" + } + } +{ "execute": "quit" } +EOF + +echo echo === Missing driver === echo
tests/qemu-iotests/087.out +13 -1
@@ -32,7 +32,7 @@ QMP_VERSION {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"} -=== Encrypted image === +=== Encrypted image QCow === Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on aes-key-secret=sec0 Testing:
@@ -44,6 +44,18 @@ QMP_VERSION {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"} +=== Encrypted image LUKS === + +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption-format=luks luks-key-secret=sec0 +Testing: +QMP_VERSION +{"return": {}} +{"return": {}} +{"return": {}} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"} + + === Missing driver === Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on aes-key-secret=sec0
tests/qemu-iotests/176 new file +76 -0
76 lines not on this page · raw message
tests/qemu-iotests/176.out new file +18 -0
@@ -0,0 +1,18 @@ +QA output created by 176 +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=16777216 encryption-format=luks luks-key-secret=sec0 luks-iter-time=10 + +== reading whole image == +read 16777216/16777216 bytes at offset 0 +16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +== rewriting whole image == +wrote 16777216/16777216 bytes at offset 0 +16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +== verify pattern == +read 16777216/16777216 bytes at offset 0 +16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +== verify open failure with wrong password == +can't open: Invalid password, cannot unlock any keyslot +*** done
tests/qemu-iotests/177 new file +86 -0
86 lines not on this page · raw message
tests/qemu-iotests/177.out new file +26 -0
@@ -0,0 +1,26 @@ +QA output created by 177 +== create base == +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=16777216 encryption-format=luks luks-key-secret=sec0 luks-iter-time=10 + +== writing whole image == +wrote 16777216/16777216 bytes at offset 0 +16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +== verify pattern == +read 16777216/16777216 bytes at offset 0 +16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +== create overlay == +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=16777216 backing_file=TEST_DIR/t.IMGFMT.base encryption-format=luks luks-key-secret=sec1 luks-iter-time=10 + +== writing part of a cluster == +wrote 1024/1024 bytes at offset 0 +1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +== verify pattern == +read 1024/1024 bytes at offset 0 +1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +== verify pattern == +read 64512/64512 bytes at offset 1024 +63 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +*** done
tests/qemu-iotests/group +2 -0
@@ -168,3 +168,5 @@ 173 rw auto 174 auto 175 rw auto backing +176 rw auto quick +177 rw auto quick
Signature · 1 line
2.9.3
PATCH v5 15/18 iotests: enable tests 134 and 158 to work with qcow (v1)
In reply to Daniel P. BerrangeThe 138 and 158 iotests exercise the legacy qcow2 aes encryption code path and they work fine with qcow v1 too.
--- tests/qemu-iotests/134 | 2 +- tests/qemu-iotests/158 | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
tests/qemu-iotests/134 +1 -1
@@ -37,7 +37,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 . ./common.rc . ./common.filter -_supported_fmt qcow2 +_supported_fmt qcow qcow2 _supported_proto generic _supported_os Linux
tests/qemu-iotests/158 +1 -1
@@ -37,7 +37,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 . ./common.rc . ./common.filter -_supported_fmt qcow2 +_supported_fmt qcow qcow2 _supported_proto generic _supported_os Linux
Signature · 1 line
2.9.3
PATCH v5 16/18 block: rip out all traces of password prompting
In reply to Daniel P. BerrangeNow that qcow & qcow2 are wired up to get encryption keys via the QCryptoSecret object, nothing is relying on the interactive prompting for passwords. All the code related to password prompting can thus be ripped out.
--- hmp.c | 31 --------------------- include/monitor/monitor.h | 7 ----- include/qemu/osdep.h | 2 -- monitor.c | 68 ----------------------------------------------- qapi-schema.json | 10 +------ qemu-img.c | 31 --------------------- qemu-io.c | 20 -------------- qmp.c | 12 +-------- util/oslib-posix.c | 66 --------------------------------------------- util/oslib-win32.c | 24 ----------------- 10 files changed, 2 insertions(+), 269 deletions(-)
hmp.c +0 -31
@@ -1071,37 +1071,12 @@ void hmp_ringbuf_read(Monitor *mon, const QDict *qdict)g_free (data); } -static void hmp_cont_cb (void *opaque,int err) -{ -if (!err) { -qmp_cont (NULL ); - } -} - -static bool key_is_missing (const BlockInfo *bdev) -{ -return (bdev->inserted && bdev->inserted->encryption_key_missing); -} -void hmp_cont (Monitor *mon,const QDict *qdict) { -BlockInfoList *bdev_list, *bdev;Error *err =NULL ; - bdev_list =qmp_query_block (NULL ); -for (bdev = bdev_list; bdev; bdev = bdev->next) { -if (key_is_missing (bdev->value)) { -monitor_read_block_device_key (mon, bdev->value->device, - hmp_cont_cb,NULL ); - goto out; - } - } -qmp_cont (&err);hmp_handle_error (mon, &err); - -out: -qapi_free_BlockInfoList (bdev_list); }void hmp_system_wakeup (Monitor *mon,const QDict *qdict)
@@ -1537,12 +1512,6 @@ void hmp_change(Monitor *mon, const QDict *qdict)qmp_blockdev_change_medium (true, device, false,NULL , target, !!arg, arg, !!read_only, read_only_mode, &err); -if (err && -error_get_class (err) ==ERROR_CLASS_DEVICE_ENCRYPTED ) { -error_free (err); -monitor_read_block_device_key (mon, device,NULL ,NULL ); -return ; - } }hmp_handle_error (mon, &err);
include/monitor/monitor.h +0 -7
@@ -22,13 +22,6 @@ void monitor_cleanup(void);int monitor_suspend (Monitor *mon);void monitor_resume (Monitor *mon); -int monitor_read_bdrv_key_start (Monitor *mon,BlockDriverState *bs, -BlockCompletionFunc *completion_cb, -void *opaque); -int monitor_read_block_device_key (Monitor *mon,const char *device, -BlockCompletionFunc *completion_cb, -void *opaque); -int monitor_get_fd (Monitor *mon,const char *fdname,Error **errp);int monitor_fd_param (Monitor *mon,const char *fdname,Error **errp);
include/qemu/osdep.h +0 -2
@@ -403,8 +403,6 @@ void qemu_set_tty_echo(int fd, bool echo);void os_mem_prealloc (int fd,char *area,size_t sz,Error **errp); -int qemu_read_password (char *buf,int buf_size); -/** * qemu_get_pid_name: * @pid: pid of a process
monitor.c +0 -68
74 lines not on this page · raw message
qapi-schema.json +1 -9
@@ -2359,8 +2359,6 @@ # Since: 0.14.0 # # Returns: If successful, nothing -# If QEMU was started with an encrypted block device and a key has -# not yet been set, DeviceEncrypted. # # Notes: This command will succeed if the guest is currently running. It # will also succeed if the guest is in the "inmigrate" state; in
@@ -2641,8 +2639,7 @@ # * This command is stateless, this means that commands that depend # on state information (such as getfd) might not work # -# * Commands that prompt the user for data (eg. 'cont' when the block -# device is encrypted) don't currently work +# * Commands that prompt the user for data don't currently work # # Example: #
@@ -2947,11 +2944,6 @@ # # Returns: Nothing on success. # If @device is not a valid block device, DeviceNotFound -# If the new block device is encrypted, DeviceEncrypted. Note that -# if this error is returned, the device has been opened successfully -# and an additional call to @block_passwd is required to set the -# device's password. The behavior of reads and writes to the block -# device between when these calls are executed is undefined. # # Notes: This interface is deprecated, and it is strongly recommended that you # avoid using it. For changing block devices, use
qemu-img.c +0 -31
@@ -243,29 +243,6 @@ static int print_block_option_help(const char *filename, const char *fmt) } -static int img_open_password (BlockBackend *blk,const char *filename, -int flags,bool quiet) -{ -BlockDriverState *bs; -char password[256 ]; - - bs =blk_bs (blk); -if (bdrv_is_encrypted (bs) &&bdrv_key_required (bs) && - !(flags &BDRV_O_NO_IO )) { -qprintf (quiet,"Disk image '%s' is encrypted.\n" , filename); -if (qemu_read_password (password,sizeof (password)) <0 ) { -error_report ("No password given" ); -return -1 ; - } -if (bdrv_set_key (bs, password) <0 ) { -error_report ("invalid password" ); -return -1 ; - } - } -return 0 ; -} - -static BlockBackend *img_open_opts (const char *optstr,QemuOpts *opts,int flags,bool writethrough,bool quiet)
@@ -281,10 +258,6 @@ static BlockBackend *img_open_opts(const char *optstr, }blk_set_enable_write_cache (blk, !writethrough); -if (img_open_password (blk, optstr, flags, quiet) <0 ) { -blk_unref (blk); -return NULL ; - }return blk; }
@@ -308,10 +281,6 @@ static BlockBackend *img_open_file(const char *filename, }blk_set_enable_write_cache (blk, !writethrough); -if (img_open_password (blk, filename, flags, quiet) <0 ) { -blk_unref (blk); -return NULL ; - }return blk; }
qemu-io.c +0 -20
@@ -56,7 +56,6 @@ static const cmdinfo_t close_cmd = {static int openfile (char *name,int flags,bool writethrough,QDict *opts) {Error *local_err =NULL ; -BlockDriverState *bs;if (qemuio_blk) {error_report ("file open already, try 'help close'" );
@@ -71,28 +70,9 @@ static int openfile(char *name, int flags, bool writethrough, QDict *opts)return 1 ; } - bs =blk_bs (qemuio_blk); -if (bdrv_is_encrypted (bs) &&bdrv_key_required (bs)) { -char password[256 ]; -printf ("Disk image '%s' is encrypted.\n" , name); -if (qemu_read_password (password,sizeof (password)) <0 ) { -error_report ("No password given" ); - goto error; - } -if (bdrv_set_key (bs, password) <0 ) { -error_report ("invalid password" ); - goto error; - } - } -blk_set_enable_write_cache (qemuio_blk, !writethrough);return 0 ; - - error: -blk_unref (qemuio_blk); - qemuio_blk =NULL ; -return 1 ; }static void open_help (void )
qmp.c +1 -11
@@ -164,10 +164,8 @@ SpiceInfo *qmp_query_spice(Error **errp)void qmp_cont (Error **errp) { -Error *local_err =NULL ;BlockBackend *blk; -BlockDriverState *bs; -BdrvNextIterator it; +Error *local_err =NULL ;/* if there is a dump in background, we should wait until the dump * finished */
@@ -187,14 +185,6 @@ void qmp_cont(Error **errp)blk_iostatus_reset (blk); } -for (bs =bdrv_first (&it); bs; bs =bdrv_next (&it)) { -bdrv_add_key (bs,NULL , &local_err); -if (local_err) { -error_propagate (errp, local_err); -return ; - } - } -/* Continuing after completed migration. Images have been inactivated to * allow the destination to take control. Need to get control back now. */ if (runstate_check(RUN_STATE_FINISH_MIGRATE) ||
util/oslib-posix.c +0 -66
72 lines not on this page · raw message
util/oslib-win32.c +0 -24
@@ -553,30 +553,6 @@ void os_mem_prealloc(int fd, char *area, size_t memory, Error **errp) } -/* XXX: put correct support for win32 */ -int qemu_read_password (char *buf,int buf_size) -{ -int c, i; - -printf ("Password: " ); -fflush (stdout); - i =0 ; -for (;;) { - c =getchar (); -if (c <0 ) { - buf[i] ='\0' ; -return -1 ; - }else if (c =='\n' ) { -break ; - }else if (i < (buf_size -1 )) { - buf[i++] = c; - } - } - buf[i] ='\0' ; -return 0 ; -} - -char *qemu_get_pid_name (pid_t pid) {/* XXX Implement me */
Signature · 1 line
2.9.3
Re: [Qemu-devel] [PATCH v5 16/18] block: rip out all traces of password prompting
Quoted 7 lines · Daniel P. Berrange
Now that qcow & qcow2 are wired up to get encryption keys via the QCryptoSecret object, nothing is relying on the interactive prompting for passwords. All the code related to password prompting can thus be ripped out.
Reviewed-by Max Reitz <[email protected]>Signed-off-by Daniel P. Berrange <[email protected]>
Berto
PATCH v5 17/18 block: remove all encryption handling APIs
In reply to Daniel P. BerrangeNow that all encryption keys must be provided upfront via the QCryptoSecret API and associated block driver properties there is no need for any explicit encryption handling APIs in the block layer. Encryption can be handled transparently within the block driver. We only retain an API for querying whether an image is encrypted or not, since that is a potentially useful piece of metadata to report to the user.
--- block.c | 77 +---------------------------------------------- block/crypto.c | 1 - block/qapi.c | 2 +- block/qcow.c | 1 - block/qcow2.c | 1 - blockdev.c | 37 ++--------------------- include/block/block.h | 3 -- include/block/block_int.h | 1 - include/qapi/error.h | 1 - qapi/block-core.json | 3 +- qapi/common.json | 5 +-- 11 files changed, 6 insertions(+), 126 deletions(-)
block.c +1 -76
@@ -1865,15 +1865,7 @@ static BlockDriverState *bdrv_open_inherit(const char *filename, goto close_and_fail; } -if (!bdrv_key_required (bs)) { -bdrv_parent_cb_change_media (bs, true); - }else if (!runstate_check (RUN_STATE_PRELAUNCH ) - && !runstate_check (RUN_STATE_INMIGRATE ) - && !runstate_check (RUN_STATE_PAUSED )) {/* HACK */ -error_setg (errp, -"Guest must be stopped for opening of encrypted image" ); - goto close_and_fail; - } +bdrv_parent_cb_change_media (bs, true);QDECREF (options);
@@ -2354,7 +2346,6 @@ static void bdrv_close(BlockDriverState *bs) bs->backing_format[0 ] ='\0' ; bs->total_sectors =0 ; bs->encrypted = false; - bs->valid_key = false; bs->sg = false;QDECREF (bs->options);QDECREF (bs->explicit_options);
@@ -2723,72 +2714,6 @@ bool bdrv_is_encrypted(BlockDriverState *bs)return bs->encrypted; } -bool bdrv_key_required (BlockDriverState *bs) -{ -BdrvChild *backing = bs->backing; - -if (backing && backing->bs->encrypted && !backing->bs->valid_key) { -return true; - } -return (bs->encrypted && !bs->valid_key); -} - -int bdrv_set_key (BlockDriverState *bs,const char *key) -{ -int ret; -if (bs->backing && bs->backing->bs->encrypted) { - ret =bdrv_set_key (bs->backing->bs, key); -if (ret <0 ) -return ret; -if (!bs->encrypted) -return 0 ; - } -if (!bs->encrypted) { -return -EINVAL ; - }else if (!bs->drv || !bs->drv->bdrv_set_key) { -return -ENOMEDIUM ; - } - ret = bs->drv->bdrv_set_key (bs, key); -if (ret <0 ) { - bs->valid_key = false; - }else if (!bs->valid_key) { -/* call the change callback now, we skipped it on open */ - bs->valid_key = true; -bdrv_parent_cb_change_media (bs, true); - } -return ret; -} - -/* -* Provide an encryption key for @bs. -* If @key is non-null: -* If @bs is not encrypted, fail. -* Else if the key is invalid, fail. -* Else set @bs's key to @key, replacing the existing key, if any. -* If @key is null: -* If @bs is encrypted and still lacks a key, fail. -* Else do nothing. -* On failure, store an error object through @errp if non-null. -*/ -void bdrv_add_key (BlockDriverState *bs,const char *key,Error **errp) -{ -if (key) { -if (!bdrv_is_encrypted (bs)) { -error_setg (errp,"Node '%s' is not encrypted" , -bdrv_get_device_or_node_name (bs)); - }else if (bdrv_set_key (bs, key) <0 ) { -error_setg (errp,QERR_INVALID_PASSWORD ); - } - }else { -if (bdrv_key_required (bs)) { -error_set (errp,ERROR_CLASS_DEVICE_ENCRYPTED , -"'%s' (%s) is encrypted" , -bdrv_get_device_or_node_name (bs), -bdrv_get_encrypted_filename (bs)); - } - } -} -const char *bdrv_get_format_name (BlockDriverState *bs) {return bs->drv ? bs->drv->format_name :NULL ;
block/crypto.c +0 -1
@@ -381,7 +381,6 @@ static int block_crypto_open_generic(QCryptoBlockFormat format, } bs->encrypted = true; - bs->valid_key = true; ret =0 ; cleanup:
block/qapi.c +1 -1
@@ -45,7 +45,7 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk, info->ro = bs->read_only; info->drv =g_strdup (bs->drv->format_name); info->encrypted = bs->encrypted; - info->encryption_key_missing =bdrv_key_required (bs); + info->encryption_key_missing = false; info->cache =g_new (BlockdevCacheInfo,1 ); *info->cache = (BlockdevCacheInfo ) {
block/qcow.c +0 -1
@@ -209,7 +209,6 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, goto fail; } bs->encrypted = true; - bs->valid_key = true; } s->cluster_bits = header.cluster_bits; s->cluster_size =1 << s->cluster_bits;
block/qcow2.c +0 -1
@@ -1148,7 +1148,6 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, } bs->encrypted = true; - bs->valid_key = true; } s->l2_bits = s->cluster_bits -3 ;/* L2 is always one cluster */
blockdev.c +2 -35
@@ -587,10 +587,6 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, bs->detect_zeroes = detect_zeroes; -if (bdrv_key_required (bs)) { - autostart =0 ; - } -block_acct_init (blk_get_stats (blk), account_invalid, account_failed);if (!parse_stats_intervals (blk_get_stats (blk), interval_list, errp)) {
@@ -2244,24 +2240,8 @@ void qmp_block_passwd(bool has_device, const char *device, bool has_node_name, const char *node_name, const char *password, Error **errp) { - Error *local_err = NULL; -BlockDriverState *bs; -AioContext *aio_context; - - bs =bdrv_lookup_bs (has_device ? device :NULL , - has_node_name ? node_name :NULL , - &local_err); -if (local_err) { -error_propagate (errp, local_err); -return ; - } - - aio_context =bdrv_get_aio_context (bs); -aio_context_acquire (aio_context); - -bdrv_add_key (bs, password, errp); - -aio_context_release (aio_context); + error_setg(errp, + "Setting block passwords directly is no longer supported"); }/*
@@ -2556,12 +2536,6 @@ void qmp_blockdev_change_medium(bool has_device, const char *device, goto fail; } -bdrv_add_key (medium_bs,NULL , &err); -if (err) { -error_propagate (errp, err); - goto fail; - } - rc =do_open_tray (has_device ? device :NULL , has_id ? id :NULL , false, &err);
@@ -3832,13 +3806,6 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)QTAILQ_INSERT_TAIL (&monitor_bdrv_states, bs, monitor_list); -if (bs &&bdrv_key_required (bs)) { -QTAILQ_REMOVE (&monitor_bdrv_states, bs, monitor_list); -bdrv_unref (bs); -error_setg (errp,"blockdev-add doesn't support encrypted devices" ); - goto fail; - } - fail:visit_free (v); }
include/block/block.h +0 -3
@@ -413,9 +413,6 @@ BlockDriverState *bdrv_next(BdrvNextIterator *it);BlockDriverState *bdrv_next_monitor_owned (BlockDriverState *bs);bool bdrv_is_encrypted (BlockDriverState *bs); -bool bdrv_key_required (BlockDriverState *bs); -int bdrv_set_key (BlockDriverState *bs,const char *key); -void bdrv_add_key (BlockDriverState *bs,const char *key,Error **errp);void bdrv_iterate_format (void (*it)(void *opaque,const char *name),void *opaque);const char *bdrv_get_node_name (const BlockDriverState *bs);
include/block/block_int.h +0 -1
@@ -436,7 +436,6 @@ struct BlockDriverState {int open_flags;/* flags used to open the file, re-used for re-open */ bool read_only;/* if true, the media is read only */ bool encrypted;/* if true, the media is encrypted */ -bool valid_key;/* if true, a valid encryption key has been set */ bool sg;/* if true, the device is a /dev/sg* */ bool probed;/* if true, format was probed rather than specified */
include/qapi/error.h +0 -1
@@ -125,7 +125,6 @@ typedef enum ErrorClass { ERROR_CLASS_GENERIC_ERROR = QAPI_ERROR_CLASS_GENERICERROR, ERROR_CLASS_COMMAND_NOT_FOUND = QAPI_ERROR_CLASS_COMMANDNOTFOUND, - ERROR_CLASS_DEVICE_ENCRYPTED = QAPI_ERROR_CLASS_DEVICEENCRYPTED, ERROR_CLASS_DEVICE_NOT_ACTIVE = QAPI_ERROR_CLASS_DEVICENOTACTIVE, ERROR_CLASS_DEVICE_NOT_FOUND = QAPI_ERROR_CLASS_DEVICENOTFOUND, ERROR_CLASS_KVM_MISSING_CAP = QAPI_ERROR_CLASS_KVMMISSINGCAP,
qapi/block-core.json +1 -2
@@ -258,8 +258,7 @@ # # @encrypted: true if the backing device is encrypted # -# @encryption_key_missing: true if the backing device is encrypted but an -# valid encryption key is missing +# @encryption_key_missing: Deprecated; always false # # @detect_zeroes: detect and optimize zero writes (Since 2.1) #
qapi/common.json +1 -4
@@ -14,9 +14,6 @@ # # @CommandNotFound: the requested command has not been found # -# @DeviceEncrypted: the requested operation can't be fulfilled because the -# selected device is encrypted -# # @DeviceNotActive: a device has failed to be become active # # @DeviceNotFound: the requested device has not been found
@@ -28,7 +25,7 @@ ## { 'enum': 'QapiErrorClass', # Keep this in sync with ErrorClass in error.h - 'data': [ 'GenericError', 'CommandNotFound', 'DeviceEncrypted', + 'data': [ 'GenericError', 'CommandNotFound', 'DeviceNotActive', 'DeviceNotFound', 'KVMMissingCap' ] } ##
Signature · 1 line
2.9.3
Re: [Qemu-devel] [PATCH v5 17/18] block: remove all encryption handling APIs
Quoted 15 lines · Daniel P. Berrange
blockdev.c +0
-11@@ -2244,24 +2240,8 @@ void qmp_block_passwd(bool has_device, const char *device, bool has_node_name, const char *node_name, const char *password, Error **errp) { - Error *local_err = NULL; -BlockDriverState *bs; -AioContext *aio_context; - - bs =bdrv_lookup_bs (has_device ? device :NULL , - has_node_name ? node_name :NULL , - &local_err); -if (local_err) { -error_propagate (errp, local_err); -return ; - }
blockdev.c void qmp_block_passwd(bool has_device, const char *device, · line 2245
Quoted 10 lines
blockdev.c +2
-7- - aio_context =bdrv_get_aio_context (bs); -aio_context_acquire (aio_context); - -bdrv_add_key (bs, password, errp); - -aio_context_release (aio_context); +error_setg (errp, +"Setting block passwords directly is no longer supported" ); }
Shouldn't you also update the command description in block-core.json and hmp-commands.hx ?
Berto
PATCH v5 18/18 block: pass option prefix down to crypto layer
In reply to Daniel P. BerrangeWhile the crypto layer uses a fixed option name "key-secret", the upper block layer may have a prefix on the options. e.g. "luks-key-secret", "aes-key-secret", in order to avoid clashes between crypto option names & other block option names. To ensure the crypto layer can report accurate error messages, we must tell it what option name prefix was used.
--- block/crypto.c | 4 ++-- block/qcow.c | 7 ++++--- block/qcow2.c | 15 +++++++++------ crypto/block-luks.c | 8 ++++++-- crypto/block-qcow.c | 8 ++++++-- crypto/block.c | 6 ++++-- crypto/blockpriv.h | 2 ++ include/crypto/block.h | 6 +++++- tests/test-crypto-block.c | 8 ++++---- 9 files changed, 42 insertions(+), 22 deletions(-)
block/crypto.c +2 -2
@@ -369,7 +369,7 @@ static int block_crypto_open_generic(QCryptoBlockFormat format,if (flags &BDRV_O_NO_IO ) { cflags |=QCRYPTO_BLOCK_OPEN_NO_IO ; } - crypto->block = qcrypto_block_open(open_opts, + crypto->block = qcrypto_block_open(open_opts, NULL, block_crypto_read_func, bs, cflags,
@@ -409,7 +409,7 @@ static int block_crypto_create_generic(QCryptoBlockFormat format,return -1 ; } - crypto = qcrypto_block_create(create_opts, + crypto = qcrypto_block_create(create_opts, NULL, block_crypto_init_func, block_crypto_write_func, &data,
block/qcow.c +4 -3
@@ -197,8 +197,8 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,if (flags &BDRV_O_NO_IO ) { cflags |=QCRYPTO_BLOCK_OPEN_NO_IO ; } - s->crypto =qcrypto_block_open (crypto_opts,NULL ,NULL , - cflags, errp); + s->crypto =qcrypto_block_open (crypto_opts,"aes-" , +NULL ,NULL , cflags, errp);if (!s->crypto) { ret = -EINVAL ; goto fail;
@@ -825,7 +825,8 @@ static int qcow_create(const char *filename, QemuOpts *opts, Error **errp) goto exit; } - crypto =qcrypto_block_create (crypto_opts,NULL ,NULL ,NULL , errp); + crypto =qcrypto_block_create (crypto_opts,"aes-" , +NULL ,NULL ,NULL , errp);if (!crypto) { ret = -EINVAL ; goto exit;
block/qcow2.c +9 -6
@@ -284,7 +284,7 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,* provide the same key-secret property against the full * backing chain */ - s->crypto =qcrypto_block_open (s->crypto_opts, + s->crypto =qcrypto_block_open (s->crypto_opts,"luks-" , qcow2_crypto_hdr_read_func, bs, cflags, errp);if (!s->crypto) {
@@ -1295,8 +1295,8 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,* provide the same key-secret property against the full * backing chain */ - s->crypto =qcrypto_block_open (s->crypto_opts,NULL ,NULL , - cflags, errp); + s->crypto =qcrypto_block_open (s->crypto_opts,"aes-" , +NULL ,NULL , cflags, errp);if (!s->crypto) { ret = -EINVAL ; goto fail;
@@ -2213,16 +2213,19 @@ static int qcow2_set_up_encryption(BlockDriverState *bs, QemuOpts *opts,QCryptoBlockCreateOptions *cryptoopts =NULL ;QCryptoBlock *crypto =NULL ;int ret = -EINVAL ; +const char *optprefix;int fmt =qcow2_crypt_method_from_format (fmtstr);switch (fmt) {case QCOW_CRYPT_LUKS : + optprefix ="luks-" ; cryptoopts =block_crypto_create_opts_init ( -Q_CRYPTO_BLOCK_FORMAT_LUKS , opts,"luks-" , errp); +Q_CRYPTO_BLOCK_FORMAT_LUKS , opts, optprefix, errp);break ;case QCOW_CRYPT_AES : + optprefix ="aes-" ; cryptoopts =block_crypto_create_opts_init ( -Q_CRYPTO_BLOCK_FORMAT_QCOW , opts,"aes-" , errp); +Q_CRYPTO_BLOCK_FORMAT_QCOW , opts, optprefix, errp);break ;default :error_setg (errp,"Unknown encryption format %s" , fmtstr);
@@ -2234,7 +2237,7 @@ static int qcow2_set_up_encryption(BlockDriverState *bs, QemuOpts *opts, goto out; } - crypto =qcrypto_block_create (cryptoopts, + crypto =qcrypto_block_create (cryptoopts, optprefix, qcow2_crypto_hdr_init_func, qcow2_crypto_hdr_write_func, bs, errp);
crypto/block-luks.c +6 -2
@@ -638,6 +638,7 @@ qcrypto_block_luks_find_key(QCryptoBlock *block, static int qcrypto_block_luks_open(QCryptoBlock *block, QCryptoBlockOpenOptions *options, + const char *optprefix, QCryptoBlockReadFunc readfunc, void *opaque, unsigned int flags,
@@ -661,7 +662,8 @@ qcrypto_block_luks_open(QCryptoBlock *block,if (!(flags &QCRYPTO_BLOCK_OPEN_NO_IO )) {if (!options->u.luks.key_secret) { -error_setg (errp,"Parameter 'key-secret' is required for cipher" ); +error_setg (errp,"Parameter '%skey-secret' is required for cipher" , + optprefix ? optprefix :"" );return -1 ; } password = qcrypto_secret_lookup_as_utf8(
@@ -885,6 +887,7 @@ qcrypto_block_luks_uuid_gen(uint8_t *uuidstr) static int qcrypto_block_luks_create(QCryptoBlock *block, QCryptoBlockCreateOptions *options, + const char *optprefix, QCryptoBlockInitFunc initfunc, QCryptoBlockWriteFunc writefunc, void *opaque,
@@ -937,7 +940,8 @@ qcrypto_block_luks_create(QCryptoBlock *block,* be silently ignored, for compatibility with dm-crypt */ if (!options->u.luks.key_secret) { -error_setg (errp,"Parameter 'key-secret' is required for cipher" ); +error_setg (errp,"Parameter '%skey-secret' is required for cipher" , + optprefix ? optprefix :"" );return -1 ; } password =qcrypto_secret_lookup_as_utf8 (luks_opts.key_secret, errp);
crypto/block-qcow.c +6 -2
@@ -94,6 +94,7 @@ qcrypto_block_qcow_init(QCryptoBlock *block, static int qcrypto_block_qcow_open(QCryptoBlock *block, QCryptoBlockOpenOptions *options, + const char *optprefix, QCryptoBlockReadFunc readfunc G_GNUC_UNUSED, void *opaque G_GNUC_UNUSED, unsigned int flags,
@@ -104,7 +105,8 @@ qcrypto_block_qcow_open(QCryptoBlock *block, }else {if (!options->u.qcow.key_secret) {error_setg (errp, -"Parameter 'key-secret' is required for cipher" ); +"Parameter '%skey-secret' is required for cipher" , + optprefix ? optprefix :"" );return -1 ; } return qcrypto_block_qcow_init(block,
@@ -116,13 +118,15 @@ qcrypto_block_qcow_open(QCryptoBlock *block,static int qcrypto_block_qcow_create (QCryptoBlock *block,QCryptoBlockCreateOptions *options, +const char *optprefix, QCryptoBlockInitFunc initfunc G_GNUC_UNUSED, QCryptoBlockWriteFunc writefunc G_GNUC_UNUSED, void *opaque G_GNUC_UNUSED,Error **errp) {if (!options->u.qcow.key_secret) { -error_setg (errp,"Parameter 'key-secret' is required for cipher" ); +error_setg (errp,"Parameter '%skey-secret' is required for cipher" , + optprefix ? optprefix :"" );return -1 ; }/* QCow2 has no special header, since everything is hardwired */
crypto/block.c +4 -2
@@ -48,6 +48,7 @@ bool qcrypto_block_has_format(QCryptoBlockFormat format, QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions *options, + const char *optprefix, QCryptoBlockReadFunc readfunc, void *opaque, unsigned int flags,
@@ -67,7 +68,7 @@ QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions *options, block->driver = qcrypto_block_drivers[options->format]; -if (block->driver->open (block, options, +if (block->driver->open (block, options, optprefix, readfunc, opaque, flags, errp) <0 ) {g_free (block);return NULL ;
@@ -78,6 +79,7 @@ QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions *options, QCryptoBlock *qcrypto_block_create(QCryptoBlockCreateOptions *options, + const char *optprefix, QCryptoBlockInitFunc initfunc, QCryptoBlockWriteFunc writefunc, void *opaque,
@@ -97,7 +99,7 @@ QCryptoBlock *qcrypto_block_create(QCryptoBlockCreateOptions *options, block->driver = qcrypto_block_drivers[options->format]; -if (block->driver->create (block, options, initfunc, +if (block->driver->create (block, options, optprefix, initfunc, writefunc, opaque, errp) <0 ) {g_free (block);return NULL ;
crypto/blockpriv.h +2 -0
@@ -41,6 +41,7 @@ struct QCryptoBlock {struct QCryptoBlockDriver { int (*open)(QCryptoBlock *block, QCryptoBlockOpenOptions *options, + const char *optprefix, QCryptoBlockReadFunc readfunc, void *opaque, unsigned int flags,
@@ -48,6 +49,7 @@ struct QCryptoBlockDriver { int (*create)(QCryptoBlock *block, QCryptoBlockCreateOptions *options, + const char *optprefix, QCryptoBlockInitFunc initfunc, QCryptoBlockWriteFunc writefunc, void *opaque,
include/crypto/block.h +5 -1
@@ -71,6 +71,7 @@ typedef enum {/** * qcrypto_block_open: * @options: the encryption options +* @optprefix: name prefix for options * @readfunc: callback for reading data from the volume * @opaque: data to pass to @readfunc * @flags: bitmask of QCryptoBlockOpenFlags values
@@ -102,6 +103,7 @@ typedef enum {* Returns: a block encryption format, or NULL on error */ QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions *options, + const char *optprefix, QCryptoBlockReadFunc readfunc, void *opaque, unsigned int flags,
@@ -109,7 +111,8 @@ QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions *options,/** * qcrypto_block_create: -* @format: the encryption format +* @options: the encryption options +* @optprefix: name prefix for options * @initfunc: callback for initializing volume header * @writefunc: callback for writing data to the volume header * @opaque: data to pass to @initfunc and @writefunc
@@ -133,6 +136,7 @@ QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions *options,* Returns: a block encryption format, or NULL on error */ QCryptoBlock *qcrypto_block_create(QCryptoBlockCreateOptions *options, + const char *optprefix, QCryptoBlockInitFunc initfunc, QCryptoBlockWriteFunc writefunc, void *opaque,
tests/test-crypto-block.c +4 -4
@@ -281,7 +281,7 @@ static void test_block(gconstpointer opaque)memset (&header,0 ,sizeof (header));buffer_init (&header,"header" ); - blk = qcrypto_block_create(data->create_opts, + blk = qcrypto_block_create(data->create_opts, NULL, test_block_init_func, test_block_write_func, &header,
@@ -300,7 +300,7 @@ static void test_block(gconstpointer opaque)object_unparent (sec);/* Ensure we can't open without the secret */ - blk = qcrypto_block_open(data->open_opts, + blk = qcrypto_block_open(data->open_opts, NULL, test_block_read_func, &header, 0,
@@ -308,7 +308,7 @@ static void test_block(gconstpointer opaque)g_assert (blk ==NULL );/* Ensure we can't open without the secret, unless NO_IO */ - blk = qcrypto_block_open(data->open_opts, + blk = qcrypto_block_open(data->open_opts, NULL, test_block_read_func, &header, QCRYPTO_BLOCK_OPEN_NO_IO,
@@ -322,7 +322,7 @@ static void test_block(gconstpointer opaque)/* Now open for real with secret */ sec =test_block_secret (); - blk = qcrypto_block_open(data->open_opts, + blk = qcrypto_block_open(data->open_opts, NULL, test_block_read_func, &header, 0,
Signature · 1 line
2.9.3
Re: [Qemu-devel] [PATCH v5 18/18] block: pass option prefix down to crypto layer
Quoted 9 lines · Daniel P. Berrange
While the crypto layer uses a fixed option name "key-secret", the upper block layer may have a prefix on the options. e.g. "luks-key-secret", "aes-key-secret", in order to avoid clashes between crypto option names & other block option names. To ensure the crypto layer can report accurate error messages, we must tell it what option name prefix was used.
Reviewed-by Max Reitz <[email protected]>Signed-off-by Daniel P. Berrange <[email protected]>
Berto