Re: [Qemu-block] [PATCH v1 1/6] crypto: add support for querying parameters for block encryption

[Eric Blake]

On 06/07/2016 04:11 AM, Daniel P. Berrange wrote:
> When creating new block encryption volumes, we accept a list of
> parameters to control the formatting process. It is useful to
> be able to query what those parameters were for existing block
> devices. Add a qcrypto_block_get_info() method which returns a
> QCryptoBlockInfo instance to report this data.
> 
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
>  crypto/block-luks.c    | 66 
> +++++++++++++++++++++++++++++++++++++++++++++++++-
>  crypto/block.c         | 17 +++++++++++++
>  crypto/blockpriv.h     |  4 +++
>  include/crypto/block.h | 16 ++++++++++++
>  qapi/crypto.json       | 65 +++++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 167 insertions(+), 1 deletion(-)
> 
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index 17c4300..1c8e4d6 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c
> @@ -201,6 +201,15 @@ QEMU_BUILD_BUG_ON(sizeof(struct QCryptoBlockLUKSHeader) 
> != 592);
>  
>  struct QCryptoBlockLUKS {
>      QCryptoBlockLUKSHeader header;
> +
> +    /* Cache parsed versions of what's in header fields.

s/\./,/


> @@ -947,7 +962,6 @@ qcrypto_block_luks_create(QCryptoBlock *block,
>      }
>      hash_alg = QCryptoHashAlgorithm_lookup[luks_opts.hash_alg];
>  
> -
>      if (strlen(cipher_alg) >= QCRYPTO_BLOCK_LUKS_CIPHER_NAME_LEN) {

Unrelated cleanup, but I guess it's okay.

> +    info->u.luks.hash_alg = luks->hash_alg;
> +    info->u.luks.payload_offset = block->payload_offset;
> +    info->u.luks.master_key_iters = luks->header.master_key_iterations;
> +    info->u.luks.uuid = g_strdup((const char *)luks->header.uuid);

Cast is necessary because the header declared it as uint8_t[]; fair enough.

> +
> +    for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
> +        slots = g_new0(QCryptoBlockInfoLUKSSlotList, 1);
> +        if (i == 0) {
> +            info->u.luks.slots = slots;
> +        } else {
> +            prev->next = slots;
> +        }
> +
> +        slots->value = slot = g_new0(QCryptoBlockInfoLUKSSlot, 1);
> +        slot->active = luks->header.key_slots[i].active ==
> +            QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED;
> +        slot->iters = luks->header.key_slots[i].iterations;
> +        slot->stripes = luks->header.key_slots[i].stripes;

See my comment on cover letter, on whether iters and stripes need to be
filled out for inactive slots.


> +++ b/include/crypto/block.h
> @@ -138,6 +138,22 @@ QCryptoBlock 
> *qcrypto_block_create(QCryptoBlockCreateOptions *options,
>                                     void *opaque,
>                                     Error **errp);
>  
> +
> +/**
> + * qcrypto_block_get_info:
> + * block:L the block encryption object

stray L

> +++ b/qapi/crypto.json
> @@ -220,3 +220,68 @@
>    'discriminator': 'format',
>    'data': { 'qcow': 'QCryptoBlockOptionsQCow',
>              'luks': 'QCryptoBlockCreateOptionsLUKS' } }
> +
> +
> +##
> +# QCryptoBlockInfoBase:
> +#
> +# The common information that applies to all full disk
> +# encryption formats
> +#
> +# @format: the encryption format
> +#
> +# Since: 2.7
> +##
> +{ 'struct': 'QCryptoBlockInfoBase',
> +  'data': { 'format': 'QCryptoBlockFormat' }}
> +

Another candidate for my anonymous union base, once my qapi patches
land. Nothing for you to change now, though.

> +
> +##
> +# QCryptoBlockInfoLUKSSlot:
> +#
> +# Information about the LUKS block encryption key
> +# slot options
> +#

Missing ## terminator and description of the members. Plus a decision on
whether things should be optional.

> +{ 'struct': 'QCryptoBlockInfoLUKSSlot',
> +  'data': {'active': 'bool',
> +           'iters': 'int',
> +           'stripes': 'int',
> +           'key-offset': 'int' } }
> +
> +
> +##
> +# QCryptoBlockInfoLUKS:
> +#
> +# Information about the LUKS block encryption options
> +#
> +# @cipher-alg: the cipher algorithm for data encryption
> +# @cipher-mode: the cipher mode for data encryption
> +# @ivgen-alg: the initialization vector generator
> +# @ivgen-hash-alg: the initialization vector generator hash

Missing #optional marker.

> +# @hash-alg: the master key hash algorithm

Missing docs on payload-offset, master-key-iters,  uuid, and slots

> +#
> +# Since: 2.7
> +##
> +{ 'struct': 'QCryptoBlockInfoLUKS',
> +  'data': {'cipher-alg': 'QCryptoCipherAlgorithm',
> +           'cipher-mode': 'QCryptoCipherMode',
> +           'ivgen-alg': 'QCryptoIVGenAlgorithm',
> +           '*ivgen-hash-alg': 'QCryptoHashAlgorithm',
> +           'hash-alg': 'QCryptoHashAlgorithm',
> +           'payload-offset': 'int',
> +           'master-key-iters': 'int',
> +           'uuid': 'str',
> +           'slots': [ 'QCryptoBlockInfoLUKSSlot' ] }}
> +
> +
> +##
> +# QCryptoBlockInfo:
> +#
> +# Information about the block encryption options
> +#
> +# Since: 2.7
> +##
> +{ 'union': 'QCryptoBlockInfo',
> +  'base': 'QCryptoBlockInfoBase',
> +  'discriminator': 'format',
> +  'data': { 'luks': 'QCryptoBlockInfoLUKS' } }
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature