[RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Ard Biesheuvel]

This series is presented as an RFC for a couple of reasons:
- it is only build tested
- it is unclear whether this is the right way to move away from the use of
  bare ciphers in non-crypto code
- we haven't really discussed whether moving away from the use of bare ciphers
  in non-crypto code is a goal we agree on

This series creates an ESSIV shash template that takes a (cipher,hash) tuple,
where the digest size of the hash must be a valid key length for the cipher.
The setkey() operation takes the hash of the input key, and sets into the
cipher as the encryption key. Digest operations accept input up to the
block size of the cipher, and perform a single block encryption operation to
produce the ESSIV output.

This matches what both users of ESSIV in the kernel do, and so it is proposed
as a replacement for those, in patches #2 and #3.

As for the discussion: the code is untested, so it is presented for discussion
only. I'd like to understand whether we agree that phasing out the bare cipher
interface from non-crypto code is a good idea, and whether this approach is
suitable for fscrypt and dm-crypt.

Remaining work:
- wiring up some essiv(x,y) combinations into the testing framework. I wonder
  if anything other than essiv(aes,sha256) makes sense.
- testing - suggestions welcome on existing testing frameworks for dm-crypt
  and/or fscrypt

Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Eric Biggers <ebiggers@google.com>

Ard Biesheuvel (3):
  crypto: essiv - create a new shash template for IV generation
  dm crypt: switch to essiv shash
  fscrypt: switch to ESSIV shash

 crypto/Kconfig              |   3 +
 crypto/Makefile             |   1 +
 crypto/essiv.c              | 275 ++++++++++++++++++++
 drivers/md/Kconfig          |   1 +
 drivers/md/dm-crypt.c       | 137 ++--------
 fs/crypto/Kconfig           |   1 +
 fs/crypto/crypto.c          |  11 +-
 fs/crypto/fscrypt_private.h |   4 +-
 fs/crypto/keyinfo.c         |  64 +----
 9 files changed, 321 insertions(+), 176 deletions(-)
 create mode 100644 crypto/essiv.c

-- 
2.20.1

[RFC PATCH 1/3] crypto: essiv - create a new shash template for IV generation

[Ard Biesheuvel]

Two different users of ESSIV (encrypted salt|sector IV) implement the
algorithm using bare shash and cipher transform. This is not a huge
deal, since the algorithm is fairly simple, but it does require us to
keep the cipher interface public. Since the cipher interface is often
used incorrectly, and typically is not the correct primitive to use
outside of the crypto API, the intention is to turn it into an internal
primitive, only to be used by other crypto code.

In anticipation of that, this driver moves the essiv handling into the
crypto subsystem, where it can keep using the cipher interface. This
will also permit accelerated implementations to be provided, that
implement all the transformations directly rather than wiring together
other transforms.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 crypto/Kconfig  |   3 +
 crypto/Makefile |   1 +
 crypto/essiv.c  | 275 ++++++++++++++++++++
 3 files changed, 279 insertions(+)

diff --git a/crypto/Kconfig b/crypto/Kconfig
index efeb307c0594..2e040514fa44 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -1882,6 +1882,9 @@ config CRYPTO_STATS
 config CRYPTO_HASH_INFO
 	bool
 
+config CRYPTO_ESSIV
+	tristate
+
 source "drivers/crypto/Kconfig"
 source "crypto/asymmetric_keys/Kconfig"
 source "certs/Kconfig"
diff --git a/crypto/Makefile b/crypto/Makefile
index 266a4cdbb9e2..ad1d99ba6d56 100644
--- a/crypto/Makefile
+++ b/crypto/Makefile
@@ -148,6 +148,7 @@ obj-$(CONFIG_CRYPTO_USER_API_AEAD) += algif_aead.o
 obj-$(CONFIG_CRYPTO_ZSTD) += zstd.o
 obj-$(CONFIG_CRYPTO_OFB) += ofb.o
 obj-$(CONFIG_CRYPTO_ECC) += ecc.o
+obj-$(CONFIG_CRYPTO_ESSIV) += essiv.o
 
 ecdh_generic-y += ecdh.o
 ecdh_generic-y += ecdh_helper.o
diff --git a/crypto/essiv.c b/crypto/essiv.c
new file mode 100644
index 000000000000..b985de394aa6
--- /dev/null
+++ b/crypto/essiv.c
@@ -0,0 +1,275 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Cryptographic API.
+ *
+ * ESSIV: encrypted sector/salt initial vector (for block encryption)
+ *
+ * Copyright (c) 2019 Ard Biesheuvel <ard.biesheuvel@linaro.org>
+ */
+
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+
+#include <crypto/internal/hash.h>
+
+struct essiv_instance_ctx {
+	struct crypto_shash_spawn	hash;
+	struct crypto_spawn		enc;
+};
+
+struct essiv_tfm_ctx {
+	struct crypto_shash		*hash;
+	struct crypto_cipher		*enc;
+};
+
+struct essiv_desc_ctx {
+	unsigned int			len;
+	unsigned int			max;
+	u8				buf[];
+};
+
+static int essiv_setkey(struct crypto_shash *tfm, const u8 *inkey,
+			unsigned int keylen)
+{
+	struct essiv_tfm_ctx *ctx = crypto_shash_ctx(tfm);
+	SHASH_DESC_ON_STACK(desc, ctx->hash);
+	u8 *digest = NULL;
+	int ret;
+
+	if (keylen) {
+		digest = kmalloc(crypto_shash_digestsize(ctx->hash),
+				 GFP_KERNEL);
+		if (!digest)
+			return -ENOMEM;
+
+		desc->tfm = ctx->hash;
+		crypto_shash_digest(desc, inkey, keylen, digest);
+	}
+	ret = crypto_cipher_setkey(ctx->enc,
+				   digest ?: page_address(ZERO_PAGE(0)),
+				   crypto_shash_digestsize(ctx->hash));
+
+	kzfree(digest);
+	return ret;
+}
+
+static int essiv_init(struct shash_desc *desc)
+{
+	struct essiv_desc_ctx *ctx = shash_desc_ctx(desc);
+
+	ctx->len = 0;
+	ctx->max = crypto_shash_digestsize(desc->tfm);
+	memset(ctx->buf, 0, ctx->max);
+
+	return 0;
+}
+
+static int essiv_update(struct shash_desc *desc, const u8 *p, unsigned int len)
+{
+	struct essiv_desc_ctx *ctx = shash_desc_ctx(desc);
+	int nbytes = min(len, ctx->max - ctx->len);
+
+	/* only permit input up to the block size of the cipher */
+	if (nbytes < len)
+		return -EINVAL;
+
+	memcpy(ctx->buf + ctx->len, p, nbytes);
+	ctx->len += nbytes;
+
+	return 0;
+}
+
+static int essiv_final(struct shash_desc *desc, u8 *out)
+{
+	struct essiv_tfm_ctx *tctx = crypto_shash_ctx(desc->tfm);
+	struct essiv_desc_ctx *ctx = shash_desc_ctx(desc);
+
+	crypto_cipher_encrypt_one(tctx->enc, out, ctx->buf);
+
+	return 0;
+}
+
+static int essiv_digest(struct shash_desc *desc, const u8 *p, unsigned int len,
+			u8 *out)
+{
+	struct essiv_tfm_ctx *tctx = crypto_shash_ctx(desc->tfm);
+	struct essiv_desc_ctx *ctx = shash_desc_ctx(desc);
+
+	if (len > ctx->max)
+		return -EINVAL;
+
+	memcpy(ctx->buf, p, len);
+	crypto_cipher_encrypt_one(tctx->enc, out, ctx->buf);
+
+	return 0;
+}
+
+static int essiv_init_tfm(struct crypto_tfm *tfm)
+{
+	struct crypto_instance *inst = (void *)tfm->__crt_alg;
+	struct essiv_instance_ctx *ictx = crypto_instance_ctx(inst);
+	struct essiv_tfm_ctx *tctx = crypto_tfm_ctx(tfm);
+
+	tctx->hash = crypto_spawn_shash(&ictx->hash);
+	if (IS_ERR(tctx->hash))
+		return PTR_ERR(tctx->hash);
+
+	tctx->enc = crypto_spawn_cipher(&ictx->enc);
+	if (IS_ERR(tctx->enc)) {
+		crypto_free_shash(tctx->hash);
+		return PTR_ERR(tctx->hash);
+	}
+	return 0;
+};
+
+static void essiv_exit_tfm(struct crypto_tfm *tfm)
+{
+	struct essiv_tfm_ctx *tctx = crypto_tfm_ctx(tfm);
+
+	crypto_free_cipher(tctx->enc);
+	crypto_free_shash(tctx->hash);
+}
+
+static void crypto_essiv_free(struct crypto_instance *inst)
+{
+	struct essiv_instance_ctx *ictx = crypto_instance_ctx(inst);
+
+	crypto_drop_shash(&ictx->hash);
+	crypto_drop_spawn(&ictx->enc);
+	kfree(inst);
+}
+
+static int crypto_essiv_create(struct crypto_template *tmpl,
+			       struct rtattr **tb)
+{
+	struct shash_instance *inst;
+	struct essiv_instance_ctx *ictx;
+	struct crypto_alg *hash_alg, *enc_alg;
+	struct cipher_alg *calg;
+	struct shash_alg *salg;
+	int err;
+
+	err = crypto_check_attr_type(tb, CRYPTO_ALG_TYPE_SHASH);
+	if (err)
+		return err;
+
+	salg = shash_attr_alg(tb[1], 0, 0);
+	if (IS_ERR(salg))
+		return PTR_ERR(salg);
+	hash_alg = &salg->base;
+
+	/* The underlying hash algorithm must be unkeyed */
+	err = -EINVAL;
+	if (crypto_shash_alg_has_setkey(salg)) {
+		pr_err("essiv: keyed hash algorithm '%s' not supported\n",
+		       hash_alg->cra_driver_name);
+		goto out_put_hash_alg;
+	}
+
+	enc_alg = crypto_attr_alg(tb[2], CRYPTO_ALG_TYPE_CIPHER,
+				     CRYPTO_ALG_TYPE_MASK);
+	if (IS_ERR(enc_alg)) {
+		err = PTR_ERR(enc_alg);
+		goto out_put_hash_alg;
+	}
+	calg = &enc_alg->cra_cipher;
+
+	if (salg->digestsize < calg->cia_min_keysize ||
+	    salg->digestsize > calg->cia_max_keysize) {
+		pr_err("essiv: digest size of '%s' unsupported as key size for '%s'\n",
+		       hash_alg->cra_driver_name, enc_alg->cra_driver_name);
+		goto out_put_algs;
+	}
+
+	inst = kzalloc(sizeof(*inst) + sizeof(*ictx), GFP_KERNEL);
+	if (!inst) {
+		err = -ENOMEM;
+		goto out_put_algs;
+	}
+	ictx = shash_instance_ctx(inst);
+
+	err = crypto_init_shash_spawn(&ictx->hash, salg,
+				      shash_crypto_instance(inst));
+	if (err)
+		goto out_free_inst;
+
+	err = crypto_init_spawn(&ictx->enc, enc_alg,
+				shash_crypto_instance(inst),
+				CRYPTO_ALG_TYPE_MASK);
+	if (err)
+		goto out_drop_hash;
+
+	err = -ENAMETOOLONG;
+	if (snprintf(inst->alg.base.cra_name, CRYPTO_MAX_ALG_NAME,
+		     "essiv(%s,%s)", enc_alg->cra_name,
+		     hash_alg->cra_name) >= CRYPTO_MAX_ALG_NAME)
+		goto out_drop_spawns;
+	if (snprintf(inst->alg.base.cra_driver_name, CRYPTO_MAX_ALG_NAME,
+		     "essiv(%s,%s)", enc_alg->cra_driver_name,
+		     hash_alg->cra_driver_name) >= CRYPTO_MAX_ALG_NAME)
+		goto out_drop_spawns;
+
+	inst->alg.base.cra_priority	= (hash_alg->cra_priority +
+				           enc_alg->cra_priority) / 2;
+	inst->alg.base.cra_blocksize	= enc_alg->cra_blocksize;
+	inst->alg.base.cra_ctxsize	= sizeof(struct essiv_tfm_ctx);
+
+	inst->alg.digestsize		= enc_alg->cra_blocksize;
+	inst->alg.descsize		= sizeof(struct essiv_desc_ctx) +
+					  enc_alg->cra_blocksize;
+	inst->alg.setkey		= essiv_setkey;
+	inst->alg.init			= essiv_init;
+	inst->alg.update		= essiv_update;
+	inst->alg.final			= essiv_final;
+	inst->alg.digest		= essiv_digest;
+
+	inst->alg.base.cra_init		= essiv_init_tfm;
+	inst->alg.base.cra_exit		= essiv_exit_tfm;
+
+	err = shash_register_instance(tmpl, inst);
+	if (err)
+		goto out_drop_spawns;
+
+	crypto_mod_put(enc_alg);
+	crypto_mod_put(hash_alg);
+	return 0;
+
+out_drop_spawns:
+	crypto_drop_spawn(&ictx->enc);
+out_drop_hash:
+	crypto_drop_shash(&ictx->hash);
+out_free_inst:
+	kfree(inst);
+out_put_algs:
+	crypto_mod_put(enc_alg);
+out_put_hash_alg:
+	crypto_mod_put(hash_alg);
+	return err;
+}
+
+static struct crypto_template crypto_essiv_tmpl = {
+	.name		= "essiv",
+	.create		= crypto_essiv_create,
+	.free		= crypto_essiv_free,
+	.module		= THIS_MODULE,
+};
+
+static int __init crypto_essiv_module_init(void)
+{
+	return crypto_register_template(&crypto_essiv_tmpl);
+}
+
+static void __exit crypto_essiv_module_exit(void)
+{
+	crypto_unregister_template(&crypto_essiv_tmpl);
+}
+
+subsys_initcall(crypto_essiv_module_init);
+module_exit(crypto_essiv_module_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("shash template to generate ESSIV tags");
+MODULE_ALIAS_CRYPTO("essiv");
-- 
2.20.1

[RFC PATCH 2/3] dm crypt: switch to essiv shash

[Ard Biesheuvel]

Replace the open coded ESSIV handling with invocations into the new
ESSIV shash, created specifically for this purpose. Using this more
abstract interface allows the crypto subsystem to refactor the way
ciphers are used, and to provide better testing coverage.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/md/Kconfig    |   1 +
 drivers/md/dm-crypt.c | 137 ++++----------------
 2 files changed, 24 insertions(+), 114 deletions(-)

diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
index 45254b3ef715..30ca87cf25db 100644
--- a/drivers/md/Kconfig
+++ b/drivers/md/Kconfig
@@ -271,6 +271,7 @@ config DM_CRYPT
 	depends on BLK_DEV_DM
 	select CRYPTO
 	select CRYPTO_CBC
+	select CRYPTO_ESSIV
 	---help---
 	  This device-mapper target allows you to create a device that
 	  transparently encrypts the data on it. You'll need to activate
diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 1b16d34bb785..b66ef3de835a 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -98,11 +98,6 @@ struct crypt_iv_operations {
 		    struct dm_crypt_request *dmreq);
 };
 
