Support LUKS encryption in block devices

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

---
 crypto/Makefile.objs    |  1 +
 crypto/random.c         | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/crypto/random.h | 43 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 94 insertions(+)
 create mode 100644 crypto/random.c
 create mode 100644 include/crypto/random.h

crypto/Makefile.objs +1 -0

index a3135f1..5f38d2d 100644
@@ -8,6 +8,7 @@ crypto-obj-y += tlscredsanon.o
 crypto-obj-y += tlscredsx509.o
 crypto-obj-y += tlssession.o
 crypto-obj-y += secret.o
+crypto-obj-y += random.o
 
 # Let the userspace emulators avoid linking gnutls/etc
 crypto-aes-obj-y = aes.o

crypto/random.c new file +0 -0

new file mode 100644
index 0000000..8257d24

crypto/random.c new file +46 -0

+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library 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
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include <config-host.h>
+
+#include "crypto/random.h"
+
+int qcrypto_random_bytes(uint8_t *buf,
+                         size_t buflen,
+                         Error **errp)
+{
+    ssize_t ret;
+    int fd = open("/dev/random", O_RDONLY);
+    if (fd < 0) {
+        error_setg_errno(errp, errno,
+                         "Unable to open /dev/random");
+        return -1;
+    }
+
+    while (buflen) {
+        ret = read(fd, buf, buflen);
+        if (ret < 0) {
+            error_setg_errno(errp, errno,
+                             "Unable to read random bytes");
+            close(fd);
+            return -1;
+        }
+        buflen -= ret;
+    }
+
+    close(fd);
+    return 0;
+}

include/crypto/random.h new file +0 -0

new file mode 100644
index 0000000..ce1626b

include/crypto/random.h new file +18 -0

+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library 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
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#ifndef QCRYPTO_RANDOM_H__
+#define QCRYPTO_RANDOM_H__

include/crypto/random.h new file +11 -0

+ *
+ * Fill @buf with @buflen bytes of random data
+ *
+ * Returns 0 on sucess, -1 on error
+ */
+int qcrypto_random_bytes(uint8_t *buf,
+                         size_t buflen,
+                         Error **errp);
+
+
+#endif /* QCRYPTO_RANDOM_H__ */

Signature · 1 line

2.5.0

crypto/pbkdf.c line 30

Quoted 7 lines · Daniel P. Berrange

crypto/pbkdf.c new file +7 -0

+#if defined CONFIG_NETTLE
+#include "crypto/pbkdf-nettle.c"
+#elif defined CONFIG_GCRYPT
+#include "crypto/pbkdf-gcrypt.c"
+#else /* ! CONFIG_GCRYPT */
+#include "crypto/pbkdf-stub.c"
+#endif /* ! CONFIG_GCRYPT */

I think the convention in QEMU is in crypto/Makefile.objs:

    crypto-obj-$(CONFIG_NETTLE) += pbkdf-nettle.o
    crypto-obj-$(if $(CONFIG_NETTLE),n,$(CONFIG_GCRYPT)) += pbkdf-gcrypt.o

And move pbkdf-stub.c to stub/pbkdf.c.

Add a generic framework for support different block encryption formats. Upon instantiating a QCryptoBlock object, it will read the encryption header and extract the encryption keys. It is then possible to call methods to encrypt/decrypt data buffers.

There is also a mode whereby it will create/initialize a new encryption header on a previously unformatted volume.

The initial framework comes with support for the legacy QCow AES based encryption. This enables code in the QCow driver to be consolidated later.

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

---

+++ b/qapi/crypto.json

qapi/crypto.json +0 -0

@@ -94,3 +94,68 @@
 { 'enum': 'QCryptoIVGenAlgorithm',

Quoted 24 lines · Daniel P. Berrange

Add a generic framework for support different block encryption formats. Upon instantiating a QCryptoBlock object, it will read the encryption header and extract the encryption keys. It is then possible to call methods to encrypt/decrypt data buffers.

There is also a mode whereby it will create/initialize a new encryption header on a previously unformatted volume.

The initial framework comes with support for the legacy QCow AES based encryption. This enables code in the QCow driver to be consolidated later.

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

---

+++ b/qapi/crypto.json

qapi/crypto.json +4 -0

@@ -94,3 +94,68 @@
 { 'enum': 'QCryptoIVGenAlgorithm',
   'prefix': 'QCRYPTO_IVGEN_ALG',
   'data': ['plain', 'plain64', 'essiv']}
+
+##
+# QCryptoBlockFormat:
+#

qapi/crypto.json line 104

Quoted 4 lines

qapi/crypto.json +4 -0

+# The supported full disk encryption formats
+#
+# @qcowaes: QCow/QCow2 built-in AES-CBC encryption. Do not use
+#

Well, the only reason to use it would be to read data off an old insecurely-encrypted qcow2 file; so maybe it should read "Do not use on new files"

qapi/crypto.json line 109

Quoted 5 lines

qapi/crypto.json +5 -0

+# Since: 2.6
+##
+{ 'enum': 'QCryptoBlockFormat',
+#  'prefix': 'QCRYPTO_BLOCK_FORMAT',
+  'data': ['qcowaes']}

Would 'qcow-aes' be any easier to read?

Quoted 6 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+# @qcowaes: QCow/QCow2 built-in AES-CBC encryption. Do not use
+#

Well, the only reason to use it would be to read data off an old insecurely-encrypted qcow2 file; so maybe it should read "Do not use on new files"

Yep

Quoted 7 lines

qapi/crypto.json line 109

Quoted 5 lines

qapi/crypto.json +5 -0

+# Since: 2.6
+##
+{ 'enum': 'QCryptoBlockFormat',
+#  'prefix': 'QCRYPTO_BLOCK_FORMAT',
+  'data': ['qcowaes']}

Would 'qcow-aes' be any easier to read?

Or just shorten to 'qcow' perhaps ?

Quoted 17 lines · Daniel P. Berrange

Quoted 6 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+# @qcowaes: QCow/QCow2 built-in AES-CBC encryption. Do not use
+#

Well, the only reason to use it would be to read data off an old insecurely-encrypted qcow2 file; so maybe it should read "Do not use on new files"

Yep

Quoted 7 lines

qapi/crypto.json line 109

Quoted 5 lines

qapi/crypto.json +5 -0

+# Since: 2.6
+##
+{ 'enum': 'QCryptoBlockFormat',
+#  'prefix': 'QCRYPTO_BLOCK_FORMAT',
+  'data': ['qcowaes']}

Would 'qcow-aes' be any easier to read?

Quoted 1 line

Or just shorten to 'qcow' perhaps ?

Or maybe 'old-qcow' to emphasize that it is old? At this point, we're painting the bikeshed, so I'll live with whatever you like.

Provide a block encryption implementation that follows the LUKS/dm-crypt specification.

This supports all combinations of hash, cipher algorithm, cipher mode and iv generator that are implemented by the current crypto layer.

The notable missing feature is support for the 'xts' cipher mode, which is commonly used for disk encryption instead of 'cbc'. This is because it is not provided by either nettle or libgcrypt. A suitable implementation will be identified & integrated later.

There is support for opening existing volumes formatted by dm-crypt, and for formatting new volumes. In the latter case it will only use key slot 0.

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

---

+++ b/qapi/crypto.json

qapi/crypto.json +2 -1

@@ -101,12 +101,13 @@
 # The supported full disk encryption formats
 #
 # @qcowaes: QCow/QCow2 built-in AES-CBC encryption. Do not use
+# @luks: LUKS encryption format. Recommended
 #
 # Since: 2.6
 ##
 { 'enum': 'QCryptoBlockFormat',
 #  'prefix': 'QCRYPTO_BLOCK_FORMAT',
-  'data': ['qcowaes']}
+  'data': ['qcowaes', 'luks']}
 
 ##
 # QCryptoBlockOptionsBase:

