From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58703) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aRSqL-0002ll-DB for qemu-devel@nongnu.org; Thu, 04 Feb 2016 17:57:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aRSqK-0002qu-1I for qemu-devel@nongnu.org; Thu, 04 Feb 2016 17:57:41 -0500 References: <1453311539-1193-1-git-send-email-berrange@redhat.com> <1453311539-1193-5-git-send-email-berrange@redhat.com> From: Eric Blake Message-ID: <56B3D75D.5080604@redhat.com> Date: Thu, 4 Feb 2016 15:57:33 -0700 MIME-Version: 1.0 In-Reply-To: <1453311539-1193-5-git-send-email-berrange@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="eutEevPM6B9o07hj7E5hhdwxpp8vvR9bh" Subject: Re: [Qemu-devel] [PATCH v2 04/17] crypto: add support for generating initialization vectors List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" , qemu-devel@nongnu.org Cc: Kevin Wolf , Fam Zheng , qemu-block@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --eutEevPM6B9o07hj7E5hhdwxpp8vvR9bh Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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. >=20 > 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 =3D g_new0(QCryptoIVGenESSIV, 1); > + > + nhash =3D qcrypto_hash_digest_len(ivgen->hash); > + /* Salt must be larger of hash size or key size */ > + salt =3D g_new0(uint8_t, nhash > nkey ? nhash : nkey); Don't we have a handy MAX() macro for that? > +++ 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 =3D cpu_to_le32((uint32_t)(sector & 0xfffffff= f)); Why do you need both the cast and the mask to 32 bits? > +++ 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. > +++ b/include/crypto/ivgen.h > @@ -0,0 +1,203 @@ > + > +/** > + * This module provides a framework for generating initialization > + * vectors for block encryption schemes using chained cipher modes > + * CBC. The principle is that each disk sector is assigned a unique > + * initialization vector for use for encryption of data in that > + * sector. > + * > + * > + * Encrypting block data with initialiation vectors > + * > + * niv =3D qcrypto_cipher_get_iv_len(QCRYPTO_CIPHER_ALG_AES_128, > + * QCRYPTO_CIPHER_MODE_CBC); > + * iv =3D g_new0(uint8_t, niv); > + * > + * > + * 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 -=3D 512;' ? > + > +/** > + * qcrypto_ivgen_new: > + * @alg: the initialization vector generation algorithm > + * @cipheralg: the cipher algorithm or 0 > + * @hash: the hash algorithm or 0, Inconsistent trailing punctuation. Why 'or 0'? These two fields are defined by QAPI enums, which start numbering at 0. So cipheralg =3D=3D 0 would by indistinguishable from requesting the 'aes-128' cipher. Is the intent to allow a sentinel for unspecified, when the algorithm can use a default of its choosing?... > + * @key: the encryption key or NULL > + * @nkey: the size of @key in bytes > + * > + * Create a new initialization vector generator that uses > + * the algorithm @alg. Whether the remaining parameters > + * are required or not depends on the choice of @alg > + * requested. =2E..Oh, because 0 is okay if @alg won't use the parameter. > + * > + * - 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. > +/** > + * 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)? > + > + > +/** > + * qcrypto_ivgen_get_algorithm: > + * @ivgen: the IV generator object > + * > + * Get the hash algorithm used by this IV generator (if > + * applicable) > + * > + * Returns: the hash algorithm > + */ > +QCryptoHashAlgorithm qcrypto_ivgen_get_hash(QCryptoIVGen *ivgen); Copy/paste mismatch in documentation name (this is get_hash, not get_algorithm) > + > + > +/** > + * qcrypto_ivgen_free: > + * @ivgen: the IV generator object > + * > + * Release all resources associated with @ivgen Worth mentioning that it works on NULL? > +++ b/qapi/crypto.json > @@ -78,3 +78,19 @@ > { 'enum': 'QCryptoCipherMode', > 'prefix': 'QCRYPTO_CIPHER_MODE', > 'data': ['ecb', 'cbc']} > + > + > +## > +# 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? > +# @plain64: 64-bit sector number > +# @essiv: 64-bit sector number encrypted with a hash of the encryption= key > +# Since: 2.6 > +## > +{ 'enum': 'QCryptoIVGenAlgorithm', > + 'prefix': 'QCRYPTO_IVGEN_ALG', > + 'data': ['plain', 'plain64', 'essiv']} --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --eutEevPM6B9o07hj7E5hhdwxpp8vvR9bh Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJWs9ddAAoJEKeha0olJ0NqbsYH/2WpFaI32zBKlVlIe19mO3iE zwZm9wYJ1SGKHZq/uyTPt+T6S0V36j0K5viGx8tAF1/QvH4bsGsu+YvhZYShdhWT OXy7yOd00f4Ou5SLoIOLbwVUnKv3hIERNJfVZecxGXJTd3PxifqRci3vWMaokBxR +RwbUGfiJ3IrTiKrS8R1seg3AaAPdpC6bSxMs5nB9DN0JtbyQC/vm0s/016txd2T QSrJz5o1AFKvRVwk5p1BApfqLMXZnfgy+1C4r0tCAYrnEZ2hH9SxtVSFbwgOO3Zl +pLsCov1m7XlAEeI2mhdVTLjy8p91GDtUATmwiHIIbnSk43xEyU2m9VK5xzNPE0= =63O5 -----END PGP SIGNATURE----- --eutEevPM6B9o07hj7E5hhdwxpp8vvR9bh--