lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Mon, 27 Apr 2020 19:14:15 -0400
From:   Scott Dial <scott@...ttdial.com>
To:     David Miller <davem@...emloft.net>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH net] net: macsec: preserve ingress frame ordering

On 4/27/2020 2:12 PM, David Miller wrote:
> It's a real shame that instead of somehow fixing the most performant
> setup to be actually usable, we are just throwing our hands up in
> the air and simply avoiding to use it.
> 
> I feel _really_ bad for the person trying to figure out why they
> aren't getting the macsec performance they expect, scratching their
> heads for hours trying to figure out why the AES-NI x86 code isn't
> being used, and after days finding a commit like this.

Like most things, there are competing interests. I was the person
scratching my head for hours trying to figure out why my packets were
arriving out of of order causing packet drops and breaking UDP streams.
In the end, the only solution that I could ship without modifying the
kernel was blacklisting aesni_intel and ghash_clmulni_intel.

To be clear, the sync version of gcm(aes) on an AES-NI will still use
AES-NI for the block cipher if the FPU is available (and otherwise falls
back to non-FPU code). Unfortunately, there is not a synchronous version
of gcm(aes) implemented by AES-NI, but that would be a logical extension
of the pattern to provide maximum performance and correctness. With
regards to correctness, you can see that same decision being made in the
mac80211 code for handling the AES-GCMP and BIP-GMAC encryption modes. I
don't know if the crypto maintainers would entertain adding a sync
aes(gcm) implementation to aesni_intel, but it seems like it would be
straightforward to implement.

Otherwise, I entertained a module option (simple and covers my use case)
or even an attribute on the RXSA (a considerably more invasive change).
However, I didn't think this would be that controversial since there are
many places in the kernel that use AEAD algorithms in synchronous mode
for the same reason, and therefore do not get the complete benefits of
AES-NI acceleration.

>> -	tfm = crypto_alloc_aead("gcm(aes)", 0, 0);
>> +	/* Pick a sync gcm(aes) cipher to ensure order is preserved. */
>> +	tfm = crypto_alloc_aead("gcm(aes)", 0, CRYPTO_ALG_ASYNC);
> 
> How does this mask argument passed to crypto_alloc_aead() work?  You
> are specifying async, does this mean you're asking for async or
> non-async?

See crypto_requires_sync(type, mask) in include/crypto/algapi.h for the
logic of evaluating the mask. In short, the mask is what bits are not
allowed to be set, so this masks out any async algorithms.

Thanks for taking the time to evaluate my change!

-- 
Scott Dial
scott@...ttdial.com

Powered by blists - more mailing lists