Re: [PATCH v6 07/14] block/crypto: implement the encryption key management

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2883 bytes --]

On 10.05.20 15:40, Maxim Levitsky wrote:
> This implements the encryption key management using the generic code in
> qcrypto layer and exposes it to the user via qemu-img
> 
> This code adds another 'write_func' because the initialization
> write_func works directly on the underlying file, and amend
> works on instance of luks device.
> 
> This commit also adds a 'hack/workaround' I and Kevin Wolf (thanks)
> made to make the driver both support write sharing (to avoid breaking the users),
> and be safe against concurrent  metadata update (the keyslots)
> 
> Eventually the 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
> and then when we want to update the keys, we unshare that permission.
> So if someone else has the image open, even readonly, encryption
> key update will fail gracefully.
> 
> Also thanks to Daniel Berrange for the idea of
> unsharing read, rather that write permission which allows
> to avoid cases when the other user had opened the image read-only.
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  block/crypto.c | 127 +++++++++++++++++++++++++++++++++++++++++++++++--
>  block/crypto.h |  34 +++++++++++++
>  2 files changed, 158 insertions(+), 3 deletions(-)
> 
> diff --git a/block/crypto.c b/block/crypto.c
> index 2e16b62bdc..b14cb0ff06 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c

[...]

> +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;
> +
> +    bdrv_filter_default_perms(bs, c, role, reopen_queue,
> +            perm, shared, nperm, nshared);
> +    /*
> +     * 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, since
> +     * we will unshare that permission while trying to
> +     * update the encryption keys
> +     */
> +    if (!(bs->open_flags & BDRV_O_NO_IO)) {
> +        *nperm |= BLK_PERM_CONSISTENT_READ;
> +    }

I’m not sure this is important, because this really means we won’t do
I/O.  Its only relevant use in this case is for qemu-img info.  Do we
really care if someone edits the key slots while qemu-img info is
processing?

OTOH, I don’t think it’ll harm much.  Well, apart from the fact that
BDRV_O_NO_IO won’t do much for LUKS images.

*shrug*

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]