Convert QCow[2] to QCryptoBlock & add LUKS support

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.

Signed-off-by Daniel P. Berrange <[email protected]>

---
 block/crypto.c | 61 +++++++--------------------------------
 block/crypto.h | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 102 insertions(+), 50 deletions(-)
 create mode 100644 block/crypto.h

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.

Signed-off-by Daniel P. Berrange <[email protected]>

---
 block/crypto.c | 61 +++++++--------------------------------
 block/crypto.h | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 102 insertions(+), 50 deletions(-)
 create mode 100644 block/crypto.h

When integrating the crypto support with qcow/qcow2, we don't want to use the bare LUKS option names "hash-alg", "key-secret", etc. We want to namespace them "luks-hash-alg", "luks-key-secret" so that they don't clash with any general qcow options at a later date.

Signed-off-by Daniel P. Berrange <[email protected]>

---
 block/crypto.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++++--------
 block/crypto.h |  42 +++++++++++-----------
 2 files changed, 118 insertions(+), 34 deletions(-)

block/crypto.c +0 -0

index d281de6..1037c70 100644

block/crypto.c +5 -0

+        }
+    }
+
+    return 0;
+}

Quoted 16 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 want to namespace them "luks-hash-alg", "luks-key-secret" so that they don't clash with any general qcow options at a later date.

Signed-off-by Daniel P. Berrange <[email protected]>

---
 block/crypto.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++++--------
 block/crypto.h |  42 +++++++++++-----------
 2 files changed, 118 insertions(+), 34 deletions(-)

block/crypto.c +0 -0

index d281de6..1037c70 100644

[...]

block/crypto.c static QemuOptsList block_crypto_create_opts_luks = { · line 180

Quoted 8 lines

block/crypto.c +8 -0

+static int block_crypto_copy_value(void *opaque, const char *name,
+                                   const char *value, Error **errp)
+{
+    struct BlockCryptoCopyData *data = opaque;
+
+    if (g_str_has_prefix(name, data->prefix)) {
+        Error *local_err = NULL;
+        const char *newname = name + strlen(data->prefix);

strstart() would be shorter:

const char *newname;

if (strstart(name, data->prefix, &newname)) {
    /* ... */
}

block/crypto.c static QemuOptsList block_crypto_create_opts_luks = { · line 185

Quoted 6 lines

block/crypto.c +5 -0

+
+        qemu_opt_set(data->opts, newname, value, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return 1;

I'd prefer -1, because 0/1 looks more like false/true to me, which in turn looks like failure/success.

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.

Signed-off-by Daniel P. Berrange <[email protected]>

---
 qemu-img.texi | 9 +++++++++
 1 file changed, 9 insertions(+)

qemu-img.texi +0 -0

index 174aae3..8efcf89 100644
@@ -554,6 +554,15 @@ 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.

qemu-img.texi +0 -0

 Use of qcow / qcow2 encryption is thus strongly discouraged. Users are
 

Quoted 17 lines · 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.

Signed-off-by Daniel P. Berrange <[email protected]>

---
 qemu-img.texi | 9 +++++++++
 1 file changed, 9 insertions(+)

qemu-img.texi +0 -0

index 174aae3..8efcf89 100644
@@ -554,6 +554,15 @@ 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.

qemu-img.texi change the passphrase to protect data in any qcow images. The files must · line 566

Quoted 10 lines

qemu-img.texi +9 -0

+@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

In the output manpage, this itemize looks pretty broken to me:

@item foo
bar baz

is formatted as:

-<foo>
bar baz

Which may be used intentionally, but it certainly isn't here.

It should probably be written as:

@item
foo bar baz

which becomes

- foo bar baz

(which is what the other itemize in qemu-img.texi does)

Do you want to fix that in this series?

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.

Signed-off-by Daniel P. Berrange <[email protected]>

---
 block/qcow.c | 6 ++++++
 1 file changed, 6 insertions(+)

block/qcow.c +0 -0

index 7540f43..8133fda 100644

block/qcow.c +0 -0

     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;

Quoted 14 lines · 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.

Signed-off-by Daniel P. Berrange <[email protected]>

---
 block/qcow.c | 6 ++++++
 1 file changed, 6 insertions(+)

block/qcow.c +0 -0

index 7540f43..8133fda 100644

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

Quoted 9 lines

block/qcow.c +6 -0

@@ -799,6 +799,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 <= 1) {
+        error_setg(errp, "Image size is too small (must be at least 2 bytes)");
+        ret = -EINVAL;
+        goto cleanup;
+    }
+

Given the ROUND_UP() before, I can't see how total_size can be 1, but I can see how it can be 0. Since that is covered by this condition:

Reviewed-by Max Reitz <[email protected]>

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.

Signed-off-by Daniel P. Berrange <[email protected]>

---
 tests/qemu-iotests/042 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Test 048 is designed to verify data preservation during an image resize. The qcow (v1) format impl has never supported resize so always fails.

Signed-off-by Daniel P. Berrange <[email protected]>

---
 tests/qemu-iotests/048 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

The previous commit:

commit a3e1505daec31ef56f0489f8c8fff1b8e4ca92bd
Author: Eric Blake <[email protected]>
Date: Mon Dec 5 09:49:34 2016 -0600

qcow2: Don't strand clusters near 2G intervals during commit

extended the 097 test case so that it did two passes, once with an internal snapshot, once without.

qcow (v1) does not support internal snapshots, so this change broke test 097 when run against qcow.

This splits 097 in two, creating a new 173 that tests the internal snapshot codepath, effectively putting 097 back to its content before the above commit.

Signed-off-by Daniel P. Berrange <[email protected]>

---
 tests/qemu-iotests/097     |  10 +---
 tests/qemu-iotests/097.out | 125 ++------------------------------------------
 tests/qemu-iotests/173     | 126 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/173.out | 119 ++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 5 files changed, 251 insertions(+), 130 deletions(-)
 create mode 100755 tests/qemu-iotests/173
 create mode 100644 tests/qemu-iotests/173.out

Quoted 19 lines · Daniel P. Berrange

The previous commit:

commit a3e1505daec31ef56f0489f8c8fff1b8e4ca92bd
Author: Eric Blake <[email protected]>
Date: Mon Dec 5 09:49:34 2016 -0600

qcow2: Don't strand clusters near 2G intervals during commit

extended the 097 test case so that it did two passes, once with an internal snapshot, once without.

qcow (v1) does not support internal snapshots, so this change broke test 097 when run against qcow.

This splits 097 in two, creating a new 173 that tests the internal snapshot codepath, effectively putting 097 back to its content before the above commit.

Signed-off-by Daniel P. Berrange <[email protected]>

Quoted 9 lines

---
 tests/qemu-iotests/097     |  10 +---
 tests/qemu-iotests/097.out | 125 ++------------------------------------------
 tests/qemu-iotests/173     | 126 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/173.out | 119 ++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 5 files changed, 251 insertions(+), 130 deletions(-)
 create mode 100755 tests/qemu-iotests/173
 create mode 100644 tests/qemu-iotests/173.out

I don't think the effort is worth it, considering that probably literally nobody is still using qcow -- or so I hope, at least.

Quoted 30 lines · Max Reitz

Quoted 19 lines · Daniel P. Berrange

The previous commit:

commit a3e1505daec31ef56f0489f8c8fff1b8e4ca92bd
Author: Eric Blake <[email protected]>
Date: Mon Dec 5 09:49:34 2016 -0600

qcow2: Don't strand clusters near 2G intervals during commit

extended the 097 test case so that it did two passes, once with an internal snapshot, once without.

qcow (v1) does not support internal snapshots, so this change broke test 097 when run against qcow.

This splits 097 in two, creating a new 173 that tests the internal snapshot codepath, effectively putting 097 back to its content before the above commit.

Signed-off-by Daniel P. Berrange <[email protected]>

Quoted 9 lines

---
 tests/qemu-iotests/097     |  10 +---
 tests/qemu-iotests/097.out | 125 ++------------------------------------------
 tests/qemu-iotests/173     | 126 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/173.out | 119 ++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 5 files changed, 251 insertions(+), 130 deletions(-)
 create mode 100755 tests/qemu-iotests/173
 create mode 100644 tests/qemu-iotests/173.out

Quoted 2 lines

I don't think the effort is worth it, considering that probably literally nobody is still using qcow -- or so I hope, at least.

The reason I fixed this was because I wanted to be able to verify my refactoring to qcow didn't break anything else. It is much easier if I can just run "check -qcow" and not have to worry about which failures are just test bugs, vs genuine code bugs I've created. IOW, as long as qcow.c exists in the code base, IMHO, we should make sure the iotests continue to pass

On that point, IMHO, it would be beneficial if we had some CI system that was setup to run the iotests on all new changes, across all the different disk formats we expect the iotests to work on. We get far too many regressions with iotests breaking and no one noticing right now :-(

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 all uses the same buffer for input/output already

and the other two callers are easily converted todo so.

Signed-off-by Daniel P. Berrange <[email protected]>

---
 block/qcow.c | 36 +++++++++++-------------------------
 1 file changed, 11 insertions(+), 25 deletions(-)

block/qcow.c +7 -11

index 8133fda..bc9fa2f 100644
@@ -310,11 +310,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];

@@ -333,14 +332,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);
         }

@@ -348,8 +345,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;
 }

@@ -469,13 +465,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);
+                    memset(s->cluster_data, 0x00, 512);