Quoted 39 lines · Daniel P. Berrange

Provide a block encryption implementation that follows the LUKS/dm-crypt specification.

This supports all combinations of hash, cipher algorithm, cipher mode and iv generator that are implemented by the current crypto layer.

The notable missing feature is support for the 'xts' cipher mode, which is commonly used for disk encryption instead of 'cbc'. This is because it is not provided by either nettle or libgcrypt. A suitable implementation will be identified & integrated later.

There is support for opening existing volumes formatted by dm-crypt, and for formatting new volumes. In the latter case it will only use key slot 0.

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

---

+++ b/qapi/crypto.json

qapi/crypto.json +2 -1

@@ -101,12 +101,13 @@
 # The supported full disk encryption formats
 #
 # @qcowaes: QCow/QCow2 built-in AES-CBC encryption. Do not use
+# @luks: LUKS encryption format. Recommended
 #
 # Since: 2.6
 ##
 { 'enum': 'QCryptoBlockFormat',
 #  'prefix': 'QCRYPTO_BLOCK_FORMAT',
-  'data': ['qcowaes']}
+  'data': ['qcowaes', 'luks']}
 
 ##
 # QCryptoBlockOptionsBase:

@@ -134,6 +135,39 @@
   'data': { '*key-id': 'str' }}
 

qapi/crypto.json line 142

Quoted 6 lines

qapi/crypto.json +5 -0

 ##
+# QCryptoBlockOptionsLUKS:
+#
+# The options that apply to LUKS encryption format
+#
+# @key-id: the ID of a QCryptoSecret object providing the decryption key

Is the key-id really optional? If so, missing the '#optional' tag.

When opening an image it is useful to know whether the caller intends to perform I/O on the image or not. In the case of encrypted images this will allow the block driver to avoid having to prompt for decryption keys when we merely want to query header metadata about the image. eg qemu-img info

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

Am 12.01.2016 um 19:56 hat Daniel P. Berrange geschrieben:

Quoted 6 lines

When opening an image it is useful to know whether the caller intends to perform I/O on the image or not. In the case of encrypted images this will allow the block driver to avoid having to prompt for decryption keys when we merely want to query header metadata about the image. eg qemu-img info

Quoted 1 line

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

Would it be worth adding assertions to the block layer I/O functions (most importantly, bdrv_aligned_preadv/pwritev) that this flag isn't set?

Add a block driver that is capable of supporting any full disk encryption format. This utilizes the previously added block encryption code, and at this time supports the LUKS format.

The driver code is capable of supporting any format supported by the QCryptoBlock module, so it registers one block driver for each format.

At this time, the "luks" driver is registered. New LUKS compatible volume can be formatted using qemu-img

$ qemu-img create --object secret,data=123456,id=sec0 \
      -f luks -o key-id=sec0,cipher-alg=aes-256,\
          cipher-mode=cbc,ivgen-alg=plain64,hash-alg=sha256 \
      demo.luks 10G

qapi/block-core.json +2 -0

@@ -1664,6 +1664,21 @@
   'data': { 'file': 'BlockdevRef' } }
 
 ##
+# @BlockdevOptionsLUKS
+#

Quoted 20 lines · Daniel P. Berrange

Add a block driver that is capable of supporting any full disk encryption format. This utilizes the previously added block encryption code, and at this time supports the LUKS format.

The driver code is capable of supporting any format supported by the QCryptoBlock module, so it registers one block driver for each format.

At this time, the "luks" driver is registered. New LUKS compatible volume can be formatted using qemu-img

$ qemu-img create --object secret,data=123456,id=sec0 \
      -f luks -o key-id=sec0,cipher-alg=aes-256,\
          cipher-mode=cbc,ivgen-alg=plain64,hash-alg=sha256 \
      demo.luks 10G

qapi/block-core.json +0 -0

@@ -1664,6 +1664,21 @@
   'data': { 'file': 'BlockdevRef' } }
 

qapi/block-core.json line 1678

Quoted 13 lines

qapi/block-core.json +12 -0

 ##
+# @BlockdevOptionsLUKS
+#
+# Driver specific block device options for LUKS.
+#
+# @key-id:  #optional the ID of a QCryptoSecret object providing
+#           the decryption key (since 2.6)
+#
+# Since: 2.6
+##
+{ 'struct': 'BlockdevOptionsLUKS',
+  'base': 'BlockdevOptionsGenericFormat',
+  'data': { '*key-id': 'str' } }

And if key-id is omitted, how does it work?

This converts the qcow2 driver to make use of the QCryptoBlock APIs for encrypting image content. As well as continued support for the legacy QCow2 encryption format, the appealing benefit is that it enables support for the LUKS format inside qcow2.

With the LUKS format it is neccessary to store the LUKS partition header and key material in the QCow2 file. This data can be many MB in size, so cannot go into the QCow2 header region directly. Thus the spec is defines a FDE (Full Disk Encryption) header extension that specifies the offset of a set of clusters to hold the FDE headers, as well as the length of that region. The LUKS header is thus stored in these extra allocated clusters before the main image payload.

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

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

The new LUKS format is set as the new default format when creating encrypted images. ie

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

Results in creation of an image using the LUKS format.

For compatibility the old qcow2 AES format can still be used via the 'encryption-format' parameter which accepts the values 'luks' or 'qcowaes'.

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

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

block/qcow2.c +2 -0

index c0fc259..288aada 100644
@@ -34,6 +34,8 @@
 #include "qapi-event.h"
 #include "trace.h"
 #include "qemu/option_int.h"
