On Mon, Dec 25, 2023 at 01:45:04PM +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.hu...@smartx.com>
> Message-Id:
> <910801f303da1601051479d3b7e5c2c6b4e01eb7.1701879996.git.yong.hu...@smartx.com>
> ---
> block/crypto.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/block/crypto.c b/block/crypto.c
> index f82b13d32b..6063879bac 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -64,12 +64,14 @@ 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);
> + ret = bdrv_pread(crypto->header ? crypto->header : bs->file,
> + offset, buflen, buf, 0);
> if (ret < 0) {
> error_setg_errno(errp, -ret, "Could not read encryption header");
> return ret;
> @@ -269,6 +271,7 @@ static int block_crypto_open_generic(QCryptoBlockFormat
> format,
> QCryptoBlockOpenOptions *open_opts = NULL;
> unsigned int cflags = 0;
> QDict *cryptoopts = NULL;
> + const char *hdr_bdref = qdict_get_try_str(options, "header");
This is an invalid check to make, because it is assuming the user is
referencing a separate blockdev node name and doesn't work for an
inline definition. eg
qemu-img info
'json:{"driver":"luks","file":{"filename":"test-payload.img"},"header":{"filename":"test-header.img"}}'
>
> GLOBAL_STATE_CODE();
>
> @@ -277,6 +280,15 @@ static int block_crypto_open_generic(QCryptoBlockFormat
> format,
> return ret;
> }
>
> + if (hdr_bdref) {
Get rid of this 'if' clause and unconditionally call the next line:
> + crypto->header = bdrv_open_child(NULL, options, "header", bs,
> + &child_of_bds, BDRV_CHILD_METADATA,
> + false, errp);
but pass 'true' instead of 'false' here to allow the child to be absent,
and thus let it return NULL.
> + if (!crypto->header) {
You'll need to then check '*errp != NULL' instead
You'll also need "ERRP_GUARD" at the start of the method
> + return -EINVAL;
> + }
> + }
> +
> GRAPH_RDLOCK_GUARD_MAINLOOP();
>
> bs->supported_write_flags = BDRV_REQ_FUA &
This patch should be combined with the previous patch that adds the new
QAPI schema element as splitting them doesn't add value.
Testing this patch with the changes I suggest above, however, still does
not work:
$ dd if=/dev/zero of=test-header.img bs=1M count=32
$ dd if=/dev/zero of=test-payload.img bs=1M count=1000
$ cryptsetup luksFormat --header test-header.img test-payload.img
--force-password --type luks1
$ qemu-img info
'json:{"driver":"luks","file":{"filename":"test-payload.img"},"header":{"filename":"test-header.img"}}'
qemu-img: Could not open
'json:{"driver":"luks","file":{"filename":"test-payload.img"},"header":{"filename":"test-header.img"}}':
LUKS payload is overlapping with the header
You need to pass some info into qcrypto_block_open to tell it that the
header is detached. Add a new enum entry to QCryptoBlockOpenFlags
perhaps. Then skip the LUKS payload overlap check.
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 :|
