Skip to contentSkip to conversation

Convert QCow[2] to QCryptoBlock & add LUKS support

PATCH Superseded Daniel P. Berrange 4 participants 34 messages May 25, 2017 – May 30, 2017

Subsystems (2)

v7 of 10 Superseded Latest is v10 →

lore
Share & tools
lore
#0 lore raw

v7 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
v4: https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg02293.html
v5: https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg04653.html
v6: https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg04561.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 v7:

- Add encryption info to 'qemu-img info' output
- List new encryption parameters in QEMU manual docs.
- Extend copyright date to include 2017 (Eric)
- Avoid local error object when not needed (Alberto)
- Ensure to set 'ret' to an errno value (Alberto)
- Fix leak of crypto options in qcow (Alberto)
- Use american spelling of 'favor' (Eric)
- Fix encryption format name in qapi (Alberto)
- Fix incorrect option name prefix
- Rename new iotests to avoid clash

Changed in v6:

- Changed QAPI / QemuOpts design to use nested struct/union for all encryption parameters (Eric/Kevin)
- Fix cleanup during error conditions (Alberto)

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 (20):
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
block: deprecate "encryption=on" in favor of "encrypt.format=aes"
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
qcow2: report encryption specific image information
docs: document encryption options for qcow, qcow2 and luks

 block.c                    |  77 +------
 block/crypto.c             |  97 ++++-----
 block/crypto.h             | 101 +++++++++
 block/qapi.c               |   2 +-
 block/qcow.c               | 266 ++++++++++++------------
 block/qcow2-cluster.c      |  56 +----
 block/qcow2-refcount.c     |  10 +
 block/qcow2.c              | 506 +++++++++++++++++++++++++++++++++++++--------
 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-commands.hx            |   2 +
 hmp.c                      |  31 ---
 include/block/block.h      |   3 -
 include/block/block_int.h  |   3 +-
 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       | 127 ++++++++----
 qapi/common.json           |   5 +-
 qemu-doc.texi              | 123 ++++++++++-
 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 | 270 +++++++++++++++++++++---
 tests/qemu-iotests/087     |  39 +++-
 tests/qemu-iotests/087.out |  16 +-
 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/183     |  76 +++++++
 tests/qemu-iotests/183.out |  18 ++
 tests/qemu-iotests/184     |  86 ++++++++
 tests/qemu-iotests/184.out |  26 +++
 tests/qemu-iotests/common  |  10 +-
 tests/qemu-iotests/group   |   2 +
 tests/test-crypto-block.c  |   8 +-
 util/oslib-posix.c         |  66 ------
 util/oslib-win32.c         |  24 ---
 52 files changed, 1621 insertions(+), 858 deletions(-)
 create mode 100644 block/crypto.h
 create mode 100755 tests/qemu-iotests/183
 create mode 100644 tests/qemu-iotests/183.out
 create mode 100755 tests/qemu-iotests/184
 create mode 100644 tests/qemu-iotests/184.out
Signature · 1 line
2.9.3
#1 lore raw

PATCH v7 1/20 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. Some helper methods taking QemuOpts are changed to take QDict to simplify usage in other places.

---
 block/crypto.c | 82 +++++++++++++++++-----------------------------------
 block/crypto.h | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 117 insertions(+), 56 deletions(-)
 create mode 100644 block/crypto.h
block/crypto.c +26 -56
index 10e5ddc..ea40ba4 100644
@@ -24,16 +24,10 @@
 #include "sysemu/block-backend.h"
 #include "crypto/block.h"
 #include "qapi/opts-visitor.h"
+#include "qapi/qobject-input-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 +129,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,49 +144,21 @@ 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,
+                            QDict *opts,
                             Error **errp)
 {
     Visitor *v;
@@ -206,7 +168,7 @@ block_crypto_open_opts_init(QCryptoBlockFormat format,
     ret = g_new0(QCryptoBlockOpenOptions, 1);
     ret->format = format;
 
-    v = opts_visitor_new(opts);
+    v = qobject_input_visitor_new_keyval(QOBJECT(opts));
 
     visit_start_struct(v, NULL, NULL, 0, &local_err);
     if (local_err) {
@@ -240,9 +202,9 @@ block_crypto_open_opts_init(QCryptoBlockFormat format,
 }
 
 
-static QCryptoBlockCreateOptions *
+QCryptoBlockCreateOptions *
 block_crypto_create_opts_init(QCryptoBlockFormat format,
-                              QemuOpts *opts,
+                              QDict *opts,
                               Error **errp)
 {
     Visitor *v;
@@ -252,7 +214,7 @@ block_crypto_create_opts_init(QCryptoBlockFormat format,
     ret = g_new0(QCryptoBlockCreateOptions, 1);
     ret->format = format;
 
-    v = opts_visitor_new(opts);
+    v = qobject_input_visitor_new_keyval(QOBJECT(opts));
 
     visit_start_struct(v, NULL, NULL, 0, &local_err);
     if (local_err) {
@@ -299,6 +261,7 @@ static int block_crypto_open_generic(QCryptoBlockFormat format,
     int ret = -EINVAL;
     QCryptoBlockOpenOptions *open_opts = NULL;
     unsigned int cflags = 0;
+    QDict *cryptoopts = NULL;
 
     bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
                                false, errp);
@@ -313,7 +276,9 @@ static int block_crypto_open_generic(QCryptoBlockFormat format,
         goto cleanup;
     }
 
-    open_opts = block_crypto_open_opts_init(format, opts, errp);
+    cryptoopts = qemu_opts_to_qdict(opts, NULL);
+
+    open_opts = block_crypto_open_opts_init(format, cryptoopts, errp);
     if (!open_opts) {
         goto cleanup;
     }
@@ -337,6 +302,7 @@ static int block_crypto_open_generic(QCryptoBlockFormat format,
 
     ret = 0;
  cleanup:
+    QDECREF(cryptoopts);
     qapi_free_QCryptoBlockOpenOptions(open_opts);
     return ret;
 }
@@ -356,8 +322,11 @@ static int block_crypto_create_generic(QCryptoBlockFormat format,
         .opts = opts,
         .filename = filename,
     };
+    QDict *cryptoopts;
+
+    cryptoopts = qemu_opts_to_qdict(opts, NULL);
 
-    create_opts = block_crypto_create_opts_init(format, opts, errp);
+    create_opts = block_crypto_create_opts_init(format, cryptoopts, errp);
     if (!create_opts) {
         return -1;
     }
@@ -375,6 +344,7 @@ static int block_crypto_create_generic(QCryptoBlockFormat format,
 
     ret = 0;
  cleanup:
+    QDECREF(cryptoopts);
     qcrypto_block_free(crypto);
     blk_unref(data.blk);
     qapi_free_QCryptoBlockCreateOptions(create_opts);
block/crypto.h new file +91 -0

91 lines not on this page · raw message

Signature · 1 line
2.9.3
#2 lore raw

PATCH v7 2/20 block: add ability to set a prefix for opt names

In reply to Daniel P. Berrange

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 need to namespace them to match the nested QAPI schema.

e.g. "encrypt.hash-alg", "encrypt.key-secret"

so that they don't clash with any general qcow options at a later date.

---
 block/crypto.c | 16 ++++++++--------
 block/crypto.h | 40 ++++++++++++++++++++--------------------
 2 files changed, 28 insertions(+), 28 deletions(-)
block/crypto.c +8 -8
index ea40ba4..9df1e5d 100644
@@ -129,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 */ }
     },
 };
@@ -144,13 +144,13 @@ 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 */ }
     },
 };
block/crypto.h +20 -20
index c0e9b54..3430dcd 100644
@@ -29,51 +29,51 @@
 #define BLOCK_CRYPTO_OPT_LUKS_HASH_ALG "hash-alg"
 #define BLOCK_CRYPTO_OPT_LUKS_ITER_TIME "iter-time"
 
-#define BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET                            \
+#define BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET(prefix)                    \
     {                                                                   \
-        .name = BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET,                       \
+        .name = prefix BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET,                \
         .type = QEMU_OPT_STRING,                                        \
         .help = "ID of the secret that provides the keyslot passphrase", \
     }
 
-#define BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_ALG               \
+#define BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_ALG(prefix)       \
     {                                                      \
-        .name = BLOCK_CRYPTO_OPT_LUKS_CIPHER_ALG,          \
+        .name = prefix BLOCK_CRYPTO_OPT_LUKS_CIPHER_ALG,   \
         .type = QEMU_OPT_STRING,                           \
         .help = "Name of encryption cipher algorithm",     \
     }
 
-#define BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_MODE         \
-    {                                                 \
-        .name = BLOCK_CRYPTO_OPT_LUKS_CIPHER_MODE,    \
-        .type = QEMU_OPT_STRING,                      \
-        .help = "Name of encryption cipher mode",     \
+#define BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_MODE(prefix)      \
+    {                                                      \
+        .name = prefix BLOCK_CRYPTO_OPT_LUKS_CIPHER_MODE,  \
+        .type = QEMU_OPT_STRING,                           \
+        .help = "Name of encryption cipher mode",          \
     }
 
-#define BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_ALG           \
-    {                                                 \
-        .name = BLOCK_CRYPTO_OPT_LUKS_IVGEN_ALG,      \
-        .type = QEMU_OPT_STRING,                      \
-        .help = "Name of IV generator algorithm",     \
+#define BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_ALG(prefix)     \
+    {                                                   \
+        .name = prefix BLOCK_CRYPTO_OPT_LUKS_IVGEN_ALG, \
+        .type = QEMU_OPT_STRING,                        \
+        .help = "Name of IV generator algorithm",       \
     }
 
-#define BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_HASH_ALG                \
+#define BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_HASH_ALG(prefix)        \
     {                                                           \
-        .name = BLOCK_CRYPTO_OPT_LUKS_IVGEN_HASH_ALG,           \
+        .name = prefix BLOCK_CRYPTO_OPT_LUKS_IVGEN_HASH_ALG,    \
         .type = QEMU_OPT_STRING,                                \
         .help = "Name of IV generator hash algorithm",          \
     }
 
-#define BLOCK_CRYPTO_OPT_DEF_LUKS_HASH_ALG               \
+#define BLOCK_CRYPTO_OPT_DEF_LUKS_HASH_ALG(prefix)       \
     {                                                    \
-        .name = BLOCK_CRYPTO_OPT_LUKS_HASH_ALG,          \
+        .name = prefix BLOCK_CRYPTO_OPT_LUKS_HASH_ALG,   \
         .type = QEMU_OPT_STRING,                         \
         .help = "Name of encryption hash algorithm",     \
     }
 
-#define BLOCK_CRYPTO_OPT_DEF_LUKS_ITER_TIME                   \
+#define BLOCK_CRYPTO_OPT_DEF_LUKS_ITER_TIME(prefix)           \
     {                                                         \
-        .name = BLOCK_CRYPTO_OPT_LUKS_ITER_TIME,              \
+        .name = prefix BLOCK_CRYPTO_OPT_LUKS_ITER_TIME,       \
         .type = QEMU_OPT_NUMBER,                              \
         .help = "Time to spend in PBKDF in milliseconds",     \
     }
Signature · 1 line
2.9.3
Reviewed-by on v7 2/20
#3 lore raw

Re: [Qemu-devel] [PATCH v7 02/20] block: add ability to set a prefix for opt names

Quoted 9 lines · Daniel P. Berrange

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 need to namespace them to match the nested QAPI schema.

e.g. "encrypt.hash-alg", "encrypt.key-secret"

so that they don't clash with any general qcow options at a later date.

Quoted 7 lines
---
 block/crypto.c | 16 ++++++++--------
 block/crypto.h | 40 ++++++++++++++++++++--------------------
 2 files changed, 28 insertions(+), 28 deletions(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
#4 lore raw

PATCH v7 3/20 qcow: document another weakness of qcow AES encryption

In reply to Daniel P. Berrange

Document 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
index 50a2364..0784e31 100644
@@ -559,16 +559,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
#5 lore raw

PATCH v7 4/20 qcow: require image size to be > 1 for new images

In reply to Daniel P. Berrange

The 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
index 95ab123..6738bc7 100644
@@ -811,6 +811,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
#6 lore raw

PATCH v7 5/20 iotests: skip 042 with qcow which dosn't support zero sized images

In reply to Daniel P. Berrange

Test 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
index 351b283..a53e7cb 100755
@@ -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
#7 lore raw

PATCH v7 6/20 iotests: skip 048 with qcow which doesn't support resize

In reply to Daniel P. Berrange

Test 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
index 203c04f..9ed04a0 100755
@@ -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
#8 lore raw

PATCH v7 7/20 block: deprecate "encryption=on" in favor of "encrypt.format=aes"

In reply to Daniel P. Berrange

Historically the qcow & qcow2 image formats supported a property "encryption=on" to enable their built-in AES encryption. We'll soon be supporting LUKS for qcow2, so need a more general purpose way to enable encryption, with a choice of formats.

This introduces an "encrypt.format" option, which will later be joined by a number of other "encrypt.XXX" options. The use of a "encrypt." prefix instead of "encrypt-" is done to facilitate mapping to a nested QAPI schema at later date.

e.g. the preferred syntax is now

qemu-img create -f qcow2 -o encrypt.format=aes demo.qcow2

---
 block/qcow.c               | 30 ++++++++++++++---
 block/qcow2.c              | 33 +++++++++++++++----
 include/block/block_int.h  |  2 +-
 qemu-img.c                 |  4 ++-
 tests/qemu-iotests/082.out | 81 ++++++++++++++++++++++++++++++----------------
 5 files changed, 110 insertions(+), 40 deletions(-)
block/qcow.c +25 -5
index 6738bc7..42f83b2 100644
@@ -803,10 +803,10 @@ static int qcow_create(const char *filename, QemuOpts *opts, Error **errp)
     uint8_t *tmp;
     int64_t total_size = 0;
     char *backing_file = NULL;
-    int flags = 0;
     Error *local_err = NULL;
     int ret;
     BlockBackend *qcow_blk;
+    const char *encryptfmt = NULL;
 
     /* Read out options */
     total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
@@ -818,8 +818,16 @@ static int qcow_create(const char *filename, QemuOpts *opts, Error **errp)
     }
 
     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;
+    encryptfmt = qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT_FORMAT);
+    if (encryptfmt) {
+        if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
+            error_setg(errp, "Options " BLOCK_OPT_ENCRYPT " and "
+                       BLOCK_OPT_ENCRYPT_FORMAT " are mutually exclusive");
+            ret = -EINVAL;
+            goto cleanup;
+        }
+    } else if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
+        encryptfmt = "aes";
     }
 
     ret = bdrv_create_file(filename, opts, &local_err);
