Change return types of hash update functions in SHA-NI

[kd]

Summary

r359374 introduced crypto_apply function which takes as argument a function pointer that is expected to return an int.
Functions used in aesni for sha updates return void and were cast with their return type changed.
This resulted in undefined behavior, with mbuf is used the following scenario occurs:
m_apply checks return value of function pointer passed to it and bogusly fails after calculating hash of the first mbuf in chain.
Because of that transmitted frames have wrong ICV and are dropped by the other side.
Fix it by wrapping calls to shaX update functions with routines that always return 0, similarly to how it is done in cryptosoft hash routines.

Test Plan

Create ipsec tunnel that uses aes-cbc-128+sha256hmac on a board with SHA-NI support (Intel Atom E3930) and ping the other side.

[cem]

Nice find! The fix looks correct, but I'd like to see us remove the erroneous casts that caused the problem as well.

sys/crypto/aesni/aesni.c

485–489 ↗

(On Diff #72326)

We could probably also just change e.g. intel_sha256_update to return int 0 without any consumer really caring?

860–861 ↗

(On Diff #72326)

Please remove the casts which masked this error; there are at least four more below.

[kd]

Change return types of sha routines directly instead of using wrappers as suggested by @cem.

sys/crypto/aesni/aesni.c

860–861 ↗

(On Diff #72326)

We still need the casts, the second argument of hash_update is "const void*" instead of "void*".
Apparently clang is pretty strict about it, without this cast build fails with:

error: incompatible function pointer types passing 'int (*)(void *, const void *, unsigned int)' to parameter of type 'int (*)(void *, void *, u_int)' (aka 'int (*)(void *, void *, unsigned int)')

[cem]

Thanks!

sys/crypto/aesni/aesni.c

860–861 ↗

(On Diff #72326)

So either correct the types in crypto_apply to const the source buffer, or remove const for these routines to match?

[kd]

sys/crypto/aesni/aesni.c

860–861 ↗

(On Diff #72326)

I've removed const from aesni routines.
Frankly adding const to the type in crypto_apply would be cleaner, but crypto_apply calls m_apply which expects "void*" and I don't want to modify m_apply right now since it is used in a few other places.
By the way similar casts are also present in every other call to crypto_apply, I checked them and return type is not changed so they should work just fine.

[jhb]

Thanks. I have on my plate to do a sweep fixing the function typedefs here to be more consistent. The GMAC software ones take a uint16_t instead of u_int for length which exposes a different bug. Does this fix the IPSec tunnel test failures reported? I thought those were also true of non-aesni, not just aesni though.

[cem]

sys/crypto/aesni/aesni.c

860–861 ↗

(On Diff #72326)

Yeah, adding const is a bigger lift. Thanks for eliding the casts here.

[kd]

In D25030#551185, @jhb wrote:

Does this fix the IPSec tunnel test failures reported? I thought those were also true of non-aesni, not just aesni though.

It doesn't. I checked both aes-cbc and aes-cbc+sha256 and both don't work when cryptosoft is used.
aes-ctr seems to work just fine though.
I suspect that there might be something wrong in swcr_encdec routine.