Re: [Qemu-devel] [PATCH v2 04/17] crypto: add support for generating initialization vectors

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6304 bytes --]

On 01/20/2016 10:38 AM, Daniel P. Berrange wrote:
> There are a number of different algorithms that can be used
> to generate initialization vectors for disk encryption. This
> introduces a simple internal QCryptoBlockIV object to provide
> a consistent internal API to the different algorithms. The
> initially implemented algorithms are 'plain', 'plain64' and
> 'essiv', each matching the same named algorithm provided
> by the Linux kernel dm-crypt driver.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---

> +++ b/crypto/ivgen-essiv.c

> +static int qcrypto_ivgen_essiv_init(QCryptoIVGen *ivgen,
> +                                    const uint8_t *key, size_t nkey,
> +                                    Error **errp)
> +{
> +    uint8_t *salt;
> +    size_t nhash;
> +    QCryptoIVGenESSIV *essiv = g_new0(QCryptoIVGenESSIV, 1);
> +
> +    nhash = qcrypto_hash_digest_len(ivgen->hash);
> +    /* Salt must be larger of hash size or key size */
> +    salt = g_new0(uint8_t, nhash > nkey ? nhash : nkey);

Don't we have a handy MAX() macro for that?

> +++ b/crypto/ivgen-plain.c
> +static int qcrypto_ivgen_plain_calculate(QCryptoIVGen *ivgen,
> +                                         uint64_t sector,
> +                                         uint8_t *iv, size_t niv,
> +                                         Error **errp)
> +{
> +    size_t ivprefix;
> +    uint32_t shortsector = cpu_to_le32((uint32_t)(sector & 0xffffffff));

Why do you need both the cast and the mask to 32 bits?

> +++ b/crypto/ivgenpriv.h
> @@ -0,0 +1,49 @@

> +struct QCryptoIVGenDriver {
> +    int (*init)(QCryptoIVGen *biv,

Where'd the name 'biv' come from? But it doesn't affect correctness.

> +++ b/include/crypto/ivgen.h
> @@ -0,0 +1,203 @@

> +
> +/**
> + * This module provides a framework for generating initialization
> + * vectors for block encryption schemes using chained cipher modes
> + * CBC. The principle is that each disk sector is assigned a unique
> + * initialization vector for use for encryption of data in that
> + * sector.
> + *
> + * <example>
> + *   <title>Encrypting block data with initialiation vectors</title>
> + *   <programlisting>

> + * niv =  qcrypto_cipher_get_iv_len(QCRYPTO_CIPHER_ALG_AES_128,
> + *                                  QCRYPTO_CIPHER_MODE_CBC);
> + * iv = g_new0(uint8_t, niv);
> + *
> + *
> + * while (ndata) {
> + *     if (qcrypto_ivgen_calculate(ivgen, sector, iv, niv, errp) < 0) {
> + *         goto error;
> + *     }
> + *     if (qcrypto_cipher_setiv(cipher, iv, niv, errp) < 0) {
> + *         goto error;
> + *     }
> + *     if (qcrypto_cipher_encrypt(cipher,
> + *                                data + (sector * 512),
> + *                                data + (sector * 512),
> + *                                512, errp) < 0) {
> + *         goto error;
> + *     }
> + * }

Umm, this loop is infinite except on errors, because ndata is never
changed.  Are you missing something like 'sector++; ndata -= 512;' ?


> +
> +/**
> + * qcrypto_ivgen_new:
> + * @alg: the initialization vector generation algorithm
> + * @cipheralg: the cipher algorithm or 0
> + * @hash: the hash algorithm or 0,

Inconsistent trailing punctuation.

Why 'or 0'? These two fields are defined by QAPI enums, which start
numbering at 0.  So cipheralg == 0 would by indistinguishable from
requesting the 'aes-128' cipher.  Is the intent to allow a sentinel for
unspecified, when the algorithm can use a default of its choosing?...

> + * @key: the encryption key or NULL
> + * @nkey: the size of @key in bytes
> + *
> + * Create a new initialization vector generator that uses
> + * the algorithm @alg. Whether the remaining parameters
> + * are required or not depends on the choice of @alg
> + * requested.

...Oh, because 0 is okay if @alg won't use the parameter.

> + *
> + * - QCRYPTO_IVGEN_ALG_PLAIN
> + *
> + * The IVs are generated by the 32-bit truncated sector
> + * number. This should never be used for block devices
> + * that are larger than 2^32 sectors in size

Should the code assert/set errp if sector is too large, rather than
blindly truncating it?  I guess it is user-triggerable so assert is
probably bad, but it may still be nice to tell the user up front that
they can't use this mode based on the size of their block device, if we
can figure that out.


> +/**
> + * qcrypto_ivgen_get_cipher:
> + * @ivgen: the IV generator object
> + *
> + * Get the cipher algorithm used by this IV generator (if
> + * applicable)
> + *
> + * Returns: the cipher algorithm
> + */
> +QCryptoCipherAlgorithm qcrypto_ivgen_get_cipher(QCryptoIVGen *ivgen);

Should this return a known and obvious sentinel when the ivgen doesn't
need a cipher, rather than just playing back the user's input (which was
likely 0)?

> +
> +
> +/**
> + * qcrypto_ivgen_get_algorithm:
> + * @ivgen: the IV generator object
> + *
> + * Get the hash algorithm used by this IV generator (if
> + * applicable)
> + *
> + * Returns: the hash algorithm
> + */
> +QCryptoHashAlgorithm qcrypto_ivgen_get_hash(QCryptoIVGen *ivgen);

Copy/paste mismatch in documentation name (this is get_hash, not
get_algorithm)

> +
> +
> +/**
> + * qcrypto_ivgen_free:
> + * @ivgen: the IV generator object
> + *
> + * Release all resources associated with @ivgen

Worth mentioning that it works on NULL?

> +++ b/qapi/crypto.json
> @@ -78,3 +78,19 @@
>  { 'enum': 'QCryptoCipherMode',
>    'prefix': 'QCRYPTO_CIPHER_MODE',
>    'data': ['ecb', 'cbc']}
> +
> +
> +##
> +# QCryptoIVGenAlgorithm:
> +#
> +# The supported algorithms for generating initialization
> +# vectors for full disk encryption
> +#
> +# @plain: 64-bit sector number truncated to 32-bits

Worth a warning to avoid this for disks larger than 2^32 bytes?

> +# @plain64: 64-bit sector number
> +# @essiv: 64-bit sector number encrypted with a hash of the encryption key
> +# Since: 2.6
> +##
> +{ 'enum': 'QCryptoIVGenAlgorithm',
> +  'prefix': 'QCRYPTO_IVGEN_ALG',
> +  'data': ['plain', 'plain64', 'essiv']}


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


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