block/qcow.c +1 -3

@@ -653,7 +648,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;
                 }

@@ -688,9 +683,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;

block/qcow.c +1 -5

                                 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);

@@ -764,7 +751,6 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,
     if (qiov->niov > 1) {
         qemu_vfree(orig_buf);
     }
-    g_free(cluster_data);
 
     return ret;
 }

Quoted 4 lines · 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 all uses the same buffer for input/output already

-all?

Quoted 1 line

and the other two callers are easily converted todo so.

s/todo/to do/

Quoted 64 lines

Signed-off-by Daniel P. Berrange <[email protected]>

---
 block/qcow.c | 36 +++++++++++-------------------------
 1 file changed, 11 insertions(+), 25 deletions(-)

block/qcow.c +7 -13

index 8133fda..bc9fa2f 100644
@@ -310,11 +310,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];

@@ -333,14 +332,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);
         }

@@ -348,8 +345,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;
 }

@@ -469,13 +465,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);
+                    memset(s->cluster_data, 0x00, 512);
                     for(i = 0; i < s->cluster_sectors; i++) {
                         if (i < n_start || i >= n_end) {
                             Error *err = NULL;
                             if (encrypt_sectors(s, start_sect + i,
-                                                s->cluster_data,
-                                                s->cluster_data + 512, 1,

block/qcow.c static uint64_t get_cluster_offset(BlockDriverState *bs, · line 476

Quoted 4 lines

block/qcow.c +1 -0

+                                                s->cluster_data, 1,
                                                 true, &err) < 0) {
                                 error_free(err);
                                 errno = EIO;

After the first iteration of the surrounding for () loop, s->cluster_data is unlikely to still be filled with zeros -- but I suspect the code intended to always write encrypted zeros.

Quoted 24 lines

block/qcow.c +1 -4

@@ -653,7 +648,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;
                 }

@@ -688,9 +683,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;

@@ -728,21 +721,15 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,
         if (bs->encrypted) {
             Error *err = NULL;
             assert(s->cipher);
-            if (!cluster_data) {

block/qcow.c static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num, · line 724

Quoted 4 lines

block/qcow.c +1 -3

-                cluster_data = g_malloc0(s->cluster_size);
-            }
-            if (encrypt_sectors(s, sector_num, cluster_data, buf,
+            if (encrypt_sectors(s, sector_num, buf,

If qiov->niov == 1, buf is not copied from the I/O vector but just the I/O vector base itself. Then, this will modify the data pointed to by that vector. I don't think that is a good idea.

This converts the qcow2 driver to make use of the QCryptoBlock APIs for encrypting image content. This is only wired up to permit use of the legacy QCow encryption format. Users who wish to have the strong LUKS format should switch to qcow2 instead.

With this change it is now required to use the QCryptoSecret object for providing passwords, instead of the current block password APIs / interactive prompting.

  $QEMU \
    -object secret,id=sec0,filename=/home/berrange/encrypted.pw \
    -drive file=/home/berrange/encrypted.qcow,aes-key-secret=sec0

Signed-off-by Daniel P. Berrange <[email protected]>

---
 block/crypto.c       |  10 +++
 block/crypto.h       |   9 +++
 block/qcow.c         | 184 +++++++++++++++++++++++----------------------------
 qapi/block-core.json |  17 ++++-
 4 files changed, 117 insertions(+), 103 deletions(-)

This converts the qcow2 driver to make use of the QCryptoBlock APIs for encrypting image content, using the legacyy QCow2 AES scheme.

With this change it is now required to use the QCryptoSecret object for providing passwords, instead of the current block password APIs / interactive prompting.

  $QEMU \
    -object secret,id=sec0,filename=/home/berrange/encrypted.pw \
    -drive file=/home/berrange/encrypted.qcow2,aes-key-secret=sec0

Signed-off-by Daniel P. Berrange <[email protected]>

---
 block/qcow2-cluster.c      |  47 +----------
 block/qcow2.c              | 190 +++++++++++++++++++++++++++++----------------
 block/qcow2.h              |   5 +-
 qapi/block-core.json       |   7 +-
 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 |   6 +-
 tests/qemu-iotests/134     |  18 +++--
 tests/qemu-iotests/134.out |  10 +--
 tests/qemu-iotests/158     |  19 +++--
 tests/qemu-iotests/158.out |  14 +---
 13 files changed, 219 insertions(+), 158 deletions(-)

block/qcow2.c +0 -0

index 3c14c86..5c9e196 100644

block/qcow2.c +10 -0

+         * backing file by default, so we don't have to manually
+         * provide the same key-secret property against the full
+         * backing chain
+         */
+        s->crypto = qcrypto_block_open(s->crypto_opts, NULL, NULL,
+                                       cflags, errp);
+        if (!s->crypto) {
+            ret = -EINVAL;
+            goto fail;
+        }

block/qcow2.c +5 -0

+{
+    BDRVQcow2State *s = bs->opaque;
+    QCryptoBlockCreateOptions *cryptoopts = NULL;
+    QCryptoBlock *crypto = NULL;
+    int ret = -EINVAL;

block/qcow2.h +3 -2

index 033d8c0..f4cb171 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

@@ -256,7 +256,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 +0 -0

index c2b70e8..2ca5674 100644

qapi/block-core.json +3 -2

+#                         when doing a metadata-only probe of the image.
 #
 # Since: 1.7
 ##

@@ -1948,8 +1951,8 @@
             '*cache-size': 'int',
             '*l2-cache-size': 'int',
             '*refcount-cache-size': 'int',
-            '*cache-clean-interval': 'int' } }
-
+            '*cache-clean-interval': 'int',
+            '*aes-key-secret': 'str' } }
 
 ##
 # @BlockdevOptionsArchipelago:

tests/qemu-iotests/049 +0 -0

index fff0760..7da4ac8 100755

tests/qemu-iotests/049 +0 -0

 echo "== Check lazy_refcounts option (only with v3) =="
 echo

tests/qemu-iotests/087 +10 -1

index 9de57dd..fe30383 100755
@@ -124,9 +124,18 @@ echo
 echo === Encrypted image ===
 echo
 
-_make_test_img -o encryption=on $size
+_make_test_img --object secret,id=sec0,data=123456 -o encryption=on,qcow-key-secret=sec0 $size
 run_qemu -S <<EOF
 { "execute": "qmp_capabilities" }
+{ "execute": "object-add",
+  "arguments": {
+      "qom-type": "secret",
+      "id": "sec0",
+      "props": {
+          "data": "123456"
+      }
+  }
+}
 { "execute": "blockdev-add",
   "arguments": {
       "driver": "$IMGFMT",

tests/qemu-iotests/087 +9 -0

     }
   }
 { "execute": "quit" }

@@ -142,6 +152,15 @@ EOF
 
 run_qemu <<EOF
 { "execute": "qmp_capabilities" }
+{ "execute": "object-add",
+  "arguments": {
+      "qom-type": "secret",
+      "id": "sec0",
+      "props": {
+          "data": "123456"
+      }
+  }
+}
 { "execute": "blockdev-add",
   "arguments": {
       "driver": "$IMGFMT",

tests/qemu-iotests/087 +0 -0

 run_qemu -S <<EOF
 { "execute": "qmp_capabilities" }
 { "execute": "blockdev-add",

tests/qemu-iotests/134 +0 -0

index af618b8..c2458d8 100755

tests/qemu-iotests/134 +2 -0

+
+QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT

tests/qemu-iotests/158 +0 -0

index a6cdd6d..2d1c015 100755

tests/qemu-iotests/158 +2 -0

+QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT
+

Quoted 28 lines · Daniel P. Berrange

This converts the qcow2 driver to make use of the QCryptoBlock APIs for encrypting image content, using the legacyy QCow2 AES scheme.

With this change it is now required to use the QCryptoSecret object for providing passwords, instead of the current block password APIs / interactive prompting.

  $QEMU \
    -object secret,id=sec0,filename=/home/berrange/encrypted.pw \
    -drive file=/home/berrange/encrypted.qcow2,aes-key-secret=sec0

Signed-off-by Daniel P. Berrange <[email protected]>

---
 block/qcow2-cluster.c      |  47 +----------
 block/qcow2.c              | 190 +++++++++++++++++++++++++++++----------------
 block/qcow2.h              |   5 +-
 qapi/block-core.json       |   7 +-
 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 |   6 +-
 tests/qemu-iotests/134     |  18 +++--
 tests/qemu-iotests/134.out |  10 +--
 tests/qemu-iotests/158     |  19 +++--
 tests/qemu-iotests/158.out |  14 +---
 13 files changed, 219 insertions(+), 158 deletions(-)

[...]

Quoted 4 lines

block/qcow2.c +0 -0

index 3c14c86..5c9e196 100644

[...]

Quoted 4 lines

block/qcow2.c +0 -0

@@ -1122,6 +1144,24 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 

block/qcow2.c static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, · line 1152

Quoted 6 lines

block/qcow2.c +6 -0

+    if (s->crypt_method_header == QCOW_CRYPT_AES) {
+        unsigned int cflags = 0;
+        if (flags & BDRV_O_NO_IO) {
+            cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
+        }
+        /* XXX how do we pass the same crypto opts down to the

I think a TODO instead of an XXX would have been sufficient, but it's your call.

Quoted 10 lines

block/qcow2.c +10 -0

+         * backing file by default, so we don't have to manually
+         * provide the same key-secret property against the full
+         * backing chain
+         */
+        s->crypto = qcrypto_block_open(s->crypto_opts, NULL, NULL,
+                                       cflags, errp);
+        if (!s->crypto) {
+            ret = -EINVAL;
+            goto fail;
+        }

[...]

block/qcow2.c static int qcow2_change_backing_file(BlockDriverState *bs, · line 2032

Quoted 7 lines

block/qcow2.c +3 -0

@@ -2022,6 +2027,44 @@ static int qcow2_change_backing_file(BlockDriverState *bs,
     return qcow2_update_header(bs);
 }
 
+
+static int qcow2_change_encryption(BlockDriverState *bs, QemuOpts *opts,
+                                   Error **errp)

I think this name is not quite appropriate, since this doesn't change the format of the file if it is already encrypted (and it will not encrypt any existing data).

Maybe set_up instead of change?

Quoted 28 lines

block/qcow2.h +3 -2

index 033d8c0..f4cb171 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

@@ -256,7 +256,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 +0 -0

index c2b70e8..2ca5674 100644

qapi/block-core.json line 1939

Quoted 5 lines

qapi/block-core.json +2 -0

@@ -1935,6 +1935,9 @@
 # @cache-clean-interval:  #optional clean unused entries in the L2 and refcount
 #                         caches. The interval is in seconds. The default value
 #                         is 0 and it disables this feature (since 2.5)
+# @aes-key-secret:        #optional the ID of a QCryptoSecret object providing
+#                         the AES decryption key (since 2.9) Mandatory except

Missing full stop after the closing parenthesis.

Also, it's mandatory only for encrypted images. I know it's obvious but that's not what it says here.

Quoted 14 lines

qapi/block-core.json +3 -2

+#                         when doing a metadata-only probe of the image.
 #
 # Since: 1.7
 ##

@@ -1948,8 +1951,8 @@
             '*cache-size': 'int',
             '*l2-cache-size': 'int',
             '*refcount-cache-size': 'int',
-            '*cache-clean-interval': 'int' } }
-
+            '*cache-clean-interval': 'int',
+            '*aes-key-secret': 'str' } }
 

tests/qemu-iotests/049 test_qemu_img create -f $IMGFMT -o preallocation=1234 "$TEST_IMG" 64M · line 109

Quoted 12 lines

qapi/block-core.json +0 -0

 ##
 # @BlockdevOptionsArchipelago:

tests/qemu-iotests/049 +1 -1

index fff0760..7da4ac8 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,qcow-key-secret=sec0 "$TEST_IMG" 64M

s/qcow-key-secret/aes-key-secret/

This converts the qcow2 driver to make use of the QCryptoBlock APIs for encrypting image content, using the legacyy QCow2 AES scheme.

With this change it is now required to use the QCryptoSecret object for providing passwords, instead of the current block password APIs / interactive prompting.

  $QEMU \
    -object secret,id=sec0,filename=/home/berrange/encrypted.pw \
    -drive file=/home/berrange/encrypted.qcow2,aes-key-secret=sec0

Signed-off-by Daniel P. Berrange <[email protected]>

---
 block/qcow2-cluster.c      |  47 +----------
 block/qcow2.c              | 190 +++++++++++++++++++++++++++++----------------
 block/qcow2.h              |   5 +-
 qapi/block-core.json       |   7 +-
 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 |   6 +-
 tests/qemu-iotests/134     |  18 +++--
 tests/qemu-iotests/134.out |  10 +--
 tests/qemu-iotests/158     |  19 +++--
 tests/qemu-iotests/158.out |  14 +---
 13 files changed, 219 insertions(+), 158 deletions(-)

tests/qemu-iotests/134 +0 -0

index af618b8..c2458d8 100755
@@ -43,23 +43,31 @@ _supported_os Linux
 
 
 size=128M

Quoted 28 lines · Daniel P. Berrange

This converts the qcow2 driver to make use of the QCryptoBlock APIs for encrypting image content, using the legacyy QCow2 AES scheme.

With this change it is now required to use the QCryptoSecret object for providing passwords, instead of the current block password APIs / interactive prompting.

  $QEMU \
    -object secret,id=sec0,filename=/home/berrange/encrypted.pw \
    -drive file=/home/berrange/encrypted.qcow2,aes-key-secret=sec0

Signed-off-by Daniel P. Berrange <[email protected]>

---
 block/qcow2-cluster.c      |  47 +----------
 block/qcow2.c              | 190 +++++++++++++++++++++++++++++----------------
 block/qcow2.h              |   5 +-
 qapi/block-core.json       |   7 +-
 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 |   6 +-
 tests/qemu-iotests/134     |  18 +++--
 tests/qemu-iotests/134.out |  10 +--
 tests/qemu-iotests/158     |  19 +++--
 tests/qemu-iotests/158.out |  14 +---
 13 files changed, 219 insertions(+), 158 deletions(-)

[...]

Quoted 8 lines

tests/qemu-iotests/134 +0 -0

index af618b8..c2458d8 100755
@@ -43,23 +43,31 @@ _supported_os Linux
 
 
 size=128M

tests/qemu-iotests/134 _supported_os Linux · line 54

Quoted 10 lines

tests/qemu-iotests/134 +9 -1

-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,qcow-key-secret=sec0" $size
+
+IMGSPEC="driver=$IMGFMT,file.filename=$TEST_IMG,qcow-key-secret=sec0"
+
+QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT

While I agree that it makes sense to have this variable, we unfortunately do not have it. Yet. ;-)

It should be defined somewhere and it should probably actually contain all non-format options (such as the cache mode).

This adds support for using LUKS as an encryption format with the qcow2 file. The use of the existing 'encryption=on' parameter is replaced by a new parameter 'encryption-format' which takes the values 'aes' or 'luks'. e.g.

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

results in the creation of an image using the LUKS format. Use of the legacy 'encryption=on' parameter still results in creation of the old qcow2 AES format, and is equivalent to the new 'encryption-format=aes'. e.g. the following are equivalent:

  # qemu-img create --object secret,data=123456,id=sec0 \
       -f qcow2 -o encryption=on,aes-key-secret=sec0 \
       test.qcow2 10G

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

Signed-off-by Daniel P. Berrange <[email protected]>

---
 block/qcow2-cluster.c      |   4 +-
 block/qcow2-refcount.c     |  10 ++
 block/qcow2.c              | 236 ++++++++++++++++++++++++++++++++++++++-------
 block/qcow2.h              |   9 ++
 docs/specs/qcow2.txt       |  99 +++++++++++++++++++

 qapi/block-core.json       |   6 +-
 tests/qemu-iotests/049     |   2 +-
 tests/qemu-iotests/082.out | 216 +++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/087     |  65 ++++++++++++-
 tests/qemu-iotests/087.out |  22 ++++-
 tests/qemu-iotests/134     |   4 +-
 tests/qemu-iotests/158     |   8 +-
 tests/qemu-iotests/174     |  76 +++++++++++++++
 tests/qemu-iotests/174.out |  19 ++++

 tests/qemu-iotests/175     |  85 ++++++++++++++++
 tests/qemu-iotests/175.out |  26 +++++
 tests/qemu-iotests/group   |   2 +
 17 files changed, 840 insertions(+), 49 deletions(-)
 create mode 100755 tests/qemu-iotests/174
 create mode 100644 tests/qemu-iotests/174.out
 create mode 100755 tests/qemu-iotests/175
 create mode 100644 tests/qemu-iotests/175.out

block/qcow2-cluster.c +3 -1

index a2103dc..866b122 100644
@@ -383,7 +383,9 @@ static int coroutine_fn do_perform_cow(BlockDriverState *bs,
 
     if (bs->encrypted) {
         Error *err = NULL;
-        int64_t sector = (src_cluster_offset + offset_in_cluster)
+        int64_t sector = (s->crypt_physical_offset ?
+                          (cluster_offset + offset_in_cluster) :
+                          (src_cluster_offset + offset_in_cluster))
                          >> BDRV_SECTOR_BITS;
         assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
         assert((bytes & ~BDRV_SECTOR_MASK) == 0);

block/qcow2-refcount.c +0 -0

index cbfb3fe..afa4636 100644

block/qcow2-refcount.c +0 -0

@@ -80,6 +81,73 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)

block/qcow2-refcount.c +7 -0

+static ssize_t qcow2_crypto_hdr_init_func(QCryptoBlock *block, size_t headerlen,
+                                          Error **errp, void *opaque)
+{
+    BlockDriverState *bs = opaque;
+    BDRVQcow2State *s = bs->opaque;
+    int64_t ret;
+

block/qcow2-refcount.c +9 -0

+    return ret;
+}
+
+
+static ssize_t qcow2_crypto_hdr_write_func(QCryptoBlock *block, size_t offset,
+                                           const uint8_t *buf, size_t buflen,
+                                           Error **errp, void *opaque)
+{
+    BlockDriverState *bs = opaque;

block/qcow2-refcount.c +8 -0

+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "Could not read encryption header");
+        return -1;
+    }
+    return ret;
+}
+
+
 /* 
  * read qcow2 extension and fill bs
  * start reading from start_offset

block/qcow2-refcount.c +12 -0

@@ -165,6 +233,45 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
             }
             break;
 
+        case QCOW2_EXT_MAGIC_CRYPTO_HEADER: {
+            unsigned int cflags = 0;
+            if (s->crypt_method_header != QCOW_CRYPT_LUKS) {
+                error_setg(errp, "CRYPTO header extension only "
+                           "expected with LUKS encryption method");
+                return -EINVAL;
+            }
+            if (ext.len != sizeof(Qcow2CryptoHeaderExtension)) {
+                error_setg(errp, "CRYPTO header extension size %u, "
+                           "but expected size %zu", ext.len,
+                           sizeof(Qcow2CryptoHeaderExtension));
+                return -EINVAL;

block/qcow2-refcount.c +0 -0

         default:
             /* unknown magic - save it in case we need to rewrite the header */
             {

block/qcow2-refcount.c +2 -14

@@ -1138,25 +1245,37 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
 
     /* read qcow2 extensions */
     if (qcow2_read_extensions(bs, header.header_length, ext_end, NULL,
-        &local_err)) {
+                              flags, &local_err)) {
         error_propagate(errp, local_err);
         ret = -EINVAL;
         goto fail;
     }
 
-    if (s->crypt_method_header == QCOW_CRYPT_AES) {
-        unsigned int cflags = 0;
-        if (flags & BDRV_O_NO_IO) {
-            cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
-        }
-        /* XXX how do we pass the same crypto opts down to the
-         * backing file by default, so we don't have to manually
-         * provide the same key-secret property against the full
-         * backing chain
-         */
-        s->crypto = qcrypto_block_open(s->crypto_opts, NULL, NULL,
-                                       cflags, errp);
-        if (!s->crypto) {
+    /* qcow2_read_extension may have set up the crypto context

block/qcow2-refcount.c +14 -0

+             * backing file by default, so we don't have to manually
+             * provide the same key-secret property against the full
+             * backing chain
+             */
+            s->crypto = qcrypto_block_open(s->crypto_opts, NULL, NULL,
+                                           cflags, errp);
+            if (!s->crypto) {
+                ret = -EINVAL;
+                goto fail;
+            }
+            s->crypt_physical_offset = false;
+        } else if (!(flags & BDRV_O_NO_IO)) {
+            error_setg(errp, "Missing CRYPTO header for crypt method %d",
+                       s->crypt_method_header);
             ret = -EINVAL;
             goto fail;
         }

block/qcow2-refcount.c +7 -0

+        if (ret < 0) {
+            goto fail;
+        }
+        buf += ret;
+        buflen -= ret;
+    }
+
     /* Feature table */
     if (s->qcow_version >= 3) {
         Qcow2Feature features[] = {

block/qcow2-refcount.c +8 -0

+        },
         BLOCK_CRYPTO_OPT_DEF_QCOW_KEY_SECRET("aes-"),
+        BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET("luks-"),
+        BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_ALG("luks-"),
+        BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_MODE("luks-"),
+        BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_ALG("luks-"),
+        BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_HASH_ALG("luks-"),
+        BLOCK_CRYPTO_OPT_DEF_LUKS_HASH_ALG("luks-"),
+        BLOCK_CRYPTO_OPT_DEF_LUKS_ITER_TIME("luks-"),
         { /* end of list */ }
     }
 };

docs/specs/qcow2.txt +0 -0

index 80cdfd0..cc90276 100644

docs/specs/qcow2.txt +4 -0

@@ -207,6 +209,103 @@ 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

docs/specs/qcow2.txt +29 -0

+                    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. Its value is not
+used, 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                |

docs/specs/qcow2.txt +21 -0

+  .                             .
+  .                             .
+  +-----------------------------+
+  | +-------------------------+ |
+  | | LUKS partition header   | |
+  | +-------------------------+ |
+  | | LUKS key material 1     | |
+  | +-------------------------+ |
+  | | LUKS key material 2     | |
+  | +-------------------------+ |
+  | | LUKS key material ...   | |
+  | +-------------------------+ |
+  | | LUKS key material 8     | |
+  | +-------------------------+ |
+  +-----------------------------+
+  | QCow2 cluster payload       |
+  .                             .
+  .                             .
+  .                             .
+  |                             |
+  +-----------------------------+

qapi/block-core.json +1 -0

+#                         except when doing a metadata-only probe of the image.
 #
 # Since: 1.7
 ##

tests/qemu-iotests/087 +0 -0

index fe30383..6690715 100755

tests/qemu-iotests/087 +27 -1

@@ -169,7 +169,62 @@ run_qemu <<EOF
           "driver": "file",
           "filename": "$TEST_IMG"
       },
-      "qcow-key-secret": "sec0"
+      "aes-key-secret": "sec0"
+    }
+  }
+{ "execute": "quit" }
+EOF
+
+echo
+echo === Encrypted image LUKS ===
+echo
+
+_make_test_img --object secret,id=sec0,data=123456 -o encryption-format=luks,luks-key-secret=sec0 $size
+run_qemu -S <<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": {