-struct iv_essiv_private {
-	struct crypto_shash *hash_tfm;
-	u8 *salt;
-};
-
 struct iv_benbi_private {
 	int shift;
 };
@@ -155,7 +150,6 @@ struct crypt_config {
 
 	const struct crypt_iv_operations *iv_gen_ops;
 	union {
-		struct iv_essiv_private essiv;
 		struct iv_benbi_private benbi;
 		struct iv_lmk_private lmk;
 		struct iv_tcw_private tcw;
@@ -165,7 +159,7 @@ struct crypt_config {
 	unsigned short int sector_size;
 	unsigned char sector_shift;
 
-	/* ESSIV: struct crypto_cipher *essiv_tfm */
+	/* ESSIV: struct crypto_shash *essiv_tfm */
 	void *iv_private;
 	union {
 		struct crypto_skcipher **tfms;
@@ -326,94 +320,27 @@ static int crypt_iv_plain64be_gen(struct crypt_config *cc, u8 *iv,
 /* Initialise ESSIV - compute salt but no local memory allocations */
 static int crypt_iv_essiv_init(struct crypt_config *cc)
 {
-	struct iv_essiv_private *essiv = &cc->iv_gen_private.essiv;
-	SHASH_DESC_ON_STACK(desc, essiv->hash_tfm);
-	struct crypto_cipher *essiv_tfm;
-	int err;
-
-	desc->tfm = essiv->hash_tfm;
-
-	err = crypto_shash_digest(desc, cc->key, cc->key_size, essiv->salt);
-	shash_desc_zero(desc);
-	if (err)
-		return err;
-
-	essiv_tfm = cc->iv_private;
+	struct crypto_shash *essiv_tfm = cc->iv_private;
 
-	err = crypto_cipher_setkey(essiv_tfm, essiv->salt,
-			    crypto_shash_digestsize(essiv->hash_tfm));
-	if (err)
-		return err;
-
-	return 0;
+	return crypto_shash_setkey(essiv_tfm, cc->key, cc->key_size);
 }
 
 /* Wipe salt and reset key derived from volume key */
 static int crypt_iv_essiv_wipe(struct crypt_config *cc)
 {
-	struct iv_essiv_private *essiv = &cc->iv_gen_private.essiv;
-	unsigned salt_size = crypto_shash_digestsize(essiv->hash_tfm);
-	struct crypto_cipher *essiv_tfm;
-	int r, err = 0;
-
-	memset(essiv->salt, 0, salt_size);
+	struct crypto_shash *essiv_tfm;
 
 	essiv_tfm = cc->iv_private;
-	r = crypto_cipher_setkey(essiv_tfm, essiv->salt, salt_size);
-	if (r)
-		err = r;
-
-	return err;
-}
-
-/* Allocate the cipher for ESSIV */
-static struct crypto_cipher *alloc_essiv_cipher(struct crypt_config *cc,
-						struct dm_target *ti,
-						const u8 *salt,
-						unsigned int saltsize)
-{
-	struct crypto_cipher *essiv_tfm;
-	int err;
-
-	/* Setup the essiv_tfm with the given salt */
-	essiv_tfm = crypto_alloc_cipher(cc->cipher, 0, 0);
-	if (IS_ERR(essiv_tfm)) {
-		ti->error = "Error allocating crypto tfm for ESSIV";
-		return essiv_tfm;
-	}
-
-	if (crypto_cipher_blocksize(essiv_tfm) != cc->iv_size) {
-		ti->error = "Block size of ESSIV cipher does "
-			    "not match IV size of block cipher";
-		crypto_free_cipher(essiv_tfm);
-		return ERR_PTR(-EINVAL);
-	}
-
-	err = crypto_cipher_setkey(essiv_tfm, salt, saltsize);
-	if (err) {
-		ti->error = "Failed to set key for ESSIV cipher";
-		crypto_free_cipher(essiv_tfm);
-		return ERR_PTR(err);
-	}
-
-	return essiv_tfm;
+	return crypto_shash_setkey(essiv_tfm, NULL, 0);
 }
 
 static void crypt_iv_essiv_dtr(struct crypt_config *cc)
 {
-	struct crypto_cipher *essiv_tfm;
-	struct iv_essiv_private *essiv = &cc->iv_gen_private.essiv;
-
-	crypto_free_shash(essiv->hash_tfm);
-	essiv->hash_tfm = NULL;
-
-	kzfree(essiv->salt);
-	essiv->salt = NULL;
+	struct crypto_shash *essiv_tfm;
 
 	essiv_tfm = cc->iv_private;
-
 	if (essiv_tfm)
-		crypto_free_cipher(essiv_tfm);
+		crypto_free_shash(essiv_tfm);
 
 	cc->iv_private = NULL;
 }
@@ -421,9 +348,8 @@ static void crypt_iv_essiv_dtr(struct crypt_config *cc)
 static int crypt_iv_essiv_ctr(struct crypt_config *cc, struct dm_target *ti,
 			      const char *opts)
 {
-	struct crypto_cipher *essiv_tfm = NULL;
-	struct crypto_shash *hash_tfm = NULL;
-	u8 *salt = NULL;
+	struct crypto_shash *essiv_tfm = NULL;
+	u8 name[CRYPTO_MAX_ALG_NAME];
 	int err;
 
 	if (!opts) {
@@ -431,51 +357,34 @@ static int crypt_iv_essiv_ctr(struct crypt_config *cc, struct dm_target *ti,
 		return -EINVAL;
 	}
 
-	/* Allocate hash algorithm */
-	hash_tfm = crypto_alloc_shash(opts, 0, 0);
-	if (IS_ERR(hash_tfm)) {
-		ti->error = "Error initializing ESSIV hash";
-		err = PTR_ERR(hash_tfm);
-		goto bad;
-	}
-
-	salt = kzalloc(crypto_shash_digestsize(hash_tfm), GFP_KERNEL);
-	if (!salt) {
-		ti->error = "Error kmallocing salt storage in ESSIV";
-		err = -ENOMEM;
-		goto bad;
-	}
-
-	cc->iv_gen_private.essiv.salt = salt;
-	cc->iv_gen_private.essiv.hash_tfm = hash_tfm;
+	snprintf(name, CRYPTO_MAX_ALG_NAME, "essiv(%s,%s)", cc->cipher, opts);
 
-	essiv_tfm = alloc_essiv_cipher(cc, ti, salt,
-				       crypto_shash_digestsize(hash_tfm));
-	if (IS_ERR(essiv_tfm)) {
-		crypt_iv_essiv_dtr(cc);
+	essiv_tfm = crypto_alloc_shash(name, 0, 0);
+	if (IS_ERR(essiv_tfm))
 		return PTR_ERR(essiv_tfm);
+
+	err = crypto_shash_setkey(essiv_tfm, cc->key, cc->key_size);
+	if (err) {
+		ti->error = "Failed to set key for ESSIV cipher";
+		crypto_free_shash(essiv_tfm);
+		return err;
 	}
+
 	cc->iv_private = essiv_tfm;
 
 	return 0;
-
-bad:
-	if (hash_tfm && !IS_ERR(hash_tfm))
-		crypto_free_shash(hash_tfm);
-	kfree(salt);
-	return err;
 }
 
 static int crypt_iv_essiv_gen(struct crypt_config *cc, u8 *iv,
 			      struct dm_crypt_request *dmreq)
 {
-	struct crypto_cipher *essiv_tfm = cc->iv_private;
+	struct crypto_shash *essiv_tfm = cc->iv_private;
+	SHASH_DESC_ON_STACK(desc, essiv_tfm);
 
 	memset(iv, 0, cc->iv_size);
 	*(__le64 *)iv = cpu_to_le64(dmreq->iv_sector);
-	crypto_cipher_encrypt_one(essiv_tfm, iv, iv);
-
-	return 0;
+	desc->tfm = essiv_tfm;
+	return crypto_shash_digest(desc, iv, sizeof(__le64), iv);
 }
 
 static int crypt_iv_benbi_ctr(struct crypt_config *cc, struct dm_target *ti,
-- 
2.20.1

[RFC PATCH 3/3] fscrypt: switch to ESSIV shash

[Ard Biesheuvel]

Instead of open coding the shash and cipher operations that make up
the ESSIV transform, switch to the new ESSIV shash template that
encapsulates all of this. Using this more abstract interface provides
more flexibility for the crypto subsystem to do refactoring, and
permits better testing.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 fs/crypto/Kconfig           |  1 +
 fs/crypto/crypto.c          | 11 ++--
 fs/crypto/fscrypt_private.h |  4 +-
 fs/crypto/keyinfo.c         | 64 +++-----------------
 4 files changed, 18 insertions(+), 62 deletions(-)

diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
index 24ed99e2eca0..b0292da8613c 100644
--- a/fs/crypto/Kconfig
+++ b/fs/crypto/Kconfig
@@ -5,6 +5,7 @@ config FS_ENCRYPTION
 	select CRYPTO_AES
 	select CRYPTO_CBC
 	select CRYPTO_ECB
+	select CRYPTO_ESSIV
 	select CRYPTO_XTS
 	select CRYPTO_CTS
 	select CRYPTO_SHA256
diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 335a362ee446..93d33b55c2fa 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -137,8 +137,13 @@ void fscrypt_generate_iv(union fscrypt_iv *iv, u64 lblk_num,
 	if (ci->ci_flags & FS_POLICY_FLAG_DIRECT_KEY)
 		memcpy(iv->nonce, ci->ci_nonce, FS_KEY_DERIVATION_NONCE_SIZE);
 
-	if (ci->ci_essiv_tfm != NULL)
-		crypto_cipher_encrypt_one(ci->ci_essiv_tfm, iv->raw, iv->raw);
+	if (ci->ci_essiv_tfm != NULL) {
+		SHASH_DESC_ON_STACK(desc, ci->ci_essiv_tfm);
+
+		desc->tfm = ci->ci_essiv_tfm;
+		crypto_shash_digest(desc, (u8 *)&iv->lblk_num,
+				    sizeof(iv->lblk_num), iv->raw);
+	}
 }
 
 int fscrypt_do_page_crypto(const struct inode *inode, fscrypt_direction_t rw,
@@ -492,8 +497,6 @@ static void __exit fscrypt_exit(void)
 		destroy_workqueue(fscrypt_read_workqueue);
 	kmem_cache_destroy(fscrypt_ctx_cachep);
 	kmem_cache_destroy(fscrypt_info_cachep);
-
-	fscrypt_essiv_cleanup();
 }
 module_exit(fscrypt_exit);
 
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 7da276159593..67ea4ca11474 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -62,10 +62,10 @@ struct fscrypt_info {
 	struct crypto_skcipher *ci_ctfm;
 
 	/*
-	 * Cipher for ESSIV IV generation.  Only set for CBC contents
+	 * Shash for ESSIV IV generation.  Only set for CBC contents
 	 * encryption, otherwise is NULL.
 	 */
-	struct crypto_cipher *ci_essiv_tfm;
+	struct crypto_shash *ci_essiv_tfm;
 
 	/*
 	 * Encryption mode used for this inode.  It corresponds to either
diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
index dcd91a3fbe49..c3d38f72506c 100644
--- a/fs/crypto/keyinfo.c
+++ b/fs/crypto/keyinfo.c
@@ -15,12 +15,10 @@
 #include <linux/ratelimit.h>
 #include <crypto/aes.h>
 #include <crypto/algapi.h>
+#include <crypto/hash.h>
 #include <crypto/sha.h>
-#include <crypto/skcipher.h>
 #include "fscrypt_private.h"
 
-static struct crypto_shash *essiv_hash_tfm;
-
 /* Table of keys referenced by FS_POLICY_FLAG_DIRECT_KEY policies */
 static DEFINE_HASHTABLE(fscrypt_master_keys, 6); /* 6 bits = 64 buckets */
 static DEFINE_SPINLOCK(fscrypt_master_keys_lock);
@@ -377,70 +375,24 @@ fscrypt_get_master_key(const struct fscrypt_info *ci, struct fscrypt_mode *mode,
 	return ERR_PTR(err);
 }
 
-static int derive_essiv_salt(const u8 *key, int keysize, u8 *salt)
-{
-	struct crypto_shash *tfm = READ_ONCE(essiv_hash_tfm);
-
-	/* init hash transform on demand */
-	if (unlikely(!tfm)) {
-		struct crypto_shash *prev_tfm;
-
-		tfm = crypto_alloc_shash("sha256", 0, 0);
-		if (IS_ERR(tfm)) {
-			fscrypt_warn(NULL,
-				     "error allocating SHA-256 transform: %ld",
-				     PTR_ERR(tfm));
-			return PTR_ERR(tfm);
-		}
-		prev_tfm = cmpxchg(&essiv_hash_tfm, NULL, tfm);
-		if (prev_tfm) {
-			crypto_free_shash(tfm);
-			tfm = prev_tfm;
-		}
-	}
-
-	{
-		SHASH_DESC_ON_STACK(desc, tfm);
-		desc->tfm = tfm;
-
-		return crypto_shash_digest(desc, key, keysize, salt);
-	}
-}
-
 static int init_essiv_generator(struct fscrypt_info *ci, const u8 *raw_key,
 				int keysize)
 {
 	int err;
-	struct crypto_cipher *essiv_tfm;
-	u8 salt[SHA256_DIGEST_SIZE];
-
-	essiv_tfm = crypto_alloc_cipher("aes", 0, 0);
-	if (IS_ERR(essiv_tfm))
-		return PTR_ERR(essiv_tfm);
-
-	ci->ci_essiv_tfm = essiv_tfm;
-
-	err = derive_essiv_salt(raw_key, keysize, salt);
-	if (err)
-		goto out;
+	struct crypto_shash *essiv_tfm;
 
 	/*
 	 * Using SHA256 to derive the salt/key will result in AES-256 being
 	 * used for IV generation. File contents encryption will still use the
 	 * configured keysize (AES-128) nevertheless.
 	 */
-	err = crypto_cipher_setkey(essiv_tfm, salt, sizeof(salt));
-	if (err)
-		goto out;
+	essiv_tfm = crypto_alloc_shash("essiv(aes,sha256)", 0, 0);
+	if (IS_ERR(essiv_tfm))
+		return PTR_ERR(essiv_tfm);
 
-out:
-	memzero_explicit(salt, sizeof(salt));
-	return err;
-}
+	ci->ci_essiv_tfm = essiv_tfm;
 
-void __exit fscrypt_essiv_cleanup(void)
-{
-	crypto_free_shash(essiv_hash_tfm);
+	return crypto_shash_setkey(essiv_tfm, raw_key, keysize);
 }
 
 /*
@@ -495,7 +447,7 @@ static void put_crypt_info(struct fscrypt_info *ci)
 		put_master_key(ci->ci_master_key);
 	} else {
 		crypto_free_skcipher(ci->ci_ctfm);
-		crypto_free_cipher(ci->ci_essiv_tfm);
+		crypto_free_shash(ci->ci_essiv_tfm);
 	}
 	kmem_cache_free(fscrypt_info_cachep, ci);
 }
-- 
2.20.1

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Milan Broz]

On 14/06/2019 10:34, Ard Biesheuvel wrote:
> This series is presented as an RFC for a couple of reasons:
> - it is only build tested
> - it is unclear whether this is the right way to move away from the use of
>   bare ciphers in non-crypto code
> - we haven't really discussed whether moving away from the use of bare ciphers
>   in non-crypto code is a goal we agree on
> 
> This series creates an ESSIV shash template that takes a (cipher,hash) tuple,
> where the digest size of the hash must be a valid key length for the cipher.
> The setkey() operation takes the hash of the input key, and sets into the
> cipher as the encryption key. Digest operations accept input up to the
> block size of the cipher, and perform a single block encryption operation to
> produce the ESSIV output.
> 
> This matches what both users of ESSIV in the kernel do, and so it is proposed
> as a replacement for those, in patches #2 and #3.
> 
> As for the discussion: the code is untested, so it is presented for discussion
> only. I'd like to understand whether we agree that phasing out the bare cipher
> interface from non-crypto code is a good idea, and whether this approach is
> suitable for fscrypt and dm-crypt.

If you want some discussion, it would be very helpful if you cc device-mapper list
to reach dm-crypt developers. Please add at least dm-devel list.

Just a few comments:

 - ESSIV is useful only for CBC mode. I wish we move to some better mode
in the future instead of cementing CBC use... But if it helps people
to actually use unpredictable IV for CBC, it is the right approach.
(yes, I know XTS has own problems as well... but IMO that should be the default
for sector/fs-block encryption these days :)

- I do not think there is a problem if ESSIV moves to crypto API,
but there it is presented as a hash... It is really just an IV generator.

> - wiring up some essiv(x,y) combinations into the testing framework. I wonder
>   if anything other than essiv(aes,sha256) makes sense.

In cryptsetup testsuite, we test serpent and twofish ciphers at least, but in
reality, essiv(aes,sha256) is the most used combination.
If it makes sense, I can run some tests with dm-crypt and this patchset.

Thanks,
Milan

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Milan Broz]

On 14/06/2019 10:34, Ard Biesheuvel wrote:
> This series is presented as an RFC for a couple of reasons:
> - it is only build tested
> - it is unclear whether this is the right way to move away from the use of
>   bare ciphers in non-crypto code
> - we haven't really discussed whether moving away from the use of bare ciphers
>   in non-crypto code is a goal we agree on
> 
> This series creates an ESSIV shash template that takes a (cipher,hash) tuple,
> where the digest size of the hash must be a valid key length for the cipher.
> The setkey() operation takes the hash of the input key, and sets into the
> cipher as the encryption key. Digest operations accept input up to the
> block size of the cipher, and perform a single block encryption operation to
> produce the ESSIV output.
> 
> This matches what both users of ESSIV in the kernel do, and so it is proposed
> as a replacement for those, in patches #2 and #3.
> 
> As for the discussion: the code is untested, so it is presented for discussion
> only. I'd like to understand whether we agree that phasing out the bare cipher
> interface from non-crypto code is a good idea, and whether this approach is
> suitable for fscrypt and dm-crypt.

If you want some discussion, it would be very helpful if you cc device-mapper list
to reach dm-crypt developers. Please add at least dm-devel list.

Just a few comments:

 - ESSIV is useful only for CBC mode. I wish we move to some better mode
in the future instead of cementing CBC use... But if it helps people
to actually use unpredictable IV for CBC, it is the right approach.
(yes, I know XTS has own problems as well... but IMO that should be the default
for sector/fs-block encryption these days :)

- I do not think there is a problem if ESSIV moves to crypto API,
but there it is presented as a hash... It is really just an IV generator.

> - wiring up some essiv(x,y) combinations into the testing framework. I wonder
>   if anything other than essiv(aes,sha256) makes sense.

In cryptsetup testsuite, we test serpent and twofish ciphers at least, but in
reality, essiv(aes,sha256) is the most used combination.
If it makes sense, I can run some tests with dm-crypt and this patchset.

Thanks,
Milan

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Ard Biesheuvel]

On Sat, 15 Jun 2019 at 20:19, Milan Broz <gmazyland@gmail.com> wrote:
>
> On 14/06/2019 10:34, Ard Biesheuvel wrote:
> > This series is presented as an RFC for a couple of reasons:
> > - it is only build tested
> > - it is unclear whether this is the right way to move away from the use of
> >   bare ciphers in non-crypto code
> > - we haven't really discussed whether moving away from the use of bare ciphers
> >   in non-crypto code is a goal we agree on
> >
> > This series creates an ESSIV shash template that takes a (cipher,hash) tuple,
> > where the digest size of the hash must be a valid key length for the cipher.
> > The setkey() operation takes the hash of the input key, and sets into the
> > cipher as the encryption key. Digest operations accept input up to the
> > block size of the cipher, and perform a single block encryption operation to
> > produce the ESSIV output.
> >
> > This matches what both users of ESSIV in the kernel do, and so it is proposed
> > as a replacement for those, in patches #2 and #3.
> >
> > As for the discussion: the code is untested, so it is presented for discussion
> > only. I'd like to understand whether we agree that phasing out the bare cipher
> > interface from non-crypto code is a good idea, and whether this approach is
> > suitable for fscrypt and dm-crypt.
>
> If you want some discussion, it would be very helpful if you cc device-mapper list
> to reach dm-crypt developers. Please add at least dm-devel list.
>
> Just a few comments:
>
>  - ESSIV is useful only for CBC mode. I wish we move to some better mode
> in the future instead of cementing CBC use... But if it helps people
> to actually use unpredictable IV for CBC, it is the right approach.
> (yes, I know XTS has own problems as well... but IMO that should be the default
> for sector/fs-block encryption these days :)
>

I agree that XTS should be preferred. But for some reason, the
kernel's XTS implementation does not support ciphertext stealing (as
opposed to, e.g., OpenSSL), and so CBC ended up being used for
encrypting the filenames in fscrypt.

I am trying to serve both customers with the same solution here,
regardless of whether it is the recommended approach or not.

> - I do not think there is a problem if ESSIV moves to crypto API,
> but there it is presented as a hash... It is really just an IV generator.
>

True. But we don't have the proper abstractions to make this
distinction, and so a shash is currently the best match.

> > - wiring up some essiv(x,y) combinations into the testing framework. I wonder
> >   if anything other than essiv(aes,sha256) makes sense.
>
> In cryptsetup testsuite, we test serpent and twofish ciphers at least, but in
> reality, essiv(aes,sha256) is the most used combination.
> If it makes sense, I can run some tests with dm-crypt and this patchset.
>

OK, that is helpful, thanks. Mind if I ping you once we reach a state
where we need to test for correctness? At the moment, this is still
mostly a discussion piece.

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Ard Biesheuvel]

On Sat, 15 Jun 2019 at 20:19, Milan Broz <gmazyland@gmail.com> wrote:
>
> On 14/06/2019 10:34, Ard Biesheuvel wrote:
> > This series is presented as an RFC for a couple of reasons:
> > - it is only build tested
> > - it is unclear whether this is the right way to move away from the use of
> >   bare ciphers in non-crypto code
> > - we haven't really discussed whether moving away from the use of bare ciphers
> >   in non-crypto code is a goal we agree on
> >
> > This series creates an ESSIV shash template that takes a (cipher,hash) tuple,
> > where the digest size of the hash must be a valid key length for the cipher.
> > The setkey() operation takes the hash of the input key, and sets into the
> > cipher as the encryption key. Digest operations accept input up to the
> > block size of the cipher, and perform a single block encryption operation to
> > produce the ESSIV output.
> >
> > This matches what both users of ESSIV in the kernel do, and so it is proposed
> > as a replacement for those, in patches #2 and #3.
> >
> > As for the discussion: the code is untested, so it is presented for discussion
> > only. I'd like to understand whether we agree that phasing out the bare cipher
> > interface from non-crypto code is a good idea, and whether this approach is
> > suitable for fscrypt and dm-crypt.
>
> If you want some discussion, it would be very helpful if you cc device-mapper list
> to reach dm-crypt developers. Please add at least dm-devel list.
>
> Just a few comments:
>
>  - ESSIV is useful only for CBC mode. I wish we move to some better mode
> in the future instead of cementing CBC use... But if it helps people
> to actually use unpredictable IV for CBC, it is the right approach.
> (yes, I know XTS has own problems as well... but IMO that should be the default
> for sector/fs-block encryption these days :)
>

I agree that XTS should be preferred. But for some reason, the
kernel's XTS implementation does not support ciphertext stealing (as
opposed to, e.g., OpenSSL), and so CBC ended up being used for
encrypting the filenames in fscrypt.

I am trying to serve both customers with the same solution here,
regardless of whether it is the recommended approach or not.

> - I do not think there is a problem if ESSIV moves to crypto API,
> but there it is presented as a hash... It is really just an IV generator.
>

True. But we don't have the proper abstractions to make this
distinction, and so a shash is currently the best match.

> > - wiring up some essiv(x,y) combinations into the testing framework. I wonder
> >   if anything other than essiv(aes,sha256) makes sense.
>
> In cryptsetup testsuite, we test serpent and twofish ciphers at least, but in
> reality, essiv(aes,sha256) is the most used combination.
> If it makes sense, I can run some tests with dm-crypt and this patchset.
>

OK, that is helpful, thanks. Mind if I ping you once we reach a state
where we need to test for correctness? At the moment, this is still
mostly a discussion piece.

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Eric Biggers]

[+Cc dm-devel and linux-fscrypt]

On Fri, Jun 14, 2019 at 10:34:01AM +0200, Ard Biesheuvel wrote:
> This series is presented as an RFC for a couple of reasons:
> - it is only build tested
> - it is unclear whether this is the right way to move away from the use of
>   bare ciphers in non-crypto code
> - we haven't really discussed whether moving away from the use of bare ciphers
>   in non-crypto code is a goal we agree on
> 
> This series creates an ESSIV shash template that takes a (cipher,hash) tuple,
> where the digest size of the hash must be a valid key length for the cipher.
> The setkey() operation takes the hash of the input key, and sets into the
> cipher as the encryption key. Digest operations accept input up to the
> block size of the cipher, and perform a single block encryption operation to
> produce the ESSIV output.
> 
> This matches what both users of ESSIV in the kernel do, and so it is proposed
> as a replacement for those, in patches #2 and #3.
> 
> As for the discussion: the code is untested, so it is presented for discussion
> only. I'd like to understand whether we agree that phasing out the bare cipher
> interface from non-crypto code is a good idea, and whether this approach is
> suitable for fscrypt and dm-crypt.
> 
> Remaining work:
> - wiring up some essiv(x,y) combinations into the testing framework. I wonder
>   if anything other than essiv(aes,sha256) makes sense.
> - testing - suggestions welcome on existing testing frameworks for dm-crypt
>   and/or fscrypt
> 
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Eric Biggers <ebiggers@google.com>
> 
> Ard Biesheuvel (3):
>   crypto: essiv - create a new shash template for IV generation
>   dm crypt: switch to essiv shash
>   fscrypt: switch to ESSIV shash
> 
>  crypto/Kconfig              |   3 +
>  crypto/Makefile             |   1 +
>  crypto/essiv.c              | 275 ++++++++++++++++++++
>  drivers/md/Kconfig          |   1 +
>  drivers/md/dm-crypt.c       | 137 ++--------
>  fs/crypto/Kconfig           |   1 +
>  fs/crypto/crypto.c          |  11 +-
>  fs/crypto/fscrypt_private.h |   4 +-
>  fs/crypto/keyinfo.c         |  64 +----
>  9 files changed, 321 insertions(+), 176 deletions(-)
>  create mode 100644 crypto/essiv.c

I agree that moving away from bare block ciphers is generally a good idea.  For
fscrypt I'm fine with moving ESSIV into the crypto API, though I'm not sure a
shash template is the best approach.  Did you also consider making it a skcipher
template so that users can do e.g. "essiv(cbc(aes),sha256,aes)"?  That would
simplify things much more on the fscrypt side, since then all the ESSIV-related
code would go away entirely except for changing the string "cbc(aes)" to
"essiv(cbc(aes),sha256,aes)".

Either way, for testing the fscrypt change, I recently added tests to xfstests
that verify the on-disk ciphertext in userspace, including for non-default modes
such as the AES-128-CBC-ESSIV in question.  So I'm not too worried about the
fscrypt encryption getting accidentally broken anymore.  If you want to run the
AES-128-CBC-ESSIV test yourself, you should be able to do it by following the
directions at
https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md
and running 'kvm-xfstests -c ext4 generic/549'.

As for adding essiv to testmgr, sha256 and aes would be enough for fscrypt.
There aren't any plans to add more ESSIV settings to fscrypt, and
AES-128-CBC-ESSIV was only added in the first place because some people wanted
to use fscrypt on platforms with CBC hardware acceleration but not XTS.

- Eric

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Eric Biggers]

[+Cc dm-devel and linux-fscrypt]

On Fri, Jun 14, 2019 at 10:34:01AM +0200, Ard Biesheuvel wrote:
> This series is presented as an RFC for a couple of reasons:
> - it is only build tested
> - it is unclear whether this is the right way to move away from the use of
>   bare ciphers in non-crypto code
> - we haven't really discussed whether moving away from the use of bare ciphers
>   in non-crypto code is a goal we agree on
> 
> This series creates an ESSIV shash template that takes a (cipher,hash) tuple,
> where the digest size of the hash must be a valid key length for the cipher.
> The setkey() operation takes the hash of the input key, and sets into the
> cipher as the encryption key. Digest operations accept input up to the
> block size of the cipher, and perform a single block encryption operation to
> produce the ESSIV output.
> 
> This matches what both users of ESSIV in the kernel do, and so it is proposed
> as a replacement for those, in patches #2 and #3.
> 
> As for the discussion: the code is untested, so it is presented for discussion
> only. I'd like to understand whether we agree that phasing out the bare cipher
> interface from non-crypto code is a good idea, and whether this approach is
> suitable for fscrypt and dm-crypt.
> 
> Remaining work:
> - wiring up some essiv(x,y) combinations into the testing framework. I wonder
>   if anything other than essiv(aes,sha256) makes sense.
> - testing - suggestions welcome on existing testing frameworks for dm-crypt
>   and/or fscrypt
> 
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Eric Biggers <ebiggers@google.com>
> 
> Ard Biesheuvel (3):
>   crypto: essiv - create a new shash template for IV generation
>   dm crypt: switch to essiv shash
>   fscrypt: switch to ESSIV shash
> 
>  crypto/Kconfig              |   3 +
>  crypto/Makefile             |   1 +
>  crypto/essiv.c              | 275 ++++++++++++++++++++
>  drivers/md/Kconfig          |   1 +
>  drivers/md/dm-crypt.c       | 137 ++--------
>  fs/crypto/Kconfig           |   1 +
>  fs/crypto/crypto.c          |  11 +-
>  fs/crypto/fscrypt_private.h |   4 +-
>  fs/crypto/keyinfo.c         |  64 +----
>  9 files changed, 321 insertions(+), 176 deletions(-)
>  create mode 100644 crypto/essiv.c

I agree that moving away from bare block ciphers is generally a good idea.  For
fscrypt I'm fine with moving ESSIV into the crypto API, though I'm not sure a
shash template is the best approach.  Did you also consider making it a skcipher
template so that users can do e.g. "essiv(cbc(aes),sha256,aes)"?  That would
simplify things much more on the fscrypt side, since then all the ESSIV-related
code would go away entirely except for changing the string "cbc(aes)" to
"essiv(cbc(aes),sha256,aes)".

Either way, for testing the fscrypt change, I recently added tests to xfstests
that verify the on-disk ciphertext in userspace, including for non-default modes
such as the AES-128-CBC-ESSIV in question.  So I'm not too worried about the
fscrypt encryption getting accidentally broken anymore.  If you want to run the
AES-128-CBC-ESSIV test yourself, you should be able to do it by following the
directions at
https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md
and running 'kvm-xfstests -c ext4 generic/549'.

As for adding essiv to testmgr, sha256 and aes would be enough for fscrypt.
There aren't any plans to add more ESSIV settings to fscrypt, and
AES-128-CBC-ESSIV was only added in the first place because some people wanted
to use fscrypt on platforms with CBC hardware acceleration but not XTS.

- Eric

Re: [dm-devel] [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Eric Biggers]

[+Cc linux-fscrypt]

On Sun, Jun 16, 2019 at 09:13:01PM +0200, Ard Biesheuvel wrote:
> >
> >  - ESSIV is useful only for CBC mode. I wish we move to some better mode
> > in the future instead of cementing CBC use... But if it helps people
> > to actually use unpredictable IV for CBC, it is the right approach.
> > (yes, I know XTS has own problems as well... but IMO that should be the default
> > for sector/fs-block encryption these days :)
> >
> 
> I agree that XTS should be preferred. But for some reason, the
> kernel's XTS implementation does not support ciphertext stealing (as
> opposed to, e.g., OpenSSL), and so CBC ended up being used for
> encrypting the filenames in fscrypt.
> 

Actually, for fscrypt CTS-CBC was also chosen because all filenames in each
directory use the same IV, in order to efficiently support all the possible
filesystem operations and to support filenames up to NAME_MAX.  So there was a
desire for there to be some propagation across ciphertext blocks rather than use
XTS which would effectively be ECB in this case.

Neither solution is great though, since CBC-CTS still has the common prefix
problem.  Long-term we're planning to switch to an AES-based wide block mode
such as AES-HEH or AES-HCTR for filenames encryption.  This is already solved
for Adiantum users since Adiantum is a wide-block mode, but there should be a
pure AES solution too to go along with AES contents encryption.

- Eric

Re: [dm-devel] [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Eric Biggers]

[+Cc linux-fscrypt]

On Sun, Jun 16, 2019 at 09:13:01PM +0200, Ard Biesheuvel wrote:
> >
> >  - ESSIV is useful only for CBC mode. I wish we move to some better mode
> > in the future instead of cementing CBC use... But if it helps people
> > to actually use unpredictable IV for CBC, it is the right approach.
> > (yes, I know XTS has own problems as well... but IMO that should be the default
> > for sector/fs-block encryption these days :)
> >
> 
> I agree that XTS should be preferred. But for some reason, the
> kernel's XTS implementation does not support ciphertext stealing (as
> opposed to, e.g., OpenSSL), and so CBC ended up being used for
> encrypting the filenames in fscrypt.
> 

Actually, for fscrypt CTS-CBC was also chosen because all filenames in each
directory use the same IV, in order to efficiently support all the possible
filesystem operations and to support filenames up to NAME_MAX.  So there was a
desire for there to be some propagation across ciphertext blocks rather than use
XTS which would effectively be ECB in this case.

Neither solution is great though, since CBC-CTS still has the common prefix
problem.  Long-term we're planning to switch to an AES-based wide block mode
such as AES-HEH or AES-HCTR for filenames encryption.  This is already solved
for Adiantum users since Adiantum is a wide-block mode, but there should be a
pure AES solution too to go along with AES contents encryption.

- Eric

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Eric Biggers]

[+Cc linux-fscrypt]

On Sun, Jun 16, 2019 at 09:13:01PM +0200, Ard Biesheuvel wrote:
> >
> >  - ESSIV is useful only for CBC mode. I wish we move to some better mode
> > in the future instead of cementing CBC use... But if it helps people
> > to actually use unpredictable IV for CBC, it is the right approach.
> > (yes, I know XTS has own problems as well... but IMO that should be the default
> > for sector/fs-block encryption these days :)
> >
> 
> I agree that XTS should be preferred. But for some reason, the
> kernel's XTS implementation does not support ciphertext stealing (as
> opposed to, e.g., OpenSSL), and so CBC ended up being used for
> encrypting the filenames in fscrypt.
> 

Actually, for fscrypt CTS-CBC was also chosen because all filenames in each
directory use the same IV, in order to efficiently support all the possible
filesystem operations and to support filenames up to NAME_MAX.  So there was a
desire for there to be some propagation across ciphertext blocks rather than use
XTS which would effectively be ECB in this case.

Neither solution is great though, since CBC-CTS still has the common prefix
problem.  Long-term we're planning to switch to an AES-based wide block mode
such as AES-HEH or AES-HCTR for filenames encryption.  This is already solved
for Adiantum users since Adiantum is a wide-block mode, but there should be a
pure AES solution too to go along with AES contents encryption.

- Eric

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Gilad Ben-Yossef]

On Sun, Jun 16, 2019 at 11:44 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> [+Cc dm-devel and linux-fscrypt]
>
> On Fri, Jun 14, 2019 at 10:34:01AM +0200, Ard Biesheuvel wrote:
> > This series is presented as an RFC for a couple of reasons:
> > - it is only build tested
> > - it is unclear whether this is the right way to move away from the use of
> >   bare ciphers in non-crypto code
> > - we haven't really discussed whether moving away from the use of bare ciphers
> >   in non-crypto code is a goal we agree on
> >
> > This series creates an ESSIV shash template that takes a (cipher,hash) tuple,
> > where the digest size of the hash must be a valid key length for the cipher.
> > The setkey() operation takes the hash of the input key, and sets into the
> > cipher as the encryption key. Digest operations accept input up to the
> > block size of the cipher, and perform a single block encryption operation to
> > produce the ESSIV output.
...
> I agree that moving away from bare block ciphers is generally a good idea.  For
> fscrypt I'm fine with moving ESSIV into the crypto API, though I'm not sure a
> shash template is the best approach.  Did you also consider making it a skcipher
> template so that users can do e.g. "essiv(cbc(aes),sha256,aes)"?  That would
> simplify things much more on the fscrypt side, since then all the ESSIV-related
> code would go away entirely except for changing the string "cbc(aes)" to
> "essiv(cbc(aes),sha256,aes)".

I will also add that going the skcipher route rather than shash will
allow hardware tfm providers like CryptoCell that can do the ESSIV
part in hardware implement that as a single API call and/or hardware
invocation flow.
For those system that benefit from hardware providers this can be beneficial.

Thanks,
Gilad

-- 
Gilad Ben-Yossef
Chief Coffee Drinker

values of β will give rise to dom!

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Gilad Ben-Yossef]

On Sun, Jun 16, 2019 at 11:44 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> [+Cc dm-devel and linux-fscrypt]
>
> On Fri, Jun 14, 2019 at 10:34:01AM +0200, Ard Biesheuvel wrote:
> > This series is presented as an RFC for a couple of reasons:
> > - it is only build tested
> > - it is unclear whether this is the right way to move away from the use of
> >   bare ciphers in non-crypto code
> > - we haven't really discussed whether moving away from the use of bare ciphers
> >   in non-crypto code is a goal we agree on
> >
> > This series creates an ESSIV shash template that takes a (cipher,hash) tuple,
> > where the digest size of the hash must be a valid key length for the cipher.
> > The setkey() operation takes the hash of the input key, and sets into the
> > cipher as the encryption key. Digest operations accept input up to the
> > block size of the cipher, and perform a single block encryption operation to
> > produce the ESSIV output.
...
> I agree that moving away from bare block ciphers is generally a good idea.  For
> fscrypt I'm fine with moving ESSIV into the crypto API, though I'm not sure a
> shash template is the best approach.  Did you also consider making it a skcipher
> template so that users can do e.g. "essiv(cbc(aes),sha256,aes)"?  That would
> simplify things much more on the fscrypt side, since then all the ESSIV-related
> code would go away entirely except for changing the string "cbc(aes)" to
> "essiv(cbc(aes),sha256,aes)".

I will also add that going the skcipher route rather than shash will
allow hardware tfm providers like CryptoCell that can do the ESSIV
part in hardware implement that as a single API call and/or hardware
invocation flow.
For those system that benefit from hardware providers this can be beneficial.

Thanks,
Gilad

-- 
Gilad Ben-Yossef
Chief Coffee Drinker

values of β will give rise to dom!

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Ard Biesheuvel]

On Mon, 17 Jun 2019 at 10:52, Gilad Ben-Yossef <gilad@benyossef.com> wrote:
>
> On Sun, Jun 16, 2019 at 11:44 PM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > [+Cc dm-devel and linux-fscrypt]
> >
> > On Fri, Jun 14, 2019 at 10:34:01AM +0200, Ard Biesheuvel wrote:
> > > This series is presented as an RFC for a couple of reasons:
> > > - it is only build tested
> > > - it is unclear whether this is the right way to move away from the use of
> > >   bare ciphers in non-crypto code
> > > - we haven't really discussed whether moving away from the use of bare ciphers
> > >   in non-crypto code is a goal we agree on
> > >
> > > This series creates an ESSIV shash template that takes a (cipher,hash) tuple,
> > > where the digest size of the hash must be a valid key length for the cipher.
> > > The setkey() operation takes the hash of the input key, and sets into the
> > > cipher as the encryption key. Digest operations accept input up to the
> > > block size of the cipher, and perform a single block encryption operation to
> > > produce the ESSIV output.
> ...
> > I agree that moving away from bare block ciphers is generally a good idea.  For
> > fscrypt I'm fine with moving ESSIV into the crypto API, though I'm not sure a
> > shash template is the best approach.  Did you also consider making it a skcipher
> > template so that users can do e.g. "essiv(cbc(aes),sha256,aes)"?  That would
> > simplify things much more on the fscrypt side, since then all the ESSIV-related
> > code would go away entirely except for changing the string "cbc(aes)" to
> > "essiv(cbc(aes),sha256,aes)".
>
> I will also add that going the skcipher route rather than shash will
> allow hardware tfm providers like CryptoCell that can do the ESSIV
> part in hardware implement that as a single API call and/or hardware
> invocation flow.
> For those system that benefit from hardware providers this can be beneficial.
>

Ah yes, thanks for reminding me. There was some debate in the past
about this, but I don't remember the details.

I think implementing essiv() as a skcipher template is indeed going to
be the best approach, I will look into that.

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Ard Biesheuvel]

On Mon, 17 Jun 2019 at 10:52, Gilad Ben-Yossef <gilad@benyossef.com> wrote:
>
> On Sun, Jun 16, 2019 at 11:44 PM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > [+Cc dm-devel and linux-fscrypt]
> >
> > On Fri, Jun 14, 2019 at 10:34:01AM +0200, Ard Biesheuvel wrote:
> > > This series is presented as an RFC for a couple of reasons:
> > > - it is only build tested
> > > - it is unclear whether this is the right way to move away from the use of
> > >   bare ciphers in non-crypto code
> > > - we haven't really discussed whether moving away from the use of bare ciphers
> > >   in non-crypto code is a goal we agree on
> > >
> > > This series creates an ESSIV shash template that takes a (cipher,hash) tuple,
> > > where the digest size of the hash must be a valid key length for the cipher.
> > > The setkey() operation takes the hash of the input key, and sets into the
> > > cipher as the encryption key. Digest operations accept input up to the
> > > block size of the cipher, and perform a single block encryption operation to
> > > produce the ESSIV output.
> ...
> > I agree that moving away from bare block ciphers is generally a good idea.  For
> > fscrypt I'm fine with moving ESSIV into the crypto API, though I'm not sure a
> > shash template is the best approach.  Did you also consider making it a skcipher
> > template so that users can do e.g. "essiv(cbc(aes),sha256,aes)"?  That would
> > simplify things much more on the fscrypt side, since then all the ESSIV-related
> > code would go away entirely except for changing the string "cbc(aes)" to
> > "essiv(cbc(aes),sha256,aes)".
>
> I will also add that going the skcipher route rather than shash will
> allow hardware tfm providers like CryptoCell that can do the ESSIV
> part in hardware implement that as a single API call and/or hardware
> invocation flow.
> For those system that benefit from hardware providers this can be beneficial.
>

Ah yes, thanks for reminding me. There was some debate in the past
about this, but I don't remember the details.

I think implementing essiv() as a skcipher template is indeed going to
be the best approach, I will look into that.

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Herbert Xu]

On Mon, Jun 17, 2019 at 11:15:01AM +0200, Ard Biesheuvel wrote:
>
> Ah yes, thanks for reminding me. There was some debate in the past
> about this, but I don't remember the details.

I think there were some controversy regarding whether the resulting
code lived in drivers/md or crypto.  I think as long as drivers/md
is the only user of the said code then having it in drivers/md should
be fine.

However, if we end up using/reusing the same code for others such as
fs/crypto then it might make more sense to have them in crypto.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Herbert Xu]

On Mon, Jun 17, 2019 at 11:15:01AM +0200, Ard Biesheuvel wrote:
>
> Ah yes, thanks for reminding me. There was some debate in the past
> about this, but I don't remember the details.

I think there were some controversy regarding whether the resulting
code lived in drivers/md or crypto.  I think as long as drivers/md
is the only user of the said code then having it in drivers/md should
be fine.

However, if we end up using/reusing the same code for others such as
fs/crypto then it might make more sense to have them in crypto.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Ard Biesheuvel]

On Mon, 17 Jun 2019 at 11:20, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Mon, Jun 17, 2019 at 11:15:01AM +0200, Ard Biesheuvel wrote:
> >
> > Ah yes, thanks for reminding me. There was some debate in the past
> > about this, but I don't remember the details.
>
> I think there were some controversy regarding whether the resulting
> code lived in drivers/md or crypto.  I think as long as drivers/md
> is the only user of the said code then having it in drivers/md should
> be fine.
>
> However, if we end up using/reusing the same code for others such as
> fs/crypto then it might make more sense to have them in crypto.
>

Agreed. We could more easily test it in isolation and ensure parity
between different implementations, especially now that we have Eric's
improved testing framework that tests against generic implementations.

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Ard Biesheuvel]

On Mon, 17 Jun 2019 at 11:20, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Mon, Jun 17, 2019 at 11:15:01AM +0200, Ard Biesheuvel wrote:
> >
> > Ah yes, thanks for reminding me. There was some debate in the past
> > about this, but I don't remember the details.
>
> I think there were some controversy regarding whether the resulting
> code lived in drivers/md or crypto.  I think as long as drivers/md
> is the only user of the said code then having it in drivers/md should
> be fine.
>
> However, if we end up using/reusing the same code for others such as
> fs/crypto then it might make more sense to have them in crypto.
>

Agreed. We could more easily test it in isolation and ensure parity
between different implementations, especially now that we have Eric's
improved testing framework that tests against generic implementations.

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Milan Broz]

On 17/06/2019 11:15, Ard Biesheuvel wrote:
>> I will also add that going the skcipher route rather than shash will
>> allow hardware tfm providers like CryptoCell that can do the ESSIV
>> part in hardware implement that as a single API call and/or hardware
>> invocation flow.
>> For those system that benefit from hardware providers this can be beneficial.
>>
> 
> Ah yes, thanks for reminding me. There was some debate in the past
> about this, but I don't remember the details.
> 
> I think implementing essiv() as a skcipher template is indeed going to
> be the best approach, I will look into that.

For ESSIV (that is de-facto standard now), I think there is no problem
to move it to crypto API.

The problem is with some other IV generators in dm-crypt.

Some do a lot of more work than just IV (it is hackish, it can modify data, this applies
for loop AES "lmk" and compatible TrueCrypt "tcw" IV implementations).

For these I would strongly say it should remain "hacked" inside dm-crypt only
(it is unusable for anything else than disk encryption and should not be visible outside).

Moreover, it is purely legacy code - we provide it for users can access old systems only.

If you end with rewriting all IVs as templates, I think it is not a good idea.

If it is only about ESSIV, and patch for dm-crypt is simple, it is a reasonable approach.

(The same applies for simple dmcryp IVs like "plain" "plain64", "plain64be and "benbi" that
are just linear IVs in various encoded variants.)

Milan

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Milan Broz]

On 17/06/2019 11:15, Ard Biesheuvel wrote:
>> I will also add that going the skcipher route rather than shash will
>> allow hardware tfm providers like CryptoCell that can do the ESSIV
>> part in hardware implement that as a single API call and/or hardware
>> invocation flow.
>> For those system that benefit from hardware providers this can be beneficial.
>>
> 
> Ah yes, thanks for reminding me. There was some debate in the past
> about this, but I don't remember the details.
> 
> I think implementing essiv() as a skcipher template is indeed going to
> be the best approach, I will look into that.

For ESSIV (that is de-facto standard now), I think there is no problem
to move it to crypto API.

The problem is with some other IV generators in dm-crypt.

Some do a lot of more work than just IV (it is hackish, it can modify data, this applies
for loop AES "lmk" and compatible TrueCrypt "tcw" IV implementations).

For these I would strongly say it should remain "hacked" inside dm-crypt only
(it is unusable for anything else than disk encryption and should not be visible outside).

Moreover, it is purely legacy code - we provide it for users can access old systems only.

If you end with rewriting all IVs as templates, I think it is not a good idea.

If it is only about ESSIV, and patch for dm-crypt is simple, it is a reasonable approach.

(The same applies for simple dmcryp IVs like "plain" "plain64", "plain64be and "benbi" that
are just linear IVs in various encoded variants.)

Milan

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Ard Biesheuvel]

On Mon, 17 Jun 2019 at 12:39, Milan Broz <gmazyland@gmail.com> wrote:
>
> On 17/06/2019 11:15, Ard Biesheuvel wrote:
> >> I will also add that going the skcipher route rather than shash will
> >> allow hardware tfm providers like CryptoCell that can do the ESSIV
> >> part in hardware implement that as a single API call and/or hardware
> >> invocation flow.
> >> For those system that benefit from hardware providers this can be beneficial.
> >>
> >
> > Ah yes, thanks for reminding me. There was some debate in the past
> > about this, but I don't remember the details.
> >
> > I think implementing essiv() as a skcipher template is indeed going to
> > be the best approach, I will look into that.
>
> For ESSIV (that is de-facto standard now), I think there is no problem
> to move it to crypto API.
>
> The problem is with some other IV generators in dm-crypt.
>
> Some do a lot of more work than just IV (it is hackish, it can modify data, this applies
> for loop AES "lmk" and compatible TrueCrypt "tcw" IV implementations).
>
> For these I would strongly say it should remain "hacked" inside dm-crypt only
> (it is unusable for anything else than disk encryption and should not be visible outside).
>
> Moreover, it is purely legacy code - we provide it for users can access old systems only.
>
> If you end with rewriting all IVs as templates, I think it is not a good idea.
>
> If it is only about ESSIV, and patch for dm-crypt is simple, it is a reasonable approach.
>
> (The same applies for simple dmcryp IVs like "plain" "plain64", "plain64be and "benbi" that
> are just linear IVs in various encoded variants.)
>

Agreed.

I am mostly only interested in ensuring that the bare 'cipher'
interface is no longer used outside of the crypto subsystem itself.
Since ESSIV is the only one using that, ESSIV is the only one I intend
to change.

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Ard Biesheuvel]

On Mon, 17 Jun 2019 at 12:39, Milan Broz <gmazyland@gmail.com> wrote:
>
> On 17/06/2019 11:15, Ard Biesheuvel wrote:
> >> I will also add that going the skcipher route rather than shash will
> >> allow hardware tfm providers like CryptoCell that can do the ESSIV
> >> part in hardware implement that as a single API call and/or hardware
> >> invocation flow.
> >> For those system that benefit from hardware providers this can be beneficial.
> >>
> >
> > Ah yes, thanks for reminding me. There was some debate in the past
> > about this, but I don't remember the details.
> >
> > I think implementing essiv() as a skcipher template is indeed going to
> > be the best approach, I will look into that.
>
> For ESSIV (that is de-facto standard now), I think there is no problem
> to move it to crypto API.
>
> The problem is with some other IV generators in dm-crypt.
>
> Some do a lot of more work than just IV (it is hackish, it can modify data, this applies
> for loop AES "lmk" and compatible TrueCrypt "tcw" IV implementations).
>
> For these I would strongly say it should remain "hacked" inside dm-crypt only
> (it is unusable for anything else than disk encryption and should not be visible outside).
>
> Moreover, it is purely legacy code - we provide it for users can access old systems only.
>
> If you end with rewriting all IVs as templates, I think it is not a good idea.
>
> If it is only about ESSIV, and patch for dm-crypt is simple, it is a reasonable approach.
>
> (The same applies for simple dmcryp IVs like "plain" "plain64", "plain64be and "benbi" that
> are just linear IVs in various encoded variants.)
>

Agreed.

I am mostly only interested in ensuring that the bare 'cipher'
interface is no longer used outside of the crypto subsystem itself.
Since ESSIV is the only one using that, ESSIV is the only one I intend
to change.

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Ard Biesheuvel]

On Mon, 17 Jun 2019 at 12:58, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Mon, 17 Jun 2019 at 12:39, Milan Broz <gmazyland@gmail.com> wrote:
> >
> > On 17/06/2019 11:15, Ard Biesheuvel wrote:
> > >> I will also add that going the skcipher route rather than shash will
> > >> allow hardware tfm providers like CryptoCell that can do the ESSIV
> > >> part in hardware implement that as a single API call and/or hardware
> > >> invocation flow.
> > >> For those system that benefit from hardware providers this can be beneficial.
> > >>
> > >
> > > Ah yes, thanks for reminding me. There was some debate in the past
> > > about this, but I don't remember the details.
> > >
> > > I think implementing essiv() as a skcipher template is indeed going to
> > > be the best approach, I will look into that.
> >
> > For ESSIV (that is de-facto standard now), I think there is no problem
> > to move it to crypto API.
> >
> > The problem is with some other IV generators in dm-crypt.
> >
> > Some do a lot of more work than just IV (it is hackish, it can modify data, this applies
> > for loop AES "lmk" and compatible TrueCrypt "tcw" IV implementations).
> >
> > For these I would strongly say it should remain "hacked" inside dm-crypt only
> > (it is unusable for anything else than disk encryption and should not be visible outside).
> >
> > Moreover, it is purely legacy code - we provide it for users can access old systems only.
> >
> > If you end with rewriting all IVs as templates, I think it is not a good idea.
> >
> > If it is only about ESSIV, and patch for dm-crypt is simple, it is a reasonable approach.
> >
> > (The same applies for simple dmcryp IVs like "plain" "plain64", "plain64be and "benbi" that
> > are just linear IVs in various encoded variants.)
> >
>
> Agreed.
>
> I am mostly only interested in ensuring that the bare 'cipher'
> interface is no longer used outside of the crypto subsystem itself.
> Since ESSIV is the only one using that, ESSIV is the only one I intend
> to change.

OK, so inferring the block cipher name from the skcipher algo name is
a bit finicky, since the dm-crypt code does that *after* allocating
the TFMs, and allocating the essiv skcipher happens before.
But more importantly, it appears to be allowed currently to use ESSIV
with authenticated modes, which means we'd need both a skcipher and a
aead essiv template, or do some non-trivial hacking to insert the
essiv template in the right place (which I'm not sure is even
possible)

So my main question/showstopper at the moment is: which modes do we
need to support for ESSIV? Only CBC? Any skcipher? Or both skciphers
and AEADs?

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Ard Biesheuvel]

On Mon, 17 Jun 2019 at 12:58, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Mon, 17 Jun 2019 at 12:39, Milan Broz <gmazyland@gmail.com> wrote:
> >
> > On 17/06/2019 11:15, Ard Biesheuvel wrote:
> > >> I will also add that going the skcipher route rather than shash will
> > >> allow hardware tfm providers like CryptoCell that can do the ESSIV
> > >> part in hardware implement that as a single API call and/or hardware
> > >> invocation flow.
> > >> For those system that benefit from hardware providers this can be beneficial.
> > >>
> > >
> > > Ah yes, thanks for reminding me. There was some debate in the past
> > > about this, but I don't remember the details.
> > >
> > > I think implementing essiv() as a skcipher template is indeed going to
> > > be the best approach, I will look into that.
> >
> > For ESSIV (that is de-facto standard now), I think there is no problem
> > to move it to crypto API.
> >
> > The problem is with some other IV generators in dm-crypt.
> >
> > Some do a lot of more work than just IV (it is hackish, it can modify data, this applies
> > for loop AES "lmk" and compatible TrueCrypt "tcw" IV implementations).
> >
> > For these I would strongly say it should remain "hacked" inside dm-crypt only
> > (it is unusable for anything else than disk encryption and should not be visible outside).
> >
> > Moreover, it is purely legacy code - we provide it for users can access old systems only.
> >
> > If you end with rewriting all IVs as templates, I think it is not a good idea.
> >
> > If it is only about ESSIV, and patch for dm-crypt is simple, it is a reasonable approach.
> >
> > (The same applies for simple dmcryp IVs like "plain" "plain64", "plain64be and "benbi" that
> > are just linear IVs in various encoded variants.)
> >
>
> Agreed.
>
> I am mostly only interested in ensuring that the bare 'cipher'
> interface is no longer used outside of the crypto subsystem itself.
> Since ESSIV is the only one using that, ESSIV is the only one I intend
> to change.

OK, so inferring the block cipher name from the skcipher algo name is
a bit finicky, since the dm-crypt code does that *after* allocating
the TFMs, and allocating the essiv skcipher happens before.
But more importantly, it appears to be allowed currently to use ESSIV
with authenticated modes, which means we'd need both a skcipher and a
aead essiv template, or do some non-trivial hacking to insert the
essiv template in the right place (which I'm not sure is even
possible)

So my main question/showstopper at the moment is: which modes do we
need to support for ESSIV? Only CBC? Any skcipher? Or both skciphers
and AEADs?

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Milan Broz]

On 17/06/2019 15:59, Ard Biesheuvel wrote:
> 
> So my main question/showstopper at the moment is: which modes do we
> need to support for ESSIV? Only CBC? Any skcipher? Or both skciphers
> and AEADs?

Support, or cover by internal test? I think you nee to support everything
what dmcrypt currently allows, if you want to port dmcrypt to new API.

I know of many systems that use aes-xts-essiv:sha256 (it does not make sense
much but people just use it).

Some people use serpent and twofish, but we allow any cipher that fits...

For the start, run this
https://gitlab.com/cryptsetup/cryptsetup/blob/master/tests/mode-test

In other words, if you add some additional limit, we are breaking backward compatibility.
(Despite the configuration is "wrong" from the security point of view.)

Milan

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Milan Broz]

On 17/06/2019 15:59, Ard Biesheuvel wrote:
> 
> So my main question/showstopper at the moment is: which modes do we
> need to support for ESSIV? Only CBC? Any skcipher? Or both skciphers
> and AEADs?

Support, or cover by internal test? I think you nee to support everything
what dmcrypt currently allows, if you want to port dmcrypt to new API.

I know of many systems that use aes-xts-essiv:sha256 (it does not make sense
much but people just use it).

Some people use serpent and twofish, but we allow any cipher that fits...

For the start, run this
https://gitlab.com/cryptsetup/cryptsetup/blob/master/tests/mode-test

In other words, if you add some additional limit, we are breaking backward compatibility.
(Despite the configuration is "wrong" from the security point of view.)

Milan

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Ard Biesheuvel]

On Mon, 17 Jun 2019 at 16:35, Milan Broz <gmazyland@gmail.com> wrote:
>
> On 17/06/2019 15:59, Ard Biesheuvel wrote:
> >
> > So my main question/showstopper at the moment is: which modes do we
> > need to support for ESSIV? Only CBC? Any skcipher? Or both skciphers
> > and AEADs?
>
> Support, or cover by internal test? I think you nee to support everything
> what dmcrypt currently allows, if you want to port dmcrypt to new API.
>
> I know of many systems that use aes-xts-essiv:sha256 (it does not make sense
> much but people just use it).
>
> Some people use serpent and twofish, but we allow any cipher that fits...
>

Sure,  that is all fine

> For the start, run this
> https://gitlab.com/cryptsetup/cryptsetup/blob/master/tests/mode-test
>
> In other words, if you add some additional limit, we are breaking backward compatibility.
> (Despite the configuration is "wrong" from the security point of view.)
>

Yes, but breaking backward compatibility only happens if you break
something that is actually being *used*. So sure,
xts(aes)-essiv:sha256 makes no sense but people use it anyway. But is
that also true for, say, gcm(aes)-essiv:sha256 ?

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Ard Biesheuvel]

On Mon, 17 Jun 2019 at 16:35, Milan Broz <gmazyland@gmail.com> wrote:
>
> On 17/06/2019 15:59, Ard Biesheuvel wrote:
> >
> > So my main question/showstopper at the moment is: which modes do we
> > need to support for ESSIV? Only CBC? Any skcipher? Or both skciphers
> > and AEADs?
>
> Support, or cover by internal test? I think you nee to support everything
> what dmcrypt currently allows, if you want to port dmcrypt to new API.
>
> I know of many systems that use aes-xts-essiv:sha256 (it does not make sense
> much but people just use it).
>
> Some people use serpent and twofish, but we allow any cipher that fits...
>

Sure,  that is all fine

> For the start, run this
> https://gitlab.com/cryptsetup/cryptsetup/blob/master/tests/mode-test
>
> In other words, if you add some additional limit, we are breaking backward compatibility.
> (Despite the configuration is "wrong" from the security point of view.)
>

Yes, but breaking backward compatibility only happens if you break
something that is actually being *used*. So sure,
xts(aes)-essiv:sha256 makes no sense but people use it anyway. But is
that also true for, say, gcm(aes)-essiv:sha256 ?

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Milan Broz]

On 17/06/2019 16:39, Ard Biesheuvel wrote:
>>
>> In other words, if you add some additional limit, we are breaking backward compatibility.
>> (Despite the configuration is "wrong" from the security point of view.)
>>
> 
> Yes, but breaking backward compatibility only happens if you break
> something that is actually being *used*. So sure,
> xts(aes)-essiv:sha256 makes no sense but people use it anyway. But is
> that also true for, say, gcm(aes)-essiv:sha256 ?

These should not be used.  The only way when ESSIV can combine with AEAD mode
is when you combine length-preserving mode with additional integrity tag, for example

  # cryptsetup luksFormat -c aes-cbc-essiv:sha256 --integrity hmac-sha256 /dev/sdb

it will produce this dm-crypt cipher spec:
  capi:authenc(hmac(sha256),cbc(aes))-essiv:sha256

the authenc(hmac(sha256),cbc(aes)) is direct crypto API cipher composition, the essiv:sha256
IV is processed inside dm-crypt as IV.

So if authenc() composition is problem, then yes, I am afraid these can be used in reality.

But for things like gcm(aes)-essiv:sha256 (IOW real AEAD mode with ESSIV) - these are
not supported by cryptsetup (we support only random IV in this case), so these should
not be used anywhere.

Milan

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Milan Broz]

On 17/06/2019 16:39, Ard Biesheuvel wrote:
>>
>> In other words, if you add some additional limit, we are breaking backward compatibility.
>> (Despite the configuration is "wrong" from the security point of view.)
>>
> 
> Yes, but breaking backward compatibility only happens if you break
> something that is actually being *used*. So sure,
> xts(aes)-essiv:sha256 makes no sense but people use it anyway. But is
> that also true for, say, gcm(aes)-essiv:sha256 ?

These should not be used.  The only way when ESSIV can combine with AEAD mode
is when you combine length-preserving mode with additional integrity tag, for example

  # cryptsetup luksFormat -c aes-cbc-essiv:sha256 --integrity hmac-sha256 /dev/sdb

it will produce this dm-crypt cipher spec:
  capi:authenc(hmac(sha256),cbc(aes))-essiv:sha256

the authenc(hmac(sha256),cbc(aes)) is direct crypto API cipher composition, the essiv:sha256
IV is processed inside dm-crypt as IV.

So if authenc() composition is problem, then yes, I am afraid these can be used in reality.

But for things like gcm(aes)-essiv:sha256 (IOW real AEAD mode with ESSIV) - these are
not supported by cryptsetup (we support only random IV in this case), so these should
not be used anywhere.

Milan

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Ard Biesheuvel]

On Mon, 17 Jun 2019 at 19:05, Milan Broz <gmazyland@gmail.com> wrote:
>
> On 17/06/2019 16:39, Ard Biesheuvel wrote:
> >>
> >> In other words, if you add some additional limit, we are breaking backward compatibility.
> >> (Despite the configuration is "wrong" from the security point of view.)
> >>
> >
> > Yes, but breaking backward compatibility only happens if you break
> > something that is actually being *used*. So sure,
> > xts(aes)-essiv:sha256 makes no sense but people use it anyway. But is
> > that also true for, say, gcm(aes)-essiv:sha256 ?
>
> These should not be used.  The only way when ESSIV can combine with AEAD mode
> is when you combine length-preserving mode with additional integrity tag, for example
>
>   # cryptsetup luksFormat -c aes-cbc-essiv:sha256 --integrity hmac-sha256 /dev/sdb
>
> it will produce this dm-crypt cipher spec:
>   capi:authenc(hmac(sha256),cbc(aes))-essiv:sha256
>
> the authenc(hmac(sha256),cbc(aes)) is direct crypto API cipher composition, the essiv:sha256
> IV is processed inside dm-crypt as IV.
>
> So if authenc() composition is problem, then yes, I am afraid these can be used in reality.
>
> But for things like gcm(aes)-essiv:sha256 (IOW real AEAD mode with ESSIV) - these are
> not supported by cryptsetup (we support only random IV in this case), so these should
> not be used anywhere.
>

OK, understood. Unfortunately, that means that the essiv template
should be dynamically instantiated as either a aead or a skcipher
depending on the context, but perhaps this is not a big deal in
reality, I will check.

One final question before I can proceed with my v2: in
crypt_ctr_blkdev_cipher(), do you think we could change the code to
look at the cipher string rather than the name of the actual cipher?
In practice, I don't think they can be different, but in order to be
able to instantiate
'essiv(authenc(hmac(sha256),cbc(aes)),sha256,aes)', the cipher part
needs to be parsed before the TFM(s) are instantiated.

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Ard Biesheuvel]

On Mon, 17 Jun 2019 at 19:05, Milan Broz <gmazyland@gmail.com> wrote:
>
> On 17/06/2019 16:39, Ard Biesheuvel wrote:
> >>
> >> In other words, if you add some additional limit, we are breaking backward compatibility.
> >> (Despite the configuration is "wrong" from the security point of view.)
> >>
> >
> > Yes, but breaking backward compatibility only happens if you break
> > something that is actually being *used*. So sure,
> > xts(aes)-essiv:sha256 makes no sense but people use it anyway. But is
> > that also true for, say, gcm(aes)-essiv:sha256 ?
>
> These should not be used.  The only way when ESSIV can combine with AEAD mode
> is when you combine length-preserving mode with additional integrity tag, for example
>
>   # cryptsetup luksFormat -c aes-cbc-essiv:sha256 --integrity hmac-sha256 /dev/sdb
>
> it will produce this dm-crypt cipher spec:
>   capi:authenc(hmac(sha256),cbc(aes))-essiv:sha256
>
> the authenc(hmac(sha256),cbc(aes)) is direct crypto API cipher composition, the essiv:sha256
> IV is processed inside dm-crypt as IV.
>
> So if authenc() composition is problem, then yes, I am afraid these can be used in reality.
>
> But for things like gcm(aes)-essiv:sha256 (IOW real AEAD mode with ESSIV) - these are
> not supported by cryptsetup (we support only random IV in this case), so these should
> not be used anywhere.
>

OK, understood. Unfortunately, that means that the essiv template
should be dynamically instantiated as either a aead or a skcipher
depending on the context, but perhaps this is not a big deal in
reality, I will check.

One final question before I can proceed with my v2: in
crypt_ctr_blkdev_cipher(), do you think we could change the code to
look at the cipher string rather than the name of the actual cipher?
In practice, I don't think they can be different, but in order to be
able to instantiate
'essiv(authenc(hmac(sha256),cbc(aes)),sha256,aes)', the cipher part
needs to be parsed before the TFM(s) are instantiated.

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Milan Broz]

On 17/06/2019 19:29, Ard Biesheuvel wrote:
> On Mon, 17 Jun 2019 at 19:05, Milan Broz <gmazyland@gmail.com> wrote:
>>
>> On 17/06/2019 16:39, Ard Biesheuvel wrote:
>>>>
>>>> In other words, if you add some additional limit, we are breaking backward compatibility.
>>>> (Despite the configuration is "wrong" from the security point of view.)
>>>>
>>>
>>> Yes, but breaking backward compatibility only happens if you break
>>> something that is actually being *used*. So sure,
>>> xts(aes)-essiv:sha256 makes no sense but people use it anyway. But is
>>> that also true for, say, gcm(aes)-essiv:sha256 ?
>>
>> These should not be used.  The only way when ESSIV can combine with AEAD mode
>> is when you combine length-preserving mode with additional integrity tag, for example
>>
>>   # cryptsetup luksFormat -c aes-cbc-essiv:sha256 --integrity hmac-sha256 /dev/sdb
>>
>> it will produce this dm-crypt cipher spec:
>>   capi:authenc(hmac(sha256),cbc(aes))-essiv:sha256
>>
>> the authenc(hmac(sha256),cbc(aes)) is direct crypto API cipher composition, the essiv:sha256
>> IV is processed inside dm-crypt as IV.
>>
>> So if authenc() composition is problem, then yes, I am afraid these can be used in reality.
>>
>> But for things like gcm(aes)-essiv:sha256 (IOW real AEAD mode with ESSIV) - these are
>> not supported by cryptsetup (we support only random IV in this case), so these should
>> not be used anywhere.
>>
> 
> OK, understood. Unfortunately, that means that the essiv template
> should be dynamically instantiated as either a aead or a skcipher
> depending on the context, but perhaps this is not a big deal in
> reality, I will check.
> 
> One final question before I can proceed with my v2: in
> crypt_ctr_blkdev_cipher(), do you think we could change the code to
> look at the cipher string rather than the name of the actual cipher?
> In practice, I don't think they can be different, but in order to be
> able to instantiate
> 'essiv(authenc(hmac(sha256),cbc(aes)),sha256,aes)', the cipher part
> needs to be parsed before the TFM(s) are instantiated.

You mean to replace crypto_tfm_alg_name() with the cipher string
from the device-mapper table constructor?

I hope I am not missing anything, but it should be ok. It just could
fail later (in tfm init).
The constructor is de-facto atomic step for device-mapper, I think
it does not matter when it fails, the effect of failure is the same
for userspace.

Milan

Re: [RFC PATCH 0/3] crypto: switch to shash for ESSIV generation

[Milan Broz]

On 17/06/2019 19:29, Ard Biesheuvel wrote:
> On Mon, 17 Jun 2019 at 19:05, Milan Broz <gmazyland@gmail.com> wrote:
>>
>> On 17/06/2019 16:39, Ard Biesheuvel wrote:
>>>>
>>>> In other words, if you add some additional limit, we are breaking backward compatibility.
>>>> (Despite the configuration is "wrong" from the security point of view.)
>>>>
>>>
>>> Yes, but breaking backward compatibility only happens if you break
>>> something that is actually being *used*. So sure,
>>> xts(aes)-essiv:sha256 makes no sense but people use it anyway. But is
>>> that also true for, say, gcm(aes)-essiv:sha256 ?
>>
>> These should not be used.  The only way when ESSIV can combine with AEAD mode
>> is when you combine length-preserving mode with additional integrity tag, for example
>>
>>   # cryptsetup luksFormat -c aes-cbc-essiv:sha256 --integrity hmac-sha256 /dev/sdb
>>
>> it will produce this dm-crypt cipher spec:
>>   capi:authenc(hmac(sha256),cbc(aes))-essiv:sha256
>>
>> the authenc(hmac(sha256),cbc(aes)) is direct crypto API cipher composition, the essiv:sha256
>> IV is processed inside dm-crypt as IV.
>>
>> So if authenc() composition is problem, then yes, I am afraid these can be used in reality.
>>
>> But for things like gcm(aes)-essiv:sha256 (IOW real AEAD mode with ESSIV) - these are
>> not supported by cryptsetup (we support only random IV in this case), so these should
>> not be used anywhere.
>>
> 
> OK, understood. Unfortunately, that means that the essiv template
> should be dynamically instantiated as either a aead or a skcipher
> depending on the context, but perhaps this is not a big deal in
> reality, I will check.
> 
> One final question before I can proceed with my v2: in
> crypt_ctr_blkdev_cipher(), do you think we could change the code to
> look at the cipher string rather than the name of the actual cipher?
> In practice, I don't think they can be different, but in order to be
> able to instantiate
> 'essiv(authenc(hmac(sha256),cbc(aes)),sha256,aes)', the cipher part
> needs to be parsed before the TFM(s) are instantiated.

You mean to replace crypto_tfm_alg_name() with the cipher string
from the device-mapper table constructor?

I hope I am not missing anything, but it should be ok. It just could
fail later (in tfm init).
The constructor is de-facto atomic step for device-mapper, I think
it does not matter when it fails, the effect of failure is the same
for userspace.

Milan