Use of this union is somewhat problematic.

It's use is very much in violation of the spirit of the C standard, around what unions are; and what behavior is defined, implementation-defined, or undefined.

I think I'd suggest just typedef __m128i aes_block_t (or removing use of the aes_block_t type entirely) instead of this union. Most of the code is already defined in terms of the __m128i type anyway.

(Style(9) nitpick on the name, anyway: unambiguously says not to define typedefs ending with "_t".)

This file is more or less new code; it's free to follow style(9) from the beginning.

So nitpicks here: Blank line between variable declarations and first statement; and return (retval);.

ditto the blank line thing

Seems like retval could be named something more descriptive, like B0 or MAC.

And temp_block is maybe auth_block or something (staging_block would match software CBC-MAC).

FWIW, this will be the 3rd copy of this code in the tree (plain software generates the B0 block, ccr has a 'generate_ccm_b0' function). Someday we should export a shared helper function, but we don't have to do that as part of this change.

Does __m128i foo = 0; generate different code than __m128i foo = _mm_setzero_si128();? It isn't important, I'm just vaguely curious.

Edit: Ahhh, __m128i is literally a vector, rather than integral, type, according to the compiler. So it cannot simply be initialized with an integer.

style nit: auth_len > 0, as auth_len is not a boolean

I'm not asking you to change these, but in new code I'd encourage use of the C89 memcpy/memmove/memset macros over the pre-ANSI bcopy etc routines.

(For me as a reviewer, I have to think about destination/source argument order every time I look at one of these b* routines, whereas I don't have to think about memcpy.)

Could just use

be16enc(&temp_block, auth_len);

What do you mean by calling convention? The immediate function takes a size_t, which can represent larger values.

I understand that internally OCF deals only in int sizes, but I'd suggest either implementing the 3rd mode or asserting we're within range of the 2nd mode anyway.

Simplified:

be16enc(&temp_block, 0xfffe);
be32enc((char *)&temp_block + 2, auth_len);

style nit: sizeof(temp_block), for consistency with the object we're acting on

typo: abytes

usually spelled "4 GB" or "4 gigabytes" but not "4gbytes"

I don't know what these pre-processor error directives (there are a few) are intended to be, but probably remove them before commit. (Figure out how to incorporate them into comments or code, if necessary.)

Ugh. I know you just inherited this mess from GCM, but what an obnoxious API :P.

Is it impossible to generate a tag for AAD without any ciphered input?

For actual CCM I think the limits are 7 and 13.

So really last_block is the rolling MAC value. I'd suggest a name incorporating MAC, for clarity.

style nit: Usually we at declare variables at the beginning of some scope, if not function scope.

Right, ok, we're generating a keystream from an encrypted series of counter blocks.

This step doesn't have any dependency on the data, right? We could do it earlier, as soon as we have saved a copy in s_x for use in generating the keystream.

style: Empty return

Here is where you would explicit_bzero() sensitive data, like maybe last_block, s0, temp_block, and X.

For example, here it is not clear which member of the union is supposed to be initialized by s0.

maybe this is just phabricator, but it looks like there are extra spaces between __m128i and s0

And here we reference the union pointer directly for initialization.

You already zero padded temp_block on lines 322-325.

empty return

probably also the last block of decrypted plaintext is sensitive (temp_block)? And perhaps the tag, hash_block.

No verifying of the tag over the AAD?

s_x is never used?

timingsafe_bcmp

style(9) nit: return (0);