See above

The function definition was too long. The comments are formatted because of the listed items.

The code does the copy first. The check is done afterwards; that's just how I wrote the code. It'd have to be refactored to do the copy afterwards.

I agree it should never be greater than; I used >= out of personal habit.

Easily enough done. It wasn't clear to me when it was and wasn't used.

How so?

See above.

Sure; now we have two copies of questionable code in the tree instead of just the one ;-). Please go ahead and fix it.

I wasn't complaining about the name of the constant; just the useless struct member, and separation between assignment and use.

The aes_cbc_mac_ctx interface is internal, not KPI, so I don't see any reason to include the tagLength for now rather than just removing the member and using the constant.

If it ever becomes necessary to support variable-length tags, it is trivial to find the places the constant is used.

Do you intend to commit that version or this one? :-)

The void 5 lines higher should tell you the function is void, and lack of return in a non-void function triggers a compiler error in the kernel build. So in addition to the explicit function type attached to any definition, it should be clear that any time a function has no explicit return, it is void.

Regardless, style(9) is pretty clear that return should be omitted for void functions, and provides 2-3 examples of that.

The previous condition allowed one to be zero, and this one doesn't permit either to be zero. I believe this function is valid with zero authData, for example, so maybe the assertion should replace && with || (De Morgan's from the previous expression)?

Nothing about the algorithm says L has to be a single octet. You've chosen to store it in a uint8_t, which is unobjectionable because of the algorithm's constraints on its range, but masking it down to a single octet is totally unacceptable. It implies that the value might be out of range, and that that would not only be possible, but accepted.

Please assert on nonceLen being in the spec range, and then it is impossible for L to be out of range. [2, 8] is well within [0, 255] and the confusing and incorrect logical AND can be removed. You could additionally assert on L being in range after the subtraction, but I suspect an optimizing compiler would just remove it due to the range implied by the earlier nonceLen assertion.

Thanks! I don't think we can rely on bitfields due to underspecification of their layout in standard C (not that some in-tree drivers don't already make assumptions), but the shifts are clear and the prevailing method.

Doh! Sorry. I did not notice the list.

Still assuming that any given Update() call will only provide AD or payload; not both. That assumption should be KASSERTed or fixed.

==?

(compiler should have thrown a warning and probably error if this was attempted to compile?)

explicit_bzero is only used when memory with sensitive data is released and might be reused by another context. So it is typically invoked to clean up state on Final(). It isn't needed when memory ownership does not change.

(It's only needed because the C compiler may recognize that the released memory is not used after a regular bzero and "optimize" out the bzero. When bzero is used during an algorithm that later references the zeroed memory, the compiler is not free to optimize out the zeroing.)

E.g.,

AES_CBC_MAC_Update("a", length=1);
AES_CBC_MAC_Update("b", length=1);
AES_CBC_MAC_Update("c", length=1);
AES_CBC_MAC_Final();

Will incorrectly produce the MAC for the message "a\0\0…b\0\0…c\0\0", rather than for the message "abc".

I just compared clang's code output for the code using uint64_t and uint32_t, for i386 and x86_64. The i386 code for both functions was pretty similar (same number of instructions, just slightly different ordering), while the x86_64 code using uint32_t was considerably worse than for uint64_t.

I haven't got easy access to the other platforms, but this does not appear to be performance fear grounded in reality.

If I commit both at the same time, would that confuse quantum encryption? :)

It's a pretty stupid rule -- there is absolutely no advantage to it, it harms readability, results in inherent inconsistency in style, and it ignores the growing number of stupidly-designed languages that set function return value to the last executed expression -- that is only mentioned in a comment when describing variardic functions. But easily changed.

I have gotten assert conditions backwards for over 30 years. This is another instance.

Ah, missed from my rename. Easily changed.

It looks like GCC -O2 -funroll-loops (or -fpeel-loops) handles this well at any copy size:

https://reviews.freebsd.org/P241

But Clang handles it poorly without the hand-unrolling (1 byte -> 8 bytes at a time), and GCC doesn't make the optimization at -O2 without -funroll-loops or -fpeel-loops (and just keeps the small code size loop).

