netdev - Re: [PATCH net] net: macsec: preserve ingress frame ordering

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