Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace crypto ops #1931

Merged
merged 12 commits into from
Nov 14, 2017
Merged

Replace crypto ops #1931

merged 12 commits into from
Nov 14, 2017

Conversation

jenswi-linaro
Copy link
Contributor

This takes care of replacing the crypto_ops struct as requested in #1908

But this doesn't solve the problem of supplying AES-GCM from an OP-TEE internal implementation and AES-CCM from LTC. How can that be done? core/tee/tee_svc_cryp.c expects crypto_authenc_*() functions so those need to be provided somewhere, but not in LTC as crypto_authenc_*() implementation will use OP-TEE internal AES-GCM implementation further on.

One way forward could be:

  1. Specify crypto_aes_gcm_*() and crypto_aes_ccm_*() in core/include/crypto/aes-{g,c}cm.h
  2. Move the crypto_authenc_*() implementation to core/crypto/crypto.c, implemented by using the new functions above.

I suppose the best would be to solve the crypto_authenc_*() problem in this pull request to avoid messing with crypto interface little by little (or more truthfully much by much).

Another thing is the core/include/tee/tee_cryp_provider.h file, it should probably be renamed to core/include/crypto/api.h or something similar.

Should I move forward as proposed above? Comments/thoughts?

@jforissier
Copy link
Contributor

TL;DR: agreed with everything,

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

Indeed, replacing crypto_ops is not sufficient in itself to address the mixed GCM/CCM implementation issue, but it is a step in the right direction anyway (unless someone chimes in and says we need to keep function pointers, but why would we need that?). We do need a lower-level API than the current crypto_ops, and I am perfectly fine with your suggestion (1) and (2). I think it belongs in this PR because then you can rebase 1908 on this.

I also agree with renaming core/include/tee/tee_cryp_provider.h. I would suggest core/include/crypto/crypto.h rather than core/include/crypto/api.h, because it's more meaningful when the full path is not mentioned ("crypto.h" rather than "api.h"). Not that it matters much however.

@jenswi-linaro
Copy link
Contributor Author

Sounds good. :-)

core/include/crypto/crypto.h, it is then. I'll squash and apply the tag first to make this first step clear.

@jenswi-linaro
Copy link
Contributor Author

Tag applied

@jforissier
Copy link
Contributor

@jbech-linaro correct for the first two, but the last one (the faq) is OK I think.

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me. few minor comments.
Acked-by Etienne Carriere <etienne.carriere@linaro.org>

