Changeset View
Standalone View
sys/opencrypto/cbc_mac.c
- This file was added.
| #include <sys/types.h> | |||||
| #include <sys/systm.h> | |||||
| #include <sys/param.h> | |||||
| #include <sys/endian.h> | |||||
| #include <opencrypto/cbc_mac.h> | |||||
| #include <opencrypto/xform_auth.h> | |||||
| /* | |||||
| * Given two CCM_CBC_BLOCK_LEN blocks, xor | |||||
| * them into dst, and then encrypt dst. | |||||
| */ | |||||
| static void | |||||
| xor_and_encrypt(struct aes_cbc_mac_ctx *ctx, | |||||
| const uint8_t *src, uint8_t *dst) | |||||
| { | |||||
| const uint64_t *b1; | |||||
| uint64_t *b2; | |||||
| uint64_t temp_block[CCM_CBC_BLOCK_LEN/sizeof(uint64_t)]; | |||||
| b1 = (const uint64_t*)src; | |||||
cem: style(9) puts a blank line between the declaration and assignment lines | |||||
| b2 = (uint64_t*)dst; | |||||
| for (size_t count = 0; | |||||
| count < CCM_CBC_BLOCK_LEN/sizeof(uint64_t); | |||||
| count++) { | |||||
| temp_block[count] = b1[count] ^ b2[count]; | |||||
| } | |||||
Done Inline ActionsDid you try this without the handcoded vectorization and find the optimizer unable to perform this basic optimization? It seems odd to me that this is needed (and seems somewhat likely to be worse than the naive loop on 32-bit platforms). cem: Did you try this without the handcoded vectorization and find the optimizer unable to perform… | |||||
Done Inline ActionsNo. I modeled a *lot* of this on the GCM code, so I used the same types it did in a lot of places. sef: No. I modeled a *lot* of this on the GCM code, so I used the same types it did in a lot of… | |||||
Not Done Inline ActionsSure; now we have two copies of questionable code in the tree instead of just the one ;-). Please go ahead and fix it. cem: Sure; now we have two copies of questionable code in the tree instead of just the one ;-). | |||||
Done Inline ActionsI 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. sef: I just compared clang's code output for the code using uint64_t and uint32_t, for i386 and… | |||||
Done Inline ActionsIt 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. cem: It looks like GCC -O2 -funroll-loops (or -fpeel-loops) handles this well at any copy size… | |||||
| rijndaelEncrypt(ctx->keysched, ctx->rounds, (void*)temp_block, dst); | |||||
| } | |||||
| void | |||||
| AES_CBC_MAC_Init(struct aes_cbc_mac_ctx *ctx) | |||||
| { | |||||
| bzero(ctx, sizeof(*ctx)); | |||||
Done Inline Actionsstyle(9): sizeof(*ctx) cem: style(9): `sizeof(*ctx)` | |||||
| } | |||||
Not Done Inline ActionsIf this is universal (and it seems to be), why waste the space in aes_cbc_mac_ctc? cem: If this is universal (and it seems to be), why waste the space in `aes_cbc_mac_ctc`? | |||||
Done Inline ActionsThe tag length is variable in the algorithm; I haven't implemented anything else however. I don't like that constant name, admittedly, but the name lengths were getting annoyingly long. sef: The tag length is variable in the algorithm; I haven't implemented anything else however. I… | |||||
Done Inline ActionsI 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. cem: I wasn't complaining about the name of the constant; just the useless struct member, and… | |||||
| void | |||||
| AES_CBC_MAC_Setkey(struct aes_cbc_mac_ctx *ctx, const uint8_t *key, uint16_t klen) | |||||
| { | |||||
| ctx->rounds = rijndaelKeySetupEnc(ctx->keysched, key, klen * 8); | |||||
| } | |||||
Done Inline ActionsProbably this should also set ctx->keyLength, except it appears nothing uses that, so maybe it should just be removed instead. cem: Probably this should also set `ctx->keyLength`, except it appears nothing uses that, so maybe… | |||||
Done Inline ActionsI used it in a version of the code. sef: I used it in a version of the code. | |||||
Not Done Inline ActionsDo you intend to commit that version or this one? :-) cem: Do you intend to commit that version or this one? :-) | |||||
Done Inline ActionsIf I commit both at the same time, would that confuse quantum encryption? :) sef: If I commit both at the same time, would that confuse quantum encryption? :)
| |||||
Done Inline Actionsstyle nit: no need for explicit return at the end of a void function cem: style nit: no need for explicit return at the end of a void function | |||||
Done Inline ActionsI find it jarring to not have a return. Among other things, it lets me know that the function is a void. sef: I find it jarring to not have a return. Among other things, it lets me know that the function… | |||||
Done Inline ActionsThe 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. cem: The `void` 5 lines higher should tell you the function is `void`, and lack of return in a non… | |||||
Done Inline ActionsIt'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. sef: It's a pretty stupid rule -- there is absolutely no advantage to it, it harms readability… | |||||
| /* | |||||
| * This is called to set the nonce, aka IV. | |||||
| * Before this call, the authDataLength and cryptDataLength fields | |||||
| * MUST have been set. Sadly, there's no way to return an error. | |||||
| * | |||||
| * The CBC-MAC algorithm requires that the first block contain the | |||||
Done Inline ActionsKASSERT on misuse cem: KASSERT on misuse | |||||
| * nonce, as well as information about the sizes and lengths involved. | |||||
| */ | |||||
| void | |||||
| AES_CBC_MAC_Reinit(struct aes_cbc_mac_ctx *ctx, const uint8_t *nonce, uint16_t nonceLen) | |||||
| { | |||||
| uint8_t b0[CCM_CBC_BLOCK_LEN]; | |||||
| uint8_t *bp = b0, flags = 0; | |||||
| uint8_t L = 0; | |||||
| uint64_t dataLength = ctx->cryptDataLength; | |||||
| KASSERT(ctx->authDataLength != 0 || ctx->cryptDataLength != 0, | |||||
Done Inline Actionsstyle(9) generally suggests keeping declarations and definitions of function-scope variables separate. While I'm not militant about that, in this specific case I find the name tmp for cryptDataLength and the distance between assignment and use confusing enough that I would prefer these be separated. cem: style(9) generally suggests keeping declarations and definitions of function-scope variables… | |||||
Done Inline ActionsI ran out of name-generating capability during this :). Changed. sef: I ran out of name-generating capability during this :). Changed. | |||||
| ("Auth Data and Data lengths cannot both be 0")); | |||||
| KASSERT(nonceLen >= 7 && nonceLen <= 13, | |||||
| ("nonceLen must be between 7 and 13 bytes")); | |||||
Done Inline ActionsThe 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)? cem: The previous condition allowed one to be zero, and this one doesn't permit either to be zero. | |||||
Done Inline ActionsI have gotten assert conditions backwards for over 30 years. This is another instance. sef: I have gotten assert conditions backwards for over 30 years. This is another instance. | |||||
| ctx->nonce = nonce; | |||||
| ctx->nonceLength = nonceLen; | |||||
| ctx->authDataCount = 0; | |||||
| ctx->blockIndex = 0; | |||||
| explicit_bzero(ctx->staging_block, sizeof(ctx->staging_block)); | |||||
| /* | |||||
| * Need to determine the L field value. This is the number of | |||||
| * bytes needed to specify the length of the message; the length | |||||
| * is whatever is left in the 16 bytes after specifying flags and | |||||
| * the nonce. | |||||
| */ | |||||
| L = 15 - nonceLen; | |||||
Done Inline ActionsThis line-wrapping seems far shorter than 80 characters. cem: This line-wrapping seems far shorter than 80 characters. | |||||
| flags = ((ctx->authDataLength > 0) << 6) + | |||||
Not Done Inline ActionsL here (called q in the NIST CCM document) cannot be outside [2, 8], and nonceLen cannot be outside [7, 13]; nonceLen + q = 15. It is unclear why & 0xff is used here. cem: `L` here (called `q` in the NIST CCM document) cannot be outside [2, 8], and `nonceLen` cannot… | |||||
Done Inline ActionsPartially paranoia, partially making clear to myself that it can only be a single octet. sef: Partially paranoia, partially making clear to myself that it can only be a single octet. | |||||
Done Inline ActionsNothing 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. cem: Nothing about the algorithm says `L` has to be a single octet. You've chosen to store it in a… | |||||
| (((AES_CBC_MAC_HASH_LEN - 2) / 2) << 3) + | |||||
| L - 1; | |||||
| /* | |||||
Not Done Inline ActionsKASSERT(tagLength < 18)? Or just remove the tagLength member entirely, since it's always AES_CBC_MAC_HASH_LEN. Usually * 64 is spelled << 6, and similarly * 8 is << 3. cem: `KASSERT(tagLength < 18)`? Or just remove the `tagLength` member entirely, since it's always… | |||||
Done Inline ActionsThe multipliers were listed in the spec, not bit shifts. But shifts do make it clearer; bitfields would be clearer still but that's kinda ridiculous. sef: The multipliers were listed in the spec, not bit shifts. But shifts do make it clearer… | |||||
Not Done Inline ActionsThanks! 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. cem: Thanks! I don't think we can rely on bitfields due to underspecification of their layout in… | |||||
| * Now we need to set up the first block, which has flags, nonce, | |||||
| * and the message length. | |||||
| */ | |||||
| b0[0] = flags; | |||||
| bcopy(nonce, b0 + 1, nonceLen); | |||||
| bp = b0 + 1 + nonceLen; | |||||
Done Inline Actionsstyle(9) generally requires space around 2-ary operators. This is another rule I don't feel strongly about in general (i.e., when space is constrained), but in this case, please add the spaces. cem: style(9) generally requires space around 2-ary operators. This is another rule I don't feel… | |||||
| /* Need to copy L' [aka L-1] bytes of cryptDataLength */ | |||||
| for (uint8_t *dst = b0 + sizeof(b0) - 1; dst >= bp; dst--) { | |||||
| *dst = (dataLength & 0xff); | |||||
| dataLength >>= 8; | |||||
Not Done Inline Actionsthis & 0xff made sense before :-) sure, without it the value is silently truncated to typeof(*dst) == uint8_t, but the bitwise AND made the truncation slightly more obvious. cem: this `& 0xff` made sense before :-)
sure, without it the value is silently truncated to… | |||||
| } | |||||
| /* Now need to encrypt b0 */ | |||||
Done Inline ActionsThis wrapping also seems too short. cem: This wrapping also seems too short.
| |||||
Done Inline ActionsIt was too long to fit on one line nicely, so I broke it up per expression. The "too long" is a personal feeling, though. sef: It was too long to fit on one line nicely, so I broke it up per expression. The "too long" is… | |||||
| rijndaelEncrypt(ctx->keysched, ctx->rounds, b0, ctx->block); | |||||
| /* If there is auth data, we need to set up the staging block */ | |||||
| if (ctx->authDataLength) { | |||||
| if (ctx->authDataLength < ((1<<16) - (1<<8))) { | |||||
| uint16_t sizeVal = htobe16(ctx->authDataLength); | |||||
| bcopy(&sizeVal, ctx->staging_block, sizeof(sizeVal)); | |||||
| ctx->blockIndex = sizeof(sizeVal); | |||||
| } else if (ctx->authDataLength < (1UL<<32)) { | |||||
| uint32_t sizeVal = htobe32(ctx->authDataLength); | |||||
| ctx->staging_block[0] = 0xff; | |||||
| ctx->staging_block[1] = 0xfe; | |||||
| bcopy(&sizeVal, ctx->staging_block+2, sizeof(sizeVal)); | |||||
| ctx->blockIndex = 2 + sizeof(sizeVal); | |||||
| } else { | |||||
| uint64_t sizeVal = htobe64(ctx->authDataLength); | |||||
| ctx->staging_block[0] = 0xff; | |||||
| ctx->staging_block[1] = 0xff; | |||||
| bcopy(&sizeVal, ctx->staging_block+2, sizeof(sizeVal)); | |||||
| ctx->blockIndex = 2 + sizeof(sizeVal); | |||||
| } | |||||
| } | |||||
| } | |||||
| int | |||||
| AES_CBC_MAC_Update(struct aes_cbc_mac_ctx *ctx, const uint8_t *data, | |||||
| uint16_t length) | |||||
Not Done Inline Actionsredundant cem: redundant | |||||
Done Inline ActionsSee above sef: See above | |||||
| { | |||||
| size_t copy_amt; | |||||
| /* | |||||
| * This will be called in one of two phases: | |||||
| * (1) Applying authentication data, or | |||||
| * (2) Applying the payload data. | |||||
| * | |||||
Done Inline Actionsshort wrapping cem: short wrapping | |||||
Done Inline ActionsThe function definition was too long. The comments are formatted because of the listed items. sef: The function definition was too long. The comments are formatted because of the listed items. | |||||
Done Inline ActionsDoh! Sorry. I did not notice the list. cem: Doh! Sorry. I did not notice the list. | |||||
| * Because CBC-MAC puts the authentication data size before the | |||||
| * data, subsequent calls won't be block-size-aligned. Which | |||||
| * complicates things a fair bit. | |||||
| * | |||||
| * The payload data doesn't have that problem. | |||||
| */ | |||||
| if (ctx->authDataCount < ctx->authDataLength) { | |||||
| /* | |||||
| * We need to process data as authentication data. | |||||
| * Since we may be out of sync, we may also need | |||||
| * to pad out the staging block. | |||||
| */ | |||||
| const uint8_t *ptr = data; | |||||
| while (length > 0) { | |||||
| copy_amt = MIN(length, | |||||
| sizeof(ctx->staging_block) - ctx->blockIndex); | |||||
Not Done Inline Actionsstyle(9): don't use non-boolean values as conditions, i.e., use length > 0 cem: style(9): don't use non-boolean values as conditions, i.e., use `length > 0` | |||||
| bcopy(ptr, ctx->staging_block + ctx->blockIndex, | |||||
Done Inline ActionsThis is a good example of a declaration and initialization that should be separated. It also assumes that any given Update() call will only provide AD or payload; not both. That assumption should be KASSERTed or fixed. cem: This is a good example of a declaration and initialization that should be separated.
It also… | |||||
Done Inline Actionssef:
| |||||
Not Done Inline ActionsStill assuming that any given Update() call will only provide AD or payload; not both. That assumption should be KASSERTed or fixed. cem: Still assuming that any given Update() call will only provide AD or payload; not both. That… | |||||
| copy_amt); | |||||
| ptr += copy_amt; | |||||
| length -= copy_amt; | |||||
| ctx->authDataCount += copy_amt; | |||||
| ctx->blockIndex += copy_amt; | |||||
| ctx->blockIndex %= sizeof(ctx->staging_block); | |||||
| if (ctx->authDataCount == ctx->authDataLength) | |||||
| length = 0; | |||||
Not Done Inline ActionsThis is backwards — this check needs to happen before the copy. The > case is invalid. cem: This is backwards — this check needs to happen before the copy. The `>` case is invalid. | |||||
Done Inline ActionsThe 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. sef: The code does the copy first. The check is done afterwards; that's just how I wrote the code. | |||||
Not Done Inline Actions==? (compiler should have thrown a warning and probably error if this was attempted to compile?) cem: `==`?
(compiler should have thrown a warning and probably error if this was attempted to… | |||||
Done Inline ActionsNot sure what you mean by that. sef: Not sure what you mean by that. | |||||
Not Done Inline ActionsOn 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. cem: On the commented revision you have `if (ctx->authDataCount = ctx->authDataLength)` — should… | |||||
| if (ctx->blockIndex == 0 || | |||||
| ctx->authDataCount >= ctx->authDataLength) { | |||||
| /* | |||||
| * We're done with this block, so we | |||||
| * xor staging_block with block, and then | |||||
| * encrypt it. | |||||
| */ | |||||
| xor_and_encrypt(ctx, ctx->staging_block, ctx->block); | |||||
| bzero(ctx->staging_block, sizeof(ctx->staging_block)); | |||||
Not Done Inline ActionsUsual convention is to only use explicit_bzero when the context is cleaned / released (i.e., Final()), not every block the algorithm processes. cem: Usual convention is to only use explicit_bzero when the context is cleaned / released (i.e. | |||||
Done Inline ActionsEasily enough done. It wasn't clear to me when it was and wasn't used. sef: Easily enough done. It wasn't clear to me when it was and wasn't used. | |||||
Not Done Inline Actionsexplicit_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.) cem: `explicit_bzero` is only used when memory with sensitive data is released and might be reused… | |||||
| ctx->blockIndex = 0; | |||||
| } | |||||
| } | |||||
| return (0); | |||||
Not Done Inline ActionsThis also assumes Update() does not contain both AD and payload data. cem: This also assumes Update() does not contain both AD and payload data. | |||||
| } | |||||
| /* | |||||
| * If we're here, then we're encoding payload data. | |||||
| * This is marginally easier, except that _Update can | |||||
| * be called with non-aligned update lengths. As a result, | |||||
| * we still need to use the staging block. | |||||
| */ | |||||
| KASSERT((length + ctx->cryptDataCount) <= ctx->cryptDataLength, | |||||
| ("More encryption data than allowed")); | |||||
| while (length) { | |||||
| uint8_t *ptr; | |||||
Not Done Inline ActionsThis path seems totally broken. cem: This path seems totally broken. | |||||
Done Inline ActionsHow so? sef: How so? | |||||
Not Done Inline ActionsE.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". cem: E.g.,
```
AES_CBC_MAC_Update("a", length=1);
AES_CBC_MAC_Update("b", length=1)… | |||||
Done Inline ActionsObviously 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. sef: Obviously I hadn't thought about it being called that way. The only one I'd thought about was… | |||||
| copy_amt = MIN(sizeof(ctx->staging_block) - ctx->blockIndex, | |||||
| length); | |||||
| ptr = ctx->staging_block + ctx->blockIndex; | |||||
| bcopy(data, ptr, copy_amt); | |||||
| data += copy_amt; | |||||
| ctx->blockIndex += copy_amt; | |||||
| ctx->cryptDataCount += copy_amt; | |||||
| length -= copy_amt; | |||||
| if (ctx->blockIndex == sizeof(ctx->staging_block)) { | |||||
| /* We've got a full block */ | |||||
| xor_and_encrypt(ctx, ctx->staging_block, ctx->block); | |||||
| ctx->blockIndex = 0; | |||||
| bzero(ctx->staging_block, sizeof(ctx->staging_block)); | |||||
| } | |||||
| } | |||||
Done Inline ActionsThis should definitely not be explicit_bzero cem: This should definitely not be `explicit_bzero` | |||||
| return (0); | |||||
Not Done Inline ActionsWhat is the 0xff for? 15 - nonceLength must be in [2, 8]. cem: What is the `0xff` for? `15 - nonceLength` must be in [2, 8]. | |||||
Done Inline ActionsSee above. sef: See above. | |||||
| } | |||||
Not Done Inline Actionsspaces around the + please cem: spaces around the `+` please | |||||
| void | |||||
| AES_CBC_MAC_Final(uint8_t *buf, struct aes_cbc_mac_ctx *ctx) | |||||
| { | |||||
| uint8_t s0[CCM_CBC_BLOCK_LEN]; | |||||
Not Done Inline Actionsredundant return cem: redundant return | |||||
| /* | |||||
| * We first need to check to see if we've got any data | |||||
| * left over to encrypt. | |||||
| */ | |||||
| if (ctx->blockIndex != 0) { | |||||
| xor_and_encrypt(ctx, ctx->staging_block, ctx->block); | |||||
| ctx->cryptDataCount += ctx->blockIndex; | |||||
| ctx->blockIndex = 0; | |||||
| explicit_bzero(ctx->staging_block, sizeof(ctx->staging_block)); | |||||
| } | |||||
| bzero(s0, sizeof(s0)); | |||||
| s0[0] = ((15 - ctx->nonceLength) & 0xff) - 1; | |||||
cemUnsubmitted Not Done Inline ActionsOpen question - What is the 0xff for? 15 - nonceLength must be in [2, 8]. cem: Open question - What is the 0xff for? 15 - nonceLength must be in [2, 8]. | |||||
sefAuthorUnsubmitted Done Inline ActionsOk, got that one, and the other unnecessary 0xff. (Updating both revisions next.) sef: Ok, got that one, and the other unnecessary 0xff. (Updating both revisions next.) | |||||
| bcopy(ctx->nonce, s0 + 1, ctx->nonceLength); | |||||
| rijndaelEncrypt(ctx->keysched, ctx->rounds, s0, s0); | |||||
| for (size_t indx = 0; indx < AES_CBC_MAC_HASH_LEN; indx++) | |||||
| buf[indx] = ctx->block[indx] ^ s0[indx]; | |||||
| explicit_bzero(s0, sizeof(s0)); | |||||
| } | |||||
style(9) puts a blank line between the declaration and assignment lines