g: broaden dcrypto mutex safety

Holding the mutex just around the dcrypto_call is not enough: dcrypto
instruction memory content might change in presence of multiple calling
tasks.

Switching to broad acquire/release pattern instead.

Note to sub-projects: pair your dcrypto_init(_and_lock) w/ matching dcrypto_unlock

BUG=none
BRANCH=cr50
TEST=tcg_tests pass

Change-Id: Idb7f2d79ce533db95cab51d89e3869ecf9f3d499
Reviewed-on: https://chromium-review.googlesource.com/535916
Commit-Ready: Marius Schilder <mschilder@chromium.org>
Tested-by: Marius Schilder <mschilder@chromium.org>
Reviewed-by: Marius Schilder <mschilder@chromium.org>
Reviewed-by: Vadim Bendebury <vbendeb@chromium.org>
Reviewed-by: Nadim Taha <ntaha@chromium.org>

4 files changed, 55 insertions, 33 deletions

diff --git a/chip/g/dcrypto/bn_hw.c b/chip/g/dcrypto/bn_hw.c
index ee5607ea64..5e5ca2dab8 100644
--- a/chip/g/dcrypto/bn_hw.c
+++ b/chip/g/dcrypto/bn_hw.c
@@ -1052,7 +1052,7 @@ void bn_mont_modexp_asm(struct LITE_BIGNUM *output,
 	struct DMEM_montmul *montmul;
 
 	/* Initialize hardware; load code page. */
-	dcrypto_init();
+	dcrypto_init_and_lock();
 	dcrypto_imem_load(0, IMEM_dcrypto, ARRAY_SIZE(IMEM_dcrypto));
 
 	/* Point to DMEM. */
@@ -1152,6 +1152,8 @@ void bn_mont_modexp_asm(struct LITE_BIGNUM *output,
 	memcpy(output->d, montmul->out, bn_size(output));
 
 	(void) (result == 0); /* end of errorcode propagation */
+
+	dcrypto_unlock();
 }
 
 /*
@@ -1182,7 +1184,6 @@ static void dcrypto_ecc_init(void)
 	struct DMEM_ecc *pEcc =
 		(struct DMEM_ecc *)GREG32_ADDR(CRYPTO, DMEM_DUMMY);
 
-	dcrypto_init();
 	dcrypto_imem_load(0, IMEM_dcrypto, ARRAY_SIZE(IMEM_dcrypto));
 
 	pEcc->pK = DMEM_INDEX(pEcc, k);
@@ -1229,6 +1230,7 @@ int dcrypto_p256_ecdsa_sign(const p256_int *key, const p256_int *message,
 	struct DMEM_ecc *pEcc =
 		(struct DMEM_ecc *)GREG32_ADDR(CRYPTO, DMEM_DUMMY);
 
+	dcrypto_init_and_lock();
 	dcrypto_ecc_init();
 	result = dcrypto_call(CF_p256init_adr);
 
@@ -1255,6 +1257,7 @@ int dcrypto_p256_ecdsa_sign(const p256_int *key, const p256_int *message,
 	cp8w(&pEcc->d, &pEcc->rnd);
 	cp8w(&pEcc->k, &pEcc->rnd);
 
+	dcrypto_unlock();
 	return result == 0;
 }
 
@@ -1264,6 +1267,7 @@ int dcrypto_p256_base_point_mul(const p256_int *k, p256_int *x, p256_int *y)
 	struct DMEM_ecc *pEcc =
 		(struct DMEM_ecc *)GREG32_ADDR(CRYPTO, DMEM_DUMMY);
 
+	dcrypto_init_and_lock();
 	dcrypto_ecc_init();
 	result = dcrypto_call(CF_p256init_adr);
 
@@ -1280,6 +1284,7 @@ int dcrypto_p256_base_point_mul(const p256_int *k, p256_int *x, p256_int *y)
 	/* Wipe d */
 	cp8w(&pEcc->d, &pEcc->rnd);
 
+	dcrypto_unlock();
 	return result == 0;
 }
 
