v3.17-rc5: alg: skcipher: Test 4 failed on encryption for ctr-aes-aesni

v3.17-rc5: alg: skcipher: Test 4 failed on encryption for ctr-aes-aesni

[Romain Francoise]

Hi,

I upgraded from v3.16 to v3.17-rc5 and the ctr-aes-aesni encryption test
fails, which makes my IPsec tunnels unhappy (see trace below). Before I
start bisecting (2cddcc7df8fd3 is probably my first guess), is this
already known?

Sep 15 08:07:56 silenus kernel: [   35.137145] alg: skcipher: Test 4 failed on encryption for ctr-aes-aesni
Sep 15 08:07:56 silenus kernel: [   35.137149] 00000000: 04 f3 d3 88 17 ef dc ef 8b 04 f8 3a 66 8d 1a 53
Sep 15 08:07:56 silenus kernel: [   35.137150] 00000010: 57 1f 4b 23 e4 a0 af f9 69 95 35 98 8d 4d 8c c1
Sep 15 08:07:56 silenus kernel: [   35.137151] 00000020: f0 b2 7f 80 bb 54 28 a2 7a 1b 9f 77 ec 0e 6e de
Sep 15 08:07:56 silenus kernel: [   35.137152] 00000030: 57 1d d4 66 07 60 e1 80 08 24 3f 93 15 54 bb 2a
Sep 15 08:07:56 silenus kernel: [   35.137153] 00000040: 9f 24 2b 17 92 60 05 68 21 74 a4 0a 28 eb 27 48
Sep 15 08:07:56 silenus kernel: [   35.137153] 00000050: 90 50 37 ca 5c 0b 67 52 27 d2 7c 39 4b 85 35 0a
Sep 15 08:07:56 silenus kernel: [   35.137154] 00000060: 23 90 a1 a0 79 8b 33 c0 73 d6 a0 9b fc 83 c9 f0
Sep 15 08:07:56 silenus kernel: [   35.137155] 00000070: ef 23 22 19 16 6d e8 f4 b1 17 16 30 31 e8 a5 53
Sep 15 08:07:56 silenus kernel: [   35.137155] 00000080: db 04 d8 bf 2e 75 9e 06 68 39 96 ec 38 1c 66 74
Sep 15 08:07:56 silenus kernel: [   35.137156] 00000090: 7f e3 85 62 d5 1c da 83 86 63 07 41 f3 ce 2e c9
Sep 15 08:07:56 silenus kernel: [   35.137157] 000000a0: 3a 6e d8 be bd f3 d7 26 a1 a3 c6 ad 6d 65 32 7b
Sep 15 08:07:56 silenus kernel: [   35.137158] 000000b0: 6a 84 9c 11 1a b2 bc 0f a9 88 1e 4c 6b 36 52 ee
Sep 15 08:07:56 silenus kernel: [   35.137158] 000000c0: eb 4d 79 9d d2 f6 af a9 8c 79 09 16 80 a4 25 9d
Sep 15 08:07:56 silenus kernel: [   35.137159] 000000d0: e1 c5 e5 8e bf 4e cd 3f dd 2d f5 33 b8 ad 3d 2c
Sep 15 08:07:56 silenus kernel: [   35.137160] 000000e0: a1 ac 58 7c 45 3f f7 18 4d 02 93 a1 53 f4 07 f4
Sep 15 08:07:56 silenus kernel: [   35.137161] 000000f0: 4c 31 1e 3a 5b 7f 2d 0a d5 e1 6a eb 1d 55 47 29
Sep 15 08:07:56 silenus kernel: [   35.137161] 00000100: ce 7b 1a 08 c6 62 1a a3 f1 bd 8e 05 7a 86 75 cd
Sep 15 08:07:56 silenus kernel: [   35.137162] 00000110: a7 8e ba 3e 1b 9a ce 2e 10 4b 06 ce ed 5e 6f 77
Sep 15 08:07:56 silenus kernel: [   35.137163] 00000120: 8e bc d0 08 40 2c 86 f2 6b 35 17 4d d7 b8 63 08
Sep 15 08:07:56 silenus kernel: [   35.137163] 00000130: af d9 ed ca ad 5e 0b a4 d9 8e ff 8a d7 9f ae 1b
Sep 15 08:07:56 silenus kernel: [   35.137164] 00000140: 11 1e 51 8e 98 22 09 99 2d ff a3 df 8a 38 76 5c
Sep 15 08:07:56 silenus kernel: [   35.137165] 00000150: df 1a b1 79 2f 00 dc 39 42 d2 fe 0f 66 2b 75 72
Sep 15 08:07:56 silenus kernel: [   35.137166] 00000160: 31 e0 59 34 2e 5a c6 51 3e 39 10 11 a6 42 48 34
Sep 15 08:07:56 silenus kernel: [   35.137166] 00000170: 72 5b 16 8d b4 f8 92 e1 9c 84 34 48 2c db 20 38
Sep 15 08:07:56 silenus kernel: [   35.137167] 00000180: ef 74 1b d1 71 f9 84 f7 17 0e df cc ec 13 80 a3
Sep 15 08:07:56 silenus kernel: [   35.137168] 00000190: 7c 66 7c 2c 1e a4 09 8e ff 4a 19 b6 5f 6d fb 84
Sep 15 08:07:56 silenus kernel: [   35.137169] 000001a0: 13 99 37 d1 b7 e6 36 06 a9 b8 40 39 46 25 56 eb
Sep 15 08:07:56 silenus kernel: [   35.137169] 000001b0: 98 59 07 b2 80 95 fb 98 47 30 e1 8f be 7f c4 7e
Sep 15 08:07:56 silenus kernel: [   35.137170] 000001c0: 77 8f 11 c9 b2 08 15 58 6c 57 20 c0 39 f8 5e f4
Sep 15 08:07:56 silenus kernel: [   35.137171] 000001d0: 0d 91 dc 86 0f b5 99 09 d4 e2 8f a0 bf 83 99 b3
Sep 15 08:07:56 silenus kernel: [   35.137171] 000001e0: c3 98 13 9c dc f7 ad 6a 1c 02 8e 45 43 da 3e c6
Sep 15 08:07:56 silenus kernel: [   35.137195] alg: aead: setkey failed on test 1 for rfc4106-gcm-aesni: flags=0

