[v2,05/11] block/crypto: implement the encryption key management
diff mbox series

Message ID 20190912223028.18496-6-mlevitsk@redhat.com
State New
Headers show
Series
  • RFC crypto/luks: encryption key managment using amend interface
Related show

Commit Message

Maxim Levitsky Sept. 12, 2019, 10:30 p.m. UTC
This implements the encryption key management
using the generic code in qcrypto layer
(currently only for qemu-img amend)

This code adds another 'write_func' because the initialization
write_func works directly on the underlying file,
because during the creation, there is no open instance
of the luks driver, but during regular use, we have it,
and should use it instead.


This commit also adds a	'hack/workaround' I and	Kevin Wolf (thanks)
made to	make the driver	still support write sharing,
but be safe against concurrent  metadata update (the keys)
Eventually write sharing for luks driver will be deprecated
and removed together with this hack.

The hack is that we ask	(as a format driver) for
BLK_PERM_CONSISTENT_READ always
(technically always unless opened with BDRV_O_NO_IO)

and then when we want to update	the keys, we
unshare	that permission. So if someone else
has the	image open, even readonly, this	will fail.

Also thanks to Daniel Berrange for the variant of
that hack that involves	asking for read,
rather that write permission

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 block/crypto.c | 118 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 115 insertions(+), 3 deletions(-)

Comments

Max Reitz Oct. 4, 2019, 6:41 p.m. UTC | #1
On 13.09.19 00:30, Maxim Levitsky wrote:
> This implements the encryption key management
> using the generic code in qcrypto layer
> (currently only for qemu-img amend)
> 
> This code adds another 'write_func' because the initialization
> write_func works directly on the underlying file,
> because during the creation, there is no open instance
> of the luks driver, but during regular use, we have it,
> and should use it instead.
> 
> 
> This commit also adds a	'hack/workaround' I and	Kevin Wolf (thanks)
> made to	make the driver	still support write sharing,
> but be safe against concurrent  metadata update (the keys)
> Eventually write sharing for luks driver will be deprecated
> and removed together with this hack.
> 
> The hack is that we ask	(as a format driver) for
> BLK_PERM_CONSISTENT_READ always
> (technically always unless opened with BDRV_O_NO_IO)
> 
> and then when we want to update	the keys, we
> unshare	that permission. So if someone else
> has the	image open, even readonly, this	will fail.
> 
> Also thanks to Daniel Berrange for the variant of
> that hack that involves	asking for read,
> rather that write permission
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  block/crypto.c | 118 +++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 115 insertions(+), 3 deletions(-)
> 
> diff --git a/block/crypto.c b/block/crypto.c
> index a6a3e1f1d8..f42fa057e6 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -36,6 +36,7 @@ typedef struct BlockCrypto BlockCrypto;
>  
>  struct BlockCrypto {
>      QCryptoBlock *block;
> +    bool updating_keys;
>  };
>  
>  
> @@ -70,6 +71,24 @@ static ssize_t block_crypto_read_func(QCryptoBlock *block,
>      return ret;
>  }
>  
> +static ssize_t block_crypto_write_func(QCryptoBlock *block,
> +                                       size_t offset,
> +                                       const uint8_t *buf,
> +                                       size_t buflen,
> +                                       void *opaque,
> +                                       Error **errp)

There’s already a function of this name for creation.

