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

[Daniel P. Berrange]

On Wed, Mar 02, 2016 at 05:31:13PM -0700, Eric Blake wrote:
> 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 <address@hidden>
> > ---
> 
> > +++ 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).

In the higher level QCryptoBlock API we encrypt in-place, but this code
example is illustrating the lower layer QCryptoCipher API usage.

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

It is a bit more complicated than that. You have to consider the
combination of ivgen + encryption mode to determine if it is
secure or not. eg plain64 + xts is secure but plain64 + cbc is not
secure. I think explaining the security of the various combinations
is too much detail for the QAPI spec here.

> Fixes are minor, so I'm okay if you add:
> Reviewed-by: Eric Blake <address@hidden>

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