tests/qemu-iotests/087 +19 -0

+{ "execute": "qmp_capabilities" }
+{ "execute": "object-add",
+  "arguments": {
+      "qom-type": "secret",
+      "id": "sec0",
+      "props": {
+          "data": "123456"
+      }
+  }
+}
+{ "execute": "blockdev-add",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "node-name": "disk",
+      "file": {
+          "driver": "file",
+          "filename": "$TEST_IMG"
+      },
+      "luks-key-secret": "sec0"
     }
   }
 { "execute": "quit" }

tests/qemu-iotests/174 new file +43 -0

+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
[email protected]
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1	# failure is the default!
+
+_cleanup()
+{
+	_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto generic
+_supported_os Linux
+
+
+size=128M
+
+SECRET="secret,id=sec0,data=astrochicken"
+SECRETALT="secret,id=sec0,data=platypus"

tests/qemu-iotests/174 new file +10 -0

+
+echo
+echo "== verify pattern failure with wrong password =="
+$QEMU_IO --object $SECRETALT -c "read -P 0xa 0 $size" --image-opts $IMGSPEC | _filter_qemu_io | _filter_testdir
+
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0

tests/qemu-iotests/174.out new file +7 -0

new file mode 100644
index 0000000..c96d0e4
@@ -0,0 +1,19 @@
+QA output created by 174
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption-format=luks luks-key-secret=sec0
+
+== reading whole image ==
+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/175 new file +30 -0

+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
[email protected]
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1	# failure is the default!
+
+_cleanup()
+{
+	_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+

Quoted 43 lines · Daniel P. Berrange

This adds support for using LUKS as an encryption format with the qcow2 file. The use of the existing 'encryption=on' parameter is replaced by a new parameter 'encryption-format' which takes the values 'aes' or 'luks'. e.g.

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

results in the creation of an image using the LUKS format. Use of the legacy 'encryption=on' parameter still results in creation of the old qcow2 AES format, and is equivalent to the new 'encryption-format=aes'. e.g. the following are equivalent:

  # qemu-img create --object secret,data=123456,id=sec0 \
       -f qcow2 -o encryption=on,aes-key-secret=sec0 \
       test.qcow2 10G

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

Quoted 7 lines

Signed-off-by Daniel P. Berrange <[email protected]>

---
 block/qcow2-cluster.c      |   4 +-
 block/qcow2-refcount.c     |  10 ++
 block/qcow2.c              | 236 ++++++++++++++++++++++++++++++++++++++-------
 block/qcow2.h              |   9 ++
 docs/specs/qcow2.txt       |  99 +++++++++++++++++++

I'd personally rather have specification changes separate from the implementation.

Quoted 9 lines

 qapi/block-core.json       |   6 +-
 tests/qemu-iotests/049     |   2 +-
 tests/qemu-iotests/082.out | 216 +++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/087     |  65 ++++++++++++-
 tests/qemu-iotests/087.out |  22 ++++-
 tests/qemu-iotests/134     |   4 +-
 tests/qemu-iotests/158     |   8 +-
 tests/qemu-iotests/174     |  76 +++++++++++++++
 tests/qemu-iotests/174.out |  19 ++++

Quoted 8 lines

 tests/qemu-iotests/175     |  85 ++++++++++++++++
 tests/qemu-iotests/175.out |  26 +++++
 tests/qemu-iotests/group   |   2 +
 17 files changed, 840 insertions(+), 49 deletions(-)
 create mode 100755 tests/qemu-iotests/174
 create mode 100644 tests/qemu-iotests/174.out
 create mode 100755 tests/qemu-iotests/175
 create mode 100644 tests/qemu-iotests/175.out

Also, in order to help reviewing by making this patch less scary (840 insertions are kind of... Urgh.), it would make sense to add these two tests in one or two separate patches.

Quoted 1 line

(quoted diff) +0 -0

@@ -80,6 +81,73 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)

[...]

Quoted 7 lines

block/qcow2.c +7 -0

+static ssize_t qcow2_crypto_hdr_init_func(QCryptoBlock *block, size_t headerlen,
+                                          Error **errp, void *opaque)
+{
+    BlockDriverState *bs = opaque;
+    BDRVQcow2State *s = bs->opaque;
+    int64_t ret;
+

block/qcow2.c static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename) · line 123

Quoted 10 lines

block/qcow2.c +10 -0

+    ret = qcow2_alloc_clusters(bs, headerlen);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret,
+                         "Cannot allocate cluster for LUKS header size %zu",
+                         headerlen);
+        return -1;
+    }
+
+    s->crypto_header.length = headerlen;
+    s->crypto_header.offset = ret;

