| Message ID | 20190912223028.18496-6-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RFC crypto/luks: encryption key managment using amend interface | expand |
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
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
On 08.11.19 10:30, Maxim Levitsky wrote: > 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. I meant: This doesn’t compile. There’s already another function of this name. Max
On Fri, 2019-11-08 at 11:49 +0100, Max Reitz wrote: > On 08.11.19 10:30, Maxim Levitsky wrote: > > 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. > > I meant: This doesn’t compile. There’s already another function of this > name. > You probably didn't apply the 'block-crypto: misc refactoring' patch, or I forgot to send it. All that patch does is to rename block_crypto_write_func to block_crypto_create_write_func and same (for consistency) for block_crypto_init_func -> block_crypto_create_init_func And then in this patch I add the block_crypto_write_func, to be used for anything but creation code, together with existing block_crypto_read_func which is already not used for creation. Best regards, Maxim Levitsky
On 08.11.19 12:04, Maxim Levitsky wrote: > On Fri, 2019-11-08 at 11:49 +0100, Max Reitz wrote: >> On 08.11.19 10:30, Maxim Levitsky wrote: >>> 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. >> >> I meant: This doesn’t compile. There’s already another function of this >> name. >> > > You probably didn't apply the 'block-crypto: misc refactoring' patch, > or I forgot to send it. Maybe you forgot to mention anywhere that I should. Max
On Fri, 2019-11-08 at 14:12 +0100, Max Reitz wrote: > On 08.11.19 12:04, Maxim Levitsky wrote: > > On Fri, 2019-11-08 at 11:49 +0100, Max Reitz wrote: > > > On 08.11.19 10:30, Maxim Levitsky wrote: > > > > 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. > > > > > > I meant: This doesn’t compile. There’s already another function of this > > > name. > > > > > > > You probably didn't apply the 'block-crypto: misc refactoring' patch, > > or I forgot to send it. > > Maybe you forgot to mention anywhere that I should. Now I remember that this patch was in the original re-factoring patch series, and for some reason it was dropped. Its now locally in my git tree, so I forgot that it wasn't in this patch series already. Sorry for the mess, I'll soon resend the updated patch series. Best regards, Maxim Levitsky
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, };
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(-)