On Thu, 7 Mar 2019 at 14:29, Jens Wiklander jens.wiklander@linaro.org wrote:
Hi Jerome,
On Thu, Mar 7, 2019 at 1:39 PM Jerome Forissier jerome.forissier@linaro.org wrote:
Hi Jens,
On Thu, 7 Mar 2019 at 11:21, Jens Wiklander jens.wiklander@linaro.org
wrote:
Hi,
We have an ongoing action to integrate mbedtls in OP-TEE to be used in OP-TEE core via the <crypto/crypto.h> interface.
Then there's also a library developed by NXP to take advantage of CAAM for crypto acceleration.
The way we're dealing with different crypto algorithms today isn't very modular. I think we can disregard the asymmetric algorithms for now. We've already put the algorithms in different groups, hashes, macs, ciphers (which includes modes like CBC, CTR etc), and authenc (authenticated encryption). This was good enough for our initial libtomcrypt integration, but I think it has been a bit painful for the ongoing mbedtls integration. With CAAM it was solved by using an entire new wrapper layer to work with libtomcrypt. Cedric has shown benchmark that the new wrapping didn't cause any noticeable degradation in performance.
To try a more fine grained modular approach I've converted the authenc algorithms to use a struct crypto_authenc_ops internally to call the needed functions [1].
Why use pointer to functions? We don't need to dynamically select an
implementation at runtime, so we could use a simple "by name" function interface instead I suppose. Basically, make crypto_aes_ccm_init(), crypto_aes_ccm_update_aad(), etc. global functions. We already removed some crypto_ops structure in the past:
https://github.com/OP-TEE/optee_os/compare/f1c1d53...3bbd3ce
Yes, but that was a completely different thing. It was a large struct with more of less one function pointer for each function in <crypto/crypto.h>. No granularity at all. It was as flexible as what we have today in crypto.c, but more complicated since all the function pointers had to be assigned too.
Agreed, and it's why it was removed.
This makes it easier to replace one implementation with another, it's just a matter of calling the correct crypto_aes_gcm_alloc_ctx() function for AES-GCM and the rest of the implementation will be selected automatically. The different functions allocating an AEC-GCM context should probably have different names instead of using the same as in [1], but that doesn't have to be decided right now.
On the contrary, I think a single name is fine and probably easier to
work with (for instance, by searching for the symbol name in vim you get all the possible implementations). Like C++ overloading if you want ;-)
This will make generic programming a bit harder. Currently we need a pointer to a context and "algo" (with [2] only the context is needed) in order to calculate a hash or doing some ciphering. If we only have
crypto_hash_{md5,sha1,sha224,sha256,sha384,sha512}_{alloc_ctx,init,update,final,free_ctx,copy_state}() then you'll need to pass ctx and a pointer to for instance crypto_hash_sha256_update() for some other function to do some generic hashing. As you can see it can quickly become quite difficult. How many places do we have in the source code that would suffer from this? I don't know, perhaps none.
It could be annoying indeed. Just consider how KDF derivation is implemented, for instance -- tee_cryp_concat_kdf(hash_id, ...) calls crypto_hash_{init,update,final}() and would rather not care about the actual value of hash_id.
Then there's the aspect with hardware accelerators. Let's say that it's probed during boot and algorithms that can be accelerated overrides the software implementations in some way. With what I'm proposing it's just a single function that need to be replaced for each algorithm, while with an interface you're talking about I'm not sure what will be the best approach.
That's a good point.
I'm fine with the function pointers, then ;-)
Cheers,