Re: v3.17-rc5: alg: skcipher: Test 4 failed on encryption for ctr-aes-aesni

[Romain Francoise]

On Mon, Sep 15, 2014 at 10:36:46AM +0200, Romain Francoise wrote:
> I upgraded from v3.16 to v3.17-rc5 and the ctr-aes-aesni encryption test
> fails, which makes my IPsec tunnels unhappy (see trace below). Before I
> start bisecting (2cddcc7df8fd3 is probably my first guess), is this
> already known?

> Sep 15 08:07:56 silenus kernel: [   35.137145] alg: skcipher: Test 4 failed on encryption for ctr-aes-aesni [...]

Update: reverting 2cddcc7df8 ("crypto: aes - AES CTR x86_64 "by8" AVX
optimization") fixes this. This machine is Sandy Bridge (Core i7-2600),
and the problem doesn't seem to occur with the exact same kernel image
on Ivy Bridge (Xeon E3-1240v2).

Re: v3.17-rc5: alg: skcipher: Test 4 failed on encryption for ctr-aes-aesni

[Herbert Xu]

On Tue, Sep 16, 2014 at 10:01:07PM +0200, Romain Francoise wrote:
> On Mon, Sep 15, 2014 at 10:36:46AM +0200, Romain Francoise wrote:
> > I upgraded from v3.16 to v3.17-rc5 and the ctr-aes-aesni encryption test
> > fails, which makes my IPsec tunnels unhappy (see trace below). Before I
> > start bisecting (2cddcc7df8fd3 is probably my first guess), is this
> > already known?
> 
> > Sep 15 08:07:56 silenus kernel: [   35.137145] alg: skcipher: Test 4 failed on encryption for ctr-aes-aesni [...]
> 
> Update: reverting 2cddcc7df8 ("crypto: aes - AES CTR x86_64 "by8" AVX
> optimization") fixes this. This machine is Sandy Bridge (Core i7-2600),
> and the problem doesn't seem to occur with the exact same kernel image
> on Ivy Bridge (Xeon E3-1240v2).

Thanks for bisecting.  If we can't fix this quickly then we should
just revert it for now.

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

Re: v3.17-rc5: alg: skcipher: Test 4 failed on encryption for ctr-aes-aesni

[Mathias Krause]

On 16 September 2014 22:01, Romain Francoise <romain@orebokech.com> wrote:
> On Mon, Sep 15, 2014 at 10:36:46AM +0200, Romain Francoise wrote:
>> I upgraded from v3.16 to v3.17-rc5 and the ctr-aes-aesni encryption test
>> fails, which makes my IPsec tunnels unhappy (see trace below). Before I
>> start bisecting (2cddcc7df8fd3 is probably my first guess), is this
>> already known?
>
>> Sep 15 08:07:56 silenus kernel: [   35.137145] alg: skcipher: Test 4 failed on encryption for ctr-aes-aesni [...]

I do not get the above message on a SandyBridge i7-2620M, even though
the module makes use of the "by8" variant on my system, too:

[    0.340626] AVX version of gcm_enc/dec engaged.
[    0.340627] AES CTR mode by8 optimization enabled
[    0.341273] alg: No test for __gcm-aes-aesni (__driver-gcm-aes-aesni)

> Update: reverting 2cddcc7df8 ("crypto: aes - AES CTR x86_64 "by8" AVX
> optimization") fixes this. This machine is Sandy Bridge (Core i7-2600),
> and the problem doesn't seem to occur with the exact same kernel image
> on Ivy Bridge (Xeon E3-1240v2).

Can you please provide the full kernel log and /proc/cpuinfo of those
machines? It would be interesting to know which variant was used on
those machines -- the new "by8" or the old one.


Thanks,
Mathias

Re: v3.17-rc5: alg: skcipher: Test 4 failed on encryption for ctr-aes-aesni

[Mathias Krause]

On 17 September 2014 22:10, Mathias Krause <minipli@googlemail.com> wrote:
> On 16 September 2014 22:01, Romain Francoise <romain@orebokech.com> wrote:
>> On Mon, Sep 15, 2014 at 10:36:46AM +0200, Romain Francoise wrote:
>>> I upgraded from v3.16 to v3.17-rc5 and the ctr-aes-aesni encryption test
>>> fails, which makes my IPsec tunnels unhappy (see trace below). Before I
>>> start bisecting (2cddcc7df8fd3 is probably my first guess), is this
>>> already known?
>>
>>> Sep 15 08:07:56 silenus kernel: [   35.137145] alg: skcipher: Test 4 failed on encryption for ctr-aes-aesni [...]
>
> I do not get the above message on a SandyBridge i7-2620M, even though
> the module makes use of the "by8" variant on my system, too:
>

Never mind! Playing a little with crconf to instantiate 'ctr(aes)' and
I can see the failing test now too.

Thanks,
Mathias

Re: v3.17-rc5: alg: skcipher: Test 4 failed on encryption for ctr-aes-aesni

[Mathias Krause]

On 17 September 2014 13:29, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Tue, Sep 16, 2014 at 10:01:07PM +0200, Romain Francoise wrote:
>> On Mon, Sep 15, 2014 at 10:36:46AM +0200, Romain Francoise wrote:
>> > I upgraded from v3.16 to v3.17-rc5 and the ctr-aes-aesni encryption test
>> > fails, which makes my IPsec tunnels unhappy (see trace below). Before I
>> > start bisecting (2cddcc7df8fd3 is probably my first guess), is this
>> > already known?
>>
>> > Sep 15 08:07:56 silenus kernel: [   35.137145] alg: skcipher: Test 4 failed on encryption for ctr-aes-aesni [...]
>>
>> Update: reverting 2cddcc7df8 ("crypto: aes - AES CTR x86_64 "by8" AVX
>> optimization") fixes this. This machine is Sandy Bridge (Core i7-2600),
>> and the problem doesn't seem to occur with the exact same kernel image
>> on Ivy Bridge (Xeon E3-1240v2).
>
> Thanks for bisecting.  If we can't fix this quickly then we should
> just revert it for now.
>

[Adding James and Sean as they're stated as "contact information"]

I compared the implementation against the original code from Intel
referenced in the source file and found a few differences. But even
after removing those, the code still generates the same error. So if
Chandramouli does not come up with something, we should revert it, as
the reference implementation from Intel is a) either wrong or b) used
wrongly in the Linux kernel.

What might be worth noting, the failing test uses an IV value of
"\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFD",
or, in other words, a counter value that'll wrap after processing
three blocks. The Crypto++ implementation explicitly states, it can
handle the wrap around (see [1]). Dumping the IV before and after the
cryptomgr tests shows, the "by8" implementation only handles the lower
32 bit as a counter. Looking at RFC 3686, it defines the "counter
block" as a 128 bit combination of nonce, IV and a 32 bit counter
value. It also defines the initial value of the counter part (1) and
how it should be incremented (increment the whole counter block, i.e.
the 128 bit value). However, it also states that the maximum number
blocks per packet are (2^32)-1. So, according to the RFC, the wrap
around cannot happen -- not even for the 32 bit part of the counter
block. However the other aesni-backed implementation does handle the
wrap around just fine. It does so by doing the increment on a integer
register so it can use the carry flag to detect the wrap around.
Changing the "by8" variant would be straight forward, but I fear
performance implications... :/

Looking back at the test vector, even if it might be inappropriate for
IPsec, it is still valid for AES-CTR in the general case. So IMHO the
"by8" implementation is wrong and should be fixed -- or reverted, for
that matter.

James, Sean: Have you observed any interoperability problems with the
Intel reference implementation for the AVX by8 variant of AES-CTR?
Especially, have you tested the code for wrapping counter values?


Regards,
Mathias

[1] http://www.cryptopp.com/wiki/CTR_Mode

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

[Mathias Krause]

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;
-- 
1.7.10.4

Re: v3.17-rc5: alg: skcipher: Test 4 failed on encryption for ctr-aes-aesni

[chandramouli narayanan]

On Mon, 2014-09-22 at 00:28 +0200, Mathias Krause wrote:
> On 17 September 2014 13:29, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > On Tue, Sep 16, 2014 at 10:01:07PM +0200, Romain Francoise wrote:
> >> On Mon, Sep 15, 2014 at 10:36:46AM +0200, Romain Francoise wrote:
> >> > I upgraded from v3.16 to v3.17-rc5 and the ctr-aes-aesni encryption test
> >> > fails, which makes my IPsec tunnels unhappy (see trace below). Before I
> >> > start bisecting (2cddcc7df8fd3 is probably my first guess), is this
> >> > already known?
> >>
> >> > Sep 15 08:07:56 silenus kernel: [   35.137145] alg: skcipher: Test 4 failed on encryption for ctr-aes-aesni [...]
> >>
> >> Update: reverting 2cddcc7df8 ("crypto: aes - AES CTR x86_64 "by8" AVX
> >> optimization") fixes this. This machine is Sandy Bridge (Core i7-2600),
> >> and the problem doesn't seem to occur with the exact same kernel image
> >> on Ivy Bridge (Xeon E3-1240v2).
> >
> > Thanks for bisecting.  If we can't fix this quickly then we should
> > just revert it for now.
> >
> 
> [Adding James and Sean as they're stated as "contact information"]
> 
> I compared the implementation against the original code from Intel
> referenced in the source file and found a few differences. But even
> after removing those, the code still generates the same error. So if
> Chandramouli does not come up with something, we should revert it, as
> the reference implementation from Intel is a) either wrong or b) used
> wrongly in the Linux kernel.
> 
> What might be worth noting, the failing test uses an IV value of
> "\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFD",
> or, in other words, a counter value that'll wrap after processing
> three blocks. The Crypto++ implementation explicitly states, it can
> handle the wrap around (see [1]). Dumping the IV before and after the
> cryptomgr tests shows, the "by8" implementation only handles the lower
> 32 bit as a counter. Looking at RFC 3686, it defines the "counter
> block" as a 128 bit combination of nonce, IV and a 32 bit counter
> value. It also defines the initial value of the counter part (1) and
> how it should be incremented (increment the whole counter block, i.e.
> the 128 bit value). However, it also states that the maximum number
> blocks per packet are (2^32)-1. So, according to the RFC, the wrap
> around cannot happen -- not even for the 32 bit part of the counter
> block. However the other aesni-backed implementation does handle the
> wrap around just fine. It does so by doing the increment on a integer
> register so it can use the carry flag to detect the wrap around.
> Changing the "by8" variant would be straight forward, but I fear
> performance implications... :/
> 
> Looking back at the test vector, even if it might be inappropriate for
> IPsec, it is still valid for AES-CTR in the general case. So IMHO the
> "by8" implementation is wrong and should be fixed -- or reverted, for
> that matter.

It will take some time for me to debug this issue. Is there a list of
test vectors to debug with?

thanks
-mouli
> 
> James, Sean: Have you observed any interoperability problems with the
> Intel reference implementation for the AVX by8 variant of AES-CTR?
> Especially, have you tested the code for wrapping counter values?
> 
> 
> Regards,
> Mathias
> 
> [1] http://www.cryptopp.com/wiki/CTR_Mode

Re: v3.17-rc5: alg: skcipher: Test 4 failed on encryption for ctr-aes-aesni

[Mathias Krause]

On 25 September 2014 00:23, chandramouli narayanan
<mouli@linux.intel.com> wrote:
> On Mon, 2014-09-22 at 00:28 +0200, Mathias Krause wrote:
>> What might be worth noting, the failing test uses an IV value of
>> "\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFD",
>> or, in other words, a counter value that'll wrap after processing
>> three blocks. The Crypto++ implementation explicitly states, it can
>> handle the wrap around (see [1]). Dumping the IV before and after the
>> cryptomgr tests shows, the "by8" implementation only handles the lower
>> 32 bit as a counter. Looking at RFC 3686, it defines the "counter
>> block" as a 128 bit combination of nonce, IV and a 32 bit counter
>> value. It also defines the initial value of the counter part (1) and
>> how it should be incremented (increment the whole counter block, i.e.
>> the 128 bit value). However, it also states that the maximum number
>> blocks per packet are (2^32)-1. So, according to the RFC, the wrap
>> around cannot happen -- not even for the 32 bit part of the counter
>> block. However the other aesni-backed implementation does handle the
>> wrap around just fine. It does so by doing the increment on a integer
>> register so it can use the carry flag to detect the wrap around.
>> Changing the "by8" variant would be straight forward, but I fear
>> performance implications... :/
>>
>
> It will take some time for me to debug this issue. Is there a list of
> test vectors to debug with?

The failing test is aes_ctr_enc_tv_template[3] from crypto/testmgr.h.
It fails because of a wrong handling of counter overflows.

I'm already working on a patch that fixes the counter overflow issue.
It passes the cryptomgr test but I like to do some more thorough
tests. Especially some performance measurements as we now have
branches in the hot path.

I don't know if we should still rush fix this for v3.17 or delay this
for the next merge window. The offending code was already disabled in
Linus' tree and the fixes would be small enough to be backported if
there is interest.

Regards,
Mathias

>
> thanks
> -mouli

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

[James Yonan]

[-- 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;

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

[Mathias Krause]

Hi James,

On 11 December 2014 at 09:52, James Yonan <james@openvpn.net> wrote:
> I'm seeing some anomalous results with the "by8" AVX CTR optimization in
> 3.18.

the patch you're replying to actually *disabled* the "by8" variant for
v3.17 as it had another bug related to wrong counter handling in GCM.
The fix for that particular issue only made it to v3.18, so the code
got re-enabled only for v3.18. But it looks like that there's yet
another bug :/

> 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.

I can reproduce your observations, so I can confirm the difference,
when using the "by8" variant compared to other AES implementations.
When applying this very patch (commit 7da4b29d496b ("crypto: aesni -
disable "by8" AVX CTR optimization")) -- the patch that disables the
"by8" variant -- on top of v3.18 the discrepancies are gone. So the
behavior is bound to the "by8" optimization, only.
As it was Chandramouli, who contributed the code, maybe he has a clue
what's wrong here. Chandramouli?


Mathias

>
> James

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

[James Yonan]

Mathias,

>> I'm seeing some anomalous results with the "by8" AVX CTR optimization in
>> 3.18.
>
> the patch you're replying to actually *disabled* the "by8" variant for
> v3.17 as it had another bug related to wrong counter handling in GCM.
> The fix for that particular issue only made it to v3.18, so the code
> got re-enabled only for v3.18. But it looks like that there's yet
> another bug :/

Right, I should have clarified that I initially suspected the "by8" 
variant was to blame because your patch that disables it resolves the 
discrepancy.

>> 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.
>
> I can reproduce your observations, so I can confirm the difference,
> when using the "by8" variant compared to other AES implementations.
> When applying this very patch (commit 7da4b29d496b ("crypto: aesni -
> disable "by8" AVX CTR optimization")) -- the patch that disables the
> "by8" variant -- on top of v3.18 the discrepancies are gone. So the
> behavior is bound to the "by8" optimization, only.

Right -- this is exactly what I'm seeing as well.

> As it was Chandramouli, who contributed the code, maybe he has a clue
> what's wrong here. Chandramouli?

A few more observations:

* Encryption produces bad ciphertext only when the size of plaintext 
exceeds a certain threshold.  In test_aead_encrypt_consistency in the 
tcrypt patch, I found that data_size must be >= 128 to produce bad 
ciphertext.

* Encrypting then decrypting data always gets back to the original
plaintext, no matter what the size.

* The bad ciphertext from encryption is only evident when the same 
encrypt operation is performed on a different AES implementation and the 
ciphertexts are compared.

* When the encrypt operation produces bad ciphertext, the generated auth 
tag is actually correct, so another AES implementation that decrypts the 
ciphertext will end up with corrupted plaintext that succeeds 
authentication.

James

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

[James Yonan]

On 15/12/2014 12:26, James Yonan wrote:
> Mathias,
>
>>> I'm seeing some anomalous results with the "by8" AVX CTR optimization in
>>> 3.18.
>>
>> the patch you're replying to actually *disabled* the "by8" variant for
>> v3.17 as it had another bug related to wrong counter handling in GCM.
>> The fix for that particular issue only made it to v3.18, so the code
>> got re-enabled only for v3.18. But it looks like that there's yet
>> another bug :/
>
> Right, I should have clarified that I initially suspected the "by8"
> variant was to blame because your patch that disables it resolves the
> discrepancy.
>
>>> 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.
>>
>> I can reproduce your observations, so I can confirm the difference,
>> when using the "by8" variant compared to other AES implementations.
>> When applying this very patch (commit 7da4b29d496b ("crypto: aesni -
>> disable "by8" AVX CTR optimization")) -- the patch that disables the
>> "by8" variant -- on top of v3.18 the discrepancies are gone. So the
>> behavior is bound to the "by8" optimization, only.
>
> Right -- this is exactly what I'm seeing as well.
>
>> As it was Chandramouli, who contributed the code, maybe he has a clue
>> what's wrong here. Chandramouli?
>
> A few more observations:
>
> * Encryption produces bad ciphertext only when the size of plaintext
> exceeds a certain threshold.  In test_aead_encrypt_consistency in the
> tcrypt patch, I found that data_size must be >= 128 to produce bad
> ciphertext.
>
> * Encrypting then decrypting data always gets back to the original
> plaintext, no matter what the size.
>
> * The bad ciphertext from encryption is only evident when the same
> encrypt operation is performed on a different AES implementation and the
> ciphertexts are compared.
>
> * When the encrypt operation produces bad ciphertext, the generated auth
> tag is actually correct, so another AES implementation that decrypts the
> ciphertext will end up with corrupted plaintext that succeeds
> authentication.

Another interesting observation:

* bug only occurs when key size is 128 bits, not 192 or 256.

James

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

[Mathias Krause]

On 16 December 2014 at 22:49, James Yonan <james@openvpn.net> wrote:
> On 15/12/2014 12:26, James Yonan wrote:
>>
>> Mathias,
>>
>>>> I'm seeing some anomalous results with the "by8" AVX CTR optimization in
>>>> 3.18.
>>>
>>>
>>> the patch you're replying to actually *disabled* the "by8" variant for
>>> v3.17 as it had another bug related to wrong counter handling in GCM.
>>> The fix for that particular issue only made it to v3.18, so the code
>>> got re-enabled only for v3.18. But it looks like that there's yet
>>> another bug :/
>>
>>
>> Right, I should have clarified that I initially suspected the "by8"
>> variant was to blame because your patch that disables it resolves the
>> discrepancy.
>>
>>>> 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.
>>>
>>>
>>> I can reproduce your observations, so I can confirm the difference,
>>> when using the "by8" variant compared to other AES implementations.
>>> When applying this very patch (commit 7da4b29d496b ("crypto: aesni -
>>> disable "by8" AVX CTR optimization")) -- the patch that disables the
>>> "by8" variant -- on top of v3.18 the discrepancies are gone. So the
>>> behavior is bound to the "by8" optimization, only.
>>
>>
>> Right -- this is exactly what I'm seeing as well.
>>
>>> As it was Chandramouli, who contributed the code, maybe he has a clue
>>> what's wrong here. Chandramouli?
>>
>>
>> A few more observations:
>>
>> * Encryption produces bad ciphertext only when the size of plaintext
>> exceeds a certain threshold.  In test_aead_encrypt_consistency in the
>> tcrypt patch, I found that data_size must be >= 128 to produce bad
>> ciphertext.
>>
>> * Encrypting then decrypting data always gets back to the original
>> plaintext, no matter what the size.
>>
>> * The bad ciphertext from encryption is only evident when the same
>> encrypt operation is performed on a different AES implementation and the
>> ciphertexts are compared.
>>
>> * When the encrypt operation produces bad ciphertext, the generated auth
>> tag is actually correct, so another AES implementation that decrypts the
>> ciphertext will end up with corrupted plaintext that succeeds
>> authentication.

Hi James,

I gave it a shot since Chandramouli does not seem to respond... :(

>
> Another interesting observation:
>
> * bug only occurs when key size is 128 bits, not 192 or 256.

Thank you for your exhaustive analysis. The data size >= 128 bytes and
a key size of 128 were the key bits to this puzzle. The code is plain
wrong for 128 bit keys.
I'll send a patch soon.


Regards,
Mathias