From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37616) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aRdY0-0001Q0-PJ for qemu-devel@nongnu.org; Fri, 05 Feb 2016 05:23:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aRdXz-0003t1-Ki for qemu-devel@nongnu.org; Fri, 05 Feb 2016 05:23:28 -0500 Date: Fri, 5 Feb 2016 10:23:18 +0000 From: "Daniel P. Berrange" Message-ID: <20160205102318.GC13989@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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <56B3D75D.5080604@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 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 > > > +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 :|