From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46677) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aRgMZ-0002Ct-TP for qemu-devel@nongnu.org; Fri, 05 Feb 2016 08:23:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aRgMY-0003ND-Pk for qemu-devel@nongnu.org; Fri, 05 Feb 2016 08:23:51 -0500 Date: Fri, 5 Feb 2016 13:23:41 +0000 From: "Daniel P. Berrange" Message-ID: <20160205132341.GK13989@redhat.com> References: <1453311539-1193-1-git-send-email-berrange@redhat.com> <1453311539-1193-5-git-send-email-berrange@redhat.com> <56B3D75D.5080604@redhat.com> <20160205102318.GC13989@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20160205102318.GC13989@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 04/17] crypto: add support for generating initialization vectors Reply-To: "Daniel P. Berrange" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Kevin Wolf , Fam Zheng , qemu-devel@nongnu.org, qemu-block@nongnu.org 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 > > > --- > > > > > +++ 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 :|