I'm okay with leaving it as-is.

aes_key isn't used either :-)

Not sure what you mean by that.

Obviously I hadn't thought about it being called that way. The only one I'd thought about was the auth data not being a block multiple. That's an annoying rewrite of the code -- _Final will have to check for data left over as well.

I noted in the initial message for this change review that I didn't strip out the value for CRYPTO_AES_CCM_16, but could if really necessary.

I know I had a reason for doing it this way, but I'm blanking on it for now. I'll leave this incomplete until I either remember why, or change it.

On the commented revision you have if (ctx->authDataCount = ctx->authDataLength) — should have been == instead of =, which you fixed in the recent update.

Modern versions of Clang and GCC typically produce a warning when = is used in if conditions, and we build the kernel with -Werror, so I suspect the submitted patch was never compiled successfully. But it's also possible we have disabled warnings / -Werror on this file.

It seems easy enough to remove here and add in the revision that adds CCM so I'd certainly prefer that.

Thanks!

Oh. I did it that way because of CRYPTO_AES_${SIZE}_NIST_GMAC.

Yes, that's the bug (there wasn't a good reason for GCM to do it). It would be really nice to not have it if we can avoid it.

It's also what CRYPTO_SHA2_${SIZE} does. So it's not just GCM. Are there any of the crypto or auth ones that do? (He asks while rewriting the code to see how annoying or not it is :).)

Well, any auth ones that do; the crypto ones have min and max keys, while the auth struct only has key size.

But AEAD (GCM and CCM) are different. They don't actually have separate keys. You use the same key for both the cipher and the auth. I think it's a bug that the CRYPTO_AES_*_NIST_GMAC constants exist at all and plan to remove them in my OCF rework branch. There should only be the GCM (cipher with tag) and GMAC (hash) algorithms in my mind with the right axf pointer chosen based on the key size. Similarly for CCM and CBC-MAC. I know we can't yet quite get there today in the current OCF, but I think we could easily use a single constant for the auth side of CCM for now and have drivers and consumers just switch on the key size to choose the right axf pointer.

https://github.com/bsdjhb/freebsd/compare/master...ocf_rework shows the WIP

I've got two responses to this, equally valid:

First, I can surely rip out all of the specific key size ones, and just have a generic one, because the code isn't plumbed up -- and won't be until the code that changes the OCF interfaces. But...

Second, that's telling me to write this code to an API that isn't currently in the system, while this is intended to go into the current system. And in the existing one, I have to have the different keysize variants as distinct entries, because the setup code will fail if the key sizes don't match.

I'm blocked on doing the other parts of this, since it was requested to be broken down into multiple smaller bits; that in turn is blocking the ZFS work we want to move forward with.

So which is it: do I write this for a future kernel change, or do I write this for the existing one?

I'm saying it's fine to have multiple struct auth_xform that differ per key-size. I would just have a single constant and have code that's using the constant do something like:

case CRYPTO_AES_CCM_CBC_MAX:
     switch (keysize) {
     case 16:
         axf = &auth_<mumble>_128;
         break;
     case 24:
         axf = &auth_<mumble>_192;
         break;
     case 32:
         axf = &auth_<mumble>_256;
         break;

Does that make sense? Even with the current API I feel like GCM could/should have used that approach IMO. I think it also makes life less complicated in consumers and drivers as you don't have to add a bunch of silly checks for things like "does the specific CCM MAC constant match the key length of the cipher key" because you are using the cipher key length directly with a single constant that doesn't on its own indicate a key size.

My recollection at having tried different things in the past is that if I use the single entry with auth, the existing setup code will look at the key size given in the crp, and compare it with the key size given in the structure, and error out if they're not the same. My recollection could be entirely wrong of course!

Either my recollection was wrong, or the code change -- it looks for "thash->keysize != 0 && sop->mackeylen > thash->keysize" which means it should work. Coming up as soon as I have some reliable networking again!

Open question - What is the 0xff for? 15 - nonceLength must be in [2, 8].

Ok, got that one, and the other unnecessary 0xff. (Updating both revisions next.)