[PATCH/proposal] dm-crypt: add digest-based iv generation mode

[PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Christophe Saout]

Hello,

since some people keep complaining that the IV generation mechanisms
supplied in cryptoloop (and now dm-crypt) are insecure, which they
somewhat are, I just hacked a small digest based IV generation mechanism.

It simply hashes the sector number and the key and uses it as IV.

You can specify the encryption mode as "cipher-digest" like aes-md5 or
serpent-sha1 or some other combination.

Consider this as a proposal, I'm not a crypto expert. Tell me if it
contains other flaws that should be fixed.

At least the "cryptoloop-exploit" Jari Ruusu posted doesn't work anymore.


--- linux-2.6.3-mm1.orig/drivers/md/dm-crypt.c	2004-02-19 17:43:53.213856776 +0100
+++ linux-2.6.3-mm1/drivers/md/dm-crypt.c	2004-02-19 17:43:59.404915592 +0100
@@ -61,11 +61,13 @@
 	/*
 	 * crypto related data
 	 */
-	struct crypto_tfm *tfm;
+	struct crypto_tfm *cipher;
+	struct crypto_tfm *digest;
 	sector_t iv_offset;
 	int (*iv_generator)(struct crypt_config *cc, u8 *iv, sector_t sector);
 	int iv_size;
 	int key_size;
+	int digest_size;
 	u8 key[0];
 };
 
@@ -102,6 +104,35 @@
 	return 0;
 }
 
+static int crypt_iv_digest(struct crypt_config *cc, u8 *iv, sector_t sector)
+{
+	static DECLARE_MUTEX(tfm_mutex);
+	struct scatterlist sg[2] = {
+		{
+			.page = virt_to_page(iv),
+			.offset = offset_in_page(iv),
+			.length = sizeof(u64) / sizeof(u8)
+		}, {
+			.page = virt_to_page(cc->key),
+			.offset = offset_in_page(cc->key),
+			.length = cc->key_size
+		}
+	};
+	int i;
+
+	*(u64 *)iv = cpu_to_le64((u64)sector);
+
+	/* digests use the context in the tfm, sigh */
+	down(&tfm_mutex);
+	crypto_digest_digest(cc->digest, sg, 2, iv);
+	up(&tfm_mutex);
+
+	for(i = cc->digest_size; i < cc->iv_size; i += cc->digest_size)
+		memcpy(iv + i, iv, min(cc->digest_size, cc->iv_size - i));
+
+	return 0;
+}
+
 static inline int
 crypt_convert_scatterlist(struct crypt_config *cc, struct scatterlist *out,
                           struct scatterlist *in, unsigned int length,
@@ -116,14 +147,14 @@
 			return r;
 
 		if (write)
-			r = crypto_cipher_encrypt_iv(cc->tfm, out, in, length, iv);
+			r = crypto_cipher_encrypt_iv(cc->cipher, out, in, length, iv);
 		else
-			r = crypto_cipher_decrypt_iv(cc->tfm, out, in, length, iv);
+			r = crypto_cipher_decrypt_iv(cc->cipher, out, in, length, iv);
 	} else {
 		if (write)
-			r = crypto_cipher_encrypt(cc->tfm, out, in, length);
+			r = crypto_cipher_encrypt(cc->cipher, out, in, length);
 		else
-			r = crypto_cipher_decrypt(cc->tfm, out, in, length);
+			r = crypto_cipher_decrypt(cc->cipher, out, in, length);
 	}
 
 	return r;
@@ -436,13 +467,26 @@
 		return -ENOMEM;
 	}
 
+	cc->digest_size = 0;
+	cc->digest = NULL;
 	if (!mode || strcmp(mode, "plain") == 0)
 		cc->iv_generator = crypt_iv_plain;
 	else if (strcmp(mode, "ecb") == 0)
 		cc->iv_generator = NULL;
 	else {
-		ti->error = "dm-crypt: Invalid chaining mode";
-		goto bad1;
+		tfm = crypto_alloc_tfm(mode, 0);
+		if (!tfm) {
+			ti->error = "dm-crypt: Error allocating digest tfm";
+			goto bad1;
+		}
+		if (crypto_tfm_alg_type(tfm) != CRYPTO_ALG_TYPE_DIGEST) {
+			ti->error = "dm-crypt: Expected digest algorithm";
+			goto bad1;
+		}
+
+		cc->digest = tfm;
+		cc->digest_size = crypto_tfm_alg_digestsize(tfm);
+		cc->iv_generator = crypt_iv_digest;
 	}
 
 	if (cc->iv_generator)
@@ -455,12 +499,18 @@
 		ti->error = "dm-crypt: Error allocating crypto tfm";
 		goto bad1;
 	}
+	if (crypto_tfm_alg_type(tfm) != CRYPTO_ALG_TYPE_CIPHER) {
+		ti->error = "dm-crypt: Expected cipher algorithm";
+		goto bad1;
+	}
 
-	if (tfm->crt_u.cipher.cit_decrypt_iv && tfm->crt_u.cipher.cit_encrypt_iv)
-		/* at least a 32 bit sector number should fit in our buffer */
+	if (tfm->crt_u.cipher.cit_decrypt_iv &&
+	    tfm->crt_u.cipher.cit_encrypt_iv) {
+		/* at least a sector number should fit in our buffer */
 		cc->iv_size = max(crypto_tfm_alg_ivsize(tfm), 
-		                  (unsigned int)(sizeof(u32) / sizeof(u8)));
-	else {
+		                  (unsigned int)(sizeof(u64) / sizeof(u8)));
+		cc->iv_size = max(cc->iv_size, cc->digest_size);
+	} else {
 		cc->iv_size = 0;
 		if (cc->iv_generator) {
 			DMWARN("dm-crypt: Selected cipher does not support IVs");
@@ -482,7 +532,7 @@
 		goto bad3;
 	}
 
-	cc->tfm = tfm;
+	cc->cipher = tfm;
 	cc->key_size = key_size;
 	if ((key_size == 0 && strcmp(argv[1], "-") != 0)
 	    || crypt_decode_key(cc->key, argv[1], key_size) < 0) {
@@ -521,6 +571,8 @@
 bad2:
 	crypto_free_tfm(tfm);
 bad1:
+	if (cc->digest)
+		crypto_free_tfm(cc->digest);
 	kfree(cc);
 	return -EINVAL;
 }
@@ -532,7 +584,10 @@
 	mempool_destroy(cc->page_pool);
 	mempool_destroy(cc->io_pool);
 
-	crypto_free_tfm(cc->tfm);
+	crypto_free_tfm(cc->cipher);
+	if (cc->digest)
+		crypto_free_tfm(cc->digest);
+
 	dm_put_device(ti, cc->dev);
 	kfree(cc);
 }
@@ -680,11 +735,14 @@
 		break;
 
 	case STATUSTYPE_TABLE:
-		cipher = crypto_tfm_alg_name(cc->tfm);
+		cipher = crypto_tfm_alg_name(cc->cipher);
 
-		switch(cc->tfm->crt_u.cipher.cit_mode) {
+		switch(cc->cipher->crt_u.cipher.cit_mode) {
 		case CRYPTO_TFM_MODE_CBC:
-			mode = "plain";
+			if (cc->digest)
+				mode = crypto_tfm_alg_name(cc->digest);
+			else
+				mode = "plain";
 			break;
 		case CRYPTO_TFM_MODE_ECB:
 			mode = "ecb";

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Andrew Morton]

Christophe Saout <christophe@saout.de> wrote:
>
> Hello,
> 
> since some people keep complaining that the IV generation mechanisms
> supplied in cryptoloop (and now dm-crypt) are insecure, which they
> somewhat are, I just hacked a small digest based IV generation mechanism.
> 
> It simply hashes the sector number and the key and uses it as IV.
> 
> You can specify the encryption mode as "cipher-digest" like aes-md5 or
> serpent-sha1 or some other combination.

hmm.

> Consider this as a proposal, I'm not a crypto expert.

Me either.  But I believe that there are crypto-savvy people reading this
list.  Help would be appreciated.

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Jean-Luc Cooke]

On Thu, Feb 19, 2004 at 11:18:35AM -0800, Andrew Morton wrote:
> Christophe Saout <christophe@saout.de> wrote:
> >
> > Hello,
> > 
> > since some people keep complaining that the IV generation mechanisms
> > supplied in cryptoloop (and now dm-crypt) are insecure, which they
> > somewhat are, I just hacked a small digest based IV generation mechanism.
> > 
> > It simply hashes the sector number and the key and uses it as IV.
> > 
> > You can specify the encryption mode as "cipher-digest" like aes-md5 or
> > serpent-sha1 or some other combination.

As for naming the cipher-hash as "aes-sha256", why not just go all the way
and specify the mode of operation as well?

cipher-hash-modeop example: aes-sha256-cbc

As for hashing the hey etc.  You should be using HMAC for that.
  Christophe - would you like to change your patch to use HMACs?
  http://www.faqs.org/rfcs/rfc2104.html

Cheers,

JLC

> hmm.
> 
> > Consider this as a proposal, I'm not a crypto expert.
> 
> Me either.  But I believe that there are crypto-savvy people reading this
> list.  Help would be appreciated.
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
http://www.certainkey.com
Suite 4560 CTTC
1125 Colonel By Dr.
Ottawa ON, K1S 5B6

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Christophe Saout]

On Fri, Feb 20, 2004 at 12:14:27PM -0500, Jean-Luc Cooke wrote:

> > > It simply hashes the sector number and the key and uses it as IV.
> > > 
> > > You can specify the encryption mode as "cipher-digest" like aes-md5 or
> > > serpent-sha1 or some other combination.
> 
> As for naming the cipher-hash as "aes-sha256", why not just go all the way
> and specify the mode of operation as well?
> 
> cipher-hash-modeop example: aes-sha256-cbc

The plan was to du <cipher>-<iv mode> where <iv mode> can be
ecb (well, no IV at all), plain (unhashed sector number) or a
digest (hmac_key sector number). CBC mode is implicit when you
have some kind of IV generation. Everything else doesn't make
sense and would be redundant. CFB and CTR are not implemented
by cryptoloop BTW.

> As for hashing the hey etc.  You should be using HMAC for that.
>   Christophe - would you like to change your patch to use HMACs?

Yes, I alread got that suggestion.

This attached patch does this, along with some cleanups.

The crypto_hmac_init part could be done in the dm target constructor
once and then call crypto_hmac_update and crypto_hmac_final in the
iv_generator but this would require some cryptoapi hacking which I'm
not too happy about. I would like to make a copy of the tfm (including
the private context) on the stack and then operate on that copy for
crypto_hmac_update and crypto_hmac_final.

Can I have a function to return the tfm size so I can do a memcpy
to a variable on the stack? I could get rid of the mutex too then.

(in case you're wondering, crypto_hmac calls crypto_hmac_init,
crypto_hmac_update and crypto_hmac_final)