@@ -872,7 +880,13 @@ static int qcow_create(const char *filename, QemuOpts *opts, Error **errp)
     l1_size = (total_size + (1LL << shift) - 1) >> shift;
 
     header.l1_table_offset = cpu_to_be64(header_size);
-    if (flags & BLOCK_FLAG_ENCRYPT) {
+    if (encryptfmt) {
+        if (!g_str_equal(encryptfmt, "aes")) {
+            error_setg(errp, "Unknown encryption format '%s', expected 'aes'",
+                       encryptfmt);
+            ret = -EINVAL;
+            goto exit;
+        }
         header.crypt_method = cpu_to_be32(QCOW_CRYPT_AES);
     } else {
         header.crypt_method = cpu_to_be32(QCOW_CRYPT_NONE);
@@ -1046,9 +1060,15 @@ static QemuOptsList qcow_create_opts = {
         {
             .name = BLOCK_OPT_ENCRYPT,
             .type = QEMU_OPT_BOOL,
-            .help = "Encrypt the image",
+            .help = "Encrypt the image with format 'aes'. (Deprecated "
+                    "in favor of " BLOCK_OPT_ENCRYPT_FORMAT "=aes)",
             .def_value_str = "off"
         },
+        {
+            .name = BLOCK_OPT_ENCRYPT_FORMAT,
+            .type = QEMU_OPT_STRING,
+            .help = "Encrypt the image, format choices: 'aes'",
+        },
         { /* end of list */ }
     }
 };
block/qcow2.c +27 -6
index a8d61f0..b7c3230 100644
@@ -2100,7 +2100,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
                          const char *backing_file, const char *backing_format,
                          int flags, size_t cluster_size, PreallocMode prealloc,
                          QemuOpts *opts, int version, int refcount_order,
-                         Error **errp)
+                         const char *encryptfmt, Error **errp)
 {
     int cluster_bits;
     QDict *options;
@@ -2229,7 +2229,13 @@ static int qcow2_create2(const char *filename, int64_t total_size,
         .header_length              = cpu_to_be32(sizeof(*header)),
     };
 
-    if (flags & BLOCK_FLAG_ENCRYPT) {
+    if (encryptfmt) {
+        if (!g_str_equal(encryptfmt, "aes")) {
+            error_setg(errp, "Unknown encryption format '%s', expected 'aes'",
+                       encryptfmt);
+            ret = -EINVAL;
+            goto out;
+        }
         header->crypt_method = cpu_to_be32(QCOW_CRYPT_AES);
     } else {
         header->crypt_method = cpu_to_be32(QCOW_CRYPT_NONE);
@@ -2358,6 +2364,7 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
     int version = 3;
     uint64_t refcount_bits = 16;
     int refcount_order;
+    const char *encryptfmt = NULL;
     Error *local_err = NULL;
     int ret;
 
@@ -2366,8 +2373,16 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
                     BDRV_SECTOR_SIZE);
     backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
     backing_fmt = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FMT);
-    if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
-        flags |= BLOCK_FLAG_ENCRYPT;
+    encryptfmt = qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT_FORMAT);
+    if (encryptfmt) {
+        if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
+            error_setg(errp, "Options " BLOCK_OPT_ENCRYPT " and "
+                       BLOCK_OPT_ENCRYPT_FORMAT " are mutually exclusive");
+            ret = -EINVAL;
+            goto finish;
+        }
+    } else if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
+        encryptfmt = "aes";
     }
     cluster_size = qemu_opt_get_size_del(opts, BLOCK_OPT_CLUSTER_SIZE,
                                          DEFAULT_CLUSTER_SIZE);
@@ -2433,7 +2448,7 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
 
     ret = qcow2_create2(filename, size, backing_file, backing_fmt, flags,
                         cluster_size, prealloc, opts, version, refcount_order,
-                        &local_err);
+                        encryptfmt, &local_err);
     error_propagate(errp, local_err);
 
 finish:
@@ -3388,10 +3403,16 @@ static QemuOptsList qcow2_create_opts = {
         {
             .name = BLOCK_OPT_ENCRYPT,
             .type = QEMU_OPT_BOOL,
-            .help = "Encrypt the image",
+            .help = "Encrypt the image with format 'aes'. (Deprecated "
+                    "in favor of " BLOCK_OPT_ENCRYPT_FORMAT "=aes)",
             .def_value_str = "off"
         },
         {
+            .name = BLOCK_OPT_ENCRYPT_FORMAT,
+            .type = QEMU_OPT_STRING,
+            .help = "Encrypt the image, format choices: 'aes'",
+        },
+        {
             .name = BLOCK_OPT_CLUSTER_SIZE,
             .type = QEMU_OPT_SIZE,
             .help = "qcow2 cluster size",
include/block/block_int.h +1 -1
index 8d3724c..7c31a83 100644
@@ -36,11 +36,11 @@
 #include "qemu/main-loop.h"
 #include "qemu/throttle.h"
 
-#define BLOCK_FLAG_ENCRYPT          1
 #define BLOCK_FLAG_LAZY_REFCOUNTS   8
 
 #define BLOCK_OPT_SIZE              "size"
 #define BLOCK_OPT_ENCRYPT           "encryption"
+#define BLOCK_OPT_ENCRYPT_FORMAT    "encrypt.format"
 #define BLOCK_OPT_COMPAT6           "compat6"
 #define BLOCK_OPT_HWVERSION         "hwversion"
 #define BLOCK_OPT_BACKING_FILE      "backing_file"
qemu-img.c +3 -1
index b506839..c906685 100644
@@ -2215,6 +2215,8 @@ static int img_convert(int argc, char **argv)
     if (s.compressed) {
         bool encryption =
             qemu_opt_get_bool(opts, BLOCK_OPT_ENCRYPT, false);
+        const char *encryptfmt =
+            qemu_opt_get(opts, BLOCK_OPT_ENCRYPT_FORMAT);
         const char *preallocation =
             qemu_opt_get(opts, BLOCK_OPT_PREALLOC);
 
@@ -2224,7 +2226,7 @@ static int img_convert(int argc, char **argv)
             goto out;
         }
 
-        if (encryption) {
+        if (encryption || encryptfmt) {
             error_report("Compression and encryption not supported at "
                          "the same time");
             ret = -1;
tests/qemu-iotests/082.out +54 -27

243 lines not on this page · raw message

Signature · 1 line
2.9.3
#9 lore raw

PATCH v7 8/20 qcow: make encrypt_sectors encrypt in place

In reply to Daniel P. Berrange

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.

---
 block/qcow.c | 45 +++++++++++++++------------------------------
 1 file changed, 15 insertions(+), 30 deletions(-)
block/qcow.c +15 -30
index 42f83b2..90a934c 100644
@@ -322,11 +322,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];
@@ -345,14 +344,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);
         }
@@ -360,8 +357,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;
 }
