Re: [PATCH] crypto: aesni - disable "by8" AVX CTR optimization
On 16 December 2014 at 22:49, James Yonan 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
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] crypto: aesni - disable "by8" AVX CTR optimization
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
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] crypto: aesni - disable "by8" AVX CTR optimization
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
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] crypto: aesni - disable "by8" AVX CTR optimization
Hi James,
On 11 December 2014 at 09:52, James Yonan 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
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] crypto: aesni - disable "by8" AVX CTR optimization
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
Signed-off-by: Mathias Krause
Cc: Chandramouli Narayanan
---
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;
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] : 55 55 55 55 55 55 55 55 55 55 55 55 55 55 55 55
[6.861671] Initial IV:
[6.861961] : 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[6.862725] Final IV:
[6.863000] : 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0a
[6.863827] AD:
[6.864077] : ad ad ad ad ad ad ad ad ad ad ad ad ad ad ad ad
[6.864831] Ciphertext:
[6.865116] : 9d 49 0e af 65 17 a3 2a 1f ef 05 27 d9 af 5e 6e
[6.865871] 0010: 9d 2b fc fa be 66 14 35 f4 b5 82 9d ee c2 be a8
[6.866695] 0020: 6e 8f af e0 f5 26 79 f9 6f ed 91 15 c3 26 30 06
[6.867463] 0030: b3 b1 cc 70 0a b7 73 6e f3 8c 96 f0 26 ab 13 ca
[6.868268] 0040: a9 4a 5f e6 1f a8 fa e5 71 f7 a6 5b 73 93 40 94
[6.869040] 0050: f1 82 5e 08 5c 85 02 02 8c 6f 4b 93 f8 10 1a f1
[6.869810] 0060: c9 5e 23 0c bc ad 0f 33 6a e7 da f3 71 b7 be 12
[6.870575] 0070: b1 a0 83 94 60 8d 70 ca 43 ff d0 e9 61 17 56 6e
[6.871386] Auth Tag:
[6.871659] : 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] : 55 55 55 55 55 55 55 55 55 55 55 55 55 55 55 55
[7.175068] Initial IV:
[7.175380] : 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[7.176237] Final IV:
[7.176520] : 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0a
[7.177360] AD:
[7.177586] : ad ad ad ad ad ad ad ad ad ad ad ad ad ad ad ad
[7.178405] Ciphertext:
[7.178721] : 16 3d ce aa 94 c5 41 ef 4f f8 38 98 b3 ec 0b 68
[7.179526] 0010: 29 c1 b6 12 10 46 3a f8 77 22 d4 df da fd 95 fc
[7.180396] 0020: 3a 15 b5 e3 01 e6 d9 9f ea 26 ae ed 98 63 6e 62
[7.181189] 0030: 0c ca dc 5b 65 98 f5 29 f5 e4 d8 3a 2e ea 6c 39
[7.181984] 0040: 7d df 67 66 ce 69 6d 74 f4 e0 e3 df ff 93 1a 9a
[7.182768] 0050: 5a a0 cb af 7b dd e9 bb dd 6a df a5 57 b9 1d 56
[7.183604] 0060: f6 21 cf 45 7d 82 bb ec a4 59 42 4b 8a 46 34 1d
[7.184613] 0070: 18 85 77
[PATCH] crypto: aesni - disable "by8" AVX CTR optimization
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
Signed-off-by: Mathias Krause
Cc: Chandramouli Narayanan
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