--- linux.orig/drivers/md/dm-crypt.c	2004-02-20 19:36:50.363184760 +0100
+++ linux/drivers/md/dm-crypt.c	2004-02-20 19:36:58.265983352 +0100
@@ -107,16 +107,10 @@
 static int crypt_iv_digest(struct crypt_config *cc, u8 *iv, sector_t sector)
 {
 	static DECLARE_MUTEX(tfm_mutex);
-	struct scatterlist sg[2] = {
-		{
-			.page = virt_to_page(iv),
-			.offset = offset_in_page(iv),
-			.length = sizeof(u64) / sizeof(u8)
-		}, {
-			.page = virt_to_page(cc->key),
-			.offset = offset_in_page(cc->key),
-			.length = cc->key_size
-		}
+	struct scatterlist sg = {
+		.page = virt_to_page(iv),
+		.offset = offset_in_page(iv),
+		.length = sizeof(u64) / sizeof(u8)
 	};
 	int i;
 
@@ -124,7 +118,8 @@
 
 	/* digests use the context in the tfm, sigh */
 	down(&tfm_mutex);
-	crypto_digest_digest(cc->digest, sg, 2, iv);
+	crypto_hmac(cc->digest, cc->key, (unsigned int *)&cc->key_size,
+	            &sg, 1, iv);
 	up(&tfm_mutex);
 
 	for(i = cc->digest_size; i < cc->iv_size; i += cc->digest_size)
@@ -504,8 +499,8 @@
 		goto bad1;
 	}
 
-	if (tfm->crt_u.cipher.cit_decrypt_iv &&
-	    tfm->crt_u.cipher.cit_encrypt_iv) {
+	if (tfm->crt_cipher.cit_decrypt_iv &&
+	    tfm->crt_cipher.cit_encrypt_iv) {
 		/* at least a sector number should fit in our buffer */
 		cc->iv_size = max(crypto_tfm_alg_ivsize(tfm), 
 		                  (unsigned int)(sizeof(u64) / sizeof(u8)));
@@ -540,7 +535,7 @@
 		goto bad4;
 	}
 
-	if (tfm->crt_u.cipher.cit_setkey(tfm, cc->key, key_size) < 0) {
+	if (tfm->crt_cipher.cit_setkey(tfm, cc->key, key_size) < 0) {
 		ti->error = "dm-crypt: Error setting key";
 		goto bad4;
 	}
@@ -737,19 +732,12 @@
 	case STATUSTYPE_TABLE:
 		cipher = crypto_tfm_alg_name(cc->cipher);
 
-		switch(cc->cipher->crt_u.cipher.cit_mode) {
-		case CRYPTO_TFM_MODE_CBC:
-			if (cc->digest)
-				mode = crypto_tfm_alg_name(cc->digest);
-			else
-				mode = "plain";
-			break;
-		case CRYPTO_TFM_MODE_ECB:
+		if (!cc->iv_generator)
 			mode = "ecb";
-			break;
-		default:
-			BUG();
-		}
+		else if (!cc->digest)
+			mode = "plain";
+		else
+			mode = crypto_tfm_alg_name(cc->digest);
 
 		snprintf(result, maxlen, "%s-%s ", cipher, mode);
 		offset = strlen(result);

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Jean-Luc Cooke]

On Fri, Feb 20, 2004 at 07:53:41PM +0100, Christophe Saout wrote:
> On Fri, Feb 20, 2004 at 12:14:27PM -0500, Jean-Luc Cooke wrote:
> 
> > > > It simply hashes the sector number and the key and uses it as IV.
> > > > 
> > > > You can specify the encryption mode as "cipher-digest" like aes-md5 or
> > > > serpent-sha1 or some other combination.
> > 
> > As for naming the cipher-hash as "aes-sha256", why not just go all the way
> > and specify the mode of operation as well?
> > 
> > cipher-hash-modeop example: aes-sha256-cbc
> 
> The plan was to du <cipher>-<iv mode> where <iv mode> can be
> ecb (well, no IV at all), plain (unhashed sector number) or a
> digest (hmac_key sector number). CBC mode is implicit when you
> have some kind of IV generation. Everything else doesn't make
> sense and would be redundant. CFB and CTR are not implemented
> by cryptoloop BTW.

jlcooke:~/kern/linux-2.6.1/crypto$ grep CTR *.c
cipher.c:       case CRYPTO_TFM_MODE_CTR:
grep CFB *.c
cipher.c:       case CRYPTO_TFM_MODE_CFB:

It should be I wrote it...the crypto part anyways.

> > As for hashing the hey etc.  You should be using HMAC for that.
> >   Christophe - would you like to change your patch to use HMACs?
> 
> Yes, I alread got that suggestion.

Ok good then thanks.

JLC

-- 
http://www.certainkey.com
Suite 4560 CTTC
1125 Colonel By Dr.
Ottawa ON, K1S 5B6

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Christophe Saout]

Am Fr, den 20.02.2004 schrieb Jean-Luc Cooke um 20:09:

> > The plan was to du <cipher>-<iv mode> where <iv mode> can be
> > ecb (well, no IV at all), plain (unhashed sector number) or a
> > digest (hmac_key sector number). CBC mode is implicit when you
> > have some kind of IV generation. Everything else doesn't make
> > sense and would be redundant. CFB and CTR are not implemented
> > by cryptoloop BTW.
> 
> jlcooke:~/kern/linux-2.6.1/crypto$ grep CTR *.c
> cipher.c:       case CRYPTO_TFM_MODE_CTR:
> grep CFB *.c
> cipher.c:       case CRYPTO_TFM_MODE_CFB:
> 
> It should be I wrote it...the crypto part anyways.

Do you think I should add support for those two (even though they're not
implemented yet)? Does this make sense?

If so, I would do: <cipher>-<cipher mode>[-<iv generator>]

aes-ecb, aes-cbc-sha1, etc...?

This would already break the userspace interface... ;)

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[James Morris]

On Fri, 20 Feb 2004, Jean-Luc Cooke wrote:

> > sense and would be redundant. CFB and CTR are not implemented
> > by cryptoloop BTW.
> 
> jlcooke:~/kern/linux-2.6.1/crypto$ grep CTR *.c
> cipher.c:       case CRYPTO_TFM_MODE_CTR:
> grep CFB *.c
> cipher.c:       case CRYPTO_TFM_MODE_CFB:
> 
> It should be I wrote it...the crypto part anyways.
> 

These are just placeholders.  Before this, we need to work out how to 
support stream ciphers in general (perhaps limit to 'byte' streams?).


- James
-- 
James Morris
<jmorris@redhat.com>

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Christophe Saout]

James,

> The crypto_hmac_init part could be done in the dm target constructor
> once and then call crypto_hmac_update and crypto_hmac_final in the
> iv_generator but this would require some cryptoapi hacking which I'm
> not too happy about. I would like to make a copy of the tfm (including
> the private context) on the stack and then operate on that copy for
> crypto_hmac_update and crypto_hmac_final.
> 
> Can I have a function to return the tfm size so I can do a memcpy
> to a variable on the stack? I could get rid of the mutex too then.

What do you think of this?

I would like to copy the tfm onto the stack so that I can
a) compute the hmac on several CPUs at the same time without locking
b) reuse a precomputed tfm from just after crypt_hmac_init

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[James Morris]

On Fri, 20 Feb 2004, Christophe Saout wrote:

> What do you think of this?
> 
> I would like to copy the tfm onto the stack so that I can
> a) compute the hmac on several CPUs at the same time without locking
> b) reuse a precomputed tfm from just after crypt_hmac_init

I think you'll run into problems on highmem boxes when the code kmaps 
pages.  IIRC, Matt Mackall has looked into this in detail.


- James
-- 
James Morris
<jmorris@redhat.com>

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Christophe Saout]

On Fri, Feb 20, 2004 at 12:14:27PM -0500, Jean-Luc Cooke wrote:

> As for naming the cipher-hash as "aes-sha256", why not just go all the way
> and specify the mode of operation as well?
> 
> cipher-hash-modeop example: aes-sha256-cbc

cipher-modeop[-hash] is less ambiguous, because the hash-less config is
the backward-compatible mode (and for Kamikaze ECB mode).

> As for hashing the hey etc.  You should be using HMAC for that.

Ok, I've rewritten it so you can now generate an IV through HMAC with
a user-specified digest and changed the config string to
<cipher>-<mode>[-<digest>].

And I'm memcpy'ing the tfm onto the stack now to have a local copy.
So I don't need to lock and it makes HMAC precomputation possible
because I don't destroy the original tfm context.

(this is a diff against the original dm-crypt version in linux-2.6.3-mm1)


diff -Nur linux-2.6.3-mm1.orig/drivers/md/dm-crypt.c linux-2.6.3-mm1/drivers/md/dm-crypt.c
--- linux-2.6.3-mm1.orig/drivers/md/dm-crypt.c	2004-02-21 03:05:21.444341304 +0100
+++ linux-2.6.3-mm1/drivers/md/dm-crypt.c	2004-02-21 03:05:44.070901544 +0100
@@ -61,11 +61,13 @@
 	/*
 	 * crypto related data
 	 */
-	struct crypto_tfm *tfm;
+	struct crypto_tfm *cipher;
+	struct crypto_tfm *digest;
 	sector_t iv_offset;
 	int (*iv_generator)(struct crypt_config *cc, u8 *iv, sector_t sector);
 	int iv_size;
 	int key_size;
+	int digest_size;
 	u8 key[0];
 };
 
@@ -102,6 +104,32 @@
 	return 0;
 }
 
+static int crypt_iv_hmac(struct crypt_config *cc, u8 *iv, sector_t sector)
+{
+	struct scatterlist sg = {
+		.page = virt_to_page(iv),
+		.offset = offset_in_page(iv),
+		.length = sizeof(u64) / sizeof(u8)
+	};
+	int i;
+	int tfm_size = sizeof(*cc->digest) + cc->digest->__crt_alg->cra_ctxsize;
+	char tfm[tfm_size];
+
+	*(u64 *)iv = cpu_to_le64((u64)sector);
+
+	/* HMAC has already been initialized, finish it on private copy */
+	memcpy(tfm, cc->digest, tfm_size);
+	crypto_hmac_update(cc->digest, &sg, 1);
+	crypto_hmac_final(cc->digest, cc->key,
+	                  (unsigned int *)&cc->key_size, iv);
+
+	/* fill the rest of the vector if it is larger */
+	for(i = cc->digest_size; i < cc->iv_size; i += cc->digest_size)
+		memcpy(iv + i, iv, min(cc->digest_size, cc->iv_size - i));
+
+	return 0;
+}
+
 static inline int
 crypt_convert_scatterlist(struct crypt_config *cc, struct scatterlist *out,
                           struct scatterlist *in, unsigned int length,
@@ -116,14 +144,14 @@
 			return r;
 
 		if (write)
-			r = crypto_cipher_encrypt_iv(cc->tfm, out, in, length, iv);
+			r = crypto_cipher_encrypt_iv(cc->cipher, out, in, length, iv);
 		else
-			r = crypto_cipher_decrypt_iv(cc->tfm, out, in, length, iv);
+			r = crypto_cipher_decrypt_iv(cc->cipher, out, in, length, iv);
 	} else {
 		if (write)
-			r = crypto_cipher_encrypt(cc->tfm, out, in, length);
+			r = crypto_cipher_encrypt(cc->cipher, out, in, length);
 		else
-			r = crypto_cipher_decrypt(cc->tfm, out, in, length);
+			r = crypto_cipher_decrypt(cc->cipher, out, in, length);
 	}
 
 	return r;
@@ -401,6 +429,17 @@
 	}
 }
 
+static struct cipher_mode {
+	const char	*name;
+	int			mode;
+} cipher_modes[] = {
+	{ "ecb", CRYPTO_TFM_MODE_ECB },
+	{ "cbc", CRYPTO_TFM_MODE_CBC },
+	{ "cfb", CRYPTO_TFM_MODE_CFB },
+	{ "ctr", CRYPTO_TFM_MODE_CTR },
+	{ NULL,  0 }
+};
+
 /*
  * Construct an encryption mapping:
  * <cipher> <key> <iv_offset> <dev_path> <start>
@@ -412,6 +451,7 @@
 	char *tmp;
 	char *cipher;
 	char *mode;
+	char *digest;
 	int crypto_flags;
 	int key_size;
 
@@ -423,10 +463,32 @@
 	tmp = argv[0];
 	cipher = strsep(&tmp, "-");
 	mode = strsep(&tmp, "-");
+	digest = strsep(&tmp, "-");
 
 	if (tmp)
 		DMWARN("dm-crypt: Unexpected additional cipher options");
 
+	if (mode) {
+		struct cipher_mode *cm;
+
+		for(cm = cipher_modes; cm->name; cm++)
+			if (strcmp(cm->name, mode) == 0)
+				break;
+		if (!cm->name) {
+			ti->error = "dm-crypt: Invalid cipher mode";
+			return -EINVAL;
+		}
+
+		crypto_flags = cm->mode;
+	} else {
+		crypto_flags = CRYPTO_TFM_MODE_CBC;
+	}
+
+	if (crypto_flags == CRYPTO_TFM_MODE_ECB && digest) {
+		ti->error = "dm-crypt: ECB does not support IVs";
+		return -EINVAL;
+	}
+
 	key_size = strlen(argv[1]) >> 1;
 
 	cc = kmalloc(sizeof(*cc) + key_size * sizeof(u8), GFP_KERNEL);
@@ -436,40 +498,55 @@
 		return -ENOMEM;
 	}
 
-	if (!mode || strcmp(mode, "plain") == 0)
-		cc->iv_generator = crypt_iv_plain;
-	else if (strcmp(mode, "ecb") == 0)
+	cc->digest_size = 0;
+	cc->digest = NULL;
+	if (crypto_flags == CRYPTO_TFM_MODE_ECB)
 		cc->iv_generator = NULL;
+	else if (!digest)
+		cc->iv_generator = crypt_iv_plain;
 	else {
-		ti->error = "dm-crypt: Invalid chaining mode";
-		goto bad1;
-	}
+		/* the IV generator is the name of a digest used for HMAC */
+		tfm = crypto_alloc_tfm(digest, 0);
+		if (!tfm) {
+			ti->error = "dm-crypt: Error allocating digest tfm";
+			goto bad1;
+		}
+		if (crypto_tfm_alg_type(tfm) != CRYPTO_ALG_TYPE_DIGEST) {
+			ti->error = "dm-crypt: Expected digest algorithm";
+			goto bad1;
+		}
 
