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]. 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.
With the approach in [1] applied on hashes, macs and ciphers too I think it would be much easier to integrate any crypto library. For paging we'll still have special interfaces to do SHA-256 or AES-GCM, this is needed in order minimize the dependency graph for unpaged code and also make sure that it's always callable from abort mode (doesn't cause deadlock or whatever).
What do you think? Is this too much, too little?
Cheers, Jens
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
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 ;-)
With the approach in [1] applied on hashes, macs and ciphers too I think it would be much easier to integrate any crypto library. For paging we'll still have special interfaces to do SHA-256 or AES-GCM, this is needed in order minimize the dependency graph for unpaged code and also make sure that it's always callable from abort mode (doesn't cause deadlock or whatever).
What do you think? Is this too much, too little?
A finer-grained interface certainly is a good idea. I agree it will likely help reduce the size and complexity of wrappers around other implementations.
Cheers,
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.
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.
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.
With the approach in [1] applied on hashes, macs and ciphers too I think it would be much easier to integrate any crypto library. For paging we'll still have special interfaces to do SHA-256 or AES-GCM, this is needed in order minimize the dependency graph for unpaged code and also make sure that it's always callable from abort mode (doesn't cause deadlock or whatever).
What do you think? Is this too much, too little?
A finer-grained interface certainly is a good idea. I agree it will likely help reduce the size and complexity of wrappers around other implementations.
Sounds good.
Cheers, Jens [2] https://github.com/OP-TEE/optee_os/pull/2857
Cheers,
Jerome
Cheers, Jens
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,
On Thu, Mar 7, 2019 at 2:51 PM Jerome Forissier jerome.forissier@linaro.org wrote:
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 ;-)
That's good. I expect that this will not take much time to convert. If I don't hear any objections I think I'll have time to care of this tomorrow since I'm a bit blocked on other tasks for the moment. The resulting PR will of course have a usual review at github so feel free to comment there.
Cheers, Jens
Cheers,
Jerome
With the approach in [1] applied on hashes, macs and ciphers too I think it would be much easier to integrate any crypto library. For paging we'll still have special interfaces to do SHA-256 or AES-GCM, this is needed in order minimize the dependency graph for unpaged code and also make sure that it's always callable from abort mode (doesn't cause deadlock or whatever).
What do you think? Is this too much, too little?
A finer-grained interface certainly is a good idea. I agree it will likely help reduce the size and complexity of wrappers around other implementations.
Sounds good.
Cheers, Jens [2] https://github.com/OP-TEE/optee_os/pull/2857
Cheers,
Jerome
Cheers, Jens
On Thu, 7 Mar 2019 at 17:08, Jens Wiklander jens.wiklander@linaro.org wrote:
On Thu, Mar 7, 2019 at 2:51 PM Jerome Forissier jerome.forissier@linaro.org wrote:
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 ;-)
That's good. I expect that this will not take much time to convert. If I don't hear any objections I think I'll have time to care of this tomorrow since I'm a bit blocked on other tasks for the moment. The resulting PR will of course have a usual review at github so feel free to comment there.
I didn't participate to the discussion but this looks fine to me. Thanks,
etienne
Cheers, Jens
Cheers,
Jerome
With the approach in [1] applied on hashes, macs and ciphers too I think it would be much easier to integrate any crypto library. For paging we'll still have special interfaces to do SHA-256 or AES-GCM, this is needed in order minimize the dependency graph for unpaged code and also make sure that it's always callable from abort mode (doesn't cause deadlock or whatever).
What do you think? Is this too much, too little?
A finer-grained interface certainly is a good idea. I agree it will likely help reduce the size and complexity of wrappers around other implementations.
Sounds good.
Cheers, Jens [2] https://github.com/OP-TEE/optee_os/pull/2857
Cheers,
Jerome
Cheers, Jens
-
Etienne Carriere -
Jens Wiklander -
Jerome Forissier