| Message ID | 54895B58.8030200@openvpn.net (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;