The specification says any unused space in the last cluster has to be set to zero. This isn't done here.

I don't have a preference whether you write zeroes to the range in question here or whether you simply relax the specification to say that anything beyond crypto_header.length is undefined.

Quoted 5 lines

block/qcow2.c +5 -0

+static ssize_t qcow2_crypto_hdr_write_func(QCryptoBlock *block, size_t offset,
+                                           const uint8_t *buf, size_t buflen,
+                                           Error **errp, void *opaque)
+{
+    BlockDriverState *bs = opaque;

block/qcow2.c static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename) · line 142

Quoted 10 lines

block/qcow2.c +10 -0

+    BDRVQcow2State *s = bs->opaque;
+    ssize_t ret;
+
+    if ((offset + buflen) > s->crypto_header.length) {
+        error_setg(errp, "Request for data outside of extension header");
+        return -1;
+    }
+
+    ret = bdrv_pwrite(bs->file,
+                      s->crypto_header.offset + offset, buf, buflen);

Theoretically, there should be a qcow2_pre_write_overlap_check() here. But in theory, qcow2_check_metadata_overlap() should also check overlaps with this new block of metadata.

I'll leave it up to you as the discussion about the future of those overlap checks is still up in the air. I really don't have a preference on what to do in this patch.

Quoted 20 lines

