blake2s: there is no use in exposing blake2s_ctx, or is there?

[cobratbq]

Is there any need to expose blake2s_ctx to the caller?

The blake2s function in firmware is a one-shot function that initializes the context and also finalizes the hash computation and stack allocations are (practically) free. Any "control" over memory-use seems to be illusionary, i.e. only the location on the stack is different. If there is no value to derive from the contents of blake2s_ctx, it seems that latest-/shortest-possible allocation is beneficial for memory-availability.

The only use I can think of, is inspecting blake2s_ctx after calling blake2s to inspect changes to the context, which is only useful for debugging the Blake2s hash-function implementation.

tkey-libs/libcommon/blake2s.c

Lines 12 to 20 in 4c57f87

	int blake2s(void *out, unsigned long outlen, const void *key,
	unsigned long keylen, const void *in, unsigned long inlen,
	blake2s_ctx *ctx)
	{
	fw_blake2s_p const fw_blake2s =
	(fw_blake2s_p) * (volatile uint32_t *)TK1_MMIO_TK1_BLAKE2S;
	return fw_blake2s(out, outlen, key, keylen, in, inlen, ctx);
	}

[mchack-work]

I think you're right. It's mostly historical baggage. However, it could be of some use in the future, perhaps...

The point of passing ctx to the firmware function fw_blakes2() is to have control of where in the memory space it's allocated. For instance if you have some special protected memory you can allocate ctx there and pass it to blake2s() instead of having the function just put it on the ordinary stack.

We had an idea to include a very small memory with hardware protection for firmware use only, FW_RAM. Since the Unique Device Secret lives for a short while in ctx explicitly allocated the context from the new FW_RAM and then passed the pointer to this function instead of letting blake2s() put it on the stack.

Later we found that we could fit in a bigger FW_RAM on the FPGA and increased it to 2 kByte! Imagine! Vast amount of space! This meant we could put the entire firmware stack in FW_RAM which probably makes passing ctx to blake2s() a moot point, at least from the firmware's point of view.

However, since we don't have any context switch, the use of the firmware's blake2s() function from an application is not protected in FW_RAM, only the firmware's own use of blake2s(). Such a context switch and hiding sensitive data would have been interesting but was deemed to complex. The FPGA is very nearly full and we wanted other features. Also, the security implications of introducing a context switch wasn't entirely clear.

We also discussed having something similar, outside of the normal RAM, for use in applications, which could have other electrical properties than static RAM. In that case it would probably make sense to keep sensitive application data, including the ctx, there. This idea didn't make it to the released design but we might want to revisit that idea eventually.... There's a lot we want to do with a bigger FPGA!

[mchack-work]

Of course, now that we have quite a number of TKeys with this signature call, the fw_blake2s(out, outlen, key, keylen, in, inlen, ctx) part, in the firmware it isn't easily changed without breaking a lot of existing hardware.

We'll have to live with it.

[mchack-work]

Or did you mean that we should change just the tkey-libs function? And have that function allocate ctx itself? Yeah, we could do that. Didn't understand if that's what you meant.

[cobratbq]

The tkey-libs function. At this point, requiring it as a parameter does not add any benefit, AFAICT. Having it in local call stack space means you know exactly how the memory is used. So, there is no real urgency or benefit to even set up an execution monitor on that memory.

To make it ridiculously concrete: construct a local variable at line 18, pass that on for blake2s_ctx into fw_blake2s, instead of the function parameter. (You'll already understand, but this should remove any miscommunication.)

I am not an expert at C programming, so I may miss some technical detail, but it seems like there is no added benefit to passing it into function call of blake2s (line 12). I am aware that it's nitpicking, but I do like to keep things simple. In this case it means developers don't even need to be aware of the blake2s_ctx struct itself. The struct type can be local to the implementation too, i.e. not in the header.

[cobratbq]

About #38 (comment), confirmed that I am not criticizing use of fw-RAM, if mostly that I do not know anything about prior choices and I haven't looked into resources/capacity. I comment on the simplification of blake2s in tkey-libs, as is obvious now. I prefer simpler myself, usually. Passing on some "working memory" as blake2s_ctx into the firmware function call seems very much acceptable.

FWIW, 128 KiB will be on the smaller side for some applications. With a few dependencies, I passed 60 KiB. I do not expect issues, but the binary (a C application) can become substantial.