[v2 0/4] Support generic Luks encryption

[v2 0/4] Support generic Luks encryption

[Hyman Huang]

v2:
- Simplify the design by reusing the LUKS driver to implement
  the generic Luks encryption, thank Daniel for the insightful 
  advice.
- rebase on master. 

This functionality was motivated by the following to-do list seen
in crypto documents:
https://wiki.qemu.org/Features/Block/Crypto 

The last chapter says we should "separate header volume": 

The LUKS format has ability to store the header in a separate volume
from the payload. We should extend the LUKS driver in QEMU to support
this use case.

By enhancing the LUKS driver, it is possible to enable
the detachable LUKS header and, as a result, achieve
general encryption for any disk format that QEMU has
supported.

Take the qcow2 as an example, the usage of the generic
LUKS encryption as follows:

1. add a protocol blockdev node of data disk
$ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> "arguments":{"node-name":"libvirt-1-storage", "driver":"file",
> "filename":"/path/to/test_disk.qcow2"}}'

2. add a protocol blockdev node of LUKS header as above.
$ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> "arguments":{"node-name":"libvirt-2-storage", "driver":"file",
> "filename": "/path/to/cipher.gluks" }}'

3. add the secret for decrypting the cipher stored in LUKS
   header above
$ virsh qemu-monitor-command vm '{"execute":"object-add",
> "arguments":{"qom-type":"secret", "id":
> "libvirt-2-storage-secret0", "data":"abc123"}}'

4. add the qcow2-drived blockdev format node
$ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> "arguments":{"node-name":"libvirt-1-format", "driver":"qcow2",
> "file":"libvirt-1-storage"}}'

5. add the luks-drived blockdev to link the qcow2 disk with
   LUKS header by specifying the field "header"
$ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> "arguments":{"node-name":"libvirt-2-format", "driver":"luks",
> "file":"libvirt-1-format", "header":"libvirt-2-storage",
> "key-secret":"libvirt-2-format-secret0"}}'

6. add the virtio-blk device finally
$ virsh qemu-monitor-command vm '{"execute":"device_add",
> "arguments": {"num-queues":"1", "driver":"virtio-blk-pci",
> "drive": "libvirt-2-format", "id":"virtio-disk2"}}'

The generic LUKS encryption method of starting a virtual
machine (VM) is somewhat similar to hot-plug in that both
maintaining the same json command while the starting VM
changes the "blockdev-add/device_add" parameters to
"blockdev/device".

Please review, thanks

Best regared,

Yong

Hyman Huang (4):
  crypto: Introduce option and structure for detached LUKS header
  crypto: Introduce payload offset set function
  crypto: Support generic LUKS encryption
  block: Support detached LUKS header creation for blockdev-create

 block/crypto.c         | 47 ++++++++++++++++++++++++++++++++++++++++--
 crypto/block.c         |  4 ++++
 include/crypto/block.h |  1 +
 qapi/block-core.json   | 11 ++++++++--
 4 files changed, 59 insertions(+), 4 deletions(-)

-- 
2.39.1

[v2 1/4] crypto: Introduce option and structure for detached LUKS header

[Hyman Huang]

Add the "header" option for the LUKS format. This field would be
used to identify the blockdev's position where a detachable LUKS
header is stored.

In addition, introduce header field in struct BlockCrypto

Signed-off-by: Hyman Huang <yong.huang@smartx.com>
---
 block/crypto.c       | 1 +
 qapi/block-core.json | 6 +++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/block/crypto.c b/block/crypto.c
index 921933a5e5..f82b13d32b 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -39,6 +39,7 @@ typedef struct BlockCrypto BlockCrypto;
 struct BlockCrypto {
     QCryptoBlock *block;
     bool updating_keys;
+    BdrvChild *header;  /* Reference to the detached LUKS header */
 };
 
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index ca390c5700..10be08d08f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3352,11 +3352,15 @@
 #     decryption key (since 2.6). Mandatory except when doing a
 #     metadata-only probe of the image.
 #
+# @header: optional reference to the location of a blockdev
+#     storing a detached LUKS header. (since 9.0)
+#
 # Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsLUKS',
   'base': 'BlockdevOptionsGenericFormat',
-  'data': { '*key-secret': 'str' } }
+  'data': { '*key-secret': 'str',
+            '*header': 'BlockdevRef'} }
 
 ##
 # @BlockdevOptionsGenericCOWFormat:
-- 
2.39.1

Re: [v2 1/4] crypto: Introduce option and structure for detached LUKS header

[Daniel P. Berrangé]

On Thu, Dec 07, 2023 at 12:37:42AM +0800, Hyman Huang wrote:
> Add the "header" option for the LUKS format. This field would be
> used to identify the blockdev's position where a detachable LUKS
> header is stored.
> 
> In addition, introduce header field in struct BlockCrypto
> 
> Signed-off-by: Hyman Huang <yong.huang@smartx.com>
> ---
>  block/crypto.c       | 1 +
>  qapi/block-core.json | 6 +++++-
>  2 files changed, 6 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[v2 2/4] crypto: Introduce payload offset set function

[Hyman Huang]

Signed-off-by: Hyman Huang <yong.huang@smartx.com>
---
 crypto/block.c         | 4 ++++
 include/crypto/block.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/crypto/block.c b/crypto/block.c