block/qcow2.c +16 -0

@@ -165,6 +233,45 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
             }
             break;
 
+        case QCOW2_EXT_MAGIC_CRYPTO_HEADER: {
+            unsigned int cflags = 0;
+            if (s->crypt_method_header != QCOW_CRYPT_LUKS) {
+                error_setg(errp, "CRYPTO header extension only "
+                           "expected with LUKS encryption method");
+                return -EINVAL;
+            }
+            if (ext.len != sizeof(Qcow2CryptoHeaderExtension)) {
+                error_setg(errp, "CRYPTO header extension size %u, "
+                           "but expected size %zu", ext.len,
+                           sizeof(Qcow2CryptoHeaderExtension));
+                return -EINVAL;
+            }
+
+            ret = bdrv_pread(bs->file, offset, &s->crypto_header, ext.len);
+            if (ret < 0) {

block/qcow2.c static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset, · line 257

Quoted 6 lines

block/qcow2.c +6 -0

+                error_setg_errno(errp, -ret,
+                                 "Unable to read CRYPTO header extension");
+                return ret;
+            }
+            be64_to_cpu(s->crypto_header.offset);
+            be64_to_cpu(s->crypto_header.length);

Shouldn't you use the result somehow (or use be64_to_cpus)...?

Also, you should check the validity of s->crypto_header.offset here, i.e. whether it is indeed aligned to a cluster boundary.

block/qcow2.c static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset, · line 262

Quoted 6 lines

block/qcow2.c +5 -0

+
+            if (flags & BDRV_O_NO_IO) {
+                cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
+            }
+            /* XXX how do we pass the same crypto opts down to the

Just as in the previous patch, a TODO would probably be sufficient here.

block/qcow2.c static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset, · line 273

Quoted 11 lines

block/qcow2.c +11 -0

+             * backing file by default, so we don't have to manually
+             * provide the same key-secret property against the full
+             * backing chain
+             */
+            s->crypto = qcrypto_block_open(s->crypto_opts,
+                                           qcow2_crypto_hdr_read_func,
+                                           bs, cflags, errp);
+            if (!s->crypto) {
+                return -EINVAL;
+            }
+            s->crypt_physical_offset = true;

I think this should be set depending on the crypt_method_header (i.e. in qcow2_open()), not depending on whether this extension exists or not.

Quoted 4 lines

block/qcow2.c +0 -0

@@ -988,12 +1101,6 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     s->refcount_max = UINT64_C(1) << (s->refcount_bits - 1);
     s->refcount_max += s->refcount_max - 1;
 

block/qcow2.c static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, · line 1106

Quoted 9 lines

block/qcow2.c +0 -6

-    if (header.crypt_method > QCOW_CRYPT_AES) {
-        error_setg(errp, "Unsupported encryption method: %" PRIu32,
-                   header.crypt_method);
-        ret = -EINVAL;
-        goto fail;
-    }
     s->crypt_method_header = header.crypt_method;
     if (s->crypt_method_header) {
         if (bdrv_uses_whitelist() &&

You could put the assignment of crypt_physical_offset into this block (at its bottom where bs->encrypted is set).

block/qcow2.c int qcow2_update_header(BlockDriverState *bs) · line 2059

Quoted 8 lines

block/qcow2.c +4 -0

@@ -1929,6 +2053,22 @@ int qcow2_update_header(BlockDriverState *bs)
         buflen -= ret;
     }
 
+    /* Full disk encryption header pointer extension */
+    if (s->crypto_header.offset != 0) {
+        cpu_to_be64(s->crypto_header.offset);
+        cpu_to_be64(s->crypto_header.length);

Should be cpu_to_be64s() or you should store the result somewhere.

block/qcow2.c int qcow2_update_header(BlockDriverState *bs) · line 2064

Quoted 5 lines

block/qcow2.c +5 -0

+        ret = header_ext_add(buf, QCOW2_EXT_MAGIC_CRYPTO_HEADER,
+                             &s->crypto_header, sizeof(s->crypto_header),
+                             buflen);
+        be64_to_cpu(s->crypto_header.offset);
+        be64_to_cpu(s->crypto_header.length);

The same with be64_to_cpus().

Quoted 4 lines

block/qcow2.c +0 -0

@@ -3426,7 +3582,19 @@ static QemuOptsList qcow2_create_opts = {
             .help = "Width of a reference count entry in bits",
             .def_value_str = "16"
         },

block/qcow2.c static QemuOptsList qcow2_create_opts = { · line 3588

Quoted 4 lines

block/qcow2.c +4 -0

+        {
+            .name = QCOW2_OPT_ENCRYPTION_FORMAT,
+            .type = QEMU_OPT_STRING,
+            .help = "Encryption data format 'luks' (default) or 'aes'",

In what way is LUKS the default? No encryption is the default; and the default encryption is the old AES-CBC because that is what you get when specifying encryption=on.

I would agree if you replaced "default" by "recommended". In addition, the help text for the "encryption" option should probably now simply say:

"Deprecated, use the " QCOW2_OPT_ENCRYPTION_FORMAT " option instead"

Quoted 4 lines

tests/qemu-iotests/087 +0 -0

index fe30383..6690715 100755

[...]

Quoted 40 lines

tests/qemu-iotests/087 +35 -1

@@ -169,7 +169,62 @@ run_qemu <<EOF
           "driver": "file",
           "filename": "$TEST_IMG"
       },
-      "qcow-key-secret": "sec0"
+      "aes-key-secret": "sec0"
+    }
+  }
+{ "execute": "quit" }
+EOF
+
+echo
+echo === Encrypted image LUKS ===
+echo
+
+_make_test_img --object secret,id=sec0,data=123456 -o encryption-format=luks,luks-key-secret=sec0 $size
+run_qemu -S <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "object-add",
+  "arguments": {
+      "qom-type": "secret",
+      "id": "sec0",
+      "props": {
+          "data": "123456"
+      }
+  }
+}
+{ "execute": "blockdev-add",
+  "arguments": {
+      "driver": "$IMGFMT",
+      "node-name": "disk",
+      "file": {
+          "driver": "file",
+          "filename": "$TEST_IMG"
+      },
+      "luks-key-secret": "sec0"
+    }
+  }
+{ "execute": "quit" }
+EOF

tests/qemu-iotests/087 run_qemu <<EOF · line 208

Quoted 2 lines

tests/qemu-iotests/087 +2 -0

+
+run_qemu <<EOF

I think we can drop one of the duplicated test cases with and without -S now. The reason for having two originally was, as far as I can remember, to test that you could blockdev-add encrypted images when the VM was not paused. However, that is no longer the case since we are no longer relying on that old infrastructure (as of this series at least).

Quoted 19 lines

tests/qemu-iotests/174 new file +13 -0

new file mode 100755
index 0000000..27d4870
 
+_supported_fmt qcow2
+_supported_proto generic
+_supported_os Linux
+
+
+size=128M
+
+SECRET="secret,id=sec0,data=astrochicken"
+SECRETALT="secret,id=sec0,data=platypus"
+
+_make_test_img --object $SECRET -o "encryption-format=luks,luks-key-secret=sec0" $size
+
+IMGSPEC="driver=$IMGFMT,file.filename=$TEST_IMG,luks-key-secret=sec0"

tests/qemu-iotests/174 line 58

Quoted 6 lines

tests/qemu-iotests/174 new file +6 -0

+
+QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT
+
+echo
+echo "== reading whole image =="
+$QEMU_IO --object $SECRET -c "read 0 $size" --image-opts $IMGSPEC | _filter_qemu_io | _filter_testdir

Shouldn't "read -P 0 0 $size" work here, too?

tests/qemu-iotests/174 line 66

Quoted 7 lines

tests/qemu-iotests/174 new file +7 -0

+echo
+echo "== rewriting whole image =="
+$QEMU_IO --object $SECRET -c "write -P 0xa 0 $size" --image-opts $IMGSPEC | _filter_qemu_io | _filter_testdir
+
+echo
+echo "== verify pattern =="
+$QEMU_IO --object $SECRET -c "read -P 0xa 0 $size"  --image-opts $IMGSPEC | _filter_qemu_io | _filter_testdir

But do you really want to read and write 128 MB...? I mean, even on an HDD, it will only take a couple of seconds at most, but won't 16 or 32 MB suffice? That way, it should always take less than a second.

Quoted 2 lines

+== verify pattern failure with wrong password == +can't open: Invalid password, cannot unlock any keyslot

Well, that's not quite a pattern failure. It's the result we want, but you should probably change the heading for this test case.

tests/qemu-iotests/174 line 45

Quoted 10 lines

tests/qemu-iotests/174 new file +10 -0

+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto generic
+_supported_os Linux
+
+
+size=128M

Same as in last test, I'm not sure 128 MB are actually necessary.

Quoted 1 line

+TEST_IMG_BASE=$TEST_IMG.base

Hmmm, does this work with spaces in $TEST_IMG?

Quoted 1 line

+SECRET="secret,id=sec0,data=astrochicken"

How about using different secrets for the base and the overlay?

Quoted 1 line · Max Reitz

On 03.01.2017 19:27, Daniel P. Berrange wrote:

Quoted 18 lines

Quoted 18 lines

tests/qemu-iotests/174 new file +13 -0

new file mode 100755
index 0000000..27d4870
+_supported_fmt qcow2
+_supported_proto generic
+_supported_os Linux
+
+
+size=128M
+
+SECRET="secret,id=sec0,data=astrochicken"
+SECRETALT="secret,id=sec0,data=platypus"
+
+_make_test_img --object $SECRET -o "encryption-format=luks,luks-key-secret=sec0" $size
+
+IMGSPEC="driver=$IMGFMT,file.filename=$TEST_IMG,luks-key-secret=sec0"

Quoted 8 lines

tests/qemu-iotests/174 line 58

Quoted 6 lines

tests/qemu-iotests/174 new file +6 -0

+
+QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT
+
+echo
+echo "== reading whole image =="
+$QEMU_IO --object $SECRET -c "read 0 $size" --image-opts $IMGSPEC | _filter_qemu_io | _filter_testdir

Shouldn't "read -P 0 0 $size" work here, too?

The underlying disk image contents will be zeros, but we'll then decrypt those zeros and get random garbage.

We could only use -P 0 if we explicitly fill with encrypted-zeros.