-	if (cc->iv_generator)
-		crypto_flags = CRYPTO_TFM_MODE_CBC;
-	else
-		crypto_flags = CRYPTO_TFM_MODE_ECB;
+		cc->digest = tfm;
+		cc->digest_size = crypto_tfm_alg_digestsize(tfm);
+		cc->iv_generator = crypt_iv_hmac;
+	}
 
 	tfm = crypto_alloc_tfm(cipher, crypto_flags);
 	if (!tfm) {
 		ti->error = "dm-crypt: Error allocating crypto tfm";
 		goto bad1;
 	}
+	if (crypto_tfm_alg_type(tfm) != CRYPTO_ALG_TYPE_CIPHER) {
+		ti->error = "dm-crypt: Expected cipher algorithm";
+		goto bad2;
+	}
 
-	if (tfm->crt_u.cipher.cit_decrypt_iv && tfm->crt_u.cipher.cit_encrypt_iv)
-		/* at least a 32 bit sector number should fit in our buffer */
+	if (tfm->crt_cipher.cit_decrypt_iv &&
+	    tfm->crt_cipher.cit_encrypt_iv) {
+		/* at least a sector number should fit in our buffer */
 		cc->iv_size = max(crypto_tfm_alg_ivsize(tfm), 
-		                  (unsigned int)(sizeof(u32) / sizeof(u8)));
-	else {
+		                  (unsigned int)(sizeof(u64) / sizeof(u8)));
+		cc->iv_size = max(cc->iv_size, cc->digest_size);
+	} else {
 		cc->iv_size = 0;
 		if (cc->iv_generator) {
-			DMWARN("dm-crypt: Selected cipher does not support IVs");
-			cc->iv_generator = NULL;
+			ti->error = "dm-crypt: Cipher does not support IVs";
+			goto bad2;
 		}
 	}
 
 	cc->io_pool = mempool_create(MIN_IOS, mempool_alloc_slab,
-				     mempool_free_slab, _crypt_io_pool);
+	                             mempool_free_slab, _crypt_io_pool);
 	if (!cc->io_pool) {
 		ti->error = "dm-crypt: Cannot allocate crypt io mempool";
 		goto bad2;
@@ -482,7 +559,7 @@
 		goto bad3;
 	}
 
-	cc->tfm = tfm;
+	cc->cipher = tfm;
 	cc->key_size = key_size;
 	if ((key_size == 0 && strcmp(argv[1], "-") != 0)
 	    || crypt_decode_key(cc->key, argv[1], key_size) < 0) {
@@ -490,11 +567,16 @@
 		goto bad4;
 	}
 
-	if (tfm->crt_u.cipher.cit_setkey(tfm, cc->key, key_size) < 0) {
+	if (tfm->crt_cipher.cit_setkey(tfm, cc->key, key_size) < 0) {
 		ti->error = "dm-crypt: Error setting key";
 		goto bad4;
 	}
 
+	/* precompute part of the HMAC here, it only depends on the key */
+	if (cc->digest)
+		crypto_hmac_init(cc->digest, cc->key,
+		                 (unsigned int *)&key_size);
+
 	if (sscanf(argv[2], SECTOR_FORMAT, &cc->iv_offset) != 1) {
 		ti->error = "dm-crypt: Invalid iv_offset sector";
 		goto bad4;
@@ -521,6 +603,8 @@
 bad2:
 	crypto_free_tfm(tfm);
 bad1:
+	if (cc->digest)
+		crypto_free_tfm(cc->digest);
 	kfree(cc);
 	return -EINVAL;
 }
@@ -532,7 +616,10 @@
 	mempool_destroy(cc->page_pool);
 	mempool_destroy(cc->io_pool);
 
-	crypto_free_tfm(cc->tfm);
+	crypto_free_tfm(cc->cipher);
+	if (cc->digest)
+		crypto_free_tfm(cc->digest);
+
 	dm_put_device(ti, cc->dev);
 	kfree(cc);
 }