@@ -1291,6 +1296,7 @@ int dcrypto_p256_point_mul(const p256_int *k,
 	struct DMEM_ecc *pEcc =
 		(struct DMEM_ecc *)GREG32_ADDR(CRYPTO, DMEM_DUMMY);
 
+	dcrypto_init_and_lock();
 	dcrypto_ecc_init();
 	result = dcrypto_call(CF_p256init_adr);
 
@@ -1311,6 +1317,7 @@ int dcrypto_p256_point_mul(const p256_int *k,
 	cp8w(&pEcc->x, &pEcc->rnd);
 	cp8w(&pEcc->y, &pEcc->rnd);
 
+	dcrypto_unlock();
 	return result == 0;
 }
 
@@ -1322,6 +1329,7 @@ int dcrypto_p256_ecdsa_verify(const p256_int *key_x, const p256_int *key_y,
 	struct DMEM_ecc *pEcc =
 		(struct DMEM_ecc *)GREG32_ADDR(CRYPTO, DMEM_DUMMY);
 
+	dcrypto_init_and_lock();
 	dcrypto_ecc_init();
 	result = dcrypto_call(CF_p256init_adr);
 
@@ -1336,6 +1344,7 @@ int dcrypto_p256_ecdsa_verify(const p256_int *key_x, const p256_int *key_y,
 	for (i = 0; i < 8; ++i)
 		result |= (pEcc->rnd.a[i] ^ r->a[i]);
 
+	dcrypto_unlock();
 	return result == 0;
 }
 
@@ -1345,6 +1354,7 @@ int dcrypto_p256_is_valid_point(const p256_int *x, const p256_int *y)
 	struct DMEM_ecc *pEcc =
 		(struct DMEM_ecc *)GREG32_ADDR(CRYPTO, DMEM_DUMMY);
 
+	dcrypto_init_and_lock();
 	dcrypto_ecc_init();
 	result = dcrypto_call(CF_p256init_adr);
 
@@ -1356,5 +1366,6 @@ int dcrypto_p256_is_valid_point(const p256_int *x, const p256_int *y)
 	for (i = 0; i < 8; ++i)
 		result |= (pEcc->r.a[i] ^ pEcc->s.a[i]);
 
+	dcrypto_unlock();
 	return result == 0;
 }
diff --git a/chip/g/dcrypto/dcrypto_runtime.c b/chip/g/dcrypto/dcrypto_runtime.c
index 65fac9d96e..3775ffa13a 100644
--- a/chip/g/dcrypto/dcrypto_runtime.c
+++ b/chip/g/dcrypto/dcrypto_runtime.c
@@ -10,15 +10,18 @@
 #define DMEM_NUM_WORDS 1024
 #define IMEM_NUM_WORDS 1024
 
+static struct mutex dcrypto_mutex;
 static volatile task_id_t my_task_id;
-
 static int dcrypto_is_initialized;
 
-void dcrypto_init(void)
+void dcrypto_init_and_lock(void)
 {
 	int i;
 	volatile uint32_t *ptr;
 
+	mutex_lock(&dcrypto_mutex);
+	my_task_id = task_get_current();
+
 	if (dcrypto_is_initialized)
 		return;
 
@@ -59,17 +62,18 @@ void dcrypto_init(void)
 	dcrypto_is_initialized = 1;
 }
 
+void dcrypto_unlock(void)
+{
+	mutex_unlock(&dcrypto_mutex);
+}
+
 #define DCRYPTO_CALL_TIMEOUT_US  (700 * 1000)
 #define TASK_EVENT_DCRYPTO_DONE  TASK_EVENT_CUSTOM(1)
 
 uint32_t dcrypto_call(uint32_t adr)
 {
-	static struct mutex cmd_mutex;
 	uint32_t event;
 
-	mutex_lock(&cmd_mutex);
-	my_task_id = task_get_current();
-
 	do {
 		/* Reset all the status bits. */
 		GREG32(CRYPTO, INT_STATE) = -1;
@@ -79,8 +83,6 @@ uint32_t dcrypto_call(uint32_t adr)
 
 	event = task_wait_event_mask(TASK_EVENT_DCRYPTO_DONE,
 				     DCRYPTO_CALL_TIMEOUT_US);
-	mutex_unlock(&cmd_mutex);
-
 	/* TODO(ngm): switch return value to an enum. */
 	switch (event) {
 	case TASK_EVENT_DCRYPTO_DONE:
diff --git a/chip/g/dcrypto/dcrypto_sha512.c b/chip/g/dcrypto/dcrypto_sha512.c
index a309604f94..10776659d7 100644
--- a/chip/g/dcrypto/dcrypto_sha512.c
+++ b/chip/g/dcrypto/dcrypto_sha512.c
@@ -490,8 +490,6 @@ static void copy_words(const void *in, uint32_t *dst, size_t nwords)
 
 static void dcrypto_SHA512_setup(void)
 {
-	/* Note both of these cache and are nop if already done */
-	dcrypto_init();
 	dcrypto_imem_load(0, IMEM_dcrypto, ARRAY_SIZE(IMEM_dcrypto));
 }
 
@@ -548,36 +546,38 @@ static void dcrypto_SHA512_update(LITE_SHA512_CTX *ctx, const void *data,
 
 	ctx->count += len;
 
+	dcrypto_init_and_lock();
 	dcrypto_SHA512_setup();
 
 	/* Take fast path for 32-bit aligned 1KB inputs */
 	if (i == 0 && len == 1024 && (((intptr_t) data) & 3) == 0) {
 		dcrypto_SHA512_Transform(ctx, (const uint32_t *) data, 8 * 32);
-		return;
-	}
-
-	if (len <= sizeof(ctx->buf) - i) {
-		memcpy(d, p, len);
-		if (len == sizeof(ctx->buf) - i) {
-			dcrypto_SHA512_Transform(ctx, (uint32_t *) (ctx->buf),
-						 32);
-		}
 	} else {
-		memcpy(d, p, sizeof(ctx->buf) - i);
-		dcrypto_SHA512_Transform(ctx, (uint32_t *) (ctx->buf), 32);
-		d = ctx->buf;
-		len -= (sizeof(ctx->buf) - i);
-		p += (sizeof(ctx->buf) - i);
-		while (len >= sizeof(ctx->buf)) {
-			memcpy(d, p, sizeof(ctx->buf));
-			p += sizeof(ctx->buf);
-			len -= sizeof(ctx->buf);
+		if (len <= sizeof(ctx->buf) - i) {
+			memcpy(d, p, len);
+			if (len == sizeof(ctx->buf) - i) {
+				dcrypto_SHA512_Transform(
+				    ctx, (uint32_t *) (ctx->buf), 32);
+			}
+		} else {
+			memcpy(d, p, sizeof(ctx->buf) - i);
 			dcrypto_SHA512_Transform(ctx, (uint32_t *) (ctx->buf),
 						 32);
+			d = ctx->buf;
+			len -= (sizeof(ctx->buf) - i);
+			p += (sizeof(ctx->buf) - i);
+			while (len >= sizeof(ctx->buf)) {
+				memcpy(d, p, sizeof(ctx->buf));
+				p += sizeof(ctx->buf);
+				len -= sizeof(ctx->buf);
+				dcrypto_SHA512_Transform(
+				    ctx, (uint32_t *) (ctx->buf), 32);
+			}
+			/* Leave remainder in ctx->buf */
+			memcpy(d, p, len);
 		}
-		/* Leave remainder in ctx->buf */
-		memcpy(d, p, len);
 	}
+	dcrypto_unlock();
 }
 
 static const uint8_t *dcrypto_SHA512_final(LITE_SHA512_CTX *ctx)
@@ -589,6 +589,7 @@ static const uint8_t *dcrypto_SHA512_final(LITE_SHA512_CTX *ctx)
 	*p++ = 0x80;
 	i++;
 
+	dcrypto_init_and_lock();
 	dcrypto_SHA512_setup();
 
 	if (i > sizeof(ctx->buf) - 16) {
@@ -622,6 +623,7 @@ static const uint8_t *dcrypto_SHA512_final(LITE_SHA512_CTX *ctx)
 		*p++ = (uint8_t)(tmp >> 0);
 	}
 
+	dcrypto_unlock();
 	return ctx->buf;
 }
 
diff --git a/chip/g/dcrypto/internal.h b/chip/g/dcrypto/internal.h
index e151a78bbc..bb88707e0f 100644
--- a/chip/g/dcrypto/internal.h
+++ b/chip/g/dcrypto/internal.h
@@ -111,8 +111,15 @@ int dcrypto_p256_is_valid_point(const p256_int *x, const p256_int *y)
 
 /*
  * Runtime.
+ *
+ * Note dcrypto_init_and_lock grabs a mutex and dcrypto_unlock releases it.
+ * Do not use dcrypto_call, dcrypto_imem_load or dcrypto_dmem_load w/o holding
+ * the mutex.
  */
-void dcrypto_init(void);
+void dcrypto_init_and_lock(void);
+/* TODO(mschilder): remove once sub projects are updated to avoid build breakages */
+#define crypto_init dcrypto_init_and_lock
+void dcrypto_unlock(void);
 uint32_t dcrypto_call(uint32_t adr) __attribute__((warn_unused_result));
 void dcrypto_imem_load(size_t offset, const uint32_t *opcodes,
 		       size_t n_opcodes);