+#include "qapi/opts-visitor.h"
+#include "qapi-visit.h"
 
 /*
   Differences with QCOW:

block/qcow2.c +4 -0

 static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
 {

@@ -74,6 +77,83 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
 }
 
 
+static ssize_t qcow2_fde_header_read_func(QCryptoBlock *block,
+                                          size_t offset,
+                                          uint8_t *buf,
+                                          size_t buflen,

block/qcow2.c +9 -0

+}
+
+
+static ssize_t qcow2_fde_header_init_func(QCryptoBlock *block,
+                                          size_t headerlen,
+                                          Error **errp,
+                                          void *opaque)
+{
+    BlockDriverState *bs = opaque;

block/qcow2.c +12 -0

+        return -1;
+    }
+
+    s->fde_header.offset = ret;
+    return 0;
+}
+
+
+static ssize_t qcow2_fde_header_write_func(QCryptoBlock *block,
+                                           size_t offset,
+                                           const uint8_t *buf,
+                                           size_t buflen,

block/qcow2.c +0 -0

 /* 
  * read qcow2 extension and fill bs
  * start reading from start_offset

@@ -83,12 +163,14 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
  */

block/qcow2.c +4 -0

 #ifdef DEBUG_EXT
     printf("qcow2_read_extensions: start=%ld end=%ld\n", start_offset, end_offset);

@@ -159,6 +241,35 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
             }
             break;
 
+        case QCOW2_EXT_MAGIC_FDE_HEADER:
+            if (s->crypt_method_header != QCOW_CRYPT_LUKS) {
+                error_setg(errp, "FDE header extension only "
+                           "expected with LUKS encryption method");

block/qcow2.c +150 -13

+            if (!s->fde) {
+                return -EINVAL;
+            }
+            break;
         default:
             /* unknown magic - save it in case we need to rewrite the header */
             {

@@ -472,6 +583,11 @@ static QemuOptsList qcow2_runtime_opts = {
             .type = QEMU_OPT_NUMBER,
             .help = "Clean unused cache entries after this time (in seconds)",
         },
+        {
+            .name = QCOW2_OPT_KEY_ID,
+            .type = QEMU_OPT_STRING,
+            .help = "ID of the secret that provides the encryption key",
+        },
         { /* end of list */ }
     },
 };

@@ -589,6 +705,113 @@ static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
     }
 }
 