@@ -481,13 +477,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;
@@ -665,7 +660,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;
                 }
@@ -700,9 +695,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;
@@ -710,7 +703,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;
@@ -740,21 +735,14 @@ 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,
-                                n, true, &err) < 0) {
+            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);
@@ -773,10 +761,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
#10 lore raw

PATCH v7 9/20 qcow: convert QCow to use QCryptoBlock for encryption

In reply to Daniel P. Berrange

This 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,encrypt.format=qcow,\
           encrypt.key-secret=sec0

Likewise when creating images with the legacy AES-CBC format

  qemu-img create -f qcow \
    -object secret,id=sec0,filename=/home/berrange/encrypted.pw \
    -o encrypt.format=aes,encrypt.key-secret=sec0 \
    /home/berrange/encrypted.qcow
---
 block/crypto.c       |  10 +++
 block/crypto.h       |  20 ++++--
 block/qcow.c         | 196 +++++++++++++++++++++++++--------------------------
 qapi/block-core.json |  38 +++++++++-
 4 files changed, 157 insertions(+), 107 deletions(-)
block/crypto.c +10 -0
index 9df1e5d..da4be74 100644
@@ -181,6 +181,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;
@@ -227,6 +232,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 +15 -5
index 3430dcd..0f985ea 100644
@@ -21,6 +21,19 @@
 #ifndef BLOCK_CRYPTO_H__
 #define BLOCK_CRYPTO_H__
 
+#define BLOCK_CRYPTO_OPT_DEF_KEY_SECRET(prefix, helpstr)                \
+    {                                                                   \
+        .name = prefix BLOCK_CRYPTO_OPT_QCOW_KEY_SECRET,                \
+        .type = QEMU_OPT_STRING,                                        \
+        .help = helpstr,                                                \
+    }
+
+#define BLOCK_CRYPTO_OPT_QCOW_KEY_SECRET "key-secret"
+
+#define BLOCK_CRYPTO_OPT_DEF_QCOW_KEY_SECRET(prefix)                    \
+    BLOCK_CRYPTO_OPT_DEF_KEY_SECRET(prefix,                             \
+        "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"
@@ -30,11 +43,8 @@
 #define BLOCK_CRYPTO_OPT_LUKS_ITER_TIME "iter-time"
 
 #define BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET(prefix)                    \
-    {                                                                   \
-        .name = prefix BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET,                \
-        .type = QEMU_OPT_STRING,                                        \
-        .help = "ID of the secret that provides the keyslot passphrase", \
-    }
+    BLOCK_CRYPTO_OPT_DEF_KEY_SECRET(prefix,                             \
+        "ID of the secret that provides the keyslot passphrase")
 
 #define BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_ALG(prefix)       \
     {                                                      \
block/qcow.c +95 -101

324 lines not on this page · raw message

qapi/block-core.json +37 -1
index ea0b3e8..65ef79c 100644
@@ -2281,6 +2281,42 @@
             'mode':  'Qcow2OverlapCheckMode' } }
 
 ##
+# @BlockdevQcowEncryptionFormat:
+#
+# @aes: AES-CBC with plain64 initialization vectors
+#
+# Since: 2.10
+##
+{ 'enum': 'BlockdevQcowEncryptionFormat',
+  'data': [ 'aes' ] }
+
+##
+# @BlockdevQcowEncryption:
+#
+# Since: 2.10
+##
+{ 'union': 'BlockdevQcowEncryption',
+  'base': { 'format': 'BlockdevQcowEncryptionFormat' },
+  'discriminator': 'format',
+  'data': { 'aes': 'QCryptoBlockOptionsQCow' } }
+
+##
+# @BlockdevOptionsQcow:
+#
+# Driver specific block device options for qcow.
+#
+# @encrypt:               Image decryption options. Mandatory for
+#                         encrypted images, except when doing a metadata-only
+#                         probe of the image.
+#
+# Since: 2.10
+##
+{ 'struct': 'BlockdevOptionsQcow',
+  'base': 'BlockdevOptionsGenericCOWFormat',
+  'data': { '*encrypt': 'BlockdevQcowEncryption' } }
+
+
+##
 # @BlockdevOptionsQcow2:
 #
 # Driver specific block device options for qcow2.
@@ -2975,7 +3011,7 @@
       'null-co':    'BlockdevOptionsNull',
       'parallels':  'BlockdevOptionsGenericFormat',
       'qcow2':      'BlockdevOptionsQcow2',
-      'qcow':       'BlockdevOptionsGenericCOWFormat',
+      'qcow':       'BlockdevOptionsQcow',
       'qed':        'BlockdevOptionsGenericCOWFormat',
       'quorum':     'BlockdevOptionsQuorum',
       'raw':        'BlockdevOptionsRaw',
Signature · 1 line
2.9.3
Reviewed-by on v7 9/20
#11 lore raw

Re: [Qemu-devel] [PATCH v7 09/20] qcow: convert QCow to use QCryptoBlock for encryption

Quoted 14 lines · Daniel P. Berrange
block/qcow.c +7 -0
@@ -105,6 +116,13 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
     int ret;
     QCowHeader header;
     Error *local_err = NULL;
+    QCryptoBlockOpenOptions *crypto_opts = NULL;
+    unsigned int cflags = 0;
+    QDict *encryptopts = NULL;
+    const char *encryptfmt;
+
+    qdict_extract_subqdict(options, &encryptopts, "encrypt.");
+    encryptfmt = qdict_get_try_str(encryptopts, "format");
 
     bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
                                false, errp);
Quoted 3 lines
     if (!bs->file) {
         return -EINVAL;
     }

You're leaking encryptopts if the function returns here.

block/qcow.c static int qcow_create(const char *filename, QemuOpts *opts, Error **errp) · line 860

Quoted 12 lines
block/qcow.c +8 -0
@@ -873,6 +850,20 @@ static int qcow_create(const char *filename, QemuOpts *opts, Error **errp)
             goto exit;
         }
         header.crypt_method = cpu_to_be32(QCOW_CRYPT_AES);
+
+        crypto_opts = block_crypto_create_opts_init(
+            Q_CRYPTO_BLOCK_FORMAT_QCOW, encryptopts, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            ret = -EINVAL;
+            goto exit;
+        }

Not very important, and my fault for not having pointed it out in my previous review, but you can spare the error_propagate() call if you pass errp directly to block_crypto_create_opts_init() and then check if crypto_opts is NULL.

Actually none of the error_propagate() calls in qcow_create() is really necessary, but that could be fixed in a separate patch, if at all (it's not so important).

The leak however needs to be fixed. With that,

Berto

#12 lore raw

Re: [Qemu-devel] [PATCH v7 09/20] qcow: convert QCow to use QCryptoBlock for encryption

Quoted 39 lines · Alberto Garcia
Quoted 14 lines · Daniel P. Berrange
block/qcow.c +7 -0
@@ -105,6 +116,13 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
     int ret;
     QCowHeader header;
     Error *local_err = NULL;
+    QCryptoBlockOpenOptions *crypto_opts = NULL;
+    unsigned int cflags = 0;
+    QDict *encryptopts = NULL;
+    const char *encryptfmt;
+
+    qdict_extract_subqdict(options, &encryptopts, "encrypt.");
+    encryptfmt = qdict_get_try_str(encryptopts, "format");
 
     bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
                                false, errp);
Quoted 3 lines
     if (!bs->file) {
         return -EINVAL;
     }

You're leaking encryptopts if the function returns here.

block/qcow.c static int qcow_create(const char *filename, QemuOpts *opts, Error **errp) · line 860

Quoted 12 lines
block/qcow.c +8 -0
@@ -873,6 +850,20 @@ static int qcow_create(const char *filename, QemuOpts *opts, Error **errp)
             goto exit;
         }
         header.crypt_method = cpu_to_be32(QCOW_CRYPT_AES);
+
+        crypto_opts = block_crypto_create_opts_init(
+            Q_CRYPTO_BLOCK_FORMAT_QCOW, encryptopts, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            ret = -EINVAL;
+            goto exit;
+        }

Not very important, and my fault for not having pointed it out in my previous review, but you can spare the error_propagate() call if you pass errp directly to block_crypto_create_opts_init() and then check if crypto_opts is NULL.

Quoted 3 lines

Actually none of the error_propagate() calls in qcow_create() is really necessary, but that could be fixed in a separate patch, if at all (it's not so important).

Since I have to re-post to fix the leak anyway, I'll eliminate the intermedia local_err usage - I like to avoid that when not needed anyway.

Quoted 4 lines

The leak however needs to be fixed. With that,

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 :|
#13 lore raw

PATCH v7 10/20 qcow2: make qcow2_encrypt_sectors encrypt in place

In reply to Daniel P. Berrange

Instead 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
index 347d94b..438499d 100644
@@ -358,11 +358,9 @@ static int count_contiguous_clusters_unallocated(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 {
@@ -382,14 +380,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);
         }
@@ -397,8 +393,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;
 }
@@ -446,7 +441,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
index b7c3230..d98b6d9 100644
@@ -1543,7 +1543,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);
@@ -1639,7 +1639,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
index 1801dc3..6b3d5de 100644
@@ -538,8 +538,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
#14 lore raw

PATCH v7 11/20 qcow2: convert QCow2 to use QCryptoBlock for encryption

