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
>>
>> [1] https://github.com/OP-TEE/optee_os/pull/2862