@@ -669,9 +756,10 @@
 			char *result, unsigned int maxlen)
 {
 	struct crypt_config *cc = (struct crypt_config *) ti->private;
+	struct cipher_mode *cm;
 	char buffer[32];
 	const char *cipher;
-	const char *mode = NULL;
+	const char *digest;
 	int offset;
 
 	switch (type) {
@@ -680,20 +768,20 @@
 		break;
 
 	case STATUSTYPE_TABLE:
-		cipher = crypto_tfm_alg_name(cc->tfm);
+		cipher = crypto_tfm_alg_name(cc->cipher);
 
-		switch(cc->tfm->crt_u.cipher.cit_mode) {
-		case CRYPTO_TFM_MODE_CBC:
-			mode = "plain";
-			break;
-		case CRYPTO_TFM_MODE_ECB:
-			mode = "ecb";
-			break;
-		default:
-			BUG();
-		}
+		for(cm = cipher_modes; cm->name; cm++)
+			if (cm->mode == cc->cipher->crt_cipher.cit_mode)
+				break;
+		BUG_ON(!cm->name);
+
+		if (cc->digest)
+			digest = crypto_tfm_alg_name(cc->digest);
+		else
+			digest = "";
 
-		snprintf(result, maxlen, "%s-%s ", cipher, mode);
+		snprintf(result, maxlen, "%s-%s%s%s ", cipher, cm->name,
+		         digest[0] ? "-" : "", digest);
 		offset = strlen(result);
 
 		if (cc->key_size > 0) {
diff -Nur linux-2.6.3-mm1.orig/drivers/md/Kconfig linux-2.6.3-mm1/drivers/md/Kconfig
--- linux-2.6.3-mm1.orig/drivers/md/Kconfig	2004-02-21 03:04:59.338701872 +0100
+++ linux-2.6.3-mm1/drivers/md/Kconfig	2004-02-21 03:05:44.071901392 +0100
@@ -174,6 +174,7 @@
 	tristate "Crypt target support"
 	depends on BLK_DEV_DM && EXPERIMENTAL
 	select CRYPTO
+	select CRYPTO_HMAC
 	---help---
 	  This device-mapper target allows you to create a device that
 	  transparently encrypts the data on it. You'll need to activate

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Matt Mackall]

On Sat, Feb 21, 2004 at 03:17:25AM +0100, Christophe Saout wrote:
> 
> And I'm memcpy'ing the tfm onto the stack now to have a local copy.
> So I don't need to lock and it makes HMAC precomputation possible
> because I don't destroy the original tfm context.
> 
> +static int crypt_iv_hmac(struct crypt_config *cc, u8 *iv, sector_t sector)
> +{
> +	struct scatterlist sg = {
> +		.page = virt_to_page(iv),
> +		.offset = offset_in_page(iv),
> +		.length = sizeof(u64) / sizeof(u8)
> +	};
> +	int i;
> +	int tfm_size = sizeof(*cc->digest) + cc->digest->__crt_alg->cra_ctxsize;
> +	char tfm[tfm_size];
> +
> +	*(u64 *)iv = cpu_to_le64((u64)sector);
> +
> +	/* HMAC has already been initialized, finish it on private copy */
> +	memcpy(tfm, cc->digest, tfm_size);

As this stands, it's rather scary.

- it will quietly break when cryptoapi gets fiddled with
- it subverts the module reference counting rules
- for a given cipher/digest, tfm_size might be large

Subverting the API this way is bad. On the other hand, I tend to think
the API does need a way to deal with problem cases like these, so I'd
support extending the API in some fashion to handle it. Related (but
not identical) issues have cropped up with a few other things that
want to avoid serializing around a single or per-cpu context.

Something like:

 /* calculate the size of a tfm so that users can manage their own
 copies */

 int crypto_alg_size(const char *name);

 /* copy a TFM to a user-managed buffer, possibly on stack, with proper
 internal reference counting and any other necessary magic, size checks
 against boneheaded buffer sizing */

 crypto_copy_tfm(char *dst, const struct crypto_tfm *src, int size);

 /* do all the necessary bookkeeping to release a user-managed TFM, use
 char pointer to avoid alloc/free mismatch */

 crypto_copy_cleanup_tfm(char *usertfm);


James, thoughts?

-- 
Matt Mackall : http://www.selenic.com : Linux development and consulting

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Christophe Saout]

Am Di, den 24.02.2004 schrieb Matt Mackall um 20:11:

> As this stands, it's rather scary.
> 
> - it will quietly break when cryptoapi gets fiddled with
> - it subverts the module reference counting rules
> - for a given cipher/digest, tfm_size might be large
>
> Subverting the API this way is bad. On the other hand,

Right. That's why I repeated several times that this is a hack and Cc'ed
James. I was either misunderstood or ignored.

> I tend to think
> the API does need a way to deal with problem cases like these, so I'd
> support extending the API in some fashion to handle it. Related (but
> not identical) issues have cropped up with a few other things that
> want to avoid serializing around a single or per-cpu context.

If you call encrypt/decrypt without the iv parameter but use cbc mode
you will run into the same problem. This is right.

BTW: I think there's a bug in the ipv6 code, it uses spin_lock to
protect itself, this will cause a sleep-inside-spinlock warning. (found
while grepping through the source for other cryptoapi users)

> Something like:
> 
>  /* calculate the size of a tfm so that users can manage their own
>  copies */
> 
>  int crypto_alg_size(const char *name);

crypto_tfm_size?

>  /* copy a TFM to a user-managed buffer, possibly on stack, with proper
>  internal reference counting and any other necessary magic, size checks
>  against boneheaded buffer sizing */
> 
>  crypto_copy_tfm(char *dst, const struct crypto_tfm *src, int size);
> 
>  /* do all the necessary bookkeeping to release a user-managed TFM, use
>  char pointer to avoid alloc/free mismatch */
> 
>  crypto_copy_cleanup_tfm(char *usertfm);

Yes, I thought of something like this.

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Matt Mackall]

On Tue, Feb 24, 2004 at 08:43:59PM +0100, Christophe Saout wrote:
>
> BTW: I think there's a bug in the ipv6 code, it uses spin_lock to
> protect itself, this will cause a sleep-inside-spinlock warning. (found
> while grepping through the source for other cryptoapi users)

Yep, I sent this to James several months ago but it appears we've both
forgotten about it:

diff -urN -X dontdiff orig/crypto/api.c work/crypto/api.c
--- orig/crypto/api.c	2003-07-13 22:38:38.000000000 -0500
+++ work/crypto/api.c	2003-08-14 01:53:07.000000000 -0500
@@ -53,7 +53,8 @@
 
 static int crypto_init_flags(struct crypto_tfm *tfm, u32 flags)
 {
-	tfm->crt_flags = 0;
+	tfm->crt_flags = flags & CRYPTO_TFM_API_MASK;
+	flags &= ~CRYPTO_TFM_API_MASK;
 	
 	switch (crypto_tfm_alg_type(tfm)) {
 	case CRYPTO_ALG_TYPE_CIPHER:
diff -urN -X dontdiff orig/crypto/internal.h work/crypto/internal.h
--- orig/crypto/internal.h	2003-07-13 22:29:11.000000000 -0500
+++ work/crypto/internal.h	2003-08-14 01:53:28.000000000 -0500
@@ -37,7 +37,7 @@
 
 static inline void crypto_yield(struct crypto_tfm *tfm)
 {
-	if (!in_softirq())
+	if (tfm->crt_flags & CRYPTO_TFM_API_YIELD)
 		cond_resched();
 }
 
diff -urN -X dontdiff orig/include/linux/crypto.h work/include/linux/crypto.h
--- orig/include/linux/crypto.h	2003-07-13 22:32:32.000000000 -0500
+++ work/include/linux/crypto.h	2003-08-14 01:53:07.000000000 -0500
@@ -36,7 +36,8 @@
  */
 #define CRYPTO_TFM_MODE_MASK		0x000000ff
 #define CRYPTO_TFM_REQ_MASK		0x000fff00
-#define CRYPTO_TFM_RES_MASK		0xfff00000
+#define CRYPTO_TFM_RES_MASK		0x7ff00000
+#define CRYPTO_TFM_API_MASK		0x80000000
 
 #define CRYPTO_TFM_MODE_ECB		0x00000001
 #define CRYPTO_TFM_MODE_CBC		0x00000002
@@ -50,6 +51,9 @@
 #define CRYPTO_TFM_RES_BAD_BLOCK_LEN 	0x00800000
 #define CRYPTO_TFM_RES_BAD_FLAGS 	0x01000000
 
+/* Allow for rescheduling after processing each sg element */
+#define CRYPTO_TFM_API_YIELD		0x80000000
+
 /*
  * Miscellaneous stuff.
  */
diff -urN -X dontdiff orig/include/linux/sched.h work/include/linux/sched.h
--- orig/include/linux/sched.h	2003-08-14 01:53:04.000000000 -0500
+++ work/include/linux/sched.h	2003-08-14 01:53:07.000000000 -0500
@@ -824,6 +824,7 @@
 extern void __cond_resched(void);
 static inline void cond_resched(void)
 {
+	might_sleep();
 	if (need_resched())
 		__cond_resched();
 }

> > Something like:
> > 
> >  /* calculate the size of a tfm so that users can manage their own
> >  copies */
> > 
> >  int crypto_alg_size(const char *name);
> 
> crypto_tfm_size?

Sure.
 
> >  /* copy a TFM to a user-managed buffer, possibly on stack, with proper
> >  internal reference counting and any other necessary magic, size checks
> >  against boneheaded buffer sizing */
> > 
> >  crypto_copy_tfm(char *dst, const struct crypto_tfm *src, int size);
> > 
> >  /* do all the necessary bookkeeping to release a user-managed TFM, use
> >  char pointer to avoid alloc/free mismatch */
> > 
> >  crypto_copy_cleanup_tfm(char *usertfm);
> 
> Yes, I thought of something like this.

Ok, I might get to this by this afternoon. 

-- 
Matt Mackall : http://www.selenic.com : Linux development and consulting

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Matt Mackall]

On Tue, Feb 24, 2004 at 02:38:25PM -0600, Matt Mackall wrote:
> On Tue, Feb 24, 2004 at 08:43:59PM +0100, Christophe Saout wrote:
> > Yes, I thought of something like this.
> 
> Ok, I might get to this by this afternoon. 
> 

Ok, here's my proposed API extension (currently untested). Christophe,
care to give it a spin?

 tiny-mpm/crypto/api.c           |   21 +++++++++++++++++++++
 tiny-mpm/include/linux/crypto.h |   17 +++++++++++++++++
 2 files changed, 38 insertions(+)

diff -puN crypto/api.c~crypto-copy crypto/api.c
--- tiny/crypto/api.c~crypto-copy	2004-02-25 15:12:43.000000000 -0600
+++ tiny-mpm/crypto/api.c	2004-02-25 15:37:39.000000000 -0600
@@ -161,6 +161,27 @@ void crypto_free_tfm(struct crypto_tfm *
 	kfree(tfm);
 }
 
+int crypto_copy_tfm(char *dst, const struct crypto_tfm *src, unsigned size)
+{
+	int s = crypto_tfm_size(src);
+
+	if (size < s)
+		return 0;
+
+	crypto_alg_get(tfm->__crt_alg);
+
+	/* currently assumes shallow copy is sufficient */
+	memcpy(tfm, cc->digest, s);
+
+	return s;
+}
+
+void crypto_cleanup_copy_tfm(char *user_tfm)
+{
+	crypto_exit_ops((struct crypto_tfm *)user_tfm);
+	crypto_alg_put((struct crypto_tfm *)user_tfm->__crt_alg);
+}
+
 int crypto_register_alg(struct crypto_alg *alg)
 {
 	int ret = 0;
diff -puN include/linux/crypto.h~crypto-copy include/linux/crypto.h
--- tiny/include/linux/crypto.h~crypto-copy	2004-02-25 15:12:43.000000000 -0600
+++ tiny-mpm/include/linux/crypto.h	2004-02-25 15:26:23.000000000 -0600
@@ -208,6 +208,23 @@ struct crypto_tfm {
 struct crypto_tfm *crypto_alloc_tfm(const char *alg_name, u32 tfm_flags);
 void crypto_free_tfm(struct crypto_tfm *tfm);
 
+
+/*
+ * These functions support user-managed crypto transforms, possibly on
+ * the stack. These can be useful in situations where preemption or the like
+ * would force serializing with preallocated transforms.
+ *
+ * This interface is intended to be safe from all contexts.
+ */
+
+static inline int crypto_tfm_size(struct crypto_tfm *tfm)
+{
+	return sizeof(*tfm->digest) + cc->digest->__crt_alg->cra_ctxsize
+}
+
+int crypto_copy_tfm(char *dst, const struct crypto_tfm *src, unsigned size);
+void crypto_cleanup_copy_tfm(char *user_tfm);
+
 /*
  * Transform helpers which query the underlying algorithm.
  */

_


-- 
Matt Mackall : http://www.selenic.com : Linux development and consulting

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Christophe Saout]

Am Mi, den 25.02.2004 schrieb Matt Mackall um 22:43:

> Ok, here's my proposed API extension (currently untested). Christophe,
> care to give it a spin?
>
> diff -puN crypto/api.c~crypto-copy crypto/api.c
> --- tiny/crypto/api.c~crypto-copy	2004-02-25 15:12:43.000000000 -0600
> +++ tiny-mpm/crypto/api.c	2004-02-25 15:37:39.000000000 -0600
> @@ -161,6 +161,27 @@ void crypto_free_tfm(struct crypto_tfm *
>  	kfree(tfm);
>  }
>  
> +int crypto_copy_tfm(char *dst, const struct crypto_tfm *src, unsigned size)
> +{
> +	int s = crypto_tfm_size(src);
> +
> +	if (size < s)
> +		return 0;

Why the extra check?

> +void crypto_cleanup_copy_tfm(char *user_tfm)
> +{
> +	crypto_exit_ops((struct crypto_tfm *)user_tfm);

This looks dangerous. The algorithm might free a buffer. This is only
safe if we introduce per-algorithm copy methods that also duplicate
external buffers.

I'd like to avoid a kmalloc in crypto_copy_tfm. This function also does
the same as crypto_free_tfm except for the final kfree(tfm). So
crypto_free_tfm could call this function. And it could have a better
name.

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Matt Mackall]

On Thu, Feb 26, 2004 at 08:35:46PM +0100, Christophe Saout wrote:
> Am Mi, den 25.02.2004 schrieb Matt Mackall um 22:43:
> 
> > Ok, here's my proposed API extension (currently untested). Christophe,
> > care to give it a spin?
> >
> > diff -puN crypto/api.c~crypto-copy crypto/api.c
> > --- tiny/crypto/api.c~crypto-copy	2004-02-25 15:12:43.000000000 -0600
> > +++ tiny-mpm/crypto/api.c	2004-02-25 15:37:39.000000000 -0600
> > @@ -161,6 +161,27 @@ void crypto_free_tfm(struct crypto_tfm *
> >  	kfree(tfm);
> >  }
> >  
> > +int crypto_copy_tfm(char *dst, const struct crypto_tfm *src, unsigned size)
> > +{
> > +	int s = crypto_tfm_size(src);
> > +
> > +	if (size < s)
> > +		return 0;
> 
> Why the extra check?

User is giving us the size of his buffer, not the size of the tfm
which we already know. We refuse to copy if buffer is not big enough,
otherwise return number of bytes copied. This may seem a little
redundant for the on-stack usage of the API, but may make sense in
other cases.
 
> > +void crypto_cleanup_copy_tfm(char *user_tfm)
> > +{
> > +	crypto_exit_ops((struct crypto_tfm *)user_tfm);
> 
> This looks dangerous. The algorithm might free a buffer. This is only
> safe if we introduce per-algorithm copy methods that also duplicate
> external buffers.

I'm currently working under the assumption that such external buffers
are unnecessary but I haven't done the audit. If and when such code
exist, such code should be added, yes. Hence the comment in the copy
function:

+       /* currently assumes shallow copy is sufficient */

> I'd like to avoid a kmalloc in crypto_copy_tfm.

Well you're in luck because there isn't one.

> This function also does the same as crypto_free_tfm except for the
> final kfree(tfm). So crypto_free_tfm could call this function. 

Yes.

> And it could have a better name.

Just about everything could have a better name if the poets among us
were to speak up.

-- 
Matt Mackall : http://www.selenic.com : Linux development and consulting

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Christophe Saout]

Am Do, den 26.02.2004 schrieb Matt Mackall um 21:02:

> User is giving us the size of his buffer, not the size of the tfm
> which we already know. We refuse to copy if buffer is not big enough,
> otherwise return number of bytes copied.

Well, I would usually except the user knows what he does, but okay, if
you think that's safer. It requires the user to carry the size of the
buffer around. Assuming he kmallocs the buffer in one function with the
correct size and wants to use it in another function (a mempool or
something, who knows). He doesn't know the size of the buffer there.

> This may seem a little
> redundant for the on-stack usage of the API, but may make sense in
> other cases.

It may, yes. But I don't think this kind of thing is done elsewhere in
the kernel. It's okay for things like user space libraries where the
libraries and users can be compiled separately to catch problems with
ABI changes, but in the kernel? I think it's overkill.
 
These things should be caught using BUG_ONs if you thing someone might
get them wrong somehow und in the future if something changes. But now
if they add additional parameters.

> > > +void crypto_cleanup_copy_tfm(char *user_tfm)
> > > +{
> > > +	crypto_exit_ops((struct crypto_tfm *)user_tfm);
> > 
> > This looks dangerous. The algorithm might free a buffer. This is only
> > safe if we introduce per-algorithm copy methods that also duplicate
> > external buffers.
> 
> I'm currently working under the assumption that such external buffers
> are unnecessary but I haven't done the audit. If and when such code
> exist, such code should be added, yes. Hence the comment in the copy
> function:
> 
> +       /* currently assumes shallow copy is sufficient */

Ok, I see.

We could add some functions so that everything is symmetric:
(the names with a star are already existing)

*crypto_alloc_tfm
\_ crypto_init_tfm

*crypto_free_tfm
\_ crypto_release_tfm

crypto_clone_tfm
\_ crypto_copy_tfm

crypto_get_alg_size
\_crypto_get_tfm_size

crypto_init_tfm does everything but the kmalloc
crypto_release_tfm everything but the kfree

So crypto_alloc_tfm and crypto_release_tfm can be changed to call
crypto_init_fdm and crypto_release_tfm plus crypto_get_alg_size/kmalloc
and kfree.

crypto_clone_tfm calls crypto_get_tfm_size, kmalloc and crypto_copy_tfm.
crypto_copy_tfm copies the tfm structure, cares about algorithm
reference counting and calls a (new) copy method. This copy method
should copy things in its context like kmalloc'ed structures (or
increment a reference count if it's a static memory structure or
something). (however kmalloc's should be avoided if possible, the
variable sized context provides some flexibility)

crypto_get_alg_size returns the size of the tfm structure when algorithm
name and flags are given, crypto_get_tfm size returns the size of an
existing tfm structure.

So we can also directly initialize a tfm structure on a stack, not only
copy it to a stack. Very flexible.

What do you think?

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Christophe Saout]

Hi,

I just hacked together what I wrote. Compiles but untested.

diff -Nur linux.orig/crypto/api.c linux/crypto/api.c
--- linux.orig/crypto/api.c	2004-02-27 19:25:58.445326056 +0100
+++ linux/crypto/api.c	2004-02-27 19:26:44.451332080 +0100
@@ -96,6 +96,26 @@
 	return -EINVAL;
 }
 
+static int crypto_copy_ops(struct crypto_tfm *tfm)
+{
+	switch (crypto_tfm_alg_type(tfm)) {
+	case CRYPTO_ALG_TYPE_CIPHER:
+		return crypto_copy_cipher_ops(tfm);
+		
+	case CRYPTO_ALG_TYPE_DIGEST:
+		return crypto_copy_digest_ops(tfm);
+		
+	case CRYPTO_ALG_TYPE_COMPRESS:
+		return crypto_copy_compress_ops(tfm);
+
+	default:
+		break;
+	}
+
+	BUG();
+	return -EINVAL;
+}
+
 static void crypto_exit_ops(struct crypto_tfm *tfm)
 {
 	switch (crypto_tfm_alg_type(tfm)) {
@@ -117,6 +137,62 @@
 	}
 }
 
+static int __crypto_init_tfm(struct crypto_tfm *tfm, struct crypto_alg *alg,
+			     u32 flags)
+{
+	memset(tfm, 0, crypto_alg_tfm_size(alg));
+
+	tfm->__crt_alg = alg;
+
+	if (crypto_init_flags(tfm, flags))
+		return -EINVAL;
+
+	if (crypto_init_ops(tfm)) {
+		crypto_exit_ops(tfm);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+unsigned int crypto_lookup_alg_tfm_size(const char *name, u32 flags)
+{
+	struct crypto_alg *alg;
+	unsigned int size;
+
+	alg = crypto_alg_mod_lookup(name);
+	if (alg == NULL)
+		return 0;
+
+	size = crypto_alg_tfm_size(alg);
+
+	crypto_alg_put(alg);
+	/* hmm, there's a window to race between here and crypto_init_tfm */
+
+	return size;
+}
+
+int crypto_init_tfm(struct crypto_tfm *tfm, const char *name, u32 flags)
+{
+	struct crypto_alg *alg;
+	int ret = -EINVAL;
+
+	alg = crypto_alg_mod_lookup(name);
+	if (alg == NULL)
+		goto out;
+
+	ret = __crypto_init_tfm(tfm, alg, flags);
+	if (ret < 0)
+		goto out_put;
+
+	goto out;
+
+out_put:
+	crypto_alg_put(alg);
+out:
+	return ret;
+}
+
 struct crypto_tfm *crypto_alloc_tfm(const char *name, u32 flags)
 {
 	struct crypto_tfm *tfm = NULL;
@@ -126,21 +202,12 @@
 	if (alg == NULL)
 		goto out;
 	
-	tfm = kmalloc(sizeof(*tfm) + alg->cra_ctxsize, GFP_KERNEL);
+	tfm = kmalloc(crypto_alg_tfm_size(alg), GFP_KERNEL);
 	if (tfm == NULL)
 		goto out_put;
 
-	memset(tfm, 0, sizeof(*tfm) + alg->cra_ctxsize);
-	
-	tfm->__crt_alg = alg;
-	
-	if (crypto_init_flags(tfm, flags))
+	if (__crypto_init_tfm(tfm, alg, flags) < 0)
 		goto out_free_tfm;
-		
-	if (crypto_init_ops(tfm)) {
-		crypto_exit_ops(tfm);
-		goto out_free_tfm;
-	}
 
 	goto out;
 
@@ -153,13 +220,52 @@
 	return tfm;
 }
 
-void crypto_free_tfm(struct crypto_tfm *tfm)
+void crypto_exit_tfm(struct crypto_tfm *tfm)
 {
 	crypto_exit_ops(tfm);
 	crypto_alg_put(tfm->__crt_alg);
+}
+
+void crypto_free_tfm(struct crypto_tfm *tfm)
+{
+	crypto_exit_tfm(tfm);
 	kfree(tfm);
 }
 
+int crypto_copy_tfm(struct crypto_tfm *dst, struct crypto_tfm *src)
+{
+	int ret;
+
+	memcpy(dst, src, crypto_tfm_size(src));
+	crypto_alg_get(dst->__crt_alg);
+
+	ret = crypto_copy_ops(dst);
+	if (ret < 0)
+		crypto_alg_put(dst->__crt_alg);
+
+	return ret;
+}
+
+struct crypto_tfm *crypto_clone_tfm(struct crypto_tfm *orig)
+{
+	struct crypto_tfm *clone = NULL;
+
+	clone = kmalloc(crypto_tfm_size(orig), GFP_KERNEL);
+	if (clone == NULL)
+		goto out;
+
+	if (crypto_copy_tfm(clone, orig) < 0)
+		goto out_free_tfm;
+
+	goto out;
+
+out_free_tfm:
+	kfree(clone);
+	clone = NULL;
+out:
+	return clone;
+}
+
 int crypto_register_alg(struct crypto_alg *alg)
 {
 	int ret = 0;
diff -Nur linux.orig/crypto/cipher.c linux/crypto/cipher.c
--- linux.orig/crypto/cipher.c	2004-02-27 19:25:58.513315720 +0100
+++ linux/crypto/cipher.c	2004-02-27 19:26:44.457331168 +0100
@@ -281,7 +281,7 @@
 	    	}
 	    	
 		ops->cit_ivsize = crypto_tfm_alg_blocksize(tfm);
-	    	ops->cit_iv = kmalloc(ops->cit_ivsize, GFP_KERNEL);
+		ops->cit_iv = kmalloc(ops->cit_ivsize, GFP_KERNEL);
 		if (ops->cit_iv == NULL)
 			ret = -ENOMEM;
 	}
@@ -290,6 +290,24 @@
 	return ret;
 }
 
+int crypto_copy_cipher_ops(struct crypto_tfm *tfm)
+{
+	struct cipher_tfm *ops = &tfm->crt_cipher;
+
+	if (tfm->crt_cipher.cit_iv) {
+		void *new_cit_iv;
+
+		new_cit_iv = kmalloc(ops->cit_ivsize, GFP_KERNEL);
+		if (new_cit_iv == NULL)
+			return -ENOMEM;
+
+		memcpy(new_cit_iv, ops->cit_iv, ops->cit_ivsize);
+		ops->cit_iv = new_cit_iv;
+	}
+
+	return 0;
+}
+
 void crypto_exit_cipher_ops(struct crypto_tfm *tfm)
 {
 	if (tfm->crt_cipher.cit_iv)
diff -Nur linux.orig/crypto/compress.c linux/crypto/compress.c
--- linux.orig/crypto/compress.c	2004-02-27 19:25:58.514315568 +0100
+++ linux/crypto/compress.c	2004-02-27 19:26:44.457331168 +0100
@@ -57,6 +57,11 @@
 	return ret;
 }
 
+int crypto_copy_compress_ops(struct crypto_tfm *tfm)
+{
+	return tfm->__crt_alg->cra_compress.coa_copy(crypto_tfm_ctx(tfm));
+}
+
 void crypto_exit_compress_ops(struct crypto_tfm *tfm)
 {
 	tfm->__crt_alg->cra_compress.coa_exit(crypto_tfm_ctx(tfm));
diff -Nur linux.orig/crypto/deflate.c linux/crypto/deflate.c
--- linux.orig/crypto/deflate.c	2004-02-27 19:25:58.515315416 +0100
+++ linux/crypto/deflate.c	2004-02-27 19:26:44.458331016 +0100
@@ -55,6 +55,40 @@
 	return 0;
 }
 
+static int deflate_copy(void *ctx)
+{
+	struct deflate_ctx *dctx = ctx;
+	struct z_stream_s *stream = &dctx->comp_stream;
+	unsigned int size;
+	void *new_workspace;
+
+	if (dctx->comp_initialized) {
+		new_workspace = __vmalloc(size = zlib_deflate_workspacesize(),
+		                          deflate_gfp()|__GFP_HIGHMEM,
+		                          PAGE_KERNEL);
+		if (!new_workspace)
+			return -ENOMEM;
+
+		memcpy(new_workspace, stream->workspace, size);
+		stream->workspace = new_workspace;
+	}
+	if (dctx->decomp_initialized) {
+		new_workspace = kmalloc(size = zlib_inflate_workspacesize(),
+		                        deflate_gfp());
+		if (!new_workspace) {
+			if (dctx->comp_initialized)
+				vfree(stream->workspace);
+
+			return -ENOMEM;
+		}
+
+		memcpy(new_workspace, stream->workspace, size);
+		stream->workspace = new_workspace;
+	}
+
+	return 0;
+}
+
 static void deflate_exit(void *ctx)
 {
 	struct deflate_ctx *dctx = ctx;
@@ -77,7 +111,7 @@
 	stream->workspace = __vmalloc(zlib_deflate_workspacesize(),
 	                              deflate_gfp()|__GFP_HIGHMEM,
 	                              PAGE_KERNEL);
-	if (!stream->workspace ) {
+	if (!stream->workspace) {
 		ret = -ENOMEM;
 		goto out;
 	}
@@ -212,6 +246,7 @@
 	.cra_list		= LIST_HEAD_INIT(alg.cra_list),
 	.cra_u			= { .compress = {
 	.coa_init		= deflate_init,
+	.coa_copy		= deflate_copy,
 	.coa_exit		= deflate_exit,
 	.coa_compress 		= deflate_compress,
 	.coa_decompress  	= deflate_decompress } }
diff -Nur linux.orig/crypto/digest.c linux/crypto/digest.c
--- linux.orig/crypto/digest.c	2004-02-27 19:25:58.517315112 +0100
+++ linux/crypto/digest.c	2004-02-27 19:26:44.459330864 +0100
@@ -76,6 +76,11 @@
 	return crypto_alloc_hmac_block(tfm);
 }
 
+int crypto_copy_digest_ops(struct crypto_tfm *tfm)
+{
+	return crypto_clone_hmac_block(tfm);
+}
+
 void crypto_exit_digest_ops(struct crypto_tfm *tfm)
 {
 	crypto_free_hmac_block(tfm);
diff -Nur linux.orig/crypto/hmac.c linux/crypto/hmac.c
--- linux.orig/crypto/hmac.c	2004-02-27 19:25:58.517315112 +0100
+++ linux/crypto/hmac.c	2004-02-27 19:26:44.459330864 +0100
@@ -44,7 +44,24 @@
 		ret = -ENOMEM;
 
 	return ret;
-		
+}
+
+int crypto_clone_hmac_block(struct crypto_tfm *tfm)
+{
+	void *new_hmac_block;
+	int ret = 0;
+
+	BUG_ON(!crypto_tfm_alg_blocksize(tfm));
+
+	new_hmac_block = kmalloc(crypto_tfm_alg_blocksize(tfm), GFP_KERNEL);
+	if (new_hmac_block != NULL) {
+		memcpy(new_hmac_block, tfm->crt_digest.dit_hmac_block,
+		       crypto_tfm_alg_blocksize(tfm));
+		tfm->crt_digest.dit_hmac_block = new_hmac_block;
+	} else
+		ret = -ENOMEM;
+
+	return ret;
 }
 
 void crypto_free_hmac_block(struct crypto_tfm *tfm)
diff -Nur linux.orig/crypto/internal.h linux/crypto/internal.h
--- linux.orig/crypto/internal.h	2004-02-27 19:25:58.518314960 +0100
+++ linux/crypto/internal.h	2004-02-27 19:26:44.459330864 +0100
@@ -59,6 +59,7 @@
 
 #ifdef CONFIG_CRYPTO_HMAC
 int crypto_alloc_hmac_block(struct crypto_tfm *tfm);
+int crypto_copy_hmac_block(struct crypto_tfm *tfm);
 void crypto_free_hmac_block(struct crypto_tfm *tfm);
 #else
 static inline int crypto_alloc_hmac_block(struct crypto_tfm *tfm)
@@ -85,6 +86,10 @@
 int crypto_init_cipher_ops(struct crypto_tfm *tfm);
 int crypto_init_compress_ops(struct crypto_tfm *tfm);
 
+int crypto_copy_digest_ops(struct crypto_tfm *tfm);
+int crypto_copy_cipher_ops(struct crypto_tfm *tfm);
+int crypto_copy_compress_ops(struct crypto_tfm *tfm);
+
 void crypto_exit_digest_ops(struct crypto_tfm *tfm);
 void crypto_exit_cipher_ops(struct crypto_tfm *tfm);
 void crypto_exit_compress_ops(struct crypto_tfm *tfm);
diff -Nur linux.orig/include/linux/crypto.h linux/include/linux/crypto.h
--- linux.orig/include/linux/crypto.h	2004-02-27 19:25:51.934315880 +0100
+++ linux/include/linux/crypto.h	2004-02-27 19:26:32.639127808 +0100
@@ -80,6 +80,7 @@
 
 struct compress_alg {
 	int (*coa_init)(void *ctx);
+	int (*coa_copy)(void *ctx);
 	void (*coa_exit)(void *ctx);
 	int (*coa_compress)(void *ctx, const u8 *src, unsigned int slen,
 	                    u8 *dst, unsigned int *dlen);
@@ -191,7 +192,13 @@
 /* 
  * Transform user interface.
  */
- 
+
+#define crypto_alg_tfm_size(alg)	(sizeof(struct crypto_tfm) + \
+					(alg)->cra_ctxsize)
+#define crypto_tfm_size(tfm)		crypto_alg_tfm_size((tfm)->__crt_alg)
+
+unsigned int crypto_lookup_alg_tfm_size(const char *alg_name, u32 tfm_flags);
+
 /*
  * crypto_alloc_tfm() will first attempt to locate an already loaded algorithm.
  * If that fails and the kernel supports dynamically loadable modules, it
@@ -200,10 +207,20 @@
  *
  * crypto_free_tfm() frees up the transform and any associated resources,
  * then drops the refcount on the associated algorithm.
+ *
+ * crypto_init_tfm() and crypto_exit_tfm() do the same except that the user
+ * has to do the memory allocation.
  */
+int crypto_init_tfm(struct crypto_tfm *tfm, const char *alg_name,
+                    u32 tfm_flags);
 struct crypto_tfm *crypto_alloc_tfm(const char *alg_name, u32 tfm_flags);
+
+void crypto_exit_tfm(struct crypto_tfm *tfm);
 void crypto_free_tfm(struct crypto_tfm *tfm);
 
+int crypto_copy_tfm(struct crypto_tfm *dst_tfm, struct crypto_tfm *src_tfm);
+struct crypto_tfm *crypto_clone_tfm(struct crypto_tfm *orig_tfm);
+
 /*
  * Transform helpers which query the underlying algorithm.
  */

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Matt Mackall]

On Fri, Feb 27, 2004 at 05:05:01PM +0100, Christophe Saout wrote:
> Am Do, den 26.02.2004 schrieb Matt Mackall um 21:02:
> 
> > User is giving us the size of his buffer, not the size of the tfm
> > which we already know. We refuse to copy if buffer is not big enough,
> > otherwise return number of bytes copied.
> 
> Well, I would usually except the user knows what he does, but okay, if
> you think that's safer. It requires the user to carry the size of the
> buffer around. Assuming he kmallocs the buffer in one function with the
> correct size and wants to use it in another function (a mempool or
> something, who knows). He doesn't know the size of the buffer there.
> 
> > This may seem a little
> > redundant for the on-stack usage of the API, but may make sense in
> > other cases.
> 
> It may, yes. But I don't think this kind of thing is done elsewhere in
> the kernel. It's okay for things like user space libraries where the
> libraries and users can be compiled separately to catch problems with
> ABI changes, but in the kernel? I think it's overkill.
>  
> These things should be caught using BUG_ONs if you thing someone might
> get them wrong somehow und in the future if something changes. But now
> if they add additional parameters.

I don't think this is a big deal either way, though my variant does
make it harder to do the wrong thing.
 
> > > > +void crypto_cleanup_copy_tfm(char *user_tfm)
> > > > +{
> > > > +	crypto_exit_ops((struct crypto_tfm *)user_tfm);
> > > 
> > > This looks dangerous. The algorithm might free a buffer. This is only
> > > safe if we introduce per-algorithm copy methods that also duplicate
> > > external buffers.
> > 
> > I'm currently working under the assumption that such external buffers
> > are unnecessary but I haven't done the audit. If and when such code
> > exist, such code should be added, yes. Hence the comment in the copy
> > function:
> > 
> > +       /* currently assumes shallow copy is sufficient */
> 
> Ok, I see.
> 
> We could add some functions so that everything is symmetric:
> (the names with a star are already existing)
> 
> *crypto_alloc_tfm
> \_ crypto_init_tfm
> 
> *crypto_free_tfm
> \_ crypto_release_tfm
> 
> crypto_clone_tfm
> \_ crypto_copy_tfm
> 
> crypto_get_alg_size
> \_crypto_get_tfm_size
> 
> crypto_init_tfm does everything but the kmalloc
> crypto_release_tfm everything but the kfree
> 
> So crypto_alloc_tfm and crypto_release_tfm can be changed to call
> crypto_init_fdm and crypto_release_tfm plus crypto_get_alg_size/kmalloc
> and kfree.
> 
> crypto_clone_tfm calls crypto_get_tfm_size, kmalloc and crypto_copy_tfm.
> crypto_copy_tfm copies the tfm structure, cares about algorithm
> reference counting and calls a (new) copy method. This copy method
> should copy things in its context like kmalloc'ed structures (or
> increment a reference count if it's a static memory structure or
> something). (however kmalloc's should be avoided if possible, the
> variable sized context provides some flexibility)
> 
> crypto_get_alg_size returns the size of the tfm structure when algorithm
> name and flags are given, crypto_get_tfm size returns the size of an
> existing tfm structure.
> 
> So we can also directly initialize a tfm structure on a stack, not only
> copy it to a stack. Very flexible.

My original proposal was something like this. Downside with
_initializing_ on stack is that it's much more heavy-weight. We can
end having to sleep on pulling in an algorithm. The copy stuff,
hopefully, can be done in any context.
 
> What do you think?

I'd like to keep it to the minimal three new external functions until
we have a case that really demonstrates a need for the other stuff.
Let's keep it simple, get it merged, and go from there. The API I
posted will work for the three other users I'm aware of, if it works
for dm-crypt let's go with it.

I also want to hold off on adding ->copy until we find an algorithm
that can't be cleanly fixed not to need it.

-- 
Matt Mackall : http://www.selenic.com : Linux development and consulting

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Christophe Saout]

Am Fr, den 27.02.2004 schrieb Matt Mackall um 21:02:

> > We could add some functions so that everything is symmetric:
> > (the names with a star are already existing)
> > 
> > *crypto_alloc_tfm
> > \_ crypto_init_tfm
> > 
> > *crypto_free_tfm
> > \_ crypto_release_tfm
> > 
> > crypto_clone_tfm
> > \_ crypto_copy_tfm
> > 
> > crypto_get_alg_size
> > \_crypto_get_tfm_size
> > 
> > crypto_init_tfm does everything but the kmalloc
> > crypto_release_tfm everything but the kfree
> > 
> > So crypto_alloc_tfm and crypto_release_tfm can be changed to call
> > crypto_init_fdm and crypto_release_tfm plus crypto_get_alg_size/kmalloc
> > and kfree.
> > 
> > crypto_clone_tfm calls crypto_get_tfm_size, kmalloc and crypto_copy_tfm.
> > crypto_copy_tfm copies the tfm structure, cares about algorithm
> > reference counting and calls a (new) copy method. This copy method
> > should copy things in its context like kmalloc'ed structures (or
> > increment a reference count if it's a static memory structure or
> > something). (however kmalloc's should be avoided if possible, the
> > variable sized context provides some flexibility)
> > 
> > crypto_get_alg_size returns the size of the tfm structure when algorithm
> > name and flags are given, crypto_get_tfm size returns the size of an
> > existing tfm structure.
> > 
> > So we can also directly initialize a tfm structure on a stack, not only
> > copy it to a stack. Very flexible.
> 
> My original proposal was something like this. Downside with
> _initializing_ on stack is that it's much more heavy-weight. We can
> end having to sleep on pulling in an algorithm. The copy stuff,
> hopefully, can be done in any context.

This was just for completeness reasons. Why being able to copy onto the
stack but not to create on the stack. It doesn't have to be on the
stack. Assuming you want to handle a lot of context and use a mempool.
You might want to have your structure and a tfm to the end of that
structure so they are allocated together. Yes, you could also create a
kmalloc'ed tfm once and copy it to every other structure. Perhaps that's
not even so bad, because in my implementation there is one problem:

You have to know the size of the structure before initializing it. So
the user might change the underlying module why querying the size and
initializing the structure. Hmm. That can be dropped if it doesn't make
sense.

> I'd like to keep it to the minimal three new external functions until
> we have a case that really demonstrates a need for the other stuff.
> Let's keep it simple, get it merged, and go from there. The API I
> posted will work for the three other users I'm aware of, if it works
> for dm-crypt let's go with it.

I've added methods for copying the tfm context because it will fail very
badly for everything that has a pointer in the private context.
Use-after-free and these things. Ugly.

> I also want to hold off on adding ->copy until we find an algorithm
> that can't be cleanly fixed not to need it.

Hmm. It should be there, but could return -EOPNOTSUPP. Copying a
compress tfm doesn't make much sense. We need a way to detect things
that are bad in a generic way, everything else is hacky.

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Matt Mackall]