In reply to Daniel P. Berrange

This converts the qcow2 driver to make use of the QCryptoBlock APIs for encrypting image content, using the legacy 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,encrypt.key-secret=sec0

The 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              | 226 ++++++++++++++++++++++++++++++---------------
 block/qcow2.h              |   5 +-
 qapi/block-core.json       |  27 +++++-
 tests/qemu-iotests/049     |   2 +-
 tests/qemu-iotests/049.out |   4 +-
 tests/qemu-iotests/082.out |  27 ++++++
 tests/qemu-iotests/087     |  28 +++---
 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, 263 insertions(+), 186 deletions(-)
block/qcow2-cluster.c +3 -44
index 438499d..4fdff1f 100644
@@ -357,47 +357,6 @@ static int count_contiguous_clusters_unallocated(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,
@@ -438,11 +397,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 +152 -74

414 lines not on this page · raw message

block/qcow2.h +3 -2
index 6b3d5de..3b42b2e 100644
@@ -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 +25 -2
index 65ef79c..c48b2fa 100644
@@ -2316,6 +2316,26 @@
   'data': { '*encrypt': 'BlockdevQcowEncryption' } }
 
 
+
+##
+# @BlockdevQcow2EncryptionFormat:
+# @aes: AES-CBC with plain64 initialization venctors
+#
+# Since: 2.10
+##
+{ 'enum': 'BlockdevQcow2EncryptionFormat',
+  'data': [ 'aes' ] }
+
+##
+# @BlockdevQcow2Encryption:
+#
+# Since: 2.10
+##
+{ 'union': 'BlockdevQcow2Encryption',
+  'base': { 'format': 'BlockdevQcow2EncryptionFormat' },
+  'discriminator': 'format',
+  'data': { 'aes': 'QCryptoBlockOptionsQCow' } }
+
 ##
 # @BlockdevOptionsQcow2:
 #
@@ -2350,6 +2370,9 @@
 # @cache-clean-interval:  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)
+# @encrypt:               Image decryption options. Mandatory for
+#                         encrypted images, except when doing a metadata-only
+#                         probe of the image. (since 2.10)
 #
 # Since: 2.9
 ##
@@ -2363,8 +2386,8 @@
             '*cache-size': 'int',
             '*l2-cache-size': 'int',
             '*refcount-cache-size': 'int',
-            '*cache-clean-interval': 'int' } }
-
+            '*cache-clean-interval': 'int',
+            '*encrypt': 'BlockdevQcow2Encryption' } }
 
 ##
 # @BlockdevOptionsSsh:
tests/qemu-iotests/049 +1 -1
index fff0760..df35b6d 100755
@@ -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,encrypt.key-secret=sec0 "$TEST_IMG" 64M
 
 echo "== Check lazy_refcounts option (only with v3) =="
 echo
tests/qemu-iotests/049.out +2 -2
index 34e66db..2e5204a 100644
@@ -190,8 +190,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,encrypt.key-secret=sec0 TEST_DIR/t.qcow2 64M
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 encryption=on encrypt.key-secret=sec0 cluster_size=65536 lazy_refcounts=off refcount_bits=16
 
 == Check lazy_refcounts option (only with v3) ==
 
tests/qemu-iotests/082.out +27 -0
index 892358c..2fb5af6 100644
@@ -50,6 +50,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
 encrypt.format   Encrypt the image, format choices: 'aes'
+encrypt.key-secret ID of the secret that provides the AES encryption key
 cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
@@ -64,6 +65,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
 encrypt.format   Encrypt the image, format choices: 'aes'
+encrypt.key-secret ID of the secret that provides the AES encryption key
 cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
@@ -78,6 +80,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
 encrypt.format   Encrypt the image, format choices: 'aes'
+encrypt.key-secret ID of the secret that provides the AES encryption key
 cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
@@ -92,6 +95,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
 encrypt.format   Encrypt the image, format choices: 'aes'
+encrypt.key-secret ID of the secret that provides the AES encryption key
 cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
@@ -106,6 +110,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
 encrypt.format   Encrypt the image, format choices: 'aes'
+encrypt.key-secret ID of the secret that provides the AES encryption key
 cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
@@ -120,6 +125,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
 encrypt.format   Encrypt the image, format choices: 'aes'
+encrypt.key-secret ID of the secret that provides the AES encryption key
 cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
@@ -134,6 +140,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
 encrypt.format   Encrypt the image, format choices: 'aes'
+encrypt.key-secret ID of the secret that provides the AES encryption key
 cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
@@ -148,6 +155,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
 encrypt.format   Encrypt the image, format choices: 'aes'
+encrypt.key-secret ID of the secret that provides the AES encryption key
 cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
@@ -177,6 +185,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
 encrypt.format   Encrypt the image, format choices: 'aes'
+encrypt.key-secret ID of the secret that provides the AES encryption key
 cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
@@ -240,6 +249,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
 encrypt.format   Encrypt the image, format choices: 'aes'
+encrypt.key-secret ID of the secret that provides the AES encryption key
 cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
@@ -254,6 +264,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
 encrypt.format   Encrypt the image, format choices: 'aes'
+encrypt.key-secret ID of the secret that provides the AES encryption key
 cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
@@ -268,6 +279,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
 encrypt.format   Encrypt the image, format choices: 'aes'
+encrypt.key-secret ID of the secret that provides the AES encryption key
 cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
@@ -282,6 +294,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
 encrypt.format   Encrypt the image, format choices: 'aes'
+encrypt.key-secret ID of the secret that provides the AES encryption key
 cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
@@ -296,6 +309,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
 encrypt.format   Encrypt the image, format choices: 'aes'
+encrypt.key-secret ID of the secret that provides the AES encryption key
 cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
@@ -310,6 +324,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
 encrypt.format   Encrypt the image, format choices: 'aes'
+encrypt.key-secret ID of the secret that provides the AES encryption key
 cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
@@ -324,6 +339,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
 encrypt.format   Encrypt the image, format choices: 'aes'
+encrypt.key-secret ID of the secret that provides the AES encryption key
 cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
@@ -338,6 +354,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
 encrypt.format   Encrypt the image, format choices: 'aes'
+encrypt.key-secret ID of the secret that provides the AES encryption key
 cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
@@ -367,6 +384,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
 encrypt.format   Encrypt the image, format choices: 'aes'
+encrypt.key-secret ID of the secret that provides the AES encryption key
 cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
@@ -427,6 +445,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
 encrypt.format   Encrypt the image, format choices: 'aes'
+encrypt.key-secret ID of the secret that provides the AES encryption key
 cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
@@ -441,6 +460,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
 encrypt.format   Encrypt the image, format choices: 'aes'
+encrypt.key-secret ID of the secret that provides the AES encryption key
 cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
@@ -455,6 +475,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
 encrypt.format   Encrypt the image, format choices: 'aes'
+encrypt.key-secret ID of the secret that provides the AES encryption key
 cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
@@ -469,6 +490,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
 encrypt.format   Encrypt the image, format choices: 'aes'
+encrypt.key-secret ID of the secret that provides the AES encryption key
 cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
@@ -483,6 +505,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
 encrypt.format   Encrypt the image, format choices: 'aes'
+encrypt.key-secret ID of the secret that provides the AES encryption key
 cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
@@ -497,6 +520,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
 encrypt.format   Encrypt the image, format choices: 'aes'
+encrypt.key-secret ID of the secret that provides the AES encryption key
 cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
@@ -511,6 +535,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
 encrypt.format   Encrypt the image, format choices: 'aes'
+encrypt.key-secret ID of the secret that provides the AES encryption key
 cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
@@ -525,6 +550,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
 encrypt.format   Encrypt the image, format choices: 'aes'
+encrypt.key-secret ID of the secret that provides the AES encryption key
 cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
@@ -556,6 +582,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
 encrypt.format   Encrypt the image, format choices: 'aes'
+encrypt.key-secret ID of the secret that provides the AES encryption key
 cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
tests/qemu-iotests/087 +13 -15
index 6d52f7d..1d595b2 100755
@@ -122,24 +122,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,encrypt.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",
@@ -147,6 +141,10 @@ run_qemu <<EOF
       "file": {
           "driver": "file",
           "filename": "$TEST_IMG"
+      },
+      "encrypt": {
+          "format": "aes",
+          "key-secret": "sec0"
       }
     }
   }
