From: Mathias Krause Subject: Re: [PATCH] crypto: aesni - disable "by8" AVX CTR optimization Date: Tue, 30 Dec 2014 22:29:39 +0100 Message-ID: References: <20140917112911.GA2129@gondor.apana.org.au> <1411504267-10170-1-git-send-email-minipli@googlemail.com> <54895B58.8030200@openvpn.net> <548F35D0.1020503@openvpn.net> <5490A8FF.2040906@openvpn.net> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Cc: Chandramouli Narayanan , Herbert Xu , "David S. Miller" , Romain Francoise , "linux-crypto@vger.kernel.org" To: James Yonan Return-path: Received: from mail-we0-f171.google.com ([74.125.82.171]:53820 "EHLO mail-we0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751779AbaL3Vhn (ORCPT ); Tue, 30 Dec 2014 16:37:43 -0500 Received: by mail-we0-f171.google.com with SMTP id u56so1598760wes.30 for ; Tue, 30 Dec 2014 13:37:42 -0800 (PST) In-Reply-To: <5490A8FF.2040906@openvpn.net> Sender: linux-crypto-owner@vger.kernel.org List-ID: 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