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

["Daniel P. Berrangé" <berrange@redhat.com>]

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 :|