Re: [PATCH] crypto: aesni - disable "by8" AVX CTR optimization

[James Yonan <james@openvpn.net>]

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

I'm seeing some anomalous results with the "by8" AVX CTR optimization in 
3.18.

In particular, crypto_aead_encrypt appears to produce different 
ciphertext from the same plaintext depending on whether or not the 
optimization is enabled.

See the attached patch to tcrypt that demonstrates the discrepancy.

James

On 23/09/2014 14:31, Mathias Krause wrote:
> The "by8" implementation introduced in commit 22cddcc7df8f ("crypto: aes
> - AES CTR x86_64 "by8" AVX optimization") is failing crypto tests as it
> handles counter block overflows differently. It only accounts the right
> most 32 bit as a counter -- not the whole block as all other
> implementations do. This makes it fail the cryptomgr test #4 that
> specifically tests this corner case.
>
> As we're quite late in the release cycle, just disable the "by8" variant
> for now.
>
> Reported-by: Romain Francoise <romain@orebokech.com>
> Signed-off-by: Mathias Krause <minipli@googlemail.com>
> Cc: Chandramouli Narayanan <mouli@linux.intel.com>
>
> ---
> I'll try to create a real fix next week but I can't promise much. If
> Linus releases v3.17 early, as he has mentioned in his -rc6
> announcement, we should hot fix this by just disabling the "by8"
> variant. The real fix would add the necessary counter block handling to
> the asm code and revert this commit afterwards. Reverting the whole code
> is not necessary, imho.
>
> Would that be okay for you, Herbert?
> ---
>   arch/x86/crypto/aesni-intel_glue.c |    4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
> index 888950f29fd9..a7ccd57f19e4 100644
> --- a/arch/x86/crypto/aesni-intel_glue.c
> +++ b/arch/x86/crypto/aesni-intel_glue.c
> @@ -481,7 +481,7 @@ static void ctr_crypt_final(struct crypto_aes_ctx *ctx,
>   	crypto_inc(ctrblk, AES_BLOCK_SIZE);
>   }
>
> -#ifdef CONFIG_AS_AVX
> +#if 0	/* temporary disabled due to failing crypto tests */
>   static void aesni_ctr_enc_avx_tfm(struct crypto_aes_ctx *ctx, u8 *out,
>   			      const u8 *in, unsigned int len, u8 *iv)
>   {
> @@ -1522,7 +1522,7 @@ static int __init aesni_init(void)
>   		aesni_gcm_dec_tfm = aesni_gcm_dec;
>   	}
>   	aesni_ctr_enc_tfm = aesni_ctr_enc;
> -#ifdef CONFIG_AS_AVX
> +#if 0	/* temporary disabled due to failing crypto tests */
>   	if (cpu_has_avx) {
>   		/* optimize performance of ctr mode encryption transform */
>   		aesni_ctr_enc_tfm = aesni_ctr_enc_avx_tfm;
>

[-- Attachment #2: tcrypt.diff --]
[-- Type: text/plain, Size: 6644 bytes --]

This is a standard 3.18 kernel with "by8" AVX CTR optimization in place.
Processor is Intel(R) Xeon(R) CPU E3-1220 V2 @ 3.10GHz.

Run tcrypt mode=600 using attached patch to tcrypt.  The input plaintext
is 128 bytes of 0xFF.

[    6.859579] test_aead_encrypt_consistency alg=gcm(aes) succeeded, hash=0x52fc2dd3
[    6.860682] Key:
[    6.860914]   00000000: 55 55 55 55 55 55 55 55 55 55 55 55 55 55 55 55
[    6.861671] Initial IV:
[    6.861961]   00000000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[    6.862725] Final IV:
[    6.863000]   00000000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0a
[    6.863827] AD:
[    6.864077]   00000000: ad ad ad ad ad ad ad ad ad ad ad ad ad ad ad ad
[    6.864831] Ciphertext:
[    6.865116]   00000000: 9d 49 0e af 65 17 a3 2a 1f ef 05 27 d9 af 5e 6e
[    6.865871]   00000010: 9d 2b fc fa be 66 14 35 f4 b5 82 9d ee c2 be a8
[    6.866695]   00000020: 6e 8f af e0 f5 26 79 f9 6f ed 91 15 c3 26 30 06
[    6.867463]   00000030: b3 b1 cc 70 0a b7 73 6e f3 8c 96 f0 26 ab 13 ca
[    6.868268]   00000040: a9 4a 5f e6 1f a8 fa e5 71 f7 a6 5b 73 93 40 94
[    6.869040]   00000050: f1 82 5e 08 5c 85 02 02 8c 6f 4b 93 f8 10 1a f1
[    6.869810]   00000060: c9 5e 23 0c bc ad 0f 33 6a e7 da f3 71 b7 be 12
[    6.870575]   00000070: b1 a0 83 94 60 8d 70 ca 43 ff d0 e9 61 17 56 6e
[    6.871386] Auth Tag:
[    6.871659]   00000000: aa fe 4e ce 3b 12 59 1d 06 93 fb 37 26 1a bb bd

Next, remove the optimization code: "rmmod aesni_intel".

Run tcrypt again and the results are different:

[    7.173145] test_aead_encrypt_consistency alg=gcm(aes) succeeded, hash=0xad4487f8
[    7.174026] Key:
[    7.174252]   00000000: 55 55 55 55 55 55 55 55 55 55 55 55 55 55 55 55
[    7.175068] Initial IV:
[    7.175380]   00000000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[    7.176237] Final IV:
[    7.176520]   00000000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0a
[    7.177360] AD:
[    7.177586]   00000000: ad ad ad ad ad ad ad ad ad ad ad ad ad ad ad ad
[    7.178405] Ciphertext:
[    7.178721]   00000000: 16 3d ce aa 94 c5 41 ef 4f f8 38 98 b3 ec 0b 68
[    7.179526]   00000010: 29 c1 b6 12 10 46 3a f8 77 22 d4 df da fd 95 fc
[    7.180396]   00000020: 3a 15 b5 e3 01 e6 d9 9f ea 26 ae ed 98 63 6e 62
[    7.181189]   00000030: 0c ca dc 5b 65 98 f5 29 f5 e4 d8 3a 2e ea 6c 39
[    7.181984]   00000040: 7d df 67 66 ce 69 6d 74 f4 e0 e3 df ff 93 1a 9a
[    7.182768]   00000050: 5a a0 cb af 7b dd e9 bb dd 6a df a5 57 b9 1d 56
[    7.183604]   00000060: f6 21 cf 45 7d 82 bb ec a4 59 42 4b 8a 46 34 1d
[    7.184613]   00000070: 18 85 77 09 b7 81 4f e2 92 fc 84 95 6e 3f 94 75
[    7.185407] Auth Tag:
[    7.185695]   00000000: de 7a 34 56 34 5a 02 19 2a 97 e7 bb 70 d0 20 89

Which version is correct?  The latter version
(without the optimization) matches the results
produced by previous kernel versions and matches
up with PolarSSL as well.


diff --git a/tcrypt.c b/tcrypt.c
index 890449e..9dd7bc9 100644
--- a/tcrypt.c
+++ b/tcrypt.c
@@ -33,6 +33,7 @@
 #include <linux/jiffies.h>
 #include <linux/timex.h>
 #include <linux/interrupt.h>
+#include <linux/jhash.h>
 #include "tcrypt.h"
 #include "internal.h"
 
@@ -265,6 +266,109 @@ static void sg_init_aead(struct scatterlist *sg, char *xbuf[XBUFSIZE],
 	}
 }
 
+static void test_aead_encrypt_consistency(const char *algo, unsigned int secs, unsigned int keylen)
+{
+	const unsigned int data_size = 128;
+	const unsigned int ad_size = 16;
+	const unsigned int auth_tag_size = 16;
+	const unsigned int data_frags = 1;
+
+	const unsigned int data_sg_base = 1;
+	const unsigned int nfrags = data_frags + 2;
+
+	unsigned char *data = NULL;
+	struct scatterlist *sg = NULL;
+	struct crypto_aead *tfm = NULL;
+	struct aead_request *req = NULL;
+	unsigned char key[keylen];
+	unsigned char auth_tag[auth_tag_size];
+	unsigned char ad[ad_size];
+	unsigned char orig_iv[16];
+	unsigned char iv[16];
+	unsigned int hash;
+	int err;
+
+	data = kmalloc(data_size, GFP_KERNEL);
+	sg = kzalloc(sizeof(struct scatterlist) * nfrags, GFP_KERNEL);
+	if (!data || !sg) {
+		printk("kmalloc failed\n");
+		goto done;
+	}
+	sg_init_table(sg, nfrags);
+	sg_set_buf(sg, ad, ad_size);
+	sg_set_buf(&sg[data_frags + 1], auth_tag, auth_tag_size);
+
+	sg_set_buf(&sg[data_sg_base+0], data, data_size);
+
+	tfm = crypto_alloc_aead(algo, 0, 0);
+	if (IS_ERR(tfm)) {
+		printk("crypto_alloc_aead failed, err=%ld\n", PTR_ERR(tfm));
+		tfm = NULL;
+		goto done;
+	}
+
+	memset(key, 0x55, keylen);
+	err = crypto_aead_setkey(tfm, key, keylen);
+	if (err < 0) {
+		printk("crypto_aead_setkey failed, err=%d\n", err);
+		goto done;
+	}
+
+	err = crypto_aead_setauthsize(tfm, auth_tag_size);
+	if (err < 0) {
+		printk("crypto_aead_setauthsize failed, err=%d\n", err);
+		goto done;
+	}
+
+	req = aead_request_alloc(tfm, GFP_KERNEL);
+	if (!req) {
+		printk("aead_request_alloc failed\n");
+		goto done;
+	}
+
+	memset(ad, 0xAD, ad_size);
+	memset(data, 0xFF, data_size);
+	memset(iv, 0, sizeof(iv));
+	memcpy(orig_iv, iv, sizeof(orig_iv));
+	memset(auth_tag, 0, auth_tag_size);
+
+	/* encrypt */
+	aead_request_set_crypt(req, &sg[data_sg_base], &sg[data_sg_base],
+			       data_size,
+			       iv);
+	aead_request_set_assoc(req, sg, ad_size);
+	err = crypto_aead_encrypt(req);
+	if (err < 0) {
+		printk("crypto_aead_encrypt failed err=%d\n", err);
+		goto done;
+	}
+
+	/* hash the ciphertext */
+	hash = jhash(data, data_size, 0);
+
+	printk("test_aead_encrypt_consistency alg=%s succeeded, hash=0x%x\n", algo, hash);	
+	printk("Key:\n");
+	print_hex_dump(KERN_INFO, "  ", DUMP_PREFIX_OFFSET, 16, 1, key, keylen, 0);
+	printk("Initial IV:\n");
+	print_hex_dump(KERN_INFO, "  ", DUMP_PREFIX_OFFSET, 16, 1, orig_iv, sizeof(orig_iv), 0);
+	printk("Final IV:\n");
+	print_hex_dump(KERN_INFO, "  ", DUMP_PREFIX_OFFSET, 16, 1, iv, sizeof(iv), 0);
+	printk("AD:\n");
+	print_hex_dump(KERN_INFO, "  ", DUMP_PREFIX_OFFSET, 16, 1, ad, ad_size, 0);
+	printk("Ciphertext:\n");
+	print_hex_dump(KERN_INFO, "  ", DUMP_PREFIX_OFFSET, 16, 1, data, data_size, 0);
+	printk("Auth Tag:\n");
+	print_hex_dump(KERN_INFO, "  ", DUMP_PREFIX_OFFSET, 16, 1, auth_tag, auth_tag_size, 0);
+
+done:
+	if (req)
+		aead_request_free(req);
+	if (tfm)
+		crypto_free_aead(tfm);
+	kfree(data);
+	kfree(sg);
+}
+
 static void test_aead_speed(const char *algo, int enc, unsigned int secs,
 			    struct aead_speed_template *template,
 			    unsigned int tcount, u8 authsize,
@@ -2119,6 +2223,10 @@ static int do_test(int m)
 				   speed_template_8_32);
 		break;
 
+	case 600:
+		test_aead_encrypt_consistency("gcm(aes)", sec, 16);
+		break;
+
 	case 1000:
 		test_available();
 		break;