On Fri, Feb 27, 2004 at 09:13:33PM +0100, Christophe Saout wrote:
> > I'd like to keep it to the minimal three new external functions until
> > we have a case that really demonstrates a need for the other stuff.
> > Let's keep it simple, get it merged, and go from there. The API I
> > posted will work for the three other users I'm aware of, if it works
> > for dm-crypt let's go with it.
> 
> I've added methods for copying the tfm context because it will fail very
> badly for everything that has a pointer in the private context.
> Use-after-free and these things. Ugly.

I certainly understand the issues of deep vs shallow copy. What I'm
saying is we should try to avoid needing deep copies in the first
place. They invite lots of complexity and for something as
straightforward as a cipher or digest should not be necessary.
 
> > I also want to hold off on adding ->copy until we find an algorithm
> > that can't be cleanly fixed not to need it.
> 
> Hmm. It should be there, but could return -EOPNOTSUPP. Copying a
> compress tfm doesn't make much sense. We need a way to detect things
> that are bad in a generic way, everything else is hacky.

Some way of preventing copies of some TFMs is called for, agreed.

-- 
Matt Mackall : http://www.selenic.com : Linux development and consulting

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Christophe Saout]

Am Fr, den 27.02.2004 schrieb Matt Mackall um 21:55:

> I certainly understand the issues of deep vs shallow copy. What I'm
> saying is we should try to avoid needing deep copies in the first
> place. They invite lots of complexity and for something as
> straightforward as a cipher or digest should not be necessary.

