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

[Daniel P. Berrange]

On Fri, Feb 05, 2016 at 10:23:18AM +0000, Daniel P. Berrange wrote:
> 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 <address@hidden>
> > > ---
> > 
> > > +++ b/crypto/ivgen-essiv.c

> > > +++ 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;
>     }

[snip]

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

Actually we must not treat this is an error - we must silently truncate
to 32-bits, in order to retain compatibility with Linux dm-crypt formatted
volumes.


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