@@ -157,7 +155,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,encrypt.key-secret=sec0 $size
 run_qemu -S <<EOF
 { "execute": "qmp_capabilities" }
 { "execute": "blockdev-add",
tests/qemu-iotests/087.out +3 -9
index dc6baf9..2f1d7d1 100644
@@ -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 encrypt.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 encrypt.key-secret=sec0
 Testing: -S
 QMP_VERSION
 {"return": {}}
tests/qemu-iotests/134 +13 -5
index acce946..f851d92 100755
@@ -44,23 +44,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,encrypt.key-secret=sec0" $size
+
+IMGSPEC="driver=$IMGFMT,file.filename=$TEST_IMG,encrypt.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
index 6493704..972be49 100644
@@ -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 encrypt.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
index ef8d70f..e280b79 100755
@@ -45,34 +45,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,encrypt.key-secret=sec0" $size
 TEST_IMG=$TEST_IMG_SAVE
 
+IMGSPECBASE="driver=$IMGFMT,file.filename=$TEST_IMG_BASE,encrypt.key-secret=sec0"
+IMGSPEC="driver=$IMGFMT,file.filename=$TEST_IMG,backing.driver=$IMGFMT,backing.file.filename=$TEST_IMG_BASE,backing.encrypt.key-secret=sec0,encrypt.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,encrypt.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
index b3f37e2..6def216 100644
@@ -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 encrypt.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 encrypt.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
index f2a7199..d34c11c 100644
@@ -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=
@@ -413,10 +414,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
Reviewed-by on v7 11/20
#15 lore raw

Re: [Qemu-devel] [PATCH v7 11/20] qcow2: convert QCow2 to use QCryptoBlock for encryption

Quoted 12 lines · Daniel P. Berrange

This converts the qcow2 driver to make use of the QCryptoBlock APIs for encrypting image content, using the legacy 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,encrypt.key-secret=sec0
Quoted 6 lines

The 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.

Berto

#16 lore raw

PATCH v7 12/20 qcow2: extend specification to cover LUKS encryption

In reply to 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

---
 docs/specs/qcow2.txt | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 96 insertions(+)
docs/specs/qcow2.txt +96 -0
index 80cdfd0..6509b91 100644
@@ -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
Reviewed-by on v7 12/20
#17 lore raw

Re: [Qemu-devel] [PATCH v7 12/20] qcow2: extend specification to cover LUKS encryption

Quoted 16 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

---
 docs/specs/qcow2.txt | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 96 insertions(+)
docs/specs/qcow2.txt +4 -0
+
+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

docs/specs/qcow2.txt The fields of the bitmaps extension are: · line 246

Quoted 10 lines
docs/specs/qcow2.txt +10 -0
+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.

Is it possible/valid for the LUKS payload-offset field to NOT be qcow2-cluster-aligned? (I'm guessing yes, for example, if we have 2M clusters; and that we don't care, because you documented that we don't actually use that offset). Is it worth documenting this?

docs/specs/qcow2.txt The fields of the bitmaps extension are: · line 288

Quoted 5 lines

+== Data encryption ==

docs/specs/qcow2.txt +4 -0
+
+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.

s/is/are/

docs/specs/qcow2.txt The fields of the bitmaps extension are: · line 297

Quoted 9 lines
docs/specs/qcow2.txt +9 -0
+
+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.

Worth documenting that implementations should avoid creating new images with this encryption (ie. make it officially part of the spec that this is not recommended, but merely documented for back-compat data liberation purposes)?

Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
#18 lore raw

Re: [Qemu-devel] [PATCH v7 12/20] qcow2: extend specification to cover LUKS encryption

Quoted 24 lines · Eric Blake
Quoted 23 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

---
 docs/specs/qcow2.txt | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 96 insertions(+)
docs/specs/qcow2.txt +11 -0
+
+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
Quoted 8 lines

docs/specs/qcow2.txt The fields of the bitmaps extension are: · line 246

Quoted 3 lines
docs/specs/qcow2.txt +3 -0
+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.

Is it possible/valid for the LUKS payload-offset field to NOT be qcow2-cluster-aligned? (I'm guessing yes, for example, if we have 2M clusters; and that we don't care, because you documented that we don't actually use that offset). Is it worth documenting this?

Yeah, it is not required to be cluster aligned, and that doesn't matter because we don't use it in context of qcow2.

Essentially the only time it would be used if is we had some optimized way in future to convert from qcow2 w/ luks over to file w/ luks, without having to decrypt+reencrypt the payload. ie we could just copy out the entire luks header as-is, and then copy out the qcow2 payload after it.

Quoted 8 lines

docs/specs/qcow2.txt The fields of the bitmaps extension are: · line 288

Quoted 5 lines

+== Data encryption ==

docs/specs/qcow2.txt +4 -0
+
+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.

s/is/are/

Quoted 14 lines

docs/specs/qcow2.txt The fields of the bitmaps extension are: · line 297

Quoted 9 lines
docs/specs/qcow2.txt +9 -0
+
+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.

Worth documenting that implementations should avoid creating new images with this encryption (ie. make it officially part of the spec that this is not recommended, but merely documented for back-compat data liberation purposes)?

I guess I could document that here too - I did add such a note to the qemu-doc.texi at the end of this series.

Quoted 2 lines

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 :|
#19 lore raw

PATCH v7 13/20 qcow2: add support for LUKS encryption format

In reply to Daniel P. Berrange

This adds support for using LUKS as an encryption format with the qcow2 file, using the new encrypt.format parameter to request "luks" format. e.g.

  # qemu-img create --object secret,data=123456,id=sec0 \
       -f qcow2 -o encrypt.format=luks,encrypt.key-secret=sec0 \
       test.qcow2 10G

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,encrypt.key-secret=sec0 \
       test.qcow2 10G
 # qemu-img create --object secret,data=123456,id=sec0 \
       -f qcow2 -o encryption-format=aes,encrypt.key-secret=sec0 \
       test.qcow2 10G

With 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              | 267 ++++++++++++++++++++++++++++++++++++++------
 block/qcow2.h              |   9 ++
 qapi/block-core.json       |   5 +-
 tests/qemu-iotests/082.out | 270 ++++++++++++++++++++++++++++++++++++---------
 6 files changed, 477 insertions(+), 88 deletions(-)
block/qcow2-cluster.c +3 -1
index 4fdff1f..57c0c67 100644
@@ -395,7 +395,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
index 7c06061..81c22e6 100644
@@ -1856,6 +1856,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 +236 -31

388 lines not on this page · raw message

block/qcow2.h +9 -0
index 3b42b2e..374b3ab 100644
@@ -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;
qapi/block-core.json +3 -2
index c48b2fa..36754f3 100644
@@ -2324,7 +2324,7 @@
 # Since: 2.10
 ##
 { 'enum': 'BlockdevQcow2EncryptionFormat',
-  'data': [ 'aes' ] }
+  'data': [ 'aes', 'luks' ] }
 
 ##
 # @BlockdevQcow2Encryption:
@@ -2334,7 +2334,8 @@
 { 'union': 'BlockdevQcow2Encryption',
   'base': { 'format': 'BlockdevQcow2EncryptionFormat' },
   'discriminator': 'format',
-  'data': { 'aes': 'QCryptoBlockOptionsQCow' } }
+  'data': { 'aes': 'QCryptoBlockOptionsQCow',
+            'luks': 'QCryptoBlockOptionsLUKS'} }
 
 ##
 # @BlockdevOptionsQcow2:
tests/qemu-iotests/082.out +216 -54

432 lines not on this page · raw message

Signature · 1 line
2.9.3
#20 lore raw

PATCH v7 14/20 qcow2: add iotests to cover LUKS encryption support

In reply to Daniel P. Berrange

This 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     | 35 ++++++++++++++++++-
 tests/qemu-iotests/087.out | 14 +++++++-
 tests/qemu-iotests/183     | 76 ++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/183.out | 18 ++++++++++
 tests/qemu-iotests/184     | 86 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/184.out | 26 ++++++++++++++
 tests/qemu-iotests/group   |  2 ++
 7 files changed, 255 insertions(+), 2 deletions(-)
 create mode 100755 tests/qemu-iotests/183
 create mode 100644 tests/qemu-iotests/183.out
 create mode 100755 tests/qemu-iotests/184
 create mode 100644 tests/qemu-iotests/184.out
tests/qemu-iotests/087 +34 -1
index 1d595b2..f8e4903 100755
@@ -119,7 +119,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,encrypt.key-secret=sec0 $size
@@ -152,6 +152,39 @@ run_qemu <<EOF
 EOF
 
 echo
+echo === Encrypted image LUKS ===
+echo
+
+_make_test_img --object secret,id=sec0,data=123456 -o encrypt.format=luks,encrypt.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"
+      },
+      "encrypt": {
+        "format": "luks",
+        "key-secret": "sec0"
+      }
+    }
+  }
+{ "execute": "quit" }
+EOF
+
+echo
 echo === Missing driver ===
 echo
 
tests/qemu-iotests/087.out +13 -1
index 2f1d7d1..6317cc3 100644
@@ -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 encrypt.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 encrypt.format=luks encrypt.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 encrypt.key-secret=sec0
tests/qemu-iotests/183 new file +76 -0

76 lines not on this page · raw message

tests/qemu-iotests/183.out new file +18 -0
new file mode 100644
index 0000000..a095077
@@ -0,0 +1,18 @@
+QA output created by 183
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=16777216 encrypt.format=luks encrypt.key-secret=sec0 encrypt.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/184 new file +86 -0

86 lines not on this page · raw message

tests/qemu-iotests/184.out new file +26 -0
new file mode 100644
index 0000000..ede63dc
@@ -0,0 +1,26 @@
+QA output created by 184
+== create base ==
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=16777216 encrypt.format=luks encrypt.key-secret=sec0 encrypt.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 encrypt.format=luks encrypt.key-secret=sec1 encrypt.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
index 5c8ea0f..aaebe71 100644
@@ -174,3 +174,5 @@
 179 rw auto quick
 181 rw auto migration
 182 rw auto quick
+183 rw auto quick
+184 rw auto quick
Signature · 1 line
2.9.3
#21 lore raw

PATCH v7 15/20 iotests: enable tests 134 and 158 to work with qcow (v1)

In reply to Daniel P. Berrange

The 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
index f851d92..9914415 100755
@@ -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
 _unsupported_proto vxhs
 _supported_os Linux
tests/qemu-iotests/158 +1 -1
index e280b79..823c120 100755
@@ -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
 _unsupported_proto vxhs
 _supported_os Linux
Signature · 1 line
2.9.3
#22 lore raw

PATCH v7 16/20 block: rip out all traces of password prompting

In reply to 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.

---
 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
index 20f5dab..7793b68 100644
@@ -1084,37 +1084,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)
@@ -1737,12 +1712,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
index d2b3aaf..83ea4a1 100644
@@ -23,13 +23,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
index 1c9f5e2..5de3a6d 100644
@@ -444,8 +444,6 @@ void qemu_set_tty_echo(int fd, bool echo);
 void os_mem_prealloc(int fd, char *area, size_t sz, int smp_cpus,
                      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
index e38c5f0..17c39cf 100644
@@ -2428,8 +2428,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
@@ -2710,8 +2708,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:
 #
@@ -3016,11 +3013,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
index c906685..636ea09 100644
@@ -259,29 +259,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, bool force_share)
@@ -305,10 +282,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;
 }
 