Right. But we should still keep the ->init, ->copy, ->exit mechanism
complete then, just because it's there and not having them fore some
cases makes things just incomplete. Or we set the methods to NULL and
the copy function only works if both init and exit are null, so we don't
need the copy method because it also would be NULL and we know the
algorithm doesn't use external structures. This would work for "fixed
up" cipher and digest algorithms.

There's just one small difficulty with having some of the structures in
the context: Their size is variable. But known after the init function.
So the cra_ctxsize isn't sufficient to describe the length of a tfm
strucure. So we need another per-algorithm-category method that returns
the additional size required. They might just return iv_size or iv_size
+ omac_pad_size for ciphers and hmac_pad_size for digests or something
like this.

> > Hmm. It should be there, but could return -EOPNOTSUPP. Copying a
> > compress tfm doesn't make much sense. We need a way to detect things
> > that are bad in a generic way, everything else is hacky.
> 
> Some way of preventing copies of some TFMs is called for, agreed.

What about the idea above?

I could think we could start from my patch and simple throw 70% away. ;)
(or yours, it isn't too far either)

I'd like too keep my names: (init), copy, exit vs. alloc, clone and
free.

Since the crypto_lookup_alg_tfm_size -> crypto_init_tfm thing is racy we
shouldn't implement it (at least for now). A solution would be to keep
an algorithm reference between both functions but that's ugly and
unintuitive. And if you want to do something like I suggested before you
can also use the copy mechanism.

So, we have an agreement then? :)

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Matt Mackall]

