Re: [Qemu-devel] [PATCH v4 03/26] crypto: add support for generating initialization vectors

[Eric Blake <eblake@redhat.com>]

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

On 02/29/2016 05:00 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;
> +    size_t nsalt;
> +    QCryptoIVGenESSIV *essiv = g_new0(QCryptoIVGenESSIV, 1);
> +
> +    /* Not neccessarily the same as nkey */

s/neccessarily/necessarily/

> +++ b/include/crypto/ivgen.h

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

Don't you reuse a single in/out buffer later in the series? If so, don't
forget to update the comment at that time (the compiler will only catch
code changes).


> + *
> + * - 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

s/$/./

> + * All the other parameters are unused.
> + *

> +++ b/qapi/crypto.json
> @@ -78,3 +78,22 @@
>  { 'enum': 'QCryptoCipherMode',
>    'prefix': 'QCRYPTO_CIPHER_MODE',
>    'data': ['ecb', 'cbc']}
> +
> +
> +##
> +# QCryptoIVGenAlgorithm:
> +#
> +# The supported algorithms for generating initialization
> +# vectors for full disk encryption. The 'plain' generator
> +# should not be used for disks with sector numbers larger
> +# than 2^32, except where compatibility with pre-existing
> +# Linux dm-crypt volumes is required.
> +#
> +# @plain: 64-bit sector number truncated to 32-bits
> +# @plain64: 64-bit sector number
> +# @essiv: 64-bit sector number encrypted with a hash of the encryption key
> +# Since: 2.6

Worth warning that 'plain' and 'plain64' expose the encrypted disk to
some weaknesses when compared to 'essiv'?

Fixes are minor, so I'm okay if you add:
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 --]