crypto: lib - implement library version of AES in CFB mode

Implement AES in CFB mode using the existing, mostly constant-time generic AES library implementation. This will be used by the TPM code to encrypt communications with TPM hardware, which is often a discrete component connected using sniffable wires or traces.

While a CFB template does exist, using a skcipher is a major pain for non-performance critical synchronous crypto where the algorithm is known at compile time and the data is in contiguous buffers with valid kernel virtual addresses.

Tested-by James Bottomley <[email protected]>

Reviewed-by James Bottomley <[email protected]>

Link https://lore.kernel.org/all/[email protected]/ open here

Signed-off-by Ard Biesheuvel <[email protected]>

---
v1 was sent out by James and is archived at the URL above

v2:
- add test cases and kerneldoc comments
- add memzero_explicit() calls to wipe the keystream buffers
- add module exports
- add James's Tb/Rb

 include/crypto/aes.h |   5 +
 lib/crypto/Kconfig   |   5 +
 lib/crypto/Makefile  |   3 +
 lib/crypto/aescfb.c  | 257 ++++++++++++++++++++
 4 files changed, 270 insertions(+)

Quoted 23 lines · Ard Biesheuvel

Implement AES in CFB mode using the existing, mostly constant-time generic AES library implementation. This will be used by the TPM code to encrypt communications with TPM hardware, which is often a discrete component connected using sniffable wires or traces.

While a CFB template does exist, using a skcipher is a major pain for non-performance critical synchronous crypto where the algorithm is known at compile time and the data is in contiguous buffers with valid kernel virtual addresses.

Tested-by James Bottomley <[email protected]>

Reviewed-by James Bottomley <[email protected]>

Link https://lore.kernel.org/all/[email protected]/ open here

Signed-off-by Ard Biesheuvel <[email protected]>

---
v1 was sent out by James and is archived at the URL above

v2:
- add test cases and kerneldoc comments
- add memzero_explicit() calls to wipe the keystream buffers
- add module exports
- add James's Tb/Rb

Quoted 5 lines

 include/crypto/aes.h |   5 +
 lib/crypto/Kconfig   |   5 +
 lib/crypto/Makefile  |   3 +
 lib/crypto/aescfb.c  | 257 ++++++++++++++++++++
 4 files changed, 270 insertions(+)

Could we remove the crypto/cfb.c implementation after this work is complete?

We would still not have any in-tree users of cfb(aes) or any other cfb(*), so in that sense, yes.

However, skciphers can be called from user space, and we also rely on this template for the extended testing of the various cfb() hardware implementations that we have in the tree.

So the answer is no, I suppose. I would like to simplify it a bit, though - it is a bit more complicated than it needs to be.

Quoted 8 lines · Ard Biesheuvel

We would still not have any in-tree users of cfb(aes) or any other cfb(*), so in that sense, yes.

However, skciphers can be called from user space, and we also rely on this template for the extended testing of the various cfb() hardware implementations that we have in the tree.

Quoted 2 lines

So the answer is no, I suppose. I would like to simplify it a bit, though - it is a bit more complicated than it needs to be.

Could we hold onto this for a little bit? I'd like to finally remove crypto_cipher, and in doing so I will add a virtual address interface (i.e., not sg) to skcipher like we do with scomp and shash.

Does that mean you are bringing back blkcipher? I think that would the right thing to do tbh, although it might make sense to enhance skcipher (and aead) to support this.

Could we perhaps update struct skcipher_request so it can describe virtually mapped address ranges, but permit this only for synchronous implementations? Then, we could update the skcipher walker code to produce a single walk step covering the entire range, and just use the provided virtual addresses directly, rather than going through a mapping interface?

Quoted 4 lines · Ard Biesheuvel

Does that mean you are bringing back blkcipher? I think that would the right thing to do tbh, although it might make sense to enhance skcipher (and aead) to support this.

I haven't gone into that kind of detail yet but my first impression is that it would be the analogue of shash and skcipher would simply wrap around it just like ahash wraps around shash.

Quoted 6 lines

Could we perhaps update struct skcipher_request so it can describe virtually mapped address ranges, but permit this only for synchronous implementations? Then, we could update the skcipher walker code to produce a single walk step covering the entire range, and just use the provided virtual addresses directly, rather than going through a mapping interface?

Since skcipher doesn't actually need to carry any state with it I'd like to avoid having an skcipher_request at all.

Doesn't that depend on the implementation? It might have a >0 size request context size, no? Or do we just allocate that on the stack?

Quoted 3 lines · Ard Biesheuvel

Doesn't that depend on the implementation? It might have a >0 size request context size, no? Or do we just allocate that on the stack?

Do you have a concrete example of something that needs this? Is this a temporary scratch buffer used only during computation?

Every call to crypto_skcipher_set_reqsize(), no?

Quoted 2 lines · Ard Biesheuvel

Every call to crypto_skcipher_set_reqsize(), no?

We'd only convert the software implementations. But you're right there does seem to be a few users such as aria that demand a large amount of temp space. I'd be tempted to just leave them on skcipher.

In other cases such as ctr we can easily put the IV on the stack.

That way, the implementations can remain the same,

Quoted 2 lines · Ard Biesheuvel

That way, the implementations can remain the same,

That's like doing a house renovation and keeping the scaffold around forever :)

Yes I agree that it would save a little bit of work for now but all the implementations would have to carry this unnecessary walking code with them forever.

With a setup like ahash/shash the walking code disappears totally from the underlying implementations.

So we are basically going back to ablkcipher/blkcipher then? How about aead?

Quoted 2 lines · Ard Biesheuvel

So we are basically going back to ablkcipher/blkcipher then? How about aead?

No I just dug up the old blkcipher code and it's based on SGs just like skcipher.

I went back to the beginning of git and we've only ever had an SG-based encryption interface. This would be the very first time that we've had this in the Crypto API.

Do we have any potential users for such an AEAD interface?

So what use case is the driver for this sync skcipher change? And how

will this work with existing templates? Do they all have to implement two flavors now?

Quoted 2 lines · Ard Biesheuvel

So what use case is the driver for this sync skcipher change? And how

The main reason I wanted to do this is because I'd like to get rid of crypto_cipher. I'm planning on replacing the underlying simple ciphers with their ECB equivalent.

Quoted 2 lines

will this work with existing templates? Do they all have to implement two flavors now?

Let's say we're calling this vkcipher. Because the existing skcipher templates should continue to work with an underlying vkcipher algorithm, I won't be adding any vkcipher template unless there is a specific use-case, such as CFB here.

But I will do the common ones like CBC/CTR.