| Message ID | 910801f303da1601051479d3b7e5c2c6b4e01eb7.1701879996.git.yong.huang@smartx.com |
|---|---|
| State | New |
| Headers | show |
| Series | Support generic Luks encryption | expand |
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
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 :| > >
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
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); /*
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(-)