index 7bb4b74a37..3dcf22a69f 100644
--- a/crypto/block.c
+++ b/crypto/block.c
@@ -319,6 +319,10 @@ QCryptoHashAlgorithm qcrypto_block_get_kdf_hash(QCryptoBlock *block)
     return block->kdfhash;
 }
 
+void qcrypto_block_set_payload_offset(QCryptoBlock *block, uint64_t offset)
+{
+    block->payload_offset = offset;
+}
 
 uint64_t qcrypto_block_get_payload_offset(QCryptoBlock *block)
 {
diff --git a/include/crypto/block.h b/include/crypto/block.h
index 4f63a37872..b47a90c529 100644
--- a/include/crypto/block.h
+++ b/include/crypto/block.h
@@ -312,4 +312,5 @@ void qcrypto_block_free(QCryptoBlock *block);
 
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(QCryptoBlock, qcrypto_block_free)
 
+void qcrypto_block_set_payload_offset(QCryptoBlock *block, uint64_t offset);
 #endif /* QCRYPTO_BLOCK_H */
-- 
2.39.1

Re: [v2 2/4] crypto: Introduce payload offset set function

[Daniel P. Berrangé]

On Thu, Dec 07, 2023 at 12:37:43AM +0800, Hyman Huang wrote:
> Signed-off-by: Hyman Huang <yong.huang@smartx.com>
> ---
>  crypto/block.c         | 4 ++++
>  include/crypto/block.h | 1 +
>  2 files changed, 5 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

however, based on my comment in patch #3, I'm not convinced this method
is needed

> 
> diff --git a/crypto/block.c b/crypto/block.c
> index 7bb4b74a37..3dcf22a69f 100644
> --- a/crypto/block.c
> +++ b/crypto/block.c
> @@ -319,6 +319,10 @@ QCryptoHashAlgorithm qcrypto_block_get_kdf_hash(QCryptoBlock *block)
>      return block->kdfhash;
>  }
>  
> +void qcrypto_block_set_payload_offset(QCryptoBlock *block, uint64_t offset)
> +{
> +    block->payload_offset = offset;
> +}
>  
>  uint64_t qcrypto_block_get_payload_offset(QCryptoBlock *block)
>  {
> diff --git a/include/crypto/block.h b/include/crypto/block.h
> index 4f63a37872..b47a90c529 100644
> --- a/include/crypto/block.h
> +++ b/include/crypto/block.h
> @@ -312,4 +312,5 @@ void qcrypto_block_free(QCryptoBlock *block);
>  
>  G_DEFINE_AUTOPTR_CLEANUP_FUNC(QCryptoBlock, qcrypto_block_free)
>  
> +void qcrypto_block_set_payload_offset(QCryptoBlock *block, uint64_t offset);
>  #endif /* QCRYPTO_BLOCK_H */
> -- 
> 2.39.1
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[v2 3/4] crypto: Support generic LUKS encryption

[Hyman Huang]

By enhancing the LUKS driver, it is possible to enable
the detachable LUKS header and, as a result, achieve
general encryption for any disk format that QEMU has
supported.

Take the qcow2 as an example, the usage of the generic
LUKS encryption as follows:

1. add a protocol blockdev node of data disk
$ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> "arguments":{"node-name":"libvirt-1-storage", "driver":"file",
> "filename":"/path/to/test_disk.qcow2"}}'

2. add a protocol blockdev node of LUKS header as above.
$ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> "arguments":{"node-name":"libvirt-2-storage", "driver":"file",
> "filename": "/path/to/cipher.gluks" }}'

3. add the secret for decrypting the cipher stored in LUKS
   header above
$ virsh qemu-monitor-command vm '{"execute":"object-add",
> "arguments":{"qom-type":"secret", "id":
> "libvirt-2-storage-secret0", "data":"abc123"}}'

4. add the qcow2-drived blockdev format node
$ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> "arguments":{"node-name":"libvirt-1-format", "driver":"qcow2",
> "file":"libvirt-1-storage"}}'

5. add the luks-drived blockdev to link the qcow2 disk with
   LUKS header by specifying the field "header"
$ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> "arguments":{"node-name":"libvirt-2-format", "driver":"luks",
> "file":"libvirt-1-format", "header":"libvirt-2-storage",
> "key-secret":"libvirt-2-format-secret0"}}'

6. add the virtio-blk device finally
$ virsh qemu-monitor-command vm '{"execute":"device_add",
> "arguments": {"num-queues":"1", "driver":"virtio-blk-pci",
> "drive": "libvirt-2-format", "id":"virtio-disk2"}}'

The generic LUKS encryption method of starting a virtual
machine (VM) is somewhat similar to hot-plug in that both
maintaining the same json command while the starting VM
changes the "blockdev-add/device_add" parameters to
"blockdev/device".

Signed-off-by: Hyman Huang <yong.huang@smartx.com>
---
 block/crypto.c | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/block/crypto.c b/block/crypto.c
index f82b13d32b..7d70349463 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -40,6 +40,7 @@ struct BlockCrypto {
     QCryptoBlock *block;
     bool updating_keys;
     BdrvChild *header;  /* Reference to the detached LUKS header */
+    bool detached_mode; /* If true, LUKS plays a detached header role */
 };
 
 
