[PATCH net] net: macsec: preserve ingress frame ordering

[PATCH net] net: macsec: preserve ingress frame ordering

[Scott Dial]

MACsec decryption always occurs in a softirq context. Since
the FPU may not be usable in the softirq context, the call to
decrypt may be scheduled on the cryptd work queue. The cryptd
work queue does not provide ordering guarantees. Therefore,
preserving order requires masking out ASYNC implementations
of gcm(aes).

For instance, an Intel CPU with AES-NI makes available the
generic-gcm-aesni driver from the aesni_intel module to
implement gcm(aes). However, this implementation requires
the FPU, so it is not always available to use from a softirq
context, and will fallback to the cryptd work queue, which
does not preserve frame ordering. With this change, such a
system would select gcm_base(ctr(aes-aesni),ghash-generic).
While the aes-aesni implementation prefers to use the FPU, it
will fallback to the aes-asm implementation if unavailable.

By using a synchronous version of gcm(aes), the decryption
will complete before returning from crypto_aead_decrypt().
Therefore, the macsec_decrypt_done() callback will be called
before returning from macsec_decrypt(). Thus, the order of
calls to macsec_post_decrypt() for the frames is preserved.

While it's presumable that the pure AES-NI version of gcm(aes)
is more performant, the hybrid solution is capable of gigabit
speeds on modest hardware. Regardless, preserving the order
of frames is paramount for many network protocols (e.g.,
triggering TCP retries). Within the MACsec driver itself, the
replay protection is tripped by the out-of-order frames, and
can cause frames to be dropped.

This bug has been present in this code since it was added in
v4.6, however it may not have been noticed since not all CPUs
have FPU offload available. Additionally, the bug manifests
as occasional out-of-order packets that are easily
misattributed to other network phenomena.

When this code was added in v4.6, the crypto/gcm.c code did
not restrict selection of the ghash function based on the
ASYNC flag. For instance, x86 CPUs with PCLMULQDQ would
select the ghash-clmulni driver instead of ghash-generic,
which submits to the cryptd work queue if the FPU is busy.
However, this bug was was corrected in v4.8 by commit
b30bdfa86431afbafe15284a3ad5ac19b49b88e3, and was backported
all the way back to the v3.14 stable branch, so this patch
should be applicable back to the v4.6 stable branch.

Signed-off-by: Scott Dial <scott@scottdial.com>
---
 drivers/net/macsec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index a183250ff66a..bce8b8fde400 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -1305,7 +1305,8 @@ static struct crypto_aead *macsec_alloc_tfm(char *key, int key_len, int icv_len)
 	struct crypto_aead *tfm;
 	int ret;
 
-	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);
 
 	if (IS_ERR(tfm))
 		return tfm;
-- 
2.26.2

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

[David Miller]

From: Scott Dial <scott@scottdial.com>
Date: Fri, 24 Apr 2020 18:51:08 -0400

> While it's presumable that the pure AES-NI version of gcm(aes)
> is more performant, the hybrid solution is capable of gigabit
> speeds on modest hardware. Regardless, preserving the order
> of frames is paramount for many network protocols (e.g.,
> triggering TCP retries). Within the MACsec driver itself, the
> replay protection is tripped by the out-of-order frames, and
> can cause frames to be dropped.

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.

> -	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?

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

[Scott Dial]

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@scottdial.com