On Fri, Feb 27, 2004 at 10:16:36PM +0100, Christophe Saout wrote:
> Am Fr, den 27.02.2004 schrieb Matt Mackall um 21:55:
> 
> > I certainly understand the issues of deep vs shallow copy. What I'm
> > saying is we should try to avoid needing deep copies in the first
> > place. They invite lots of complexity and for something as
> > straightforward as a cipher or digest should not be necessary.
> 
> Right. But we should still keep the ->init, ->copy, ->exit mechanism
> complete then, just because it's there and not having them fore some
> cases makes things just incomplete. Or we set the methods to NULL and
> the copy function only works if both init and exit are null, so we don't
> need the copy method because it also would be NULL and we know the
> algorithm doesn't use external structures. This would work for "fixed
> up" cipher and digest algorithms.
> 
> There's just one small difficulty with having some of the structures in
> the context: Their size is variable. But known after the init function.
> So the cra_ctxsize isn't sufficient to describe the length of a tfm
> strucure. So we need another per-algorithm-category method that returns
> the additional size required. They might just return iv_size or iv_size
> + omac_pad_size for ciphers and hmac_pad_size for digests or something
> like this.

Hmmm, crypto_alloc_tfm does:

        tfm = kmalloc(sizeof(*tfm) + alg->cra_ctxsize, GFP_KERNEL);

So I'm not clear on how the necessary size can be anything else at
copy time?
 
> > > Hmm. It should be there, but could return -EOPNOTSUPP. Copying a
> > > compress tfm doesn't make much sense. We need a way to detect things
> > > that are bad in a generic way, everything else is hacky.
> > 
> > Some way of preventing copies of some TFMs is called for, agreed.
> 
> What about the idea above?
> 
> I could think we could start from my patch and simple throw 70% away. ;)
> (or yours, it isn't too far either)

Either way.

> I'd like too keep my names: (init), copy, exit vs. alloc, clone and
> free.

Not terribly concerned about the naming, so long as the header file
makes it clear that every copy needs a matching cleanup call and not a
free.

-- 
Matt Mackall : http://www.selenic.com : Linux development and consulting

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Christophe Saout]

Am Sa, den 28.02.2004 schrieb Matt Mackall um 01:39:

> > There's just one small difficulty with having some of the structures in
> > the context: Their size is variable. But known after the init function.
> > So the cra_ctxsize isn't sufficient to describe the length of a tfm
> > strucure. So we need another per-algorithm-category method that returns
> > the additional size required. They might just return iv_size or iv_size
> > + omac_pad_size for ciphers and hmac_pad_size for digests or something
> > like this.
> 
> Hmmm, crypto_alloc_tfm does:
> 
>         tfm = kmalloc(sizeof(*tfm) + alg->cra_ctxsize, GFP_KERNEL);
> 
> So I'm not clear on how the necessary size can be anything else at
> copy time?

Forget it, my mistake. cra_ctxsize is set by the algorithms themselves.

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[James Morris]

On Tue, 24 Feb 2004, Christophe Saout wrote:

> BTW: I think there's a bug in the ipv6 code, it uses spin_lock to
> protect itself, this will cause a sleep-inside-spinlock warning. (found
> while grepping through the source for other cryptoapi users)

Where is the bug?


- James
-- 
James Morris
<jmorris@redhat.com>

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Christophe Saout]

Am Di, den 24.02.2004 schrieb James Morris um 23:26:

> > BTW: I think there's a bug in the ipv6 code, it uses spin_lock to
> > protect itself, this will cause a sleep-inside-spinlock warning. (found
> > while grepping through the source for other cryptoapi users)
> 
> Where is the bug?

net/ipv6/addrconf.c: __ipv6_regen_rndid

>        spin_lock(&md5_tfm_lock);
>        if (unlikely(md5_tfm == NULL)) {
>                spin_unlock(&md5_tfm_lock);
>                return -1;
>        }
>        crypto_digest_init(md5_tfm);
>        crypto_digest_update(md5_tfm, sg, 2);
>        crypto_digest_final(md5_tfm, idev->work_digest);
>        spin_unlock(&md5_tfm_lock);

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[James Morris]

On Tue, 24 Feb 2004, Christophe Saout wrote:

> Am Di, den 24.02.2004 schrieb James Morris um 23:26:
> 
> > > BTW: I think there's a bug in the ipv6 code, it uses spin_lock to
> > > protect itself, this will cause a sleep-inside-spinlock warning. (found
> > > while grepping through the source for other cryptoapi users)
> > 
> > Where is the bug?
> 
> net/ipv6/addrconf.c: __ipv6_regen_rndid
> 
> >        spin_lock(&md5_tfm_lock);
> >        if (unlikely(md5_tfm == NULL)) {
> >                spin_unlock(&md5_tfm_lock);
> >                return -1;
> >        }
> >        crypto_digest_init(md5_tfm);
> >        crypto_digest_update(md5_tfm, sg, 2);
> >        crypto_digest_final(md5_tfm, idev->work_digest);
> >        spin_unlock(&md5_tfm_lock);

This looks like too much work to be done under a spinlock, and generally,
that's why the API does not support these kinds of functions from being
called under one.


- James
-- 
James Morris
<jmorris@redhat.com>

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[James Morris]

On Tue, 24 Feb 2004, Matt Mackall wrote:

> Something like:
> 
>  /* calculate the size of a tfm so that users can manage their own
>  copies */
> 
>  int crypto_alg_size(const char *name);
> 
>  /* copy a TFM to a user-managed buffer, possibly on stack, with proper
>  internal reference counting and any other necessary magic, size checks
>  against boneheaded buffer sizing */
> 
>  crypto_copy_tfm(char *dst, const struct crypto_tfm *src, int size);

Does it need to be copied from an existing tfm?  I think it would be 
cleaner to provide just a way to initialize a tfm.

>  /* do all the necessary bookkeeping to release a user-managed TFM, use
>  char pointer to avoid alloc/free mismatch */
> 
>  crypto_copy_cleanup_tfm(char *usertfm);
> 

This is doable.


- James
-- 
James Morris
<jmorris@redhat.com>

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Matt Mackall]

On Tue, Feb 24, 2004 at 03:01:03PM -0500, James Morris wrote:
> On Tue, 24 Feb 2004, Matt Mackall wrote:
> 
> > Something like:
> > 
> >  /* calculate the size of a tfm so that users can manage their own
> >  copies */
> > 
> >  int crypto_alg_size(const char *name);
> > 
> >  /* copy a TFM to a user-managed buffer, possibly on stack, with proper
> >  internal reference counting and any other necessary magic, size checks
> >  against boneheaded buffer sizing */
> > 
> >  crypto_copy_tfm(char *dst, const struct crypto_tfm *src, int size);
> 
> Does it need to be copied from an existing tfm?  I think it would be 
> cleaner to provide just a way to initialize a tfm.

Not sure about Christopher's case, but I think it actually might be
easier generally. First, copying rather than initializing ensures
we've already got the algorithm loaded and locked and don't have to
worry particularly whether we're in a difficult context. 

Second, if there are cases (and this may be one, I forget the details
of HMAC) where there's significant per-use setup costs, having a
copying interface saves work. If you've got a copy interface, you
don't really need the second kind of user-managed initialize interface
I proposed earlier.

> >  /* do all the necessary bookkeeping to release a user-managed TFM, use
> >  char pointer to avoid alloc/free mismatch */
> > 
> >  crypto_copy_cleanup_tfm(char *usertfm);
> > 
> 
> This is doable.

Ok, I probably spin something like this in the next couple days.

-- 
Matt Mackall : http://www.selenic.com : Linux development and consulting

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Christophe Saout]

Am Di, den 24.02.2004 schrieb Matt Mackall um 20:11:

> > +	int tfm_size = sizeof(*cc->digest) + cc->digest->__crt_alg->cra_ctxsize;
> > +	char tfm[tfm_size];
> > [...]
> > +	memcpy(tfm, cc->digest, tfm_size);
> 
> As this stands, it's rather scary.
> 
> - it will quietly break when cryptoapi gets fiddled with

Yes, and it's already broken. When putting a lot of stress to the
filesystem data corruption pops up.

It turned out the hmac code uses an additional scratch pad which is used
in crypto_hmac_final (the "opad") which was kmalloc'ed. So it isn't even
inside the context (the one after struct tfm with length cra_ctxsize).

Why that? That kmalloc could have been avoided and the opad could store
after the tfm struct too (or on the stack of the crypto_hmac_final or is
it too large?). Yes, I know, ... but it would really be nice not to put
locks around the calls.

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Jean-Luc Cooke]

On Wed, Feb 25, 2004 at 03:25:24AM +0100, Christophe Saout wrote:
> Am Di, den 24.02.2004 schrieb Matt Mackall um 20:11:
> 
> > > +	int tfm_size = sizeof(*cc->digest) + cc->digest->__crt_alg->cra_ctxsize;
> > > +	char tfm[tfm_size];
> > > [...]
> > > +	memcpy(tfm, cc->digest, tfm_size);
> > 
> > As this stands, it's rather scary.
> > 
> > - it will quietly break when cryptoapi gets fiddled with
> 
> Yes, and it's already broken. When putting a lot of stress to the
> filesystem data corruption pops up.
> 
> It turned out the hmac code uses an additional scratch pad which is used
> in crypto_hmac_final (the "opad") which was kmalloc'ed. So it isn't even
> inside the context (the one after struct tfm with length cra_ctxsize).
> 
> Why that? That kmalloc could have been avoided and the opad could store
> after the tfm struct too (or on the stack of the crypto_hmac_final or is
> it too large?). Yes, I know, ... but it would really be nice not to put
> locks around the calls.


This is insine, there is no reason to have that outside of function scope at
all.

Here's the fix.
  http://jlcooke.ca/lkml/hmac_reent.patch

Uses the stack now (peak stack usage will not go up)

James - I'll wrap this one up with my other in one patch.  This is a "look
see, say 'OK'" patch.

JLC

-- 
http://www.certainkey.com
Suite 4560 CTTC
1125 Colonel By Dr.
Ottawa ON, K1S 5B6

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Fruhwirth Clemens]

[-- Attachment #1: Type: text/plain, Size: 1776 bytes --]

On Thu, Feb 19, 2004 at 06:02:32PM +0100, Christophe Saout wrote:
> since some people keep complaining that the IV generation mechanisms
> supplied in cryptoloop (and now dm-crypt) are insecure, which they
> somewhat are, I just hacked a small digest based IV generation mechanism.
> 
> It simply hashes the sector number and the key and uses it as IV.
> 
> You can specify the encryption mode as "cipher-digest" like aes-md5 or
> serpent-sha1 or some other combination.

I actually would prefer that cipher algorithm and cipher mode are seperated.
It's just a matter of personal taste that I'd like to stick things where I
think they belong. So imho it would be nicer to split this string into two
seperate: one for the cipher algorithm, the other for the mode of operation,
namely, "ecb" or "cbc-<ivmode>". 

> Consider this as a proposal, I'm not a crypto expert. Tell me if it
> contains other flaws that should be fixed.

No obvious flaws for me. I've already argued in private different IV mode,
but one more time for the public ;) : I embraced the principal of reusing
components in security systems instead of depending on a large number on
different subsystems. The only IV mode which can finally make sence for me
is the use of cipher algorithm as hash algorithm. This will keep the risk of
breakage of the system by the insecurity of one component to a minimum. A
pratical reason for using one algorithm is that just one algorithm has to be
optimized (f.e. assembler optimized or hardware offloading). I'd like to
submit a patch for this as soon as dm-crypt is merged. Andrew: any ideas if
this will happen soon?