+
+static QCryptoBlockOpenOptions *
+qcow2_fde_open_opts_init(QCryptoBlockFormat format,
+                         QemuOpts *opts,
+                         Error **errp)
+{
+    OptsVisitor *ov;
+    QCryptoBlockOpenOptions *ret;
+    Error *local_err = NULL;
+
+    ret = g_new0(QCryptoBlockOpenOptions, 1);
+    ret->format = format;
+
+    ov = opts_visitor_new(opts);
+
+    switch (format) {
+    case Q_CRYPTO_BLOCK_FORMAT_QCOWAES:
+        ret->u.qcowaes = g_new0(QCryptoBlockOptionsQCowAES, 1);
+        visit_type_QCryptoBlockOptionsQCowAES(opts_get_visitor(ov),
+                                              &ret->u.qcowaes,
+                                              "qcowaes", &local_err);
+        break;
+
+    case Q_CRYPTO_BLOCK_FORMAT_LUKS:
+        ret->u.luks = g_new0(QCryptoBlockOptionsLUKS, 1);
+        visit_type_QCryptoBlockOptionsLUKS(opts_get_visitor(ov),
+                                           &ret->u.luks,
+                                           "luks", &local_err);
+        break;
+
+    default:
+        error_setg(&local_err, "Unsupported block format %d", format);
+        break;
+    }
+
+    if (local_err) {
+        error_propagate(errp, local_err);
+        opts_visitor_cleanup(ov);
+        qapi_free_QCryptoBlockOpenOptions(ret);
+        return NULL;
+    }
+
+    opts_visitor_cleanup(ov);
+    return ret;
+}
+
+
+static QCryptoBlockCreateOptions *
+qcow2_fde_create_opts_init(QCryptoBlockFormat format,
+                           QemuOpts *opts,
+                           Error **errp)
+{
+    OptsVisitor *ov;
+    QCryptoBlockCreateOptions *ret;
+    Error *local_err = NULL, *end_err = NULL;
+    Visitor *v;
+
+    ret = g_new0(QCryptoBlockCreateOptions, 1);
+    ret->format = format;
+
+    ov = opts_visitor_new(opts);
+    v = opts_get_visitor(ov);
+    visit_start_struct(v, NULL, NULL, NULL, 0, &local_err);
+    if (local_err) {
+        goto cleanup;
+    }
+
+    switch (format) {
+    case Q_CRYPTO_BLOCK_FORMAT_QCOWAES:
+        ret->u.qcowaes = g_new0(QCryptoBlockOptionsQCowAES, 1);
+        visit_type_QCryptoBlockOptionsQCowAES(v, &ret->u.qcowaes,
+                                              "qcowaes", &local_err);
+        break;
+
+    case Q_CRYPTO_BLOCK_FORMAT_LUKS:
+        ret->u.luks = g_new0(QCryptoBlockCreateOptionsLUKS, 1);
+        visit_type_QCryptoBlockCreateOptionsLUKS(v, &ret->u.luks,
+                                                 "luks", &local_err);
+        break;
+
+    default:
+        error_setg(&local_err, "Unsupported block format %d", format);
+        break;
+    }
+
+    visit_end_struct(v, &end_err);
+    if (end_err) {
+        if (!local_err) {
+            error_propagate(&local_err, end_err);
+        } else {
+            error_free(end_err);
+        }
+    }
+
+ cleanup:
+    if (local_err) {
+        error_propagate(errp, local_err);
+        qapi_free_QCryptoBlockCreateOptions(ret);
+        ret = NULL;
+        return NULL;
+    }
+
+    opts_visitor_cleanup(ov);
+    return ret;
+}
+
+
 typedef struct Qcow2ReopenState {
     Qcow2Cache *l2_table_cache;
     Qcow2Cache *refcount_block_cache;

@@ -596,6 +819,7 @@ typedef struct Qcow2ReopenState {
     int overlap_check;
     bool discard_passthrough[QCOW2_DISCARD_MAX];
     uint64_t cache_clean_interval;
+    QCryptoBlockOpenOptions *fde_opts; /* Disk encryption runtime options */
 } Qcow2ReopenState;
 
 static int qcow2_update_options_prepare(BlockDriverState *bs,

@@ -754,6 +978,33 @@ static int qcow2_update_options_prepare(BlockDriverState *bs,
     r->discard_passthrough[QCOW2_DISCARD_OTHER] =
         qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_OTHER, false);
 
+    switch (s->crypt_method_header) {
+    case QCOW_CRYPT_NONE:
+        break;
+
+    case QCOW_CRYPT_AES:
+        r->fde_opts = qcow2_fde_open_opts_init(
+            Q_CRYPTO_BLOCK_FORMAT_QCOWAES,
+            opts,
+            errp);
+        break;
+
+    case QCOW_CRYPT_LUKS:
+        r->fde_opts = qcow2_fde_open_opts_init(
+            Q_CRYPTO_BLOCK_FORMAT_LUKS,
+            opts,
+            errp);
+        break;
+
+    default:
+        g_assert_not_reached();
+    }
+    if (s->crypt_method_header &&
+        !r->fde_opts) {
+        ret = -EINVAL;
+        goto fail;
+    }
+
     ret = 0;
 fail:
     qemu_opts_del(opts);

@@ -788,6 +1039,9 @@ static void qcow2_update_options_commit(BlockDriverState *bs,
         s->cache_clean_interval = r->cache_clean_interval;
         cache_clean_timer_init(bs, bdrv_get_aio_context(bs));
     }
+
+    qapi_free_QCryptoBlockOpenOptions(s->fde_opts);
+    s->fde_opts = r->fde_opts;
 }
 
 static void qcow2_update_options_abort(BlockDriverState *bs,

@@ -799,6 +1053,7 @@ static void qcow2_update_options_abort(BlockDriverState *bs,
     if (r->refcount_block_cache) {
         qcow2_cache_destroy(bs, r->refcount_block_cache);
     }
+    qapi_free_QCryptoBlockOpenOptions(r->fde_opts);
 }
 
 static int qcow2_update_options(BlockDriverState *bs, QDict *options,

@@ -932,7 +1187,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     if (s->incompatible_features & ~QCOW2_INCOMPAT_MASK) {
         void *feature_table = NULL;
         qcow2_read_extensions(bs, header.header_length, ext_end,
-                              &feature_table, NULL);
+                              &feature_table, flags, NULL);
         report_unsupported_feature(bs, errp, feature_table,
                                    s->incompatible_features &
                                    ~QCOW2_INCOMPAT_MASK);

@@ -964,17 +1219,6 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     s->refcount_max = UINT64_C(1) << (s->refcount_bits - 1);
     s->refcount_max += s->refcount_max - 1;
 
-    if (header.crypt_method > QCOW_CRYPT_AES) {
-        error_setg(errp, "Unsupported encryption method: %" PRIu32,
-                   header.crypt_method);
-        ret = -EINVAL;
-        goto fail;
-    }
-    if (!qcrypto_cipher_supports(QCRYPTO_CIPHER_ALG_AES_128)) {
-        error_setg(errp, "AES cipher not available");
-        ret = -EINVAL;
-        goto fail;
-    }
     s->crypt_method_header = header.crypt_method;
     if (s->crypt_method_header) {
         bs->encrypted = 1;

@@ -1104,12 +1348,40 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
 
     /* read qcow2 extensions */
     if (qcow2_read_extensions(bs, header.header_length, ext_end, NULL,
-        &local_err)) {
+                              flags, &local_err)) {
         error_propagate(errp, local_err);

block/qcow2.c +106 -52

+        if (s->crypt_method_header == QCOW_CRYPT_AES) {
+            unsigned int cflags = 0;
+            if (flags & BDRV_O_NO_IO) {
+                cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
+            }
+            s->fde = qcrypto_block_open(s->fde_opts,
+                                        NULL, NULL,
+                                        cflags,
+                                        errp);
+            if (!s->fde) {
+                error_setg(errp, "Could not setup encryption layer");
+                ret = -EINVAL;
+                goto fail;
+            }
+        } else if (!(flags & BDRV_O_NO_IO)) {
+            error_setg(errp, "Missing FDE header for crypt method %d",
+                       s->crypt_method_header);
+            ret = -EINVAL;
+            goto fail;
+        }
+    }
+
     /* read the backing file name */
     if (header.backing_file_offset != 0) {
         len = header.backing_file_size;

@@ -1199,41 +1471,6 @@ static void qcow2_refresh_limits(BlockDriverState *bs, Error **errp)
     bs->bl.write_zeroes_alignment = s->cluster_sectors;
 }
 
-static int qcow2_set_key(BlockDriverState *bs, const char *key)
-{
-    BDRVQcow2State *s = bs->opaque;
-    uint8_t keybuf[16];
-    int len, i;
-    Error *err = NULL;
-
-    memset(keybuf, 0, 16);
-    len = strlen(key);
-    if (len > 16)
-        len = 16;
-    /* XXX: we could compress the chars to 7 bits to increase
-       entropy */
-    for(i = 0;i < len;i++) {
-        keybuf[i] = key[i];
-    }
-    assert(bs->encrypted);
-
-    qcrypto_cipher_free(s->cipher);
-    s->cipher = qcrypto_cipher_new(
-        QCRYPTO_CIPHER_ALG_AES_128,
-        QCRYPTO_CIPHER_MODE_CBC,
-        keybuf, G_N_ELEMENTS(keybuf),
-        &err);
-
-    if (!s->cipher) {
-        /* XXX would be nice if errors in this method could
-         * be properly propagate to the caller. Would need
-         * the bdrv_set_key() API signature to be fixed. */
-        error_free(err);
-        return -1;
-    }
-    return 0;
-}
-
 static int qcow2_reopen_prepare(BDRVReopenState *state,
                                 BlockReopenQueue *queue, Error **errp)
 {

@@ -1345,7 +1582,7 @@ static int64_t coroutine_fn qcow2_co_get_block_status(BlockDriverState *bs,
     }
 
     if (cluster_offset != 0 && ret != QCOW2_CLUSTER_COMPRESSED &&
-        !s->cipher) {
+        !s->fde) {
         index_in_cluster = sector_num & (s->cluster_sectors - 1);
         cluster_offset |= (index_in_cluster << BDRV_SECTOR_BITS);
         status |= BDRV_BLOCK_OFFSET_VALID | cluster_offset;

@@ -1395,7 +1632,7 @@ static coroutine_fn int qcow2_co_readv(BlockDriverState *bs, int64_t sector_num,
 
         /* prepare next request */
         cur_nr_sectors = remaining_sectors;
-        if (s->cipher) {
+        if (s->fde) {
             cur_nr_sectors = MIN(cur_nr_sectors,
                 QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors);
         }

@@ -1467,7 +1704,7 @@ static coroutine_fn int qcow2_co_readv(BlockDriverState *bs, int64_t sector_num,
             }
 
             if (bs->encrypted) {
-                assert(s->cipher);
+                assert(s->fde);
 
                 /*
                  * For encrypted images, read everything into a temporary

@@ -1501,10 +1738,12 @@ static coroutine_fn int qcow2_co_readv(BlockDriverState *bs, int64_t sector_num,
                 goto fail;
             }
             if (bs->encrypted) {
-                assert(s->cipher);
+                assert(s->fde);
                 Error *err = NULL;
-                if (qcow2_encrypt_sectors(s, sector_num,  cluster_data,
-                                          cur_nr_sectors, false,
+                if (qcrypto_block_decrypt(s->fde,
+                                          sector_num,
+                                          cluster_data,
+                                          cur_nr_sectors,
                                           &err) < 0) {
                     error_free(err);
                     ret = -EIO;

@@ -1588,7 +1827,7 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
 
         if (bs->encrypted) {
             Error *err = NULL;
-            assert(s->cipher);
+            assert(s->fde);
             if (!cluster_data) {
                 cluster_data = qemu_try_blockalign(bs->file->bs,
                                                    QCOW_MAX_CRYPT_CLUSTERS

@@ -1603,8 +1842,11 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
                    QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size);
             qemu_iovec_to_buf(&hd_qiov, 0, cluster_data, hd_qiov.size);
 
-            if (qcow2_encrypt_sectors(s, sector_num, cluster_data,
-                                      cur_nr_sectors, true, &err) < 0) {
+            if (qcrypto_block_encrypt(s->fde,
+                                      sector_num,
+                                      cluster_data,
+                                      cur_nr_sectors,
+                                      &err) < 0) {
                 error_free(err);
                 ret = -EIO;
                 goto fail;

@@ -1715,8 +1957,8 @@ static void qcow2_close(BlockDriverState *bs)
     qcow2_cache_destroy(bs, s->l2_table_cache);
     qcow2_cache_destroy(bs, s->refcount_block_cache);
 
-    qcrypto_cipher_free(s->cipher);
-    s->cipher = NULL;
+    qcrypto_block_free(s->fde);
+    s->fde = NULL;
 
     g_free(s->unknown_header_fields);
     cleanup_unknown_header_ext(bs);

@@ -1734,7 +1976,7 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
 {
     BDRVQcow2State *s = bs->opaque;
     int flags = s->flags;
-    QCryptoCipher *cipher = NULL;
+    QCryptoBlock *fde = NULL;
     QDict *options;
     Error *local_err = NULL;
     int ret;

@@ -1744,8 +1986,8 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
      * that means we don't have to worry about reopening them here.
      */
 
-    cipher = s->cipher;
-    s->cipher = NULL;
+    fde = s->fde;
+    s->fde = NULL;
 
     qcow2_close(bs);
 

@@ -1770,7 +2012,7 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
         return;
     }
 
-    s->cipher = cipher;
+    s->fde = fde;
 }
 
 static size_t header_ext_add(char *buf, uint32_t magic, const void *s,

@@ -1893,6 +2135,23 @@ int qcow2_update_header(BlockDriverState *bs)
         buflen -= ret;
     }
 
+    /* Full disk encryption header pointer extension */
+    if (s->fde_header.offset != 0) {
+        cpu_to_be64(s->fde_header.offset);
+        cpu_to_be64(s->fde_header.length);
+        ret = header_ext_add(buf, QCOW2_EXT_MAGIC_FDE_HEADER,
+                             &s->fde_header,
+                             sizeof(s->fde_header),
+                             buflen);
+        be64_to_cpu(s->fde_header.offset);
+        be64_to_cpu(s->fde_header.length);
+        if (ret < 0) {
+            goto fail;
+        }
+        buf += ret;
+        buflen -= ret;
+    }
+
     /* Feature table */
     Qcow2Feature features[] = {
         {

@@ -2056,6 +2315,10 @@ static int qcow2_create2(const char *filename, int64_t total_size,
 {
     int cluster_bits;
     QDict *options;
+    const char *fdestr;
+    QCryptoBlockCreateOptions *fdeopts = NULL;
+    QCryptoBlock *fde = NULL;
+    size_t i;
 
     /* Calculate cluster_bits */
     cluster_bits = ctz32(cluster_size);

@@ -2179,7 +2442,48 @@ static int qcow2_create2(const char *filename, int64_t total_size,
     };
 
     if (flags & BLOCK_FLAG_ENCRYPT) {
-        header->crypt_method = cpu_to_be32(QCOW_CRYPT_AES);
+        /* Default to LUKS if fde-format is not set */
+        fdestr = qemu_opt_get_del(opts, QCOW2_OPT_ENC_FORMAT);
+        if (fdestr) {
+            for (i = 0; i < Q_CRYPTO_BLOCK_FORMAT__MAX; i++) {
+                if (g_str_equal(QCryptoBlockFormat_lookup[i],
+                                fdestr)) {
+                    fdeopts = qcow2_fde_create_opts_init(i,
+                                                         opts,
+                                                         errp);
+                    if (!fdeopts) {
+                        ret = -EINVAL;
+                        goto out;
+                    }
+                    break;
+                }
+            }
+            if (!fdeopts) {
+                error_setg(errp, "Unknown fde format %s", fdestr);
+                ret = -EINVAL;
+                goto out;
+            }
+        } else {
+            fdeopts = qcow2_fde_create_opts_init(
+                Q_CRYPTO_BLOCK_FORMAT_LUKS,
+                opts, errp);
+            if (!fdeopts) {
+                ret = -EINVAL;
+                goto out;
+            }
+        }
+        switch (fdeopts->format) {
+        case Q_CRYPTO_BLOCK_FORMAT_QCOWAES:
+            header->crypt_method = cpu_to_be32(QCOW_CRYPT_AES);
+            break;
+        case Q_CRYPTO_BLOCK_FORMAT_LUKS:
+            header->crypt_method = cpu_to_be32(QCOW_CRYPT_LUKS);
+            break;
+        default:
+            error_setg(errp, "Unsupported fde format %s", fdestr);
+            ret = -EINVAL;
+            goto out;
+        }
     } else {
         header->crypt_method = cpu_to_be32(QCOW_CRYPT_NONE);
     }

@@ -2213,12 +2517,15 @@ static int qcow2_create2(const char *filename, int64_t total_size,
     /*
      * And now open the image and make it consistent first (i.e. increase the
      * refcount of the cluster that is occupied by the header and the refcount
-     * table)
+     * table). Using BDRV_O_NO_IO since we've not written any encryption

block/qcow2.c +57 -5

+    if (fdeopts) {
+        fde = qcrypto_block_create(fdeopts,
+                                   qcow2_fde_header_init_func,
+                                   qcow2_fde_header_write_func,
+                                   bs,
+                                   errp);
+        if (!fde) {
+            ret = -EINVAL;
+            goto out;
+        }
+        ret = qcow2_update_header(bs);
+        if (ret < 0) {
+            error_setg_errno(errp, -ret, "Could not write encryption header");
+            goto out;
+        }
+    }
+
     /* And if we're supposed to preallocate metadata, do that now */
     if (prealloc != PREALLOC_MODE_OFF) {
         BDRVQcow2State *s = bs->opaque;

@@ -2272,7 +2597,8 @@ static int qcow2_create2(const char *filename, int64_t total_size,
     options = qdict_new();
     qdict_put(options, "driver", qstring_from_str("qcow2"));
     ret = bdrv_open(&bs, filename, NULL, options,
-                    BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_BACKING,
+                    BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_BACKING |
+                    BDRV_O_NO_IO,
                     &local_err);
     if (local_err) {
         error_propagate(errp, local_err);

@@ -3047,10 +3373,10 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
             backing_format = qemu_opt_get(opts, BLOCK_OPT_BACKING_FMT);
         } else if (!strcmp(desc->name, BLOCK_OPT_ENCRYPT)) {
             encrypt = qemu_opt_get_bool(opts, BLOCK_OPT_ENCRYPT,
-                                        !!s->cipher);
+                                        !!s->fde);
 
-            if (encrypt != !!s->cipher) {
-                error_report("Changing the encryption flag is not supported");
+            if (encrypt != !!s->fde) {
+                fprintf(stderr, "Changing the encryption flag is not supported");
                 return -ENOTSUP;
             }
         } else if (!strcmp(desc->name, BLOCK_OPT_CLUSTER_SIZE)) {

@@ -3285,6 +3611,41 @@ static QemuOptsList qcow2_create_opts = {
             .help = "Width of a reference count entry in bits",
             .def_value_str = "16"
         },
+        {
+            .name = QCOW2_OPT_ENC_FORMAT,
+            .type = QEMU_OPT_STRING,
+            .help = "Disk encryption format, 'luks' (default) or 'qcowaes' (deprecated)",
+        },
+        {
+            .name = QCOW2_OPT_KEY_ID,
+            .type = QEMU_OPT_STRING,
+            .help = "ID of the secret that provides the encryption key",
+        },
+        {
+            .name = QCOW2_OPT_CIPHER_ALG,
+            .type = QEMU_OPT_STRING,
+            .help = "Name of encryption cipher algorithm",
+        },
+        {
+            .name = QCOW2_OPT_CIPHER_MODE,
+            .type = QEMU_OPT_STRING,
+            .help = "Name of encryption cipher mode",
+        },
+        {
+            .name = QCOW2_OPT_IVGEN_ALG,
+            .type = QEMU_OPT_STRING,
+            .help = "Name of IV generator algorithm",
+        },
+        {
+            .name = QCOW2_OPT_IVGEN_HASH_ALG,
+            .type = QEMU_OPT_STRING,
+            .help = "Name of IV generator hash algorithm",
+        },
+        {
+            .name = QCOW2_OPT_HASH_ALG,
+            .type = QEMU_OPT_STRING,
+            .help = "Name of encryption hash algorithm",
+        },
         { /* end of list */ }
     }
 };

@@ -3302,7 +3663,6 @@ BlockDriver bdrv_qcow2 = {
     .bdrv_create        = qcow2_create,
     .bdrv_has_zero_init = bdrv_has_zero_init_1,
     .bdrv_co_get_block_status = qcow2_co_get_block_status,
-    .bdrv_set_key       = qcow2_set_key,
 
     .bdrv_co_readv          = qcow2_co_readv,
     .bdrv_co_writev         = qcow2_co_writev,

block/qcow2.h +11 -1

index ae04285..d33afb2 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

@@ -36,6 +36,7 @@
 
 #define QCOW_CRYPT_NONE 0
 #define QCOW_CRYPT_AES  1
+#define QCOW_CRYPT_LUKS 2
 
 #define QCOW_MAX_CRYPT_CLUSTERS 32
 #define QCOW_MAX_SNAPSHOTS 65536

@@ -98,6 +99,15 @@
 #define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size"
 #define QCOW2_OPT_CACHE_CLEAN_INTERVAL "cache-clean-interval"
 
+#define QCOW2_OPT_ENC_FORMAT "encryption-format"
+#define QCOW2_OPT_KEY_ID "key-id"
+#define QCOW2_OPT_CIPHER_ALG "cipher-alg"
+#define QCOW2_OPT_CIPHER_MODE "cipher-mode"
+#define QCOW2_OPT_IVGEN_ALG "ivgen-alg"
+#define QCOW2_OPT_IVGEN_HASH_ALG "ivgen-hash-alg"
+#define QCOW2_OPT_HASH_ALG "hash-alg"
+
+
 typedef struct QCowHeader {
     uint32_t magic;
     uint32_t version;

block/qcow2.h +3 -1

 typedef struct Qcow2UnknownHeaderExtension {
     uint32_t magic;
     uint32_t len;

@@ -256,7 +271,9 @@ typedef struct BDRVQcow2State {
 
     CoMutex lock;
 
-    QCryptoCipher *cipher; /* current cipher, NULL if no key yet */
+    Qcow2FDEHeaderExtension fde_header; /* QCow2 header extension */
+    QCryptoBlockOpenOptions *fde_opts; /* Disk encryption runtime options */
+    QCryptoBlock *fde; /* Disk encryption format driver */
     uint32_t crypt_method_header;
     uint64_t snapshots_offset;
     int snapshots_size;

docs/specs/qcow2.txt +2 -0

index f236d8c..dcbe3c3 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

@@ -123,6 +124,7 @@ be stored. Each extension has a structure like the following:
                         0x00000000 - End of the header extension area
                         0xE2792ACA - Backing file format name
                         0x6803f857 - Feature name table
+                        0x4c554b53 - Full disk encryption header pointer
                         other      - Unknown header extension, can be safely
                                      ignored
 

It provides the offset at which the crypt method

docs/specs/qcow2.txt +64 -0

+can store its additional data, as well as the length of such data.
+
+    Byte  0 -  7:   Offset into the image file at which the encryption
+                    header starts. Must be aligned to a cluster boundary.
+    Byte  8 - 16:   Length of the encryption header. Must be a multiple
+                    of the cluster size.
+
+While the header extension allocates the length as a multiple of the
+cluster size, the encryption header may not consume the full permitted
+allocation.
+
+For the LUKS crypt method, the encryption header works as follows.
+
+The first 592 bytes of the header clusters will contain the LUKS
+partition header. This is then followed by the key material data areas.
+The size of the key material data areas is determined by the number of
+stripes in the key slot and key size. Refer to the LUKS format
+specification ('docs/on-disk-format.pdf' in the cryptsetup source
+package) for details of the LUKS partition header format.
+
+In the LUKS partition header, the "payload-offset" field does not refer
+to the offset of the QCow2 payload. Instead it simply refers to the
+total required length of the LUKS header plus key material regions.
+
+In the LUKS key slots header, the "key-material-offset" is relative
+to the start of the LUKS header clusters in the qcow2 container,
+not the start of the qcow2 file.
+
+Logically the layout looks like
+
+  +-----------------------------+
+  | QCow2 header                |
+  +-----------------------------+
+  | QCow2 header extension X    |
+  +-----------------------------+
+  | QCow2 header extension FDE  |
+  +-----------------------------+
+  | QCow2 header extension ...  |
+  +-----------------------------+
+  | QCow2 header extension Z    |
+  +-----------------------------+
+  | ....other QCow2 tables....  |
+  .                             .
+  .                             .
+  +-----------------------------+
+  | +-------------------------+ |
+  | | LUKS partition header   | |
+  | +-------------------------+ |
+  | | LUKS key material 1     | |
+  | +-------------------------+ |
+  | | LUKS key material 2     | |
+  | +-------------------------+ |
+  | | LUKS key material ...   | |
+  | +-------------------------+ |
+  | | LUKS key material 8     | |
+  | +-------------------------+ |
+  +-----------------------------+
+  | QCow2 cluster payload       |
+  .                             .
+  .                             .
+  .                             .
+  |                             |
+  +-----------------------------+
+
 == Host cluster management ==
 
 qcow2 manages the allocation of host clusters by maintaining a reference count

qapi/block-core.json +4 -2

index 200d907..40fe3ba 100644
@@ -1789,6 +1789,8 @@
 # @cache-clean-interval:  #optional clean unused entries in the L2 and refcount
 #                         caches. The interval is in seconds. The default value
 #                         is 0 and it disables this feature (since 2.5)
+# @key-id:                #optional the ID of a QCryptoSecret object providing
+#                         the decryption key (since 2.6)
 #
 # Since: 1.7
 ##

@@ -1802,8 +1804,8 @@
             '*cache-size': 'int',
             '*l2-cache-size': 'int',
             '*refcount-cache-size': 'int',
-            '*cache-clean-interval': 'int' } }
-
+            '*cache-clean-interval': 'int',
+            '*key-id': 'str' } }
 
 ##
 # @BlockdevOptionsArchipelago

Signature · 1 line

2.5.0

Am 12.01.2016 um 19:56 hat Daniel P. Berrange geschrieben:

Quoted 40 lines

This converts the qcow2 driver to make use of the QCryptoBlock APIs for encrypting image content. As well as continued support for the legacy QCow2 encryption format, the appealing benefit is that it enables support for the LUKS format inside qcow2.

With the LUKS format it is neccessary to store the LUKS partition header and key material in the QCow2 file. This data can be many MB in size, so cannot go into the QCow2 header region directly. Thus the spec is defines a FDE (Full Disk Encryption) header extension that specifies the offset of a set of clusters to hold the FDE headers, as well as the length of that region. The LUKS header is thus stored in these extra allocated clusters before the main image payload.

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

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

The new LUKS format is set as the new default format when creating encrypted images. ie

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

Results in creation of an image using the LUKS format.

For compatibility the old qcow2 AES format can still be used via the 'encryption-format' parameter which accepts the values 'luks' or 'qcowaes'.

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

Quoted 1 line

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

I think for your additional pointer to some clusters you need to change some function(s) called by qcow2_check_refcounts(). Otherwise 'qemu-img check' won't count the reference and helpfully free the "leaked" cluster.

block/qcow2.c typedef struct { · line 65

Quoted 5 lines

block/qcow2.c +1 -0

@@ -60,6 +62,7 @@ typedef struct {
 #define  QCOW2_EXT_MAGIC_END 0
 #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
 #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
+#define  QCOW2_EXT_MAGIC_FDE_HEADER 0x4c554b53

General naming comment on this series: I would prefer avoiding "FDE" in favour of "encryption" or "crypt" in the block layer parts. With all image formats having their own terminology, "FDE" could mean anything.

Quoted 15 lines

block/qcow2.c +9 -0

 static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
 {

@@ -74,6 +77,83 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
 }
 
 
+static ssize_t qcow2_fde_header_read_func(QCryptoBlock *block,
+                                          size_t offset,
+                                          uint8_t *buf,
+                                          size_t buflen,
+                                          Error **errp,
+                                          void *opaque)
+{
+    BlockDriverState *bs = opaque;
+    BDRVQcow2State *s = bs->opaque;

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

Quoted 5 lines

block/qcow2.c +5 -0

+    ssize_t ret;
+
+    if ((offset + buflen) > s->fde_header.length) {
+        error_setg_errno(errp, EINVAL,
+                         "Request for data outside of extension header");

error_setg_errno() with a constant errno doesn't look very useful. Better use plain error_setg() in such cases.

Review static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename) · line 94

Quoted 1 line

(quoted diff) +1 -0

+        return -1;

Here returning -EINVAL could be useful, I'm not sure what your crypto API requires. At least you seem to be returning -errno below and mixing -1 and -errno is probably a bad idea.

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

Quoted 10 lines

block/qcow2.c +9 -0

+    }
+
+    ret = bdrv_pread(bs->file->bs,
+                     s->fde_header.offset + offset, buf, buflen);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "Could not read encryption header");
+        return ret;
+    }
+    return ret;

return 0? You already processed ret in the if block and two 'return ret' in a row look odd.

Quoted 13 lines

block/qcow2.c +13 -0

+}
+
+
+static ssize_t qcow2_fde_header_init_func(QCryptoBlock *block,
+                                          size_t headerlen,
+                                          Error **errp,
+                                          void *opaque)
+{
+    BlockDriverState *bs = opaque;
+    BDRVQcow2State *s = bs->opaque;
+    int64_t ret;
+
+    s->fde_header.length = headerlen + (headerlen % s->cluster_size);

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

Quoted 6 lines

block/qcow2.c +6 -0

+
+    ret = qcow2_alloc_clusters(bs, s->fde_header.length);
+    if (ret < 0) {
+        s->fde_header.length = 0;
+        error_setg(errp, "Cannot allocate cluster for LUKS header size %zu",
+                   headerlen);

I think ret is -errno on failure, so use error_setg_errno()?

Quoted 13 lines

block/qcow2.c +12 -0

+        return -1;
+    }
+
+    s->fde_header.offset = ret;
+    return 0;
+}
+
+
+static ssize_t qcow2_fde_header_write_func(QCryptoBlock *block,
+                                           size_t offset,
+                                           const uint8_t *buf,
+                                           size_t buflen,

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

Quoted 10 lines

block/qcow2.c +10 -0

+                                           Error **errp,
+                                           void *opaque)
+{
+    BlockDriverState *bs = opaque;
+    BDRVQcow2State *s = bs->opaque;
+    ssize_t ret;
+
+    if ((offset + buflen) > s->fde_header.length) {
+        error_setg_errno(errp, EINVAL,
+                         "Request for data outside of extension header");

error_setg(). Probably worth checking all error paths whether there is a useful errno or not. I won't comment on additional instances.

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

Quoted 10 lines

block/qcow2.c +10 -0

+        return -1;
+    }
+
+    ret = bdrv_pwrite(bs->file->bs, s->fde_header.offset + offset, buf, buflen);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "Could not read encryption header");
+        return ret;
+    }
+    return ret;
+}

Mixing -1 and -errno again.

Quoted 7 lines

block/qcow2.c +1 -0

@@ -83,12 +163,14 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
  */
 static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
                                  uint64_t end_offset, void **p_feature_table,
+                                 int flags,
                                  Error **errp)
 {

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

Quoted 5 lines

block/qcow2.c +1 -0

     BDRVQcow2State *s = bs->opaque;
     QCowExtension ext;
     uint64_t offset;
     int ret;
+    unsigned int cflags = 0;

Should we create a block for QCOW2_EXT_MAGIC_FDE_HEADER and declare it there?

Quoted 10 lines

block/qcow2.c +4 -0

 #ifdef DEBUG_EXT
     printf("qcow2_read_extensions: start=%ld end=%ld\n", start_offset, end_offset);

@@ -159,6 +241,35 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
             }
             break;
 
+        case QCOW2_EXT_MAGIC_FDE_HEADER:
+            if (s->crypt_method_header != QCOW_CRYPT_LUKS) {
+                error_setg(errp, "FDE header extension only "
+                           "expected with LUKS encryption method");

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

Quoted 10 lines

block/qcow2.c +10 -0

+                return -EINVAL;
+            }
+            if (ext.len != sizeof(Qcow2FDEHeaderExtension)) {
+                error_setg(errp, "LUKS header extension size %u, "
+                           "but expected size %zu", ext.len,
+                           sizeof(Qcow2FDEHeaderExtension));
+                return -EINVAL;
+            }
+
+            ret = bdrv_pread(bs->file->bs, offset, &s->fde_header, ext.len);

No error check?

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

Quoted 11 lines

block/qcow2.c +11 -0

+            be64_to_cpu(s->fde_header.offset);
+            be64_to_cpu(s->fde_header.length);
+
+            if (flags & BDRV_O_NO_IO) {
+                cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
+            }
+            s->fde = qcrypto_block_open(s->fde_opts,
+                                        qcow2_fde_header_read_func,
+                                        bs,
+                                        cflags,
+                                        errp);

The surrounding code doesn't put every parameter on its own line if the next parameter can fit on the same line. There are more instances of this in the patch, I won't comment on each one.

Quoted 11 lines

block/qcow2.c +3 -1

@@ -2213,12 +2517,15 @@ static int qcow2_create2(const char *filename, int64_t total_size,
     /*
      * And now open the image and make it consistent first (i.e. increase the
      * refcount of the cluster that is occupied by the header and the refcount
-     * table)
+     * table). Using BDRV_O_NO_IO since we've not written any encryption
+     * header yet and thus should not read/write payload data or initialize
+     * the decryption context
      */
     options = qdict_new();
     qdict_put(options, "driver", qstring_from_str("qcow2"));

block/qcow2.c static int qcow2_create2(const char *filename, int64_t total_size, · line 2529

Quoted 5 lines

block/qcow2.c +2 -1

     ret = bdrv_open(&bs, filename, NULL, options,
-                    BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH,
+                    BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH |
+                    BDRV_O_NO_IO,
                     &local_err);

Somehow I feel that saying BDRV_O_NO_IO, but doing I/O will bite us sooner or later.

Why not leave header->crypt_method = 0 initially and only set up encryption after opening the image with the qcow2 driver?

Quoted 41 lines

block/qcow2.h +11 -1

index ae04285..d33afb2 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

@@ -36,6 +36,7 @@
 
 #define QCOW_CRYPT_NONE 0
 #define QCOW_CRYPT_AES  1
+#define QCOW_CRYPT_LUKS 2
 
 #define QCOW_MAX_CRYPT_CLUSTERS 32
 #define QCOW_MAX_SNAPSHOTS 65536

@@ -98,6 +99,15 @@
 #define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size"
 #define QCOW2_OPT_CACHE_CLEAN_INTERVAL "cache-clean-interval"
 
+#define QCOW2_OPT_ENC_FORMAT "encryption-format"
+#define QCOW2_OPT_KEY_ID "key-id"
+#define QCOW2_OPT_CIPHER_ALG "cipher-alg"
+#define QCOW2_OPT_CIPHER_MODE "cipher-mode"
+#define QCOW2_OPT_IVGEN_ALG "ivgen-alg"
+#define QCOW2_OPT_IVGEN_HASH_ALG "ivgen-hash-alg"
+#define QCOW2_OPT_HASH_ALG "hash-alg"
+
+
 typedef struct QCowHeader {
     uint32_t magic;
     uint32_t version;

@@ -163,6 +173,11 @@ typedef struct QCowSnapshot {
 struct Qcow2Cache;
 typedef struct Qcow2Cache Qcow2Cache;
 

block/qcow2.h typedef struct QCowSnapshot { · line 179

Quoted 4 lines

block/qcow2.h +4 -0

+typedef struct Qcow2FDEHeaderExtension {
+    uint64_t offset;
+    uint64_t length;
+} Qcow2FDEHeaderExtension;

Packed? It seems to be read directly from the file.

block/qcow2.h the header extension data. Each entry look like this: · line 175

Quoted 9 lines

block/qcow2.h +5 -0

@@ -166,6 +168,75 @@ the header extension data. Each entry look like this:
                     terminated if it has full length)
 
 
+== Full disk encryption (FDE) header pointer ==
+
+For 'crypt_method' header values which require additional header metadata
+to be stored (eg 'LUKS' header), the full disk encryption header pointer
+extension is mandatory.

I think you want to make that "is present if, and only if, the 'crypt_method' header value requires metadata".

At least, we need to forbid it for the existing AES images, because old qemu-img version would stll fix the "leaked clusters", without removing the header extension that refers to them.

Quoted 10 lines

Am 12.01.2016 um 19:56 hat Daniel P. Berrange geschrieben:

Quoted 9 lines

block/qcow2.c +9 -0

+static ssize_t qcow2_fde_header_read_func(QCryptoBlock *block,
+                                          size_t offset,
+                                          uint8_t *buf,
+                                          size_t buflen,
+                                          Error **errp,
+                                          void *opaque)
+{
+    BlockDriverState *bs = opaque;
+    BDRVQcow2State *s = bs->opaque;

Quoted 8 lines

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

Quoted 5 lines

block/qcow2.c +5 -0

+    ssize_t ret;
+
+    if ((offset + buflen) > s->fde_header.length) {
+        error_setg_errno(errp, EINVAL,
+                         "Request for data outside of extension header");

error_setg_errno() with a constant errno doesn't look very useful. Better use plain error_setg() in such cases.

I wasn't too sure - I figured since the block layer seems to propagate errno's around alot, that I ought to report an errno here, but will happiyl drop it.

Quoted 5 lines

Quoted 1 line

+ return -1;

Here returning -EINVAL could be useful, I'm not sure what your crypto API requires. At least you seem to be returning -errno below and mixing -1 and -errno is probably a bad idea.

The crypto API doesn't deal with errno's at all - it uses the Error object exclusively, so yeah, I can drop it from the place below.