Re: [PATCH v2 05/11] block/crypto: implement the encryption key management

[Maxim Levitsky <mlevitsk@redhat.com>]

On Fri, 2019-10-04 at 20:41 +0200, Max Reitz wrote:
> 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.

There is a long story why two write functions are needed.
i tried to use only one, but at the end I and Daniel both agreed
that its just better to have two functions.

The reason is that during creation, the luks BlockDriverState doesn't exist yet,
and so the creation routine basically just writes to the underlying protocol driver.

Thats is why the block_crypto_create_write_func receives a BlockBackend pointer,
to which the BlockDriverState of the underlying protocol driver is inserted.


On the other hand, for amend, the luks block device is open, and it only knows
about its own BlockDriverState, and thus the io should be done on bs->file

So instead of trying to coerce a single callback to do both of this,
we decided to just have a little code duplication.


> 
> > +{
> > +    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.))

You are right! I made this change now and include
it in the next iteration of the patches.

> 
> Max
> 

Best regards,
	Maxim Levitsky