@@ -64,12 +65,16 @@ static int block_crypto_read_func(QCryptoBlock *block,
                                   Error **errp)
 {
     BlockDriverState *bs = opaque;
+    BlockCrypto *crypto = bs->opaque;
     ssize_t ret;
 
     GLOBAL_STATE_CODE();
     GRAPH_RDLOCK_GUARD_MAINLOOP();
 
-    ret = bdrv_pread(bs->file, offset, buflen, buf, 0);
+    if (crypto->detached_mode)
+        ret = bdrv_pread(crypto->header, offset, buflen, buf, 0);
+    else
+        ret = bdrv_pread(bs->file, offset, buflen, buf, 0);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not read encryption header");
         return ret;
@@ -269,6 +274,8 @@ static int block_crypto_open_generic(QCryptoBlockFormat format,
     QCryptoBlockOpenOptions *open_opts = NULL;
     unsigned int cflags = 0;
     QDict *cryptoopts = NULL;
+    const char *header_bdref =
+        qdict_get_try_str(options, "header");
 
     GLOBAL_STATE_CODE();
 
@@ -277,6 +284,16 @@ static int block_crypto_open_generic(QCryptoBlockFormat format,
         return ret;
     }
 
+    if (header_bdref) {
+        crypto->detached_mode = true;
+        crypto->header = bdrv_open_child(NULL, options, "header", bs,
+                                         &child_of_bds, BDRV_CHILD_METADATA,
+                                         false, errp);
+        if (!crypto->header) {
+            return -EINVAL;
+        }
+    }
+
     GRAPH_RDLOCK_GUARD_MAINLOOP();
 
     bs->supported_write_flags = BDRV_REQ_FUA &
@@ -312,6 +329,14 @@ static int block_crypto_open_generic(QCryptoBlockFormat format,
         goto cleanup;
     }
 
+    if (crypto->detached_mode) {
+        /*
+         * Set payload offset to zero as the file bdref has no LUKS
+         * header under detached mode.
+         */
+        qcrypto_block_set_payload_offset(crypto->block, 0);
+    }
+
     bs->encrypted = true;
 
     ret = 0;
@@ -903,6 +928,17 @@ block_crypto_child_perms(BlockDriverState *bs, BdrvChild *c,
 
     BlockCrypto *crypto = bs->opaque;
 
+    if (role == (role & BDRV_CHILD_METADATA)) {
+        /* Assign read permission only */
+        perm |= BLK_PERM_CONSISTENT_READ;
+        /* Share all permissions */
+        shared |= BLK_PERM_ALL;
+
+        *nperm = perm;
+        *nshared = shared;
+        return;
+    }
+
     bdrv_default_perms(bs, c, role, reopen_queue, perm, shared, nperm, nshared);
 
     /*
-- 
2.39.1

Re: [v2 3/4] crypto: Support generic LUKS encryption

[Daniel P. Berrangé]

On Thu, Dec 07, 2023 at 12:37:44AM +0800, Hyman Huang wrote:
> By enhancing the LUKS driver, it is possible to enable
> the detachable LUKS header and, as a result, achieve
> general encryption for any disk format that QEMU has
> supported.
> 
> Take the qcow2 as an example, the usage of the generic
> LUKS encryption as follows:
> 
> 1. add a protocol blockdev node of data disk
> $ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> > "arguments":{"node-name":"libvirt-1-storage", "driver":"file",
> > "filename":"/path/to/test_disk.qcow2"}}'
> 
> 2. add a protocol blockdev node of LUKS header as above.
> $ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> > "arguments":{"node-name":"libvirt-2-storage", "driver":"file",
> > "filename": "/path/to/cipher.gluks" }}'
> 
> 3. add the secret for decrypting the cipher stored in LUKS
>    header above
> $ virsh qemu-monitor-command vm '{"execute":"object-add",
> > "arguments":{"qom-type":"secret", "id":
> > "libvirt-2-storage-secret0", "data":"abc123"}}'
> 
> 4. add the qcow2-drived blockdev format node
> $ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> > "arguments":{"node-name":"libvirt-1-format", "driver":"qcow2",
> > "file":"libvirt-1-storage"}}'
> 
> 5. add the luks-drived blockdev to link the qcow2 disk with
>    LUKS header by specifying the field "header"
> $ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> > "arguments":{"node-name":"libvirt-2-format", "driver":"luks",
> > "file":"libvirt-1-format", "header":"libvirt-2-storage",
> > "key-secret":"libvirt-2-format-secret0"}}'
> 
> 6. add the virtio-blk device finally
> $ virsh qemu-monitor-command vm '{"execute":"device_add",
> > "arguments": {"num-queues":"1", "driver":"virtio-blk-pci",
> > "drive": "libvirt-2-format", "id":"virtio-disk2"}}'
> 
> The generic LUKS encryption method of starting a virtual
> machine (VM) is somewhat similar to hot-plug in that both
> maintaining the same json command while the starting VM
> changes the "blockdev-add/device_add" parameters to
> "blockdev/device".
> 
> Signed-off-by: Hyman Huang <yong.huang@smartx.com>
> ---
>  block/crypto.c | 38 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/block/crypto.c b/block/crypto.c
> index f82b13d32b..7d70349463 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -40,6 +40,7 @@ struct BlockCrypto {
>      QCryptoBlock *block;
>      bool updating_keys;
>      BdrvChild *header;  /* Reference to the detached LUKS header */
> +    bool detached_mode; /* If true, LUKS plays a detached header role */
>  };
>  
>  
> @@ -64,12 +65,16 @@ static int block_crypto_read_func(QCryptoBlock *block,
>                                    Error **errp)
>  {
>      BlockDriverState *bs = opaque;
> +    BlockCrypto *crypto = bs->opaque;
>      ssize_t ret;
>  
>      GLOBAL_STATE_CODE();
>      GRAPH_RDLOCK_GUARD_MAINLOOP();
>  
> -    ret = bdrv_pread(bs->file, offset, buflen, buf, 0);
> +    if (crypto->detached_mode)
> +        ret = bdrv_pread(crypto->header, offset, buflen, buf, 0);
> +    else
> +        ret = bdrv_pread(bs->file, offset, buflen, buf, 0);

This can be simplified to:

    ret = bdrv_pread(bs->header ? bs->header : file, offset, buflen, buf, 0);

>      if (ret < 0) {
>          error_setg_errno(errp, -ret, "Could not read encryption header");
>          return ret;
> @@ -269,6 +274,8 @@ static int block_crypto_open_generic(QCryptoBlockFormat format,
>      QCryptoBlockOpenOptions *open_opts = NULL;
>      unsigned int cflags = 0;
>      QDict *cryptoopts = NULL;
> +    const char *header_bdref =
> +        qdict_get_try_str(options, "header");
>  
>      GLOBAL_STATE_CODE();
>  
> @@ -277,6 +284,16 @@ static int block_crypto_open_generic(QCryptoBlockFormat format,
>          return ret;
>      }
>  
> +    if (header_bdref) {
> +        crypto->detached_mode = true;

Drop this flag since it has no benefit.

> +        crypto->header = bdrv_open_child(NULL, options, "header", bs,
> +                                         &child_of_bds, BDRV_CHILD_METADATA,
> +                                         false, errp);
> +        if (!crypto->header) {
> +            return -EINVAL;
> +        }
> +    }
> +
>      GRAPH_RDLOCK_GUARD_MAINLOOP();
>  
>      bs->supported_write_flags = BDRV_REQ_FUA &
> @@ -312,6 +329,14 @@ static int block_crypto_open_generic(QCryptoBlockFormat format,
>          goto cleanup;
>      }
>  
> +    if (crypto->detached_mode) {

  if (crypto->header != NULL)

> +        /*
> +         * Set payload offset to zero as the file bdref has no LUKS
> +         * header under detached mode.
> +         */
> +        qcrypto_block_set_payload_offset(crypto->block, 0);

The LUKS header stores the payload offset.  If someone creates the LUKS
volume with a detached header, they may still choose to put the payload
at a non-zero offset.

So AFAICT, we should always honour the payload offset from the header,
even when detached.   When formatting the header, we should default to
a zero offset though

> +    }
> +
>      bs->encrypted = true;
>  
>      ret = 0;
> @@ -903,6 +928,17 @@ block_crypto_child_perms(BlockDriverState *bs, BdrvChild *c,
>  
>      BlockCrypto *crypto = bs->opaque;
>  
> +    if (role == (role & BDRV_CHILD_METADATA)) {
> +        /* Assign read permission only */
> +        perm |= BLK_PERM_CONSISTENT_READ;
> +        /* Share all permissions */
> +        shared |= BLK_PERM_ALL;
> +
> +        *nperm = perm;
> +        *nshared = shared;
> +        return;
> +    }

What is code doing ?  You've set  BDRV_CHILD_METADATA role on the
crypto->header  object, but how does this block_crypto_child_perms
method get run against crypto->header ? 

> +
>      bdrv_default_perms(bs, c, role, reopen_queue, perm, shared, nperm, nshared);
>  
>      /*
> -- 
> 2.39.1
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [v2 3/4] crypto: Support generic LUKS encryption

[Yong Huang]

[-- Attachment #1: Type: text/plain, Size: 7192 bytes --]

On Mon, Dec 18, 2023 at 7:16 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Thu, Dec 07, 2023 at 12:37:44AM +0800, Hyman Huang wrote:
> > By enhancing the LUKS driver, it is possible to enable
> > the detachable LUKS header and, as a result, achieve
> > general encryption for any disk format that QEMU has
> > supported.
> >
> > Take the qcow2 as an example, the usage of the generic
> > LUKS encryption as follows:
> >
> > 1. add a protocol blockdev node of data disk
> > $ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> > > "arguments":{"node-name":"libvirt-1-storage", "driver":"file",
> > > "filename":"/path/to/test_disk.qcow2"}}'
> >
> > 2. add a protocol blockdev node of LUKS header as above.
> > $ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> > > "arguments":{"node-name":"libvirt-2-storage", "driver":"file",
> > > "filename": "/path/to/cipher.gluks" }}'
> >
> > 3. add the secret for decrypting the cipher stored in LUKS
> >    header above
> > $ virsh qemu-monitor-command vm '{"execute":"object-add",
> > > "arguments":{"qom-type":"secret", "id":
> > > "libvirt-2-storage-secret0", "data":"abc123"}}'
> >
> > 4. add the qcow2-drived blockdev format node
> > $ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> > > "arguments":{"node-name":"libvirt-1-format", "driver":"qcow2",
> > > "file":"libvirt-1-storage"}}'
> >
> > 5. add the luks-drived blockdev to link the qcow2 disk with
> >    LUKS header by specifying the field "header"
> > $ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> > > "arguments":{"node-name":"libvirt-2-format", "driver":"luks",
> > > "file":"libvirt-1-format", "header":"libvirt-2-storage",
> > > "key-secret":"libvirt-2-format-secret0"}}'
> >
> > 6. add the virtio-blk device finally
> > $ virsh qemu-monitor-command vm '{"execute":"device_add",
> > > "arguments": {"num-queues":"1", "driver":"virtio-blk-pci",
> > > "drive": "libvirt-2-format", "id":"virtio-disk2"}}'
> >
> > The generic LUKS encryption method of starting a virtual
> > machine (VM) is somewhat similar to hot-plug in that both
> > maintaining the same json command while the starting VM
> > changes the "blockdev-add/device_add" parameters to
> > "blockdev/device".
> >
> > Signed-off-by: Hyman Huang <yong.huang@smartx.com>
> > ---
> >  block/crypto.c | 38 +++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 37 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/crypto.c b/block/crypto.c
> > index f82b13d32b..7d70349463 100644
> > --- a/block/crypto.c
> > +++ b/block/crypto.c
> > @@ -40,6 +40,7 @@ struct BlockCrypto {
> >      QCryptoBlock *block;
> >      bool updating_keys;
> >      BdrvChild *header;  /* Reference to the detached LUKS header */
> > +    bool detached_mode; /* If true, LUKS plays a detached header role */
> >  };
> >
> >
> > @@ -64,12 +65,16 @@ static int block_crypto_read_func(QCryptoBlock
> *block,
> >                                    Error **errp)
> >  {
> >      BlockDriverState *bs = opaque;
> > +    BlockCrypto *crypto = bs->opaque;
> >      ssize_t ret;
> >
> >      GLOBAL_STATE_CODE();
> >      GRAPH_RDLOCK_GUARD_MAINLOOP();
> >
> > -    ret = bdrv_pread(bs->file, offset, buflen, buf, 0);
> > +    if (crypto->detached_mode)
> > +        ret = bdrv_pread(crypto->header, offset, buflen, buf, 0);
> > +    else
> > +        ret = bdrv_pread(bs->file, offset, buflen, buf, 0);
>
> This can be simplified to:
>
>     ret = bdrv_pread(bs->header ? bs->header : file, offset, buflen, buf,
> 0);
>

Ok.

>
> >      if (ret < 0) {
> >          error_setg_errno(errp, -ret, "Could not read encryption
> header");
> >          return ret;
> > @@ -269,6 +274,8 @@ static int
> block_crypto_open_generic(QCryptoBlockFormat format,
> >      QCryptoBlockOpenOptions *open_opts = NULL;
> >      unsigned int cflags = 0;
> >      QDict *cryptoopts = NULL;
> > +    const char *header_bdref =
> > +        qdict_get_try_str(options, "header");
> >
> >      GLOBAL_STATE_CODE();
> >
> > @@ -277,6 +284,16 @@ static int
> block_crypto_open_generic(QCryptoBlockFormat format,
> >          return ret;
> >      }
> >
> > +    if (header_bdref) {
> > +        crypto->detached_mode = true;
>
> Drop this flag since it has no benefit.
>
> > +        crypto->header = bdrv_open_child(NULL, options, "header", bs,
> > +                                         &child_of_bds,
> BDRV_CHILD_METADATA,
> > +                                         false, errp);
> > +        if (!crypto->header) {
> > +            return -EINVAL;
> > +        }
> > +    }
> > +
> >      GRAPH_RDLOCK_GUARD_MAINLOOP();
> >
> >      bs->supported_write_flags = BDRV_REQ_FUA &
> > @@ -312,6 +329,14 @@ static int
> block_crypto_open_generic(QCryptoBlockFormat format,
> >          goto cleanup;
> >      }
> >
> > +    if (crypto->detached_mode) {
>
>   if (crypto->header != NULL)
>
> > +        /*
> > +         * Set payload offset to zero as the file bdref has no LUKS
> > +         * header under detached mode.
> > +         */
> > +        qcrypto_block_set_payload_offset(crypto->block, 0);
>
> The LUKS header stores the payload offset.  If someone creates the LUKS
> volume with a detached header, they may still choose to put the payload
> at a non-zero offset.
>
> So AFAICT, we should always honour the payload offset from the header,
> even when detached.   When formatting the header, we should default to
> a zero offset though
>

Agree, I'll code in that way in the next version.

>
> > +    }
> > +
> >      bs->encrypted = true;
> >
> >      ret = 0;
> > @@ -903,6 +928,17 @@ block_crypto_child_perms(BlockDriverState *bs,
> BdrvChild *c,
> >
> >      BlockCrypto *crypto = bs->opaque;
> >
> > +    if (role == (role & BDRV_CHILD_METADATA)) {
> > +        /* Assign read permission only */
> > +        perm |= BLK_PERM_CONSISTENT_READ;
> > +        /* Share all permissions */
> > +        shared |= BLK_PERM_ALL;
> > +
> > +        *nperm = perm;
> > +        *nshared = shared;
> > +        return;
> > +    }
>
> What is code doing ?  You've set  BDRV_CHILD_METADATA role on the
> crypto->header  object, but how does this block_crypto_child_perms
> method get run against crypto->header ?
>
This paragraph originally aims to provide a function that multiple disks
could share
the same LUKS header (with read-only permission).
But I've found that it may not work when updating the detached header after
reviewing the patch recently :(.

Should it be a separate commit, Since it kind of has nothing to do with the
basic
function?

> > patchset.


>
> > +
> >      bdrv_default_perms(bs, c, role, reopen_queue, perm, shared, nperm,
> nshared);
> >
> >      /*
> > --
> > 2.39.1
> >
>
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>
>

-- 
Best regards

[-- Attachment #2: Type: text/html, Size: 11874 bytes --]

Re: [v2 3/4] crypto: Support generic LUKS encryption

[Daniel P. Berrangé]

On Mon, Dec 18, 2023 at 10:15:34PM +0800, Yong Huang wrote:
> On Mon, Dec 18, 2023 at 7:16 PM Daniel P. Berrangé <berrange@redhat.com>
> wrote:

> > > @@ -903,6 +928,17 @@ block_crypto_child_perms(BlockDriverState *bs,
> > BdrvChild *c,
> > >
> > >      BlockCrypto *crypto = bs->opaque;
> > >
> > > +    if (role == (role & BDRV_CHILD_METADATA)) {
> > > +        /* Assign read permission only */
> > > +        perm |= BLK_PERM_CONSISTENT_READ;
> > > +        /* Share all permissions */
> > > +        shared |= BLK_PERM_ALL;
> > > +
> > > +        *nperm = perm;
> > > +        *nshared = shared;
> > > +        return;
> > > +    }
> >
> > What is code doing ?  You've set  BDRV_CHILD_METADATA role on the
> > crypto->header  object, but how does this block_crypto_child_perms
> > method get run against crypto->header ?
> >
> This paragraph originally aims to provide a function that multiple disks
> could share
> the same LUKS header (with read-only permission).
> But I've found that it may not work when updating the detached header after
> reviewing the patch recently :(.
> 
> Should it be a separate commit, Since it kind of has nothing to do with the
> basic
> function?

Yes, that would be better as a separate commit.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[v2 4/4] block: Support detached LUKS header creation for blockdev-create

[Hyman Huang]

Provide the "detached-mode" option for detached LUKS header
formatting.

To format the LUKS header on the pre-creating disk, example
as follows:

1. add a protocol blockdev node of LUKS header
$ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> "arguments":{"node-name":"libvirt-1-storage", "driver":"file",
> "filename":"/path/to/cipher.gluks" }}'

2. add the secret for encrypting the cipher stored in LUKS
   header above
$ virsh qemu-monitor-command vm '{"execute":"object-add",
> "arguments":{"qom-type": "secret", "id":
> "libvirt-1-storage-secret0", "data": "abc123"}}'

3. format the disk node
$ virsh qemu-monitor-command vm '{"execute":"blockdev-create",
> "arguments":{"job-id":"job0", "options":{"driver":"luks",
> "size":0, "file":"libvirt-1-storage", "detached-mode":true,
> "cipher-alg":"aes-256",
> "key-secret":"libvirt-3-storage-encryption-secret0"}}}'

Signed-off-by: Hyman Huang <yong.huang@smartx.com>
---
 block/crypto.c       | 8 +++++++-
 qapi/block-core.json | 5 ++++-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index 7d70349463..e77c49bd0c 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -667,10 +667,12 @@ block_crypto_co_create_luks(BlockdevCreateOptions *create_options, Error **errp)
     BlockDriverState *bs = NULL;
     QCryptoBlockCreateOptions create_opts;
     PreallocMode preallocation = PREALLOC_MODE_OFF;
+    int64_t size;
     int ret;
 
     assert(create_options->driver == BLOCKDEV_DRIVER_LUKS);
     luks_opts = &create_options->u.luks;
+    size = luks_opts->size;
 
     bs = bdrv_co_open_blockdev_ref(luks_opts->file, errp);
     if (bs == NULL) {
@@ -686,7 +688,11 @@ block_crypto_co_create_luks(BlockdevCreateOptions *create_options, Error **errp)
         preallocation = luks_opts->preallocation;
     }
 
-    ret = block_crypto_co_create_generic(bs, luks_opts->size, &create_opts,
+    if (luks_opts->detached_mode) {
+        size = 0;
+    }
+
+    ret = block_crypto_co_create_generic(bs, size, &create_opts,
                                          preallocation, errp);
     if (ret < 0) {
         goto fail;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 10be08d08f..1e7a7e1b05 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4952,13 +4952,16 @@
 # @preallocation: Preallocation mode for the new image (since: 4.2)
 #     (default: off; allowed values: off, metadata, falloc, full)
 #
+# @detached-mode: create a detached LUKS header. (since 9.0)
+#
 # Since: 2.12
 ##
 { 'struct': 'BlockdevCreateOptionsLUKS',
   'base': 'QCryptoBlockCreateOptionsLUKS',
   'data': { 'file':             'BlockdevRef',
             'size':             'size',
-            '*preallocation':   'PreallocMode' } }
+            '*preallocation':   'PreallocMode',
+            '*detached-mode':   'bool'}}
 
 ##
 # @BlockdevCreateOptionsNfs:
-- 
2.39.1

Re: [v2 4/4] block: Support detached LUKS header creation for blockdev-create

[Daniel P. Berrangé]

On Thu, Dec 07, 2023 at 12:37:45AM +0800, Hyman Huang wrote:
> Provide the "detached-mode" option for detached LUKS header
> formatting.
> 
> To format the LUKS header on the pre-creating disk, example
> as follows:
> 
> 1. add a protocol blockdev node of LUKS header
> $ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> > "arguments":{"node-name":"libvirt-1-storage", "driver":"file",
> > "filename":"/path/to/cipher.gluks" }}'
> 
> 2. add the secret for encrypting the cipher stored in LUKS
>    header above
> $ virsh qemu-monitor-command vm '{"execute":"object-add",
> > "arguments":{"qom-type": "secret", "id":
> > "libvirt-1-storage-secret0", "data": "abc123"}}'
> 
> 3. format the disk node
> $ virsh qemu-monitor-command vm '{"execute":"blockdev-create",
> > "arguments":{"job-id":"job0", "options":{"driver":"luks",
> > "size":0, "file":"libvirt-1-storage", "detached-mode":true,
> > "cipher-alg":"aes-256",
> > "key-secret":"libvirt-3-storage-encryption-secret0"}}}'
> 
> Signed-off-by: Hyman Huang <yong.huang@smartx.com>
> ---
>  block/crypto.c       | 8 +++++++-
>  qapi/block-core.json | 5 ++++-
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/block/crypto.c b/block/crypto.c
> index 7d70349463..e77c49bd0c 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -667,10 +667,12 @@ block_crypto_co_create_luks(BlockdevCreateOptions *create_options, Error **errp)
>      BlockDriverState *bs = NULL;
>      QCryptoBlockCreateOptions create_opts;
>      PreallocMode preallocation = PREALLOC_MODE_OFF;
> +    int64_t size;
>      int ret;
>  
>      assert(create_options->driver == BLOCKDEV_DRIVER_LUKS);
>      luks_opts = &create_options->u.luks;
> +    size = luks_opts->size;
>  
>      bs = bdrv_co_open_blockdev_ref(luks_opts->file, errp);
>      if (bs == NULL) {
> @@ -686,7 +688,11 @@ block_crypto_co_create_luks(BlockdevCreateOptions *create_options, Error **errp)
>          preallocation = luks_opts->preallocation;
>      }
>  
> -    ret = block_crypto_co_create_generic(bs, luks_opts->size, &create_opts,
> +    if (luks_opts->detached_mode) {
> +        size = 0;
> +    }
> +
> +    ret = block_crypto_co_create_generic(bs, size, &create_opts,
>                                           preallocation, errp);
>      if (ret < 0) {
>          goto fail;
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 10be08d08f..1e7a7e1b05 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4952,13 +4952,16 @@
>  # @preallocation: Preallocation mode for the new image (since: 4.2)
>  #     (default: off; allowed values: off, metadata, falloc, full)
>  #
> +# @detached-mode: create a detached LUKS header. (since 9.0)
> +#
>  # Since: 2.12
>  ##
>  { 'struct': 'BlockdevCreateOptionsLUKS',
>    'base': 'QCryptoBlockCreateOptionsLUKS',
>    'data': { 'file':             'BlockdevRef',
>              'size':             'size',
> -            '*preallocation':   'PreallocMode' } }
> +            '*preallocation':   'PreallocMode',
> +            '*detached-mode':   'bool'}}

Using a bool flag here is insufficiently flexible. We need to be able to
honour preallocation of the payload device, while using a separate
header.

You need to make the existing 'file' optional, while also adding an
extra optional 'header' field. ie

  { 'struct': 'BlockdevCreateOptionsLUKS',
    'base': 'QCryptoBlockCreateOptionsLUKS',
    'data': { '*file':            'BlockdevRef',
              '*header':          'BlockdevRef',
              'size':             'size',
              '*preallocation':   'PreallocMode' } }


If 'preallocation' is requested, then we must enforce that 'file' is
non-NULL in the code.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [v2 4/4] block: Support detached LUKS header creation for blockdev-create

[Yong Huang]

[-- Attachment #1: Type: text/plain, Size: 4442 bytes --]

On Mon, Dec 18, 2023 at 7:19 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Thu, Dec 07, 2023 at 12:37:45AM +0800, Hyman Huang wrote:
> > Provide the "detached-mode" option for detached LUKS header
> > formatting.
> >
> > To format the LUKS header on the pre-creating disk, example
> > as follows:
> >
> > 1. add a protocol blockdev node of LUKS header
> > $ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> > > "arguments":{"node-name":"libvirt-1-storage", "driver":"file",
> > > "filename":"/path/to/cipher.gluks" }}'
> >
> > 2. add the secret for encrypting the cipher stored in LUKS
> >    header above
> > $ virsh qemu-monitor-command vm '{"execute":"object-add",
> > > "arguments":{"qom-type": "secret", "id":
> > > "libvirt-1-storage-secret0", "data": "abc123"}}'
> >
> > 3. format the disk node
> > $ virsh qemu-monitor-command vm '{"execute":"blockdev-create",
> > > "arguments":{"job-id":"job0", "options":{"driver":"luks",
> > > "size":0, "file":"libvirt-1-storage", "detached-mode":true,
> > > "cipher-alg":"aes-256",
> > > "key-secret":"libvirt-3-storage-encryption-secret0"}}}'
> >
> > Signed-off-by: Hyman Huang <yong.huang@smartx.com>
> > ---
> >  block/crypto.c       | 8 +++++++-
> >  qapi/block-core.json | 5 ++++-
> >  2 files changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/block/crypto.c b/block/crypto.c
> > index 7d70349463..e77c49bd0c 100644
> > --- a/block/crypto.c
> > +++ b/block/crypto.c
> > @@ -667,10 +667,12 @@ block_crypto_co_create_luks(BlockdevCreateOptions
> *create_options, Error **errp)
> >      BlockDriverState *bs = NULL;
> >      QCryptoBlockCreateOptions create_opts;
> >      PreallocMode preallocation = PREALLOC_MODE_OFF;
> > +    int64_t size;
> >      int ret;
> >
> >      assert(create_options->driver == BLOCKDEV_DRIVER_LUKS);
> >      luks_opts = &create_options->u.luks;
> > +    size = luks_opts->size;
> >
> >      bs = bdrv_co_open_blockdev_ref(luks_opts->file, errp);
> >      if (bs == NULL) {
> > @@ -686,7 +688,11 @@ block_crypto_co_create_luks(BlockdevCreateOptions
> *create_options, Error **errp)
> >          preallocation = luks_opts->preallocation;
> >      }
> >
> > -    ret = block_crypto_co_create_generic(bs, luks_opts->size,
> &create_opts,
> > +    if (luks_opts->detached_mode) {
> > +        size = 0;
> > +    }
> > +
> > +    ret = block_crypto_co_create_generic(bs, size, &create_opts,
> >                                           preallocation, errp);
> >      if (ret < 0) {
> >          goto fail;
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 10be08d08f..1e7a7e1b05 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -4952,13 +4952,16 @@
> >  # @preallocation: Preallocation mode for the new image (since: 4.2)
> >  #     (default: off; allowed values: off, metadata, falloc, full)
> >  #
> > +# @detached-mode: create a detached LUKS header. (since 9.0)
> > +#
> >  # Since: 2.12
> >  ##
> >  { 'struct': 'BlockdevCreateOptionsLUKS',
> >    'base': 'QCryptoBlockCreateOptionsLUKS',
> >    'data': { 'file':             'BlockdevRef',
> >              'size':             'size',
> > -            '*preallocation':   'PreallocMode' } }
> > +            '*preallocation':   'PreallocMode',
> > +            '*detached-mode':   'bool'}}
>
> Using a bool flag here is insufficiently flexible. We need to be able to
> honour preallocation of the payload device, while using a separate
> header.
>
> You need to make the existing 'file' optional, while also adding an
> extra optional 'header' field. ie
>
>   { 'struct': 'BlockdevCreateOptionsLUKS',
>     'base': 'QCryptoBlockCreateOptionsLUKS',
>     'data': { '*file':            'BlockdevRef',
>               '*header':          'BlockdevRef',
>               'size':             'size',
>               '*preallocation':   'PreallocMode' } }
>
>
> If 'preallocation' is requested, then we must enforce that 'file' is
> non-NULL in the code.
>

Ok, thanks for the advice, I'll modify it in the next version.

>
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>
>

-- 
Best regards

[-- Attachment #2: Type: text/html, Size: 7228 bytes --]

Re: [v2 0/4] Support generic Luks encryption

[Daniel P. Berrangé]

On Thu, Dec 07, 2023 at 12:37:41AM +0800, Hyman Huang wrote:
> v2:
> - Simplify the design by reusing the LUKS driver to implement
>   the generic Luks encryption, thank Daniel for the insightful 
>   advice.
> - rebase on master. 
> 

> Hyman Huang (4):
>   crypto: Introduce option and structure for detached LUKS header
>   crypto: Introduce payload offset set function
>   crypto: Support generic LUKS encryption
>   block: Support detached LUKS header creation for blockdev-create
> 
>  block/crypto.c         | 47 ++++++++++++++++++++++++++++++++++++++++--
>  crypto/block.c         |  4 ++++
>  include/crypto/block.h |  1 +
>  qapi/block-core.json   | 11 ++++++++--
>  4 files changed, 59 insertions(+), 4 deletions(-)

Could you add a scenario tests/qemu-iotests/tests/luks-detached-header
to provide coverage of this method feature.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [v2 0/4] Support generic Luks encryption

[Yong Huang]

[-- Attachment #1: Type: text/plain, Size: 1316 bytes --]

On Mon, Dec 18, 2023 at 7:21 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Thu, Dec 07, 2023 at 12:37:41AM +0800, Hyman Huang wrote:
> > v2:
> > - Simplify the design by reusing the LUKS driver to implement
> >   the generic Luks encryption, thank Daniel for the insightful
> >   advice.
> > - rebase on master.
> >
>
> > Hyman Huang (4):
> >   crypto: Introduce option and structure for detached LUKS header
> >   crypto: Introduce payload offset set function
> >   crypto: Support generic LUKS encryption
> >   block: Support detached LUKS header creation for blockdev-create
> >
> >  block/crypto.c         | 47 ++++++++++++++++++++++++++++++++++++++++--
> >  crypto/block.c         |  4 ++++
> >  include/crypto/block.h |  1 +
> >  qapi/block-core.json   | 11 ++++++++--
> >  4 files changed, 59 insertions(+), 4 deletions(-)
>
> Could you add a scenario tests/qemu-iotests/tests/luks-detached-header
> to provide coverage of this method feature.
>

Sure, of course.

>
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>
>

-- 
Best regards

[-- Attachment #2: Type: text/html, Size: 2840 bytes --]