TEE_Result crypto_hash_get_ctx_size(uint32_t algo __unused,
size_t *size __unused)
{
return TEE_ERROR_NOT_SUPPORTED;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TEE_ERROR_NOT_IMPLEMENTED instead, for consistency ?
Only hash function use here TEE_ERROR_NOT_SUPPORTED. Others uses TEE_ERROR_NOT_IMPLEMENTED.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll replace all those.

/*
* Key allocation functions
* Allocate the bignum's inside a key structure.
* TEE core will later use bignum.free().
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: s/bignum.free/crypto_bignum_free/

TEE_ALG_HMAC_SHA256,
&rpmb_ctx->hash_ctx_size))) {
(crypto_mac_get_ctx_size(TEE_ALG_HMAC_SHA256,
&rpmb_ctx->hash_ctx_size))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: can remove the parentheses around crypto_mac_get_ctx_size().

@jbech-linaro
Copy link
Contributor

@jbech-linaro correct for the first two, but the last one (the faq) is OK I think.

You're probably right, but is it really an abstraction layer anymore? Maybe?

@jenswi-linaro
Copy link
Contributor Author

Yes, it's still an abstraction layer.

TEE_Result crypto_acipher_ecc_shared_secret(struct ecc_keypair *private_key,
struct ecc_public_key *public_key,
void *secret,
unsigned long *secret_len);

/*
* Verifies a SHA-256 hash, doesn't require tee_cryp_init() to be called in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/tee_cryp_init()/crypto_init()/

#include <kernel/panic.h>
#include <tee/tee_cryp_provider.h>

#if !defined(_CFG_CRYPTO_WITH_HASH)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about making the symbols __weak instead? It looks like we could get rid of all the #ifdef-ery in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking the same first, but the problem is that some functions are supposed to be provided in groups and that's not possible with weak functions.

@jenswi-linaro
Copy link
Contributor Author

Addressed comments, moved crypto_authenc_*() as agreed.

@jforissier
Copy link
Contributor

Section "How to add a new crypto implementation" in documentation/crypto.md needs updating, too.
Other than that LGTM, thanks.

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

* @algo: algorithm identifier (TEE_ALG_*)
*/


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: remove 1 empty line

@@ -28,6 +28,7 @@

#include <assert.h>
#include <compiler.h>
#include <crypto/crypto.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: seems useless here.

@@ -25,6 +25,7 @@
* POSSIBILITY OF SUCH DAMAGE.
*/

#include <crypto/crypto.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: seems useless here.

@@ -26,6 +26,7 @@
*/

#include <assert.h>
#include <crypto/crypto.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: seems useless here.

* Implementation of crypto_authenc_*() due to divided implementation,
* AES-GCM is normally using an OP-TEE internal implementation while
* AES-CCM is using the LibTomCrypt implementation.
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this comment

TEE_Result crypto_hash_get_ctx_size(uint32_t algo __unused,
size_t *size __unused)
{
return TEE_ERROR_NOT_SUPPORTED;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/TEE_ERROR_NOT_SUPPORTED/TEE_ERROR_NOT_IMPLEMENTED/

@jenswi-linaro
Copy link
Contributor Author

Update

the top level of your new directory.
- Although not all pointers in **crypto_ops** need to be defined, all are
required for compliance to the GlobalPlatform specification.
- Although not crypto families need to be defines, all are required for
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"... not all crypto families ..." ?

#if defined(CFG_CRYPTO_GCM)
case TEE_ALG_AES_GCM:
if (*dst_len < src_len)
return TEE_ERROR_SHORT_BUFFER;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should set *dst_len = src_len; before returning TEE_ERROR_SHORT_BUFFER.
here and below, and in few below functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@jenswi-linaro
Copy link
Contributor Author

Update

@jforissier
Copy link
Contributor

Tested-by: Jerome Forissier <jerome.forissier@linaro.org> (HiKey960, GP)

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trailing minor comments.
Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>

/* Add entropy to PRNG entropy pool. */
TEE_Result crypto_rng_add_entropy(const uint8_t *inbuf, size_t len);

/* To read random data from PRNG implementation. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: trailing spaces inside comment.

#include <tee/tee_cryp_provider.h>
#include <crypto/crypto.h>
#include <crypto/aes-gcm.h>
#include <crypto/aes-ccm.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: sort

TEE_Result res = TEE_ERROR_BAD_STATE;
struct tee_ccm_state *ccm = ctx;
int ltc_res;
uint8_t dst_tag[TEE_xCM_TAG_MAX_LENGTH];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: s/TEE_xCM_TAG_MAX_LENGTH/TEE_CCM_TAG_MAX_LENGTH/ here and line 2665.

(definition of TEE_xCM_TAG_MAX_LENGTH could be removed.

#if defined(CFG_CRYPTO_GCM)
struct tee_gcm_state *gcm;
#endif
struct tee_gcm_state *gcm = ctx;
int ltc_res;
uint8_t dst_tag[TEE_xCM_TAG_MAX_LENGTH];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: s/TEE_xCM_TAG_MAX_LENGTH/TEE_GCM_TAG_MAX_LENGTH/ here and line 2823.

@jenswi-linaro
Copy link
Contributor Author

Addressed comments
@jforissier and @etienne-lms are you still OK with this? If so I'll squash and rebase.

@etienne-lms
Copy link
Contributor

fine for me

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Adds crypto_hash_get_ctx_size(), crypto_hash_init(),
crypto_hash_update() and crypto_hash_final() replacing struct hash_ops
in crypto_ops.

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Adds crypto_cipher_get_ctx_size(), crypto_cipher_init(),
crypto_cipher_update(), crypto_cipher_final() and
crypto_cipher_get_block_size() replacing struct cipher_ops in
crypto_ops.

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Adds mac_cipher_get_ctx_size(), mac_cipher_init(), mac_cipher_update()
and  mac_cipher_final() replacing struct mac_ops in crypto_ops.

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Adds crypto_authenc_*() replacing struct authenc_ops in crypto_ops.

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Adds crypto_bignum_*() replacing struct bignum_ops in crypto_ops.

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Adds crypto_acipher_*() replacing struct acipher_ops in crypto_ops.

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Removes struct crypto_ops and adds crypto_init()

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Renames core/include/tee/tee_cryp_provider.h to
core/include/crypto/crypto.h

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
* Moves crypto_authenc_*() from LTC to core/crypto/crypto.c
* Defines <crypto/aes-gcm.h> and <crypto/aes-ccm.h> and
  implements the functions in
  core/lib/libtomcrypt/src/tee_ltc_provider.c based on the
  old implementations of crypto_authenc_*().

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Update documentation of the Crypto API with the new <crypto/crypto.h>
replacing the old crypto_ops based API.

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Fixes the build error:
kern.ld:153: undefined symbol `__asan_shadow_start' referenced in expression

Tested-by: Jerome Forissier <jerome.forissier@linaro.org> (HiKey960, GP)
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
@jenswi-linaro
Copy link
Contributor Author

Squashed, rebased and tags applied.

@jforissier jforissier merged commit 11a9c2b into OP-TEE:master Nov 14, 2017
@jenswi-linaro jenswi-linaro deleted the rem_crypto_ops branch November 14, 2017 12:48
@jforissier jforissier mentioned this pull request Nov 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants