Replace crypto ops #1931

jenswi-linaro · 2017-11-10T11:20:02Z

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:

Specify crypto_aes_gcm_*() and crypto_aes_ccm_*() in core/include/crypto/aes-{g,c}cm.h
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 · 2017-11-10T13:19:29Z

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 · 2017-11-10T13:32:25Z

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 · 2017-11-10T13:34:32Z

Tag applied

jbech-linaro · 2017-11-10T15:35:30Z

So, besides the code we have some more places that needs to be fixed due to this

jforissier · 2017-11-10T15:45:39Z

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

etienne-lms

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

etienne-lms · 2017-11-10T15:36:17Z

core/crypto/crypto.c

+TEE_Result crypto_hash_get_ctx_size(uint32_t algo __unused,
+				    size_t *size __unused)
+{
+	return TEE_ERROR_NOT_SUPPORTED;


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

Thanks, I'll replace all those.

etienne-lms · 2017-11-10T15:42:54Z

core/include/tee/tee_cryp_provider.h

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


minor: s/bignum.free/crypto_bignum_free/

etienne-lms · 2017-11-10T15:48:53Z

core/tee/tee_rpmb_fs.c

-			    TEE_ALG_HMAC_SHA256,
-			    &rpmb_ctx->hash_ctx_size))) {
+		    (crypto_mac_get_ctx_size(TEE_ALG_HMAC_SHA256,
+					     &rpmb_ctx->hash_ctx_size))) {


minor: can remove the parentheses around crypto_mac_get_ctx_size().

jbech-linaro · 2017-11-10T16:11:04Z

@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 · 2017-11-10T16:17:17Z

Yes, it's still an abstraction layer.

jforissier · 2017-11-10T16:34:11Z

core/include/tee/tee_cryp_provider.h

+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


s/tee_cryp_init()/crypto_init()/

jforissier · 2017-11-10T17:15:31Z

core/crypto/crypto.c

+#include <kernel/panic.h>
+#include <tee/tee_cryp_provider.h>
+
+#if !defined(_CFG_CRYPTO_WITH_HASH)


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

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 · 2017-11-13T08:13:34Z

Addressed comments, moved crypto_authenc_*() as agreed.

jforissier · 2017-11-13T09:37:07Z

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>

etienne-lms · 2017-11-13T09:53:09Z

core/include/crypto/crypto.h

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


minor: remove 1 empty line

etienne-lms · 2017-11-13T09:56:46Z

core/arch/arm/kernel/user_ta.c

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

 #include <assert.h>
 #include <compiler.h>
+#include <crypto/crypto.h>


minor: seems useless here.

etienne-lms · 2017-11-13T09:57:18Z

core/arch/arm/tee/init.c

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

+#include <crypto/crypto.h>


minor: seems useless here.

etienne-lms · 2017-11-13T09:59:05Z

core/tee/tee_ree_fs.c

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

 #include <assert.h>
+#include <crypto/crypto.h>


minor: seems useless here.

etienne-lms · 2017-11-13T10:06:13Z

documentation/crypto.md

+ * 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.
+ */


remove this comment

etienne-lms · 2017-11-13T10:06:55Z

documentation/crypto.md

+TEE_Result crypto_hash_get_ctx_size(uint32_t algo __unused,
+                                    size_t *size __unused)
+{
+        return TEE_ERROR_NOT_SUPPORTED;


s/TEE_ERROR_NOT_SUPPORTED/TEE_ERROR_NOT_IMPLEMENTED/

jenswi-linaro · 2017-11-13T10:45:12Z

Update

etienne-lms · 2017-11-13T11:00:13Z

documentation/crypto.md

-  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


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

etienne-lms · 2017-11-13T11:04:27Z

core/crypto/crypto.c

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


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

jenswi-linaro · 2017-11-13T12:17:07Z

Update

jforissier · 2017-11-13T12:54:22Z

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

etienne-lms

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

etienne-lms · 2017-11-13T12:24:46Z

core/include/crypto/crypto.h

+/* 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.	*/


minor: trailing spaces inside comment.

etienne-lms · 2017-11-13T12:25:21Z

core/lib/libtomcrypt/src/tee_ltc_provider.c

-#include <tee/tee_cryp_provider.h>
+#include <crypto/crypto.h>
+#include <crypto/aes-gcm.h>
+#include <crypto/aes-ccm.h>


minor: sort

etienne-lms · 2017-11-13T14:29:00Z

core/lib/libtomcrypt/src/tee_ltc_provider.c

+	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];


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.

etienne-lms · 2017-11-13T14:30:08Z

core/lib/libtomcrypt/src/tee_ltc_provider.c

-#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];


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

jenswi-linaro · 2017-11-13T17:35:34Z

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

etienne-lms · 2017-11-14T10:01:40Z

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 · 2017-11-14T11:16:37Z

Squashed, rebased and tags applied.

jenswi-linaro force-pushed the rem_crypto_ops branch from 8a45563 to 070c65f Compare November 10, 2017 13:34

etienne-lms reviewed Nov 10, 2017

View reviewed changes

jforissier reviewed Nov 10, 2017

View reviewed changes

etienne-lms reviewed Nov 13, 2017

View reviewed changes

jenswi-linaro added 9 commits November 14, 2017 12:09

core: crypto: fix crypto_rng_read() comment

323f59e

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>

core: remove struct crypto_ops

cbccb60

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>

core: rename to <crypto/crypto.h>

8759aec

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>

jenswi-linaro added 3 commits November 14, 2017 12:11

jenswi-linaro force-pushed the rem_crypto_ops branch from cabda6d to 04c628d Compare November 14, 2017 11:16

jforissier merged commit 11a9c2b into OP-TEE:master Nov 14, 2017

jenswi-linaro deleted the rem_crypto_ops branch November 14, 2017 12:48

jforissier mentioned this pull request Nov 17, 2017

Gcm #1908

Closed

Replace crypto ops #1931

Replace crypto ops #1931

Conversation

jenswi-linaro commented Nov 10, 2017

jforissier commented Nov 10, 2017

jenswi-linaro commented Nov 10, 2017

jenswi-linaro commented Nov 10, 2017

jbech-linaro commented Nov 10, 2017

jforissier commented Nov 10, 2017

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbech-linaro commented Nov 10, 2017

jenswi-linaro commented Nov 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented Nov 13, 2017

jforissier commented Nov 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented Nov 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented Nov 13, 2017

jforissier commented Nov 13, 2017

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented Nov 13, 2017

etienne-lms commented Nov 14, 2017

jenswi-linaro commented Nov 14, 2017