@@ -335,10 +308,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
index 34fa8a1..5232678 100644
@@ -58,7 +58,6 @@ static int openfile(char *name, int flags, bool writethrough, bool force_share,
                     QDict *opts)
 {
     Error *local_err = NULL;
-    BlockDriverState *bs;
 
     if (qemuio_blk) {
         error_report("file open already, try 'help close'");
@@ -85,28 +84,9 @@ static int openfile(char *name, int flags, bool writethrough, bool force_share,
         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
index f656940..8dcac6d 100644
@@ -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.
      *
util/oslib-posix.c +0 -66

72 lines not on this page · raw message

util/oslib-win32.c +0 -24
index 80e4668..aacdaed 100644
@@ -554,30 +554,6 @@ void os_mem_prealloc(int fd, char *area, size_t memory, int smp_cpus,
 }
 
 
-/* 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
#23 lore raw

PATCH v7 17/20 block: remove all encryption handling APIs

In reply to Daniel P. Berrange

Now 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              |  8 ++++-
 block/qcow2.c             |  1 -
 blockdev.c                | 37 ++---------------------
 hmp-commands.hx           |  2 ++
 include/block/block.h     |  3 --
 include/block/block_int.h |  1 -
 include/qapi/error.h      |  1 -
 qapi/block-core.json      | 37 ++---------------------
 qapi/common.json          |  5 +--
 12 files changed, 16 insertions(+), 159 deletions(-)
block.c +1 -76
index 50ba264..9677f08 100644
@@ -2529,15 +2529,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);
 
@@ -3028,7 +3020,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);
@@ -3458,72 +3449,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
index da4be74..3ad4b20 100644
@@ -308,7 +308,6 @@ static int block_crypto_open_generic(QCryptoBlockFormat format,
     }
 
     bs->encrypted = true;
-    bs->valid_key = true;
 
     ret = 0;
  cleanup:
block/qapi.c +1 -1
index a40922e..9d724c2 100644
@@ -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 +7 -1
index 307a97a..67f2d53 100644
@@ -219,7 +219,13 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
             goto fail;
         }
         bs->encrypted = true;
-        bs->valid_key = true;
+    } else {
+        if (encryptfmt) {
+            error_setg(errp, "No encryption in image header, but options "
+                       "specified format '%s'", encryptfmt);
+            ret = -EINVAL;
+            goto fail;
+        }
     }
     s->cluster_bits = header.cluster_bits;
     s->cluster_size = 1 << s->cluster_bits;
block/qcow2.c +0 -1
index dddb62f..f537b68 100644
@@ -1171,7 +1171,6 @@ static int qcow2_do_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
index c63f4e8..858e756 100644
@@ -591,10 +591,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)) {
@@ -2261,24 +2257,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");
 }
 
 /*
@@ -2587,12 +2567,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);
@@ -3878,13 +3852,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);
 }
hmp-commands.hx +2 -0
index baeac47..231b79f 100644
@@ -1644,6 +1644,8 @@ STEXI
 @item block_passwd @var{device} @var{password}
 @findex block_passwd
 Set the encrypted device @var{device} password to @var{password}
+
+This command is now obsolete and will always return an error since 2.10
 ETEXI
 
     {
include/block/block.h +0 -3
index 9b355e9..eaaf298 100644
@@ -469,9 +469,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
index 7c31a83..3d08d5c 100644
@@ -528,7 +528,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 */
     bool force_share; /* if true, always allow all shared permissions */
include/qapi/error.h +0 -1
index 7e532d0..5d5e737 100644
@@ -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 +2 -35
index 36754f3..a4e47a1 100644
@@ -259,8 +259,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)
 #
@@ -945,39 +944,7 @@
 # This command sets the password of a block device that has not been open
 # with a password and requires one.
 #
-# The two cases where this can happen are a block device is created through
-# QEMU's initial command line or a block device is changed through the legacy
-# @change interface.
-#
-# In the event that the block device is created through the initial command
-# line, the VM will start in the stopped state regardless of whether '-S' is
-# used.  The intention is for a management tool to query the block devices to
-# determine which ones are encrypted, set the passwords with this command, and
-# then start the guest with the @cont command.
-#
-# Either @device or @node-name must be set but not both.
-#
-# @device: the name of the block backend device to set the password on
-#
-# @node-name: graph node name to set the password on (Since 2.0)
-#
-# @password: the password to use for the device
-#
-# Returns: nothing on success
-#          If @device is not a valid block device, DeviceNotFound
-#          If @device is not encrypted, DeviceNotEncrypted
-#
-# Notes:  Not all block formats support encryption and some that do are not
-#         able to validate that a password is correct.  Disk corruption may
-#         occur if an invalid password is specified.
-#
-# Since: 0.14.0
-#
-# Example:
-#
-# -> { "execute": "block_passwd", "arguments": { "device": "ide0-hd0",
-#                                                "password": "12345" } }
-# <- { "return": {} }
+# This command is now obsolete and will always return an error since 2.10
 #
 ##
 { 'command': 'block_passwd', 'data': {'*device': 'str',
qapi/common.json +1 -4
index b626647..8355d5a 100644
@@ -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
Reviewed-by on v7 17/20
#24 lore raw

Re: [Qemu-devel] [PATCH v7 17/20] block: remove all encryption handling APIs

Quoted 10 lines · Daniel P. Berrange

Now 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.

Berto

#25 lore raw

PATCH v7 18/20 block: pass option prefix down to crypto layer

In reply to 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. "encrypt.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             | 8 ++++----
 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, 37 insertions(+), 20 deletions(-)
block/crypto.c +2 -2
index 3ad4b20..c561cba 100644
@@ -296,7 +296,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,
@@ -340,7 +340,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
index 67f2d53..13532f4 100644
@@ -207,8 +207,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, "encrypt.",
+                                           NULL, NULL, cflags, errp);
             if (!s->crypto) {
                 ret = -EINVAL;
                 goto fail;
@@ -865,7 +865,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, "encrypt.",
+                                      NULL, NULL, NULL, errp);
         if (!crypto) {
             ret = -EINVAL;
             goto exit;
block/qcow2.c +4 -4
index f537b68..38f9eb5 100644
@@ -279,7 +279,7 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
             if (flags & BDRV_O_NO_IO) {
                 cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
             }
-            s->crypto = qcrypto_block_open(s->crypto_opts,
+            s->crypto = qcrypto_block_open(s->crypto_opts, "encrypt.",
                                            qcow2_crypto_hdr_read_func,
                                            bs, cflags, errp);
             if (!s->crypto) {
@@ -1313,8 +1313,8 @@ static int qcow2_do_open(BlockDriverState *bs, QDict *options, int flags,
             if (flags & BDRV_O_NO_IO) {
                 cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
             }
-            s->crypto = qcrypto_block_open(s->crypto_opts, NULL, NULL,
-                                           cflags, errp);
+            s->crypto = qcrypto_block_open(s->crypto_opts, "encrypt.",
+                                           NULL, NULL, cflags, errp);
             if (!s->crypto) {
                 ret = -EINVAL;
                 goto fail;
@@ -2273,7 +2273,7 @@ static int qcow2_set_up_encryption(BlockDriverState *bs, const char *encryptfmt,
     }
     s->crypt_method_header = fmt;
 
-    crypto = qcrypto_block_create(cryptoopts,
+    crypto = qcrypto_block_create(cryptoopts, "encrypt.",
                                   qcow2_crypto_hdr_init_func,
                                   qcow2_crypto_hdr_write_func,
                                   bs, errp);
crypto/block-luks.c +6 -2
index 2b97d89..afb8543 100644
@@ -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
index be88c6f..a456fe3 100644
@@ -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
index 64c8420..b097d45 100644
@@ -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
index 68f0f06..0edb810 100644
@@ -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
index 013a435..f0e543b 100644
@@ -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
index 95c4bd5..bd7fe59 100644
@@ -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
#26 lore raw

PATCH v7 19/20 qcow2: report encryption specific image information

In reply to Daniel P. Berrange

Currently 'qemu-img info' reports a simple "encrypted: yes" field. This is not very useful now that qcow2 can support multiple encryption formats. Users want to know which format is in use and some data related to it.

Wire up usage of the qcrypto_block_get_info() method so that 'qemu-img info' can report about the encryption format and parameters in use

  $ qemu-img create \
      --object secret,id=sec0,data=123456 \
      -o encrypt.format=luks,encrypt.key-secret=sec0 \
      -f qcow2 demo.qcow2 1G
  Formatting 'demo.qcow2', fmt=qcow2 size=1073741824 \
  encryption=off encrypt.format=luks encrypt.key-secret=sec0 \
  cluster_size=65536 lazy_refcounts=off refcount_bits=16
  $ qemu-img info demo.qcow2
  image: demo.qcow2
  file format: qcow2
  virtual size: 1.0G (1073741824 bytes)
  disk size: 480K
  encrypted: yes
  cluster_size: 65536
  Format specific information:
      compat: 1.1
      lazy refcounts: false
      refcount bits: 16
      encrypt:
          ivgen alg: plain64
          hash alg: sha256
          cipher alg: aes-256
          uuid: 3fa930c4-58c8-4ef7-b3c5-314bb5af21f3
          format: luks
          cipher mode: xts
          slots:
              [0]:
                  active: true
                  iters: 1839058
                  key offset: 4096
                  stripes: 4000
              [1]:
                  active: false
                  key offset: 262144
              [2]:
                  active: false
                  key offset: 520192
              [3]:
                  active: false
                  key offset: 778240
              [4]:
                  active: false
                  key offset: 1036288
              [5]:
                  active: false
                  key offset: 1294336
              [6]:
                  active: false
                  key offset: 1552384
              [7]:
                  active: false
                  key offset: 1810432
          payload offset: 2068480
          master key iters: 438487
      corrupt: false

With the legacy "AES" encryption we just report the format name

  $ qemu-img create \
      --object secret,id=sec0,data=123456 \
      -o encrypt.format=aes,encrypt.key-secret=sec0 \
      -f qcow2 demo.qcow2 1G
  Formatting 'demo.qcow2', fmt=qcow2 size=1073741824 \
  encryption=off encrypt.format=aes encrypt.key-secret=sec0 \
  cluster_size=65536 lazy_refcounts=off refcount_bits=16
  $ ./qemu-img info demo.qcow2
  image: demo.qcow2
  file format: qcow2
  virtual size: 1.0G (1073741824 bytes)
  disk size: 196K
  encrypted: yes
  cluster_size: 65536
  Format specific information:
      compat: 1.1
      lazy refcounts: false
      refcount bits: 16
      encrypt:
          format: aes
      corrupt: false
---
 block/qcow2.c        | 35 ++++++++++++++++++++++++++++++++++-
 qapi/block-core.json | 24 +++++++++++++++++++++++-
 2 files changed, 57 insertions(+), 2 deletions(-)
block/qcow2.c +34 -1
index 38f9eb5..cb321a2 100644
@@ -3196,8 +3196,17 @@ static int qcow2_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
 static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
 {
     BDRVQcow2State *s = bs->opaque;
-    ImageInfoSpecific *spec_info = g_new(ImageInfoSpecific, 1);
+    ImageInfoSpecific *spec_info;
+    QCryptoBlockInfo *encrypt_info = NULL;
 
+    if (s->crypto != NULL) {
+        encrypt_info = qcrypto_block_get_info(s->crypto, NULL);
+        if (!encrypt_info) {
+            return NULL;
+        }
+    }
+
+    spec_info = g_new(ImageInfoSpecific, 1);
     *spec_info = (ImageInfoSpecific){
         .type  = IMAGE_INFO_SPECIFIC_KIND_QCOW2,
         .u.qcow2.data = g_new(ImageInfoSpecificQCow2, 1),
@@ -3224,6 +3233,30 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
         assert(false);
     }
 
+    if (encrypt_info) {
+        ImageInfoSpecificQCow2Encryption *qencrypt =
+            g_new(ImageInfoSpecificQCow2Encryption, 1);
+        switch (encrypt_info->format) {
+        case Q_CRYPTO_BLOCK_FORMAT_QCOW:
+            qencrypt->format = BLOCKDEV_QCOW2_ENCRYPTION_FORMAT_AES;
+            qencrypt->u.aes = encrypt_info->u.qcow;
+            break;
+        case Q_CRYPTO_BLOCK_FORMAT_LUKS:
+            qencrypt->format = BLOCKDEV_QCOW2_ENCRYPTION_FORMAT_LUKS;
+            qencrypt->u.luks = encrypt_info->u.luks;
+            break;
+        default:
+            assert(false);
+        }
+        /* Since we did shallow copy above, erase any pointers
+         * in the original info */
+        memset(&encrypt_info->u, 0, sizeof(encrypt_info->u));
+        qapi_free_QCryptoBlockInfo(encrypt_info);
+
+        spec_info->u.qcow2.data->has_encrypt = true;
+        spec_info->u.qcow2.data->encrypt = qencrypt;
+    }
+
     return spec_info;
 }
 
qapi/block-core.json +23 -1
index a4e47a1..23b8f3f 100644
@@ -33,6 +33,27 @@
             'vm-clock-sec': 'int', 'vm-clock-nsec': 'int' } }
 
 ##
+# @ImageInfoSpecificQCow2EncryptionBase:
+#
+# @format: The encryption format
+#
+# Since: 2.10
+##
+{ 'struct': 'ImageInfoSpecificQCow2EncryptionBase',
+  'data': { 'format': 'BlockdevQcow2EncryptionFormat'}}
+
+##
+# @ImageInfoSpecificQCow2Encryption:
+#
+# Since: 2.10
+##
+{ 'union': 'ImageInfoSpecificQCow2Encryption',
+  'base': 'ImageInfoSpecificQCow2EncryptionBase',
+  'discriminator': 'format',
+  'data': { 'aes': 'QCryptoBlockInfoQCow',
+            'luks': 'QCryptoBlockInfoLUKS' } }
+
+##
 # @ImageInfoSpecificQCow2:
 #
 # @compat: compatibility level
@@ -51,7 +72,8 @@
       'compat': 'str',
       '*lazy-refcounts': 'bool',
       '*corrupt': 'bool',
-      'refcount-bits': 'int'
+      'refcount-bits': 'int',
+      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
   } }
 
 ##
Signature · 1 line
2.9.3
#27 lore raw

Re: [Qemu-devel] [PATCH v7 19/20] qcow2: report encryption specific image information

Quoted 21 lines · Daniel P. Berrange

Currently 'qemu-img info' reports a simple "encrypted: yes" field. This is not very useful now that qcow2 can support multiple encryption formats. Users want to know which format is in use and some data related to it.

Wire up usage of the qcrypto_block_get_info() method so that 'qemu-img info' can report about the encryption format and parameters in use

---
 block/qcow2.c        | 35 ++++++++++++++++++++++++++++++++++-
 qapi/block-core.json | 24 +++++++++++++++++++++++-
 2 files changed, 57 insertions(+), 2 deletions(-)
block/qcow2.c +0 -0
index 38f9eb5..cb321a2 100644

block/qcow2.c static int qcow2_get_info(BlockDriverState *bs, BlockDriverInfo *bdi) · line 3203

Quoted 9 lines
block/qcow2.c +4 -1
@@ -3196,8 +3196,17 @@ static int qcow2_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
 static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
 {
     BDRVQcow2State *s = bs->opaque;
-    ImageInfoSpecific *spec_info = g_new(ImageInfoSpecific, 1);
+    ImageInfoSpecific *spec_info;
+    QCryptoBlockInfo *encrypt_info = NULL;
 
+    if (s->crypto != NULL) {
+        encrypt_info = qcrypto_block_get_info(s->crypto, NULL);

Worth using &error_abort instead of silently ignoring the error? Is an error even possible in our output visitor [adding Markus for reference]?

block/qcow2.c static int qcow2_get_info(BlockDriverState *bs, BlockDriverInfo *bdi) · line 3206

Quoted 3 lines
block/qcow2.c +3 -0
+        if (!encrypt_info) {
+            return NULL;
+        }

If you use &error_abort, this is a dead check.

Quoted 19 lines
block/qcow2.c +12 -0
+    }
+
+    spec_info = g_new(ImageInfoSpecific, 1);
     *spec_info = (ImageInfoSpecific){
         .type  = IMAGE_INFO_SPECIFIC_KIND_QCOW2,
         .u.qcow2.data = g_new(ImageInfoSpecificQCow2, 1),
@@ -3224,6 +3233,30 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
         assert(false);
     }
 
+    if (encrypt_info) {
+        ImageInfoSpecificQCow2Encryption *qencrypt =
+            g_new(ImageInfoSpecificQCow2Encryption, 1);
+        switch (encrypt_info->format) {
+        case Q_CRYPTO_BLOCK_FORMAT_QCOW:
+            qencrypt->format = BLOCKDEV_QCOW2_ENCRYPTION_FORMAT_AES;
+            qencrypt->u.aes = encrypt_info->u.qcow;
+            break;
+        case Q_CRYPTO_BLOCK_FORMAT_LUKS:

block/qcow2.c static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs) · line 3254

Quoted 10 lines
block/qcow2.c +10 -0
+            qencrypt->format = BLOCKDEV_QCOW2_ENCRYPTION_FORMAT_LUKS;
+            qencrypt->u.luks = encrypt_info->u.luks;
+            break;
+        default:
+            assert(false);
+        }
+        /* Since we did shallow copy above, erase any pointers
+         * in the original info */
+        memset(&encrypt_info->u, 0, sizeof(encrypt_info->u));
+        qapi_free_QCryptoBlockInfo(encrypt_info);

Does QAPI_CLONE_MEMBERS (commit 4626a19c) make this code any easier to write? Then again, malloc'ing duplicates to then free the unchanged original is a bit slower than stealing references from the original then making sure the original can be freed without problem.

block/qcow2.c line 76

Quoted 10 lines
block/qcow2.c +2 -1
 # @ImageInfoSpecificQCow2:
 #
 # @compat: compatibility level
@@ -51,7 +72,8 @@
       'compat': 'str',
       '*lazy-refcounts': 'bool',
       '*corrupt': 'bool',
-      'refcount-bits': 'int'
+      'refcount-bits': 'int',
+      '*encrypt': 'ImageInfoSpecificQCow2Encryption'

Missing documentation of the @encrypt field.

Close!

Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
#28 lore raw

Re: [Qemu-devel] [PATCH v7 19/20] qcow2: report encryption specific image information

Quoted 29 lines · Eric Blake
Quoted 28 lines · Daniel P. Berrange

Currently 'qemu-img info' reports a simple "encrypted: yes" field. This is not very useful now that qcow2 can support multiple encryption formats. Users want to know which format is in use and some data related to it.

Wire up usage of the qcrypto_block_get_info() method so that 'qemu-img info' can report about the encryption format and parameters in use

---
 block/qcow2.c        | 35 ++++++++++++++++++++++++++++++++++-
 qapi/block-core.json | 24 +++++++++++++++++++++++-
 2 files changed, 57 insertions(+), 2 deletions(-)
block/qcow2.c +2 -1
index 38f9eb5..cb321a2 100644
@@ -3196,8 +3196,17 @@ static int qcow2_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
 static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
 {
     BDRVQcow2State *s = bs->opaque;
-    ImageInfoSpecific *spec_info = g_new(ImageInfoSpecific, 1);
+    ImageInfoSpecific *spec_info;
+    QCryptoBlockInfo *encrypt_info = NULL;
 
Quoted 5 lines

block/qcow2.c static int qcow2_get_info(BlockDriverState *bs, BlockDriverInfo *bdi) · line 3203

Quoted 2 lines
block/qcow2.c +2 -0
+    if (s->crypto != NULL) {
+        encrypt_info = qcrypto_block_get_info(s->crypto, NULL);

Worth using &error_abort instead of silently ignoring the error? Is an error even possible in our output visitor [adding Markus for reference]?

In fact the qcrypto_block_get_info() will never return an error right now as implemented. So I guess we could even just remove the Error **errp parameter from that call

Quoted 31 lines

block/qcow2.c static int qcow2_get_info(BlockDriverState *bs, BlockDriverInfo *bdi) · line 3206

Quoted 4 lines
block/qcow2.c +3 -0
+        if (!encrypt_info) {
+            return NULL;
+        }

If you use &error_abort, this is a dead check.

Quoted 13 lines
block/qcow2.c +12 -0
+    }
+
+    spec_info = g_new(ImageInfoSpecific, 1);
 
+    if (encrypt_info) {
+        ImageInfoSpecificQCow2Encryption *qencrypt =
+            g_new(ImageInfoSpecificQCow2Encryption, 1);
+        switch (encrypt_info->format) {
+        case Q_CRYPTO_BLOCK_FORMAT_QCOW:
+            qencrypt->format = BLOCKDEV_QCOW2_ENCRYPTION_FORMAT_AES;
+            qencrypt->u.aes = encrypt_info->u.qcow;
+            break;
+        case Q_CRYPTO_BLOCK_FORMAT_LUKS:

block/qcow2.c static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs) · line 3254

Quoted 10 lines
block/qcow2.c +10 -0
+            qencrypt->format = BLOCKDEV_QCOW2_ENCRYPTION_FORMAT_LUKS;
+            qencrypt->u.luks = encrypt_info->u.luks;
+            break;
+        default:
+            assert(false);
+        }
+        /* Since we did shallow copy above, erase any pointers
+         * in the original info */
+        memset(&encrypt_info->u, 0, sizeof(encrypt_info->u));
+        qapi_free_QCryptoBlockInfo(encrypt_info);
Quoted 4 lines

Does QAPI_CLONE_MEMBERS (commit 4626a19c) make this code any easier to write? Then again, malloc'ing duplicates to then free the unchanged original is a bit slower than stealing references from the original then making sure the original can be freed without problem.

I don't think CLONE would make a signijficant difference to the size of the code, since we have to remap the enum constants regardless.

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 :|
#29 lore raw

Re: [Qemu-devel] [PATCH v7 19/20] qcow2: report encryption specific image information

Quoted 36 lines · Daniel P. Berrange
Quoted 29 lines · Eric Blake
Quoted 28 lines · Daniel P. Berrange

Currently 'qemu-img info' reports a simple "encrypted: yes" field. This is not very useful now that qcow2 can support multiple encryption formats. Users want to know which format is in use and some data related to it.

Wire up usage of the qcrypto_block_get_info() method so that 'qemu-img info' can report about the encryption format and parameters in use

---
 block/qcow2.c        | 35 ++++++++++++++++++++++++++++++++++-
 qapi/block-core.json | 24 +++++++++++++++++++++++-
 2 files changed, 57 insertions(+), 2 deletions(-)
block/qcow2.c +2 -1
index 38f9eb5..cb321a2 100644
@@ -3196,8 +3196,17 @@ static int qcow2_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
 static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
 {
     BDRVQcow2State *s = bs->opaque;
-    ImageInfoSpecific *spec_info = g_new(ImageInfoSpecific, 1);
+    ImageInfoSpecific *spec_info;
+    QCryptoBlockInfo *encrypt_info = NULL;
 
Quoted 5 lines

block/qcow2.c static int qcow2_get_info(BlockDriverState *bs, BlockDriverInfo *bdi) · line 3203

Quoted 2 lines
block/qcow2.c +2 -0
+    if (s->crypto != NULL) {
+        encrypt_info = qcrypto_block_get_info(s->crypto, NULL);

Worth using &error_abort instead of silently ignoring the error? Is an error even possible in our output visitor [adding Markus for reference]?

Quoted 3 lines

In fact the qcrypto_block_get_info() will never return an error right now as implemented. So I guess we could even just remove the Error **errp parameter from that call

Do you still need advice on QAPI from me here?

[...]

#30 lore raw

Re: [Qemu-devel] [PATCH v7 19/20] qcow2: report encryption specific image information

Quoted 38 lines · Markus Armbruster
Quoted 36 lines · Daniel P. Berrange
Quoted 34 lines · Eric Blake
Quoted 21 lines · Daniel P. Berrange

Currently 'qemu-img info' reports a simple "encrypted: yes" field. This is not very useful now that qcow2 can support multiple encryption formats. Users want to know which format is in use and some data related to it.

Wire up usage of the qcrypto_block_get_info() method so that 'qemu-img info' can report about the encryption format and parameters in use

---
 block/qcow2.c        | 35 ++++++++++++++++++++++++++++++++++-
 qapi/block-core.json | 24 +++++++++++++++++++++++-
 2 files changed, 57 insertions(+), 2 deletions(-)
block/qcow2.c +0 -0
index 38f9eb5..cb321a2 100644

block/qcow2.c static int qcow2_get_info(BlockDriverState *bs, BlockDriverInfo *bdi) · line 3203

Quoted 9 lines
block/qcow2.c +4 -1
@@ -3196,8 +3196,17 @@ static int qcow2_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
 static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
 {
     BDRVQcow2State *s = bs->opaque;
-    ImageInfoSpecific *spec_info = g_new(ImageInfoSpecific, 1);
+    ImageInfoSpecific *spec_info;
+    QCryptoBlockInfo *encrypt_info = NULL;
 
+    if (s->crypto != NULL) {
+        encrypt_info = qcrypto_block_get_info(s->crypto, NULL);

Worth using &error_abort instead of silently ignoring the error? Is an error even possible in our output visitor [adding Markus for reference]?

Quoted 5 lines
Quoted 3 lines

In fact the qcrypto_block_get_info() will never return an error right now as implemented. So I guess we could even just remove the Error **errp parameter from that call

Do you still need advice on QAPI from me here?

No need, thanks.

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 :|
#31 lore raw

PATCH v7 20/20 docs: document encryption options for qcow, qcow2 and luks

In reply to Daniel P. Berrange

Expand the image format docs to cover the new options for the qcow, qcow2 and luks disk image formats

---
 qemu-doc.texi | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 115 insertions(+), 8 deletions(-)
qemu-doc.texi +115 -8
index de0cc30..2b31cc2 100644
@@ -546,10 +546,20 @@ File name of a base image (see @option{create} subcommand)
 @item backing_fmt
 Image format of the base image
 @item encryption
-If this option is set to @code{on}, the image is encrypted with 128-bit AES-CBC.
+This option is deprecated and equivalent to @code{encrypt.format=aes}
 
-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:
+@item encrypt.format
+
+If this is set to @code{luks}, it requests that the qcow2 payload (not
+qcow2 header) be encrypted using the LUKS format. The passphrase to
+use to unlock the LUKS key slot is given by the @code{encrypt.key-secret}
+parameter. LUKS encryption parameters can be tuned with the other
+@code{encrypt.*} parameters.
+
+If this is set to @code{aes}, the image is encrypted with 128-bit AES-CBC.
+The encryption key is given by the @code{encrypt.key-secret} parameter.
+This encryption format 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
@@ -564,10 +574,45 @@ original file must then be securely erased using a program like shred,
 though even this is ineffective with many modern storage technologies.
 @end itemize
 
-Use of qcow / qcow2 encryption with QEMU is deprecated, and support for
-it will go away in a future release.  Users are recommended to use an
-alternative encryption technology such as the Linux dm-crypt / LUKS
-system.
+The use of this is no longer supported in system emulators. Support only
+remains in the command line utilities, for the purposes of data liberation
+and interoperability with old versions of QEMU. The @code{luks} format
+should be used instead.
+
+@item encrypt.key-secret
+
+Provides the ID of a @code{secret} object that contains the passphrase
+(@code{encrypt.format=luks}) or encryption key (@code{encrypt.format=aes}).
+
+@item encrypt.cipher-alg
+
+Name of the cipher algorithm and key length. Currently defaults
+to @code{aes-256}. Only used when @code{encrypt.format=luks}.
+
+@item encrypt.cipher-mode
+
+Name of the encryption mode to use. Currently defaults to @code{xts}.
+Only used when @code{encrypt.format=luks}.
+
+@item encrypt.ivgen-alg
+
+Name of the initialization vector generator algorithm. Currently defaults
+to @code{plain64}. Only used when @code{encrypt.format=luks}.
+
+@item encrypt.ivgen-hash-alg
+
+Name of the hash algorithm to use with the initialization vector generator
+(if required). Defaults to @code{sha256}. Only used when @code{encrypt.format=luks}.
+
+@item encrypt.hash-alg
+
+Name of the hash algorithm to use for PBKDF algorithm
+Defaults to @code{sha256}. Only used when @code{encrypt.format=luks}.
+
+@item encrypt.iter-time
+
+Amount of time, in milliseconds, to use for PBKDF algorithm per key slot.
+Defaults to @code{2000}. Only used when @code{encrypt.format=luks}.
 
 @item cluster_size
 Changes the qcow2 cluster size (must be between 512 and 2M). Smaller cluster
@@ -642,7 +687,69 @@ Supported options:
 @item backing_file
 File name of a base image (see @option{create} subcommand)
 @item encryption
-If this option is set to @code{on}, the image is encrypted.
+This option is deprecated and equivalent to @code{encrypt.format=aes}
+
+@item encrypt.format
+If this is set to @code{aes}, the image is encrypted with 128-bit AES-CBC.
+The encryption key is given by the @code{encrypt.key-secret} parameter.
+This encryption format is considered to be flawed by modern cryptography
+standards, suffering from a number of design problems enumerated previously
+against the @code{qcow2} image format.
+
+The use of this is no longer supported in system emulators. Support only
+remains in the command line utilities, for the purposes of data liberation
+and interoperability with old versions of QEMU.
+
+Users requiring native encryption should use the @code{qcow2} format
+instead with @code{encrypt.format=luks}.
+
+@item encrypt.key-secret
+
+Provides the ID of a @code{secret} object that contains the encryption
+key (@code{encrypt.format=aes}).
+
+@end table
+
+@item luks
+
+LUKS v1 encryption format, compatible with Linux dm-crypt/cryptsetup
+
+Supported options:
+@table @code
+
+@item key-secret
+
+Provides the ID of a @code{secret} object that contains the passphrase.
+
+@item cipher-alg
+
+Name of the cipher algorithm and key length. Currently defaults
+to @code{aes-256}.
+
+@item cipher-mode
+
+Name of the encryption mode to use. Currently defaults to @code{xts}.
+
+@item ivgen-alg
+
+Name of the initialization vector generator algorithm. Currently defaults
+to @code{plain64}.
+
+@item ivgen-hash-alg
+
+Name of the hash algorithm to use with the initialization vector generator
+(if required). Defaults to @code{sha256}.
+
+@item hash-alg
+
+Name of the hash algorithm to use for PBKDF algorithm
+Defaults to @code{sha256}.
+
+@item iter-time
+
+Amount of time, in milliseconds, to use for PBKDF algorithm per key slot.
+Defaults to @code{2000}.
+
 @end table
 
 @item vdi
Signature · 1 line
2.9.3
Reviewed-by on v7 20/20
#32 lore raw

Re: [Qemu-devel] [PATCH v7 20/20] docs: document encryption options for qcow, qcow2 and luks

Quoted 7 lines · Daniel P. Berrange

Expand the image format docs to cover the new options for the qcow, qcow2 and luks disk image formats

---
 qemu-doc.texi | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 115 insertions(+), 8 deletions(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org