> +{
> +    BlockDriverState *bs = opaque;
> +    ssize_t ret;
> +
> +    ret = bdrv_pwrite(bs->file, offset, buf, buflen);
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret, "Could not write encryption header");
> +        return ret;
> +    }
> +    return ret;
> +}
> +
>  
>  struct BlockCryptoCreateData {
>      BlockBackend *blk;

[...]

> +static void
> +block_crypto_child_perms(BlockDriverState *bs, BdrvChild *c,
> +                         const BdrvChildRole *role,
> +                         BlockReopenQueue *reopen_queue,
> +                         uint64_t perm, uint64_t shared,
> +                         uint64_t *nperm, uint64_t *nshared)
> +{
> +
> +    BlockCrypto *crypto = bs->opaque;
> +
> +    /*
> +     * Ask for consistent read permission so that if
> +     * someone else tries to open this image with this permission
> +     * neither will be able to edit encryption keys
> +     */
> +    if (!(bs->open_flags & BDRV_O_NO_IO)) {
> +        perm |= BLK_PERM_CONSISTENT_READ;
> +    }
> +
> +    /*
> +     * This driver doesn't modify LUKS metadata except
> +     * when updating the encryption slots.
> +     * Thus unlike a proper format driver we don't ask for
> +     * shared write permission. However we need it
> +     * when we area updating keys, to ensure that only we
> +     * had opened the device r/w
> +     *
> +     * Encryption update will set the crypto->updating_keys
> +     * during that period and refresh permissions
> +     *
> +     */
> +
> +    if (crypto->updating_keys) {
> +        /*need exclusive write access for header update  */
> +        perm |= BLK_PERM_WRITE;
> +        shared &= ~(BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE);
> +    }
> +
> +    bdrv_filter_default_perms(bs, c, role, reopen_queue,
> +            perm, shared, nperm, nshared);
> +}

This will probably work, but usually drivers do it the other way around:
First call any of the default_perms(), and then adjust *nperm and
*nshared as required.

(perm/shared are what the parents need, *nperm/*nshared is what this
driver needs, so it makes more sense that way; and this way nobody has
to check whether the settings survived the default_perms() call.)

((But the permissions themselves do look correct.))

Max

Patch
diff mbox series

diff --git a/block/crypto.c b/block/crypto.c
index a6a3e1f1d8..f42fa057e6 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -36,6 +36,7 @@  typedef struct BlockCrypto BlockCrypto;
 
 struct BlockCrypto {
     QCryptoBlock *block;
+    bool updating_keys;
 };
 
 
@@ -70,6 +71,24 @@  static ssize_t block_crypto_read_func(QCryptoBlock *block,
     return ret;
 }
 
+static ssize_t block_crypto_write_func(QCryptoBlock *block,
+                                       size_t offset,
+                                       const uint8_t *buf,
+                                       size_t buflen,
+                                       void *opaque,
+                                       Error **errp)
+{
+    BlockDriverState *bs = opaque;
+    ssize_t ret;
+
+    ret = bdrv_pwrite(bs->file, offset, buf, buflen);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "Could not write encryption header");
+        return ret;
+    }
+    return ret;
+}
+
 
 struct BlockCryptoCreateData {
     BlockBackend *blk;
@@ -647,6 +666,100 @@  block_crypto_get_specific_info_luks(BlockDriverState *bs, Error **errp)
     return spec_info;
 }
 
+
+static int
+block_crypto_amend_options(BlockDriverState *bs,
+                           QemuOpts *opts,
+                           BlockDriverAmendStatusCB *status_cb,
+                           void *cb_opaque,
+                           bool force,
+                           Error **errp)
+{
+    BlockCrypto *crypto = bs->opaque;
+    QDict *cryptoopts = NULL;
+    QCryptoBlockCreateOptions *amend_options = NULL;
+    int ret;
+
+    assert(crypto);
+    assert(crypto->block);
+
+    crypto->updating_keys = true;
+
+    ret = bdrv_child_refresh_perms(bs, bs->file, errp);
+    if (ret < 0) {
+        goto cleanup;
+    }
+
+    cryptoopts = qemu_opts_to_qdict_filtered(opts, NULL,
+                                             &block_crypto_create_opts_luks,
+                                             true);
+
+    qdict_put_str(cryptoopts, "format", "luks");
+    amend_options = block_crypto_create_opts_init(cryptoopts, errp);
+    if (!amend_options) {
+        ret = -EINVAL;
+        goto cleanup;
+    }
+
+    ret = qcrypto_block_amend_options(crypto->block,
+                                      block_crypto_read_func,
+                                      block_crypto_write_func,
+                                      bs,
+                                      amend_options,
+                                      force,
+                                      errp);
+cleanup:
+    crypto->updating_keys = false;
+    bdrv_child_refresh_perms(bs, bs->file, errp);
+    qapi_free_QCryptoBlockCreateOptions(amend_options);
+    qobject_unref(cryptoopts);
+    return ret;
+}
+
+
+static void
+block_crypto_child_perms(BlockDriverState *bs, BdrvChild *c,
+                         const BdrvChildRole *role,
+                         BlockReopenQueue *reopen_queue,
+                         uint64_t perm, uint64_t shared,
+                         uint64_t *nperm, uint64_t *nshared)
+{
+
+    BlockCrypto *crypto = bs->opaque;
+
+    /*
+     * Ask for consistent read permission so that if
+     * someone else tries to open this image with this permission
+     * neither will be able to edit encryption keys
+     */
+    if (!(bs->open_flags & BDRV_O_NO_IO)) {
+        perm |= BLK_PERM_CONSISTENT_READ;
+    }
+
+    /*
+     * This driver doesn't modify LUKS metadata except
+     * when updating the encryption slots.
+     * Thus unlike a proper format driver we don't ask for
+     * shared write permission. However we need it
+     * when we area updating keys, to ensure that only we
+     * had opened the device r/w
+     *
+     * Encryption update will set the crypto->updating_keys
+     * during that period and refresh permissions
+     *
+     */
+
+    if (crypto->updating_keys) {
+        /*need exclusive write access for header update  */
+        perm |= BLK_PERM_WRITE;
+        shared &= ~(BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE);
+    }
+
+    bdrv_filter_default_perms(bs, c, role, reopen_queue,
+            perm, shared, nperm, nshared);
+}
+
+
 static const char *const block_crypto_strong_runtime_opts[] = {
     BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET,
 
@@ -659,9 +772,7 @@  static BlockDriver bdrv_crypto_luks = {
     .bdrv_probe         = block_crypto_probe_luks,
     .bdrv_open          = block_crypto_open_luks,
     .bdrv_close         = block_crypto_close,
-    /* This driver doesn't modify LUKS metadata except when creating image.
-     * Allow share-rw=on as a special case. */
-    .bdrv_child_perm    = bdrv_filter_default_perms,
+    .bdrv_child_perm    = block_crypto_child_perms,
     .bdrv_co_create     = block_crypto_co_create_luks,
     .bdrv_co_create_opts = block_crypto_co_create_opts_luks,
     .bdrv_co_truncate   = block_crypto_co_truncate,
@@ -674,6 +785,7 @@  static BlockDriver bdrv_crypto_luks = {
     .bdrv_getlength     = block_crypto_getlength,
     .bdrv_get_info      = block_crypto_get_info_luks,
     .bdrv_get_specific_info = block_crypto_get_specific_info_luks,
+    .bdrv_amend_options = block_crypto_amend_options,
 
     .strong_runtime_opts = block_crypto_strong_runtime_opts,
 };