> At least the "cryptoloop-exploit" Jari Ruusu posted doesn't work anymore.

"""exploit""".

Best Regards,
Clemens

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Jean-Luc Cooke]

On Mon, Feb 23, 2004 at 01:35:04AM +0100, Fruhwirth Clemens wrote:
> No obvious flaws for me. I've already argued in private different IV mode,
> but one more time for the public ;) : I embraced the principal of reusing
> components in security systems instead of depending on a large number on
> different subsystems. The only IV mode which can finally make sence for me
> is the use of cipher algorithm as hash algorithm. This will keep the risk of
> breakage of the system by the insecurity of one component to a minimum. A
> pratical reason for using one algorithm is that just one algorithm has to be
> optimized (f.e. assembler optimized or hardware offloading). I'd like to
> submit a patch for this as soon as dm-crypt is merged. Andrew: any ideas if
> this will happen soon?

Using a block cipher as a hash algorithm isn't nessesarily more secure.  The
"less moving parts" argument is quickly pushed aside by the "round peg in a
round hole" argument.

SHA-1/SHA-256/384/512 were *designed* to be message digests.  AES was not (as
a requirement anyways).

I have a CTR mode patch ready for crypto/cipher.c.  I would like to implement
OMAC (the 6th AES approved mode of opereation) before giving the patch, but
you can't use OMAC the way you use ECB/CBC/CTR mode.

The analogy of:

for (i=0; i<len; i++)
  omac_encrypt(tfm, dst[i], src[i], nbytes);

Will not work with OMAC since it creates a MAC and not a ciphertext stream
like the other modes.

for (i=0; i<len; i++)
  omac_encrypt(tfm, dst[0], src[i], nbytes);
/*                      ^ see here!           */
memcpy(mac, dest, ...); /* store the mac */

Is more appropriate.  James - is this possible?

> > At least the "cryptoloop-exploit" Jari Ruusu posted doesn't work anymore.

Oooh, Jari.

JLC

-- 
http://www.certainkey.com
Suite 4560 CTTC
1125 Colonel By Dr.
Ottawa ON, K1S 5B6

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[James Morris]

On Mon, 23 Feb 2004, Jean-Luc Cooke wrote:

> The analogy of:
> 
> for (i=0; i<len; i++)
>   omac_encrypt(tfm, dst[i], src[i], nbytes);
> 
> Will not work with OMAC since it creates a MAC and not a ciphertext stream
> like the other modes.
> 
> for (i=0; i<len; i++)
>   omac_encrypt(tfm, dst[0], src[i], nbytes);
> /*                      ^ see here!           */
> memcpy(mac, dest, ...); /* store the mac */
> 
> Is more appropriate.  James - is this possible?

What exactly are you trying to do?


- James
-- 
James Morris
<jmorris@redhat.com>

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Jean-Luc Cooke]

On Mon, Feb 23, 2004 at 05:11:45PM -0500, James Morris wrote:
> As far as I can tell, you should just be adding a new wrapper interface 
> like HMAC.  Have a look at that code.

OK got the hint. ;)

I'll repeat my recommendation that OMACs be computed at the same time as
encryption/decryption and not in a "two pass" approach like this.  Ideally by
passing two cipher contexts (one for encryption other for MACing) into
functions like cbc_encrypt() - alas I'm too nervous with this API to
implement that.

A Not-So-Ideal compromise would be using the same cipher context and
storing the MAC in a new member of the cipher_tfm struct.  This would require
minimal amount of new code to test and will probably go faster as well.

The two patches are:
 - http://jlcooke.ca/lkml/ctr_and_omac.patch
   (added ctr to cipher.c and omac.c)
   Using the init/update/final interface.
 - http://jlcooke.ca/lkml/ctr_and_omac2.patch
   (added ctr to cipher.c and integrated OMAC into all
   existing modes of operation. If cipher_tfm.cit_omac!=NULL, OMAC is stored
   into cipher_tfm.cit_omac)
   So in other words, current implementations using the CryptoAPI will not see
   performance change and no new .c files.
    No OMAC:
      tfm = crypto_alloc_tfm (algo, CRYPTO_TFM_MODE_CBC);
    With OMAC:
      tfm = crypto_alloc_tfm (algo, CRYPTO_TFM_MODE_CBC | CRYPTO_TFM_MODE_OMAC);
      ...
      crypto_cipher_get_omac(tfm, omac_val, 128/8);
    ** J-L likes this one more! **

Now implementing CCM mode (the final mode of operation specified for use with
AES by NIST) will be my next task.   CCM is an IEEE standard and is going
through the motions of becoming a FIPS standard as well.  Reference:
  http://csrc.nist.gov/publications/drafts/Draft_SP_800-38C_9-04-2003.pdf

It's effectively CTR mode with a CBC MAC at the end of it.  I'll start
implementing it, but we'll hold off until the spec is out of draft.

JLC

ps. Will crypto_cipher_encrypt/crypto_cipher_decrypt *always* be called in
onesies?  I need to perform come final() code on the OMAC before it's
ready to pass test vectors - how do I know when we're done?

-- 
http://www.certainkey.com
Suite 4560 CTTC
1125 Colonel By Dr.
Ottawa ON, K1S 5B6

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[James Morris]

On Tue, 24 Feb 2004, Jean-Luc Cooke wrote:

> The two patches are:
>  - http://jlcooke.ca/lkml/ctr_and_omac.patch
>    (added ctr to cipher.c and omac.c)
>    Using the init/update/final interface.
>  - http://jlcooke.ca/lkml/ctr_and_omac2.patch
>    (added ctr to cipher.c and integrated OMAC into all
>    existing modes of operation. If cipher_tfm.cit_omac!=NULL, OMAC is stored
>    into cipher_tfm.cit_omac)

Looks good so far, although the duplicated scatterwalk code needs to be 
put into a separate file (e.g. scatterwalk.c).


> ps. Will crypto_cipher_encrypt/crypto_cipher_decrypt *always* be called in
> onesies?  I need to perform come final() code on the OMAC before it's
> ready to pass test vectors - how do I know when we're done?

I don't understand what you mean here.


Thanks for all this work!


- James
-- 
James Morris
<jmorris@redhat.com>

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Jean-Luc Cooke]

On Tue, Feb 24, 2004 at 05:17:12PM -0500, James Morris wrote:
> On Tue, 24 Feb 2004, Jean-Luc Cooke wrote:
> 
> > The two patches are:
> >  - http://jlcooke.ca/lkml/ctr_and_omac.patch
> >    (added ctr to cipher.c and omac.c)
> >    Using the init/update/final interface.
> >  - http://jlcooke.ca/lkml/ctr_and_omac2.patch
> >    (added ctr to cipher.c and integrated OMAC into all
> >    existing modes of operation. If cipher_tfm.cit_omac!=NULL, OMAC is stored
> >    into cipher_tfm.cit_omac)
> 
> Looks good so far, although the duplicated scatterwalk code needs to be 
> put into a separate file (e.g. scatterwalk.c).

OK.  So which patch do you want?  :)  The omac.c with scatterwalk, or the
cipher.c with omac performed in-place when needed?

> > ps. Will crypto_cipher_encrypt/crypto_cipher_decrypt *always* be called in
> > onesies?  I need to perform come final() code on the OMAC before it's
> > ready to pass test vectors - how do I know when we're done?
> 
> I don't understand what you mean here.

finish_omac() needs to be called once all data is processed.  How do I know
for sure the caller is done with this cipher context instance?

For example:
  loop {
    /* get more data into sgin */
    crypto_cipher_encrypt(tfm, sgout, sgin, len);
    /* send our data out of sgout */
  }
  /* get the 128bit OMAC and send it since we're done with all our encryption */

And decryption:
  loop {
    /* get more data into sgin */
    crypto_cipher_decrypt(tfm, sgout, sgin, len);
    /* send our data out of sgout */
  }
  /* get the 128bit OMAC and compare it with transmitted OMAC since we're done
     with all our decryption */

There is no explicit/implicit "crypto_cipher_encrypt_final()" to inform the
API to finalise the OMAC.

> Thanks for all this work!

Gladly.  Makes the beer at the pub go down with less guilt when I can say my
name has appear in one more kernel source file.  :)

JLC

-- 
http://www.certainkey.com
Suite 4560 CTTC
1125 Colonel By Dr.
Ottawa ON, K1S 5B6

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[James Morris]

On Tue, 24 Feb 2004, Jean-Luc Cooke wrote:

> On Tue, Feb 24, 2004 at 05:17:12PM -0500, James Morris wrote:
> > On Tue, 24 Feb 2004, Jean-Luc Cooke wrote:
> > 
> > > The two patches are:
> > >  - http://jlcooke.ca/lkml/ctr_and_omac.patch
> > >    (added ctr to cipher.c and omac.c)
> > >    Using the init/update/final interface.
> > >  - http://jlcooke.ca/lkml/ctr_and_omac2.patch
> > >    (added ctr to cipher.c and integrated OMAC into all
> > >    existing modes of operation. If cipher_tfm.cit_omac!=NULL, OMAC is stored
> > >    into cipher_tfm.cit_omac)
> > 
> > Looks good so far, although the duplicated scatterwalk code needs to be 
> > put into a separate file (e.g. scatterwalk.c).
> 
> OK.  So which patch do you want?  :)  The omac.c with scatterwalk, or the
> cipher.c with omac performed in-place when needed?

I think the latter is preferrable.  OMAC should probably be a config 
option as well.

> 
> > > ps. Will crypto_cipher_encrypt/crypto_cipher_decrypt *always* be called in
> > > onesies?  I need to perform come final() code on the OMAC before it's
> > > ready to pass test vectors - how do I know when we're done?
> > 
> > I don't understand what you mean here.
> 
> finish_omac() needs to be called once all data is processed.  How do I know
> for sure the caller is done with this cipher context instance?
> 
> For example:
>   loop {
>     /* get more data into sgin */
>     crypto_cipher_encrypt(tfm, sgout, sgin, len);
>     /* send our data out of sgout */
>   }
>   /* get the 128bit OMAC and send it since we're done with all our encryption */
> 
> And decryption:
>   loop {
>     /* get more data into sgin */
>     crypto_cipher_decrypt(tfm, sgout, sgin, len);
>     /* send our data out of sgout */
>   }
>   /* get the 128bit OMAC and compare it with transmitted OMAC since we're done
>      with all our decryption */
> 
> There is no explicit/implicit "crypto_cipher_encrypt_final()" to inform the
> API to finalise the OMAC.

You could add something like crypto_cipher_omac_final(), to be used by the 
calling code once it needs the OMAC value, which then calls 
crypto_cipher_get_omac().


- James
-- 
James Morris
<jmorris@redhat.com>

Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Jean-Luc Cooke]

Alright then.

http://jlcooke.ca/lkml/crypto_24feb2004.patch

Changes:
 - Added CTR mode to cipher.c
 - Fixed Bug where CTR wasn't getting allocated an IV
 - Added config for OMAC option on encryption
 - Moved scatterwalk stuff to scatterwalk.c added to makefile
 - Removed kmalloc'd scratch space in hmac.c which caused a reported bug.
 - kernel config defaults to build all the IPSec required algorithms as
   well as OMAC (OMAC is only computed if specified by caller)

JLC

On Wed, Feb 25, 2004 at 08:52:36AM -0500, James Morris wrote:
> I think the latter is preferrable.  OMAC should probably be a config 
> option as well.
...
> You could add something like crypto_cipher_omac_final(), to be used by the 
> calling code once it needs the OMAC value, which then calls 
> crypto_cipher_get_omac().

-- 
http://www.certainkey.com
Suite 4560 CTTC
1125 Colonel By Dr.
Ottawa ON, K1S 5B6