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

["Daniel P. Berrange" <berrange@redhat.com>]

On Thu, Feb 04, 2016 at 03:57:33PM -0700, Eric Blake wrote:
> 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?

Yes

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

I'll remove the cast. I'll also add in

    if (shortsector != sector) {
        error_setg(errp, "Sector '%llu' is too large for 'plain' "
                   "initialization vector generator",
                   (long long unsigned)sector);
        return -1;
    }

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

An older version of the code called the struct  QCryptoBlockIV.
I'll change this to 'ivgen' to match elsewhere.


> > + * 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;' ?

Yes, will add those.

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

I put an error check in as mentioned above.

> > +/**
> > + * 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)?

I feel it is nicer to just be consistent with the user's input.

> > +/**
> > + * qcrypto_ivgen_free:
> > + * @ivgen: the IV generator object
> > + *
> > + * Release all resources associated with @ivgen
> 
> Worth mentioning that it works on NULL?

Yep.

> > +# 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?

Yes, will do.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|