Differential D18592 Diff 52494 sys/opencrypto/cbc_mac.c

Changeset View

Standalone View

sys/opencrypto/cbc_mac.c

Property	Old Value	New Value
svn:eol-style	null	native \ No newline at end of property
svn:keywords	null	FreeBSD=%H \ No newline at end of property
svn:mime-type	null	text/plain \ No newline at end of property

				#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;
				cemUnsubmitted Done Inline Actions style(9) puts a blank line between the declaration and assignment lines 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];
				}
				cemUnsubmitted Done Inline Actions Did 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…
				sefAuthorUnsubmitted Done Inline Actions No. 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…
				cemUnsubmitted Not Done Inline Actions Sure; 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 ;-).
				sefAuthorUnsubmitted Done Inline Actions 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. sef: I just compared clang's code output for the code using uint64_t and uint32_t, for i386 and…
				cemUnsubmitted Done Inline Actions 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. 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));
				cemUnsubmitted Done Inline Actions style(9): `sizeof(ctx)` cem:* style(9): `sizeof(*ctx)`
				}
				cemUnsubmitted Not Done Inline Actions If 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`?
				sefAuthorUnsubmitted Done Inline Actions The 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…
				cemUnsubmitted Done Inline Actions 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. 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);
				}
				cemUnsubmitted Done Inline Actions Probably 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…
				sefAuthorUnsubmitted Done Inline Actions I used it in a version of the code. sef: I used it in a version of the code.
				cemUnsubmitted Not Done Inline Actions Do you intend to commit that version or this one? :-) cem: Do you intend to commit that version or this one? :-)
				sefAuthorUnsubmitted Done Inline Actions If 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? :)

				cemUnsubmitted Done Inline Actions style 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
				sefAuthorUnsubmitted Done Inline Actions I 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…
				cemUnsubmitted Done Inline Actions 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. cem: The `void` 5 lines higher should tell you the function is `void`, and lack of return in a non…
				sefAuthorUnsubmitted Done Inline Actions 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. 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
				cemUnsubmitted Done Inline Actions KASSERT 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,
				cemUnsubmitted Done Inline Actions style(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…
				sefAuthorUnsubmitted Done Inline Actions I 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"));
				cemUnsubmitted Done Inline Actions 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)? cem: The previous condition allowed one to be zero, and this one doesn't permit either to be zero.
				sefAuthorUnsubmitted Done Inline Actions I 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;
				cemUnsubmitted Done Inline Actions This line-wrapping seems far shorter than 80 characters. cem: This line-wrapping seems far shorter than 80 characters.

				flags = (ctx->authDataLength > 0) << 6 +
				cemUnsubmitted Not Done Inline Actions `L` 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…
				sefAuthorUnsubmitted Done Inline Actions Partially 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.
				cemUnsubmitted Done Inline Actions 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. 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;
				/*
				cemUnsubmitted Not Done Inline Actions `KASSERT(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…
				sefAuthorUnsubmitted Done Inline Actions The 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…
				cemUnsubmitted Not Done Inline Actions 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. 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;

				cemUnsubmitted Done Inline Actions style(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;
				cemUnsubmitted Not Done Inline Actions this `& 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 */
				cemUnsubmitted Done Inline Actions This wrapping also seems too short. cem: This wrapping also seems too short.
				sefAuthorUnsubmitted Done Inline Actions It 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)
				cemUnsubmitted Not Done Inline Actions redundant cem: redundant
				sefAuthorUnsubmitted Done Inline Actions See above sef: See above
				{

				/*
				* This will be called in one of two phases:
				* (1) Applying authentication data, or
				* (2) Applying the payload data.
				*
				* Because CBC-MAC puts the authentication data size before the
				cemUnsubmitted Done Inline Actions short wrapping cem: short wrapping
				sefAuthorUnsubmitted Done Inline Actions The 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.
				cemUnsubmitted Done Inline Actions Doh! Sorry. I did not notice the list. cem: Doh! Sorry. I did not notice the list.
				* 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) {
				size_t copy_amt;

				copy_amt = MIN(length,
				sizeof(ctx->staging_block) - ctx->blockIndex);
				cemUnsubmitted Not Done Inline Actions style(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,
				cemUnsubmitted Done Inline Actions This 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 `KASSERT`ed or fixed. cem: This is a good example of a declaration and initialization that should be separated. It also…
				sefAuthorUnsubmitted Done Inline Actions sef:
				cemUnsubmitted Not Done Inline Actions Still 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;
				cemUnsubmitted Not Done Inline Actions This 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.
				sefAuthorUnsubmitted Done Inline Actions 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. sef: The code does the copy first. The check is done afterwards; that's just how I wrote the code.
				cemUnsubmitted 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…
				sefAuthorUnsubmitted Done Inline Actions Not sure what you mean by that. sef: Not sure what you mean by that.
				cemUnsubmitted Not Done Inline Actions 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. 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));
				cemUnsubmitted Not Done Inline Actions Usual 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.
				sefAuthorUnsubmitted Done Inline Actions Easily 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.
				cemUnsubmitted Not Done Inline Actions `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.) cem: `explicit_bzero` is only used when memory with sensitive data is released and might be reused…
				ctx->blockIndex = 0;
				}
				}
				return (0);
				cemUnsubmitted Not Done Inline Actions This 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 easier, as we just xor&encrypt.
				*/
				while (length) {
				const uint8_t *ptr;

				if (length < sizeof(ctx->block)) {
				explicit_bzero(ctx->staging_block, sizeof(ctx->staging_block));
				bcopy(data, ctx->staging_block, length);
				ptr = ctx->staging_block;
				length = 0;
				cemUnsubmitted Not Done Inline Actions This path seems totally broken. cem: This path seems totally broken.
				sefAuthorUnsubmitted Done Inline Actions How so? sef: How so?
				cemUnsubmitted Not Done Inline Actions 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"`. cem: E.g., ``` AES_CBC_MAC_Update("a", length=1); AES_CBC_MAC_Update("b", length=1)…
				sefAuthorUnsubmitted Done Inline Actions 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. sef: Obviously I hadn't thought about it being called that way. The only one I'd thought about was…
				} else {
				ptr = data;
				length -= sizeof(ctx->block);
				}
				xor_and_encrypt(ctx, ptr, ctx->block);
				}
				return (0);
				}

				void
				AES_CBC_MAC_Final(uint8_t buf, struct aes_cbc_mac_ctx ctx)
				{
				uint8_t s0[CCM_CBC_BLOCK_LEN];

				bzero(s0, sizeof(s0));
				cemUnsubmitted Done Inline Actions This should definitely not be `explicit_bzero` cem: This should definitely not be `explicit_bzero`
				s0[0] = ((15 - ctx->nonceLength) & 0xff) - 1;
				cemUnsubmitted Not Done Inline Actions What is the `0xff` for? `15 - nonceLength` must be in [2, 8]. cem: What is the `0xff` for? `15 - nonceLength` must be in [2, 8].
				sefAuthorUnsubmitted Done Inline Actions See above. sef: See above.
				bcopy(ctx->nonce, s0 + 1, ctx->nonceLength);
				cemUnsubmitted Not Done Inline Actions spaces around the `+` please cem: spaces around the `+` please
				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));
				}
				cemUnsubmitted Not Done Inline Actions redundant return cem: redundant return
				cemUnsubmitted Not Done Inline Actions Open 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 Actions Ok, 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.)