[PATCH v2 0/6] crypto/fsl: add RNG support

[PATCH v2 0/6] crypto/fsl: add RNG support

[Michael Walle]

First, improve the compatibility on newer Era CAAMs. These introduced new
version registers. Secondly, add RNG support for the CAAM. This way we get
random number generator support for EFI for free and KASLR will work with
ARM64 kernels booted with bootefi.

Changes since v1:
 - instantiate RNG with prediction resistance
 - add annotation for caam_get_era()
 - fix all prints to match u-boots status lines
 - remove superfluous #ifdef CONFIG_DM_RNG
 - changed kconfig help text

Michael Walle (6):
  crypto/fsl: make SEC%u status line consistent
  crypto/fsl: export caam_get_era()
  crypto/fsl: support newer SEC modules
  crypto/fsl: don't regenerate secure keys
  crypto/fsl: instantiate the RNG with prediciton resistance
  crypto/fsl: add RNG support

 drivers/crypto/fsl/Kconfig   |  14 +++++
 drivers/crypto/fsl/Makefile  |   1 +
 drivers/crypto/fsl/desc.h    |   2 +
 drivers/crypto/fsl/jobdesc.c |  27 ++++++++-
 drivers/crypto/fsl/jobdesc.h |   8 ++-
 drivers/crypto/fsl/jr.c      | 110 +++++++++++++++++++++++++++++------
 drivers/crypto/fsl/rng.c     |  84 ++++++++++++++++++++++++++
 drivers/crypto/fsl/sec.c     |  10 +++-
 include/fsl_sec.h            |  61 +++++++++++++++----
 9 files changed, 282 insertions(+), 35 deletions(-)
 create mode 100644 drivers/crypto/fsl/rng.c

-- 
2.20.1

[PATCH v2 1/6] crypto/fsl: make SEC%u status line consistent

[Michael Walle]

Align the status line with all the other output in U-Boot.

Before the change:
DDR    3.9 GiB (DDR3, 32-bit, CL=11, ECC on)
SEC0: RNG instantiated
WDT:   Started with servicing (60s timeout)

After the change:
DDR    3.9 GiB (DDR3, 32-bit, CL=11, ECC on)
SEC0:  RNG instantiated
WDT:   Started with servicing (60s timeout)

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/crypto/fsl/jr.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/crypto/fsl/jr.c b/drivers/crypto/fsl/jr.c
index e2d9216cfc..bbdbcb8e58 100644
--- a/drivers/crypto/fsl/jr.c
+++ b/drivers/crypto/fsl/jr.c
@@ -478,8 +478,8 @@ static int instantiate_rng(uint8_t sec_idx)
 		ret = run_descriptor_jr_idx(desc, sec_idx);
 
 		if (ret)
-			printf("RNG: Instantiation failed with error 0x%x\n",
-			       ret);
+			printf("SEC%u:  RNG4 SH%d instantiation failed with error 0x%x\n",
+			       sec_idx, sh_idx, ret);
 
 		rdsta_val = sec_in32(&rng->rdsta) & RNG_STATE_HANDLE_MASK;
 		if (!(rdsta_val & (1 << sh_idx))) {
@@ -569,7 +569,7 @@ static int rng_init(uint8_t sec_idx)
 		ret = instantiate_rng(sec_idx);
 	} while ((ret == -1) && (ent_delay < RTSDCTL_ENT_DLY_MAX));
 	if (ret) {
-		printf("RNG: Failed to instantiate RNG\n");
+		printf("SEC%u:  Failed to instantiate RNG\n", sec_idx);
 		return ret;
 	}
 
@@ -592,7 +592,7 @@ int sec_init_idx(uint8_t sec_idx)
 #endif
 
 	if (!(sec_idx < CONFIG_SYS_FSL_MAX_NUM_OF_SEC)) {
-		printf("SEC initialization failed\n");
+		printf("SEC%u:  initialization failed\n", sec_idx);
 		return -1;
 	}
 
@@ -640,7 +640,7 @@ int sec_init_idx(uint8_t sec_idx)
 
 	ret = jr_init(sec_idx);
 	if (ret < 0) {
-		printf("SEC initialization failed\n");
+		printf("SEC%u:  initialization failed\n", sec_idx);
 		return -1;
 	}
 
@@ -654,10 +654,10 @@ int sec_init_idx(uint8_t sec_idx)
 #ifndef CONFIG_SPL_BUILD
 	if (get_rng_vid(sec_idx) >= 4) {
 		if (rng_init(sec_idx) < 0) {
-			printf("SEC%u: RNG instantiation failed\n", sec_idx);
+			printf("SEC%u:  RNG instantiation failed\n", sec_idx);
 			return -1;
 		}
-		printf("SEC%u: RNG instantiated\n", sec_idx);
+		printf("SEC%u:  RNG instantiated\n", sec_idx);
 	}
 #endif
 	return ret;
-- 
2.20.1

[PATCH v2 2/6] crypto/fsl: export caam_get_era()

[Michael Walle]

We need the era in other modules, too. For example, to get the RNG
version.

Signed-off-by: Michael Walle <michael@walle.cc>
Reviewed-by: Horia Geant? <horia.geanta@nxp.com>
---
 drivers/crypto/fsl/sec.c | 10 +++++++++-
 include/fsl_sec.h        |  2 ++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/fsl/sec.c b/drivers/crypto/fsl/sec.c
index a2c0bfaf44..18fc0c66e2 100644
--- a/drivers/crypto/fsl/sec.c
+++ b/drivers/crypto/fsl/sec.c
@@ -98,7 +98,15 @@ void fdt_fixup_crypto_node(void *blob, int sec_rev)
 		       fdt_strerror(err));
 }
 #elif CONFIG_SYS_FSL_SEC_COMPAT >= 4  /* SEC4 */
-static u8 caam_get_era(void)
+/**
+ * caam_get_era() - fetch the CAAM's era
+ *
+ * The SEC module povides an "Era" which can be used to differentiate
+ * between different revisions.
+ *
+ * Return: era of the SEC.
+ */
+u8 caam_get_era(void)
 {
 	static const struct {
 		u16 ip_id;
diff --git a/include/fsl_sec.h b/include/fsl_sec.h
index c0d2c7e866..2ebb75c9b2 100644
--- a/include/fsl_sec.h
+++ b/include/fsl_sec.h
@@ -316,6 +316,8 @@ int blob_dek(const u8 *src, u8 *dst, u8 len);
 int sec_init_idx(uint8_t);
 #endif
 int sec_init(void);
+
+u8 caam_get_era(void);
 #endif
 
 #endif /* __FSL_SEC_H */
-- 
2.20.1

[PATCH v2 3/6] crypto/fsl: support newer SEC modules

[Michael Walle]

Since Era 10, the version registers changed. Add the version registers
and use them on newer modules.

Signed-off-by: Michael Walle <michael@walle.cc>
Reviewed-by: Horia Geant? <horia.geanta@nxp.com>
---
 drivers/crypto/fsl/jr.c | 12 ++++++++--
 include/fsl_sec.h       | 51 +++++++++++++++++++++++++++++++++++------
 2 files changed, 54 insertions(+), 9 deletions(-)

diff --git a/drivers/crypto/fsl/jr.c b/drivers/crypto/fsl/jr.c
index bbdbcb8e58..5275c50e8b 100644
--- a/drivers/crypto/fsl/jr.c
+++ b/drivers/crypto/fsl/jr.c
@@ -498,9 +498,17 @@ static int instantiate_rng(uint8_t sec_idx)
 static u8 get_rng_vid(uint8_t sec_idx)
 {
 	ccsr_sec_t *sec = (void *)SEC_ADDR(sec_idx);
-	u32 cha_vid = sec_in32(&sec->chavid_ls);
+	u8 vid;
 
-	return (cha_vid & SEC_CHAVID_RNG_LS_MASK) >> SEC_CHAVID_LS_RNG_SHIFT;
+	if (caam_get_era() < 10) {
+		vid = (sec_in32(&sec->chavid_ls) & SEC_CHAVID_RNG_LS_MASK)
+		       >> SEC_CHAVID_LS_RNG_SHIFT;
+	} else {
+		vid = (sec_in32(&sec->vreg.rng) & CHA_VER_VID_MASK)
+		       >> CHA_VER_VID_SHIFT;
+	}
+
+	return vid;
 }
 
 /*
diff --git a/include/fsl_sec.h b/include/fsl_sec.h
index 2ebb75c9b2..8dce0bbb1b 100644
--- a/include/fsl_sec.h
+++ b/include/fsl_sec.h
@@ -73,6 +73,41 @@ struct rng4tst {
 	u32 rsvd2[15];
 };
 
+/* Version registers (Era 10+) */
+struct version_regs {
+	u32 crca;	/* CRCA_VERSION */
+	u32 afha;	/* AFHA_VERSION */
+	u32 kfha;	/* KFHA_VERSION */
+	u32 pkha;	/* PKHA_VERSION */
+	u32 aesa;	/* AESA_VERSION */
+	u32 mdha;	/* MDHA_VERSION */
+	u32 desa;	/* DESA_VERSION */
+	u32 snw8a;	/* SNW8A_VERSION */
+	u32 snw9a;	/* SNW9A_VERSION */
+	u32 zuce;	/* ZUCE_VERSION */
+	u32 zuca;	/* ZUCA_VERSION */
+	u32 ccha;	/* CCHA_VERSION */
+	u32 ptha;	/* PTHA_VERSION */
+	u32 rng;	/* RNG_VERSION */
+	u32 trng;	/* TRNG_VERSION */
+	u32 aaha;	/* AAHA_VERSION */
+	u32 rsvd[10];
+	u32 sr;		/* SR_VERSION */
+	u32 dma;	/* DMA_VERSION */
+	u32 ai;		/* AI_VERSION */
+	u32 qi;		/* QI_VERSION */
+	u32 jr;		/* JR_VERSION */
+	u32 deco;	/* DECO_VERSION */
+};
+
+#define CHA_VER_NUM_MASK	0x000000ff
+#define CHA_VER_MISC_SHIFT	8
+#define CHA_VER_MISC_MASK	0x0000ff00
+#define CHA_VER_REV_SHIFT	16
+#define CHA_VER_REV_MASK	0x00ff0000
+#define CHA_VER_VID_SHIFT	24
+#define CHA_VER_VID_MASK	0xff000000
+
 typedef struct ccsr_sec {
 	u32	res0;
 	u32	mcfgr;		/* Master CFG Register */
@@ -98,17 +133,19 @@ typedef struct ccsr_sec {
 	u32	drr;		/* DECO Reset Register */
 	u8	res5[0x4d8];
 	struct rng4tst rng;	/* RNG Registers */
-	u8	res6[0x8a0];
+	u8	res6[0x780];
+	struct version_regs vreg; /* version registers since era 10 */
+	u8	res7[0xa0];
 	u32	crnr_ms;	/* CHA Revision Number Register, MS */
 	u32	crnr_ls;	/* CHA Revision Number Register, LS */
 	u32	ctpr_ms;	/* Compile Time Parameters Register, MS */
 	u32	ctpr_ls;	/* Compile Time Parameters Register, LS */
-	u8	res7[0x10];
+	u8	res8[0x10];
 	u32	far_ms;		/* Fault Address Register, MS */
 	u32	far_ls;		/* Fault Address Register, LS */
 	u32	falr;		/* Fault Address LIODN Register */
 	u32	fadr;		/* Fault Address Detail Register */
-	u8	res8[0x4];
+	u8	res9[0x4];
 	u32	csta;		/* CAAM Status Register */
 	u32	smpart;		/* Secure Memory Partition Parameters */
 	u32	smvid;		/* Secure Memory Version ID */
@@ -121,16 +158,16 @@ typedef struct ccsr_sec {
 	u32	secvid_ms;	/* SEC Version ID Register, MS */
 	u32	secvid_ls;	/* SEC Version ID Register, LS */
 #if defined(CONFIG_FSL_LSCH2) || defined(CONFIG_FSL_LSCH3)
-	u8	res9[0x6f020];
+	u8	res10[0x6f020];
 #else
-	u8	res9[0x6020];
+	u8	res10[0x6020];
 #endif
 	u32	qilcr_ms;	/* Queue Interface LIODN CFG Register, MS */
 	u32	qilcr_ls;	/* Queue Interface LIODN CFG Register, LS */
 #if defined(CONFIG_FSL_LSCH2) || defined(CONFIG_FSL_LSCH3)
-	u8	res10[0x8ffd8];
+	u8	res11[0x8ffd8];
 #else
-	u8	res10[0x8fd8];
+	u8	res11[0x8fd8];
 #endif
 } ccsr_sec_t;
 
-- 
2.20.1

[PATCH v2 4/6] crypto/fsl: don't regenerate secure keys

[Michael Walle]

The secure keys (TDKEK, JDKEK, TDSK) can only be generated once after a
POR. Otherwise the RNG4 will throw an error.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/crypto/fsl/jobdesc.c | 4 ++--
 drivers/crypto/fsl/jobdesc.h | 2 +-
 drivers/crypto/fsl/jr.c      | 9 +++++----
 include/fsl_sec.h            | 1 +
 4 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/crypto/fsl/jobdesc.c b/drivers/crypto/fsl/jobdesc.c
index 2f35e0c90b..6102e9c06b 100644
--- a/drivers/crypto/fsl/jobdesc.c
+++ b/drivers/crypto/fsl/jobdesc.c
@@ -258,7 +258,7 @@ void inline_cnstr_jobdesc_blob_decap(uint32_t *desc, uint8_t *key_idnfr,
  * Descriptor to instantiate RNG State Handle 0 in normal mode and
  * load the JDKEK, TDKEK and TDSK registers
  */
-void inline_cnstr_jobdesc_rng_instantiation(uint32_t *desc, int handle)
+void inline_cnstr_jobdesc_rng_instantiation(u32 *desc, int handle, int do_sk)
 {
 	u32 *jump_cmd;
 
@@ -269,7 +269,7 @@ void inline_cnstr_jobdesc_rng_instantiation(uint32_t *desc, int handle)
 			(handle << OP_ALG_AAI_SHIFT) | OP_ALG_AS_INIT);
 
 	/* For SH0, Secure Keys must be generated as well */
-	if (handle == 0) {
+	if (!handle && do_sk) {
 		/* wait for done */
 		jump_cmd = append_jump(desc, JUMP_CLASS_CLASS1);
 		set_jump_tgt_here(desc, jump_cmd);
diff --git a/drivers/crypto/fsl/jobdesc.h b/drivers/crypto/fsl/jobdesc.h
index d782c46b9d..14b2a119d7 100644
--- a/drivers/crypto/fsl/jobdesc.h
+++ b/drivers/crypto/fsl/jobdesc.h
@@ -39,7 +39,7 @@ void inline_cnstr_jobdesc_blob_decap(uint32_t *desc, uint8_t *key_idnfr,
 				     uint8_t *enc_blob, uint8_t *plain_txt,
 				     uint32_t out_sz);
 
-void inline_cnstr_jobdesc_rng_instantiation(uint32_t *desc, int handle);
+void inline_cnstr_jobdesc_rng_instantiation(u32 *desc, int handle, int do_sk);
 
 void inline_cnstr_jobdesc_pkha_rsaexp(uint32_t *desc,
 				      struct pk_in_params *pkin, uint8_t *out,
diff --git a/drivers/crypto/fsl/jr.c b/drivers/crypto/fsl/jr.c
index 5275c50e8b..42865a6cd7 100644
--- a/drivers/crypto/fsl/jr.c
+++ b/drivers/crypto/fsl/jr.c
@@ -446,7 +446,7 @@ int sec_reset(void)
 	return sec_reset_idx(0);
 }
 #ifndef CONFIG_SPL_BUILD
-static int instantiate_rng(uint8_t sec_idx)
+static int instantiate_rng(u8 sec_idx, int gen_sk)
 {
 	u32 *desc;
 	u32 rdsta_val;
@@ -470,7 +470,7 @@ static int instantiate_rng(uint8_t sec_idx)
 		if (rdsta_val & (1 << sh_idx))
 			continue;
 
-		inline_cnstr_jobdesc_rng_instantiation(desc, sh_idx);
+		inline_cnstr_jobdesc_rng_instantiation(desc, sh_idx, gen_sk);
 		size = roundup(sizeof(uint32_t) * 6, ARCH_DMA_MINALIGN);
 		flush_dcache_range((unsigned long)desc,
 				   (unsigned long)desc + size);
@@ -546,12 +546,13 @@ static void kick_trng(int ent_delay, uint8_t sec_idx)
 
 static int rng_init(uint8_t sec_idx)
 {
-	int ret, ent_delay = RTSDCTL_ENT_DLY_MIN;
+	int ret, gen_sk, ent_delay = RTSDCTL_ENT_DLY_MIN;
 	ccsr_sec_t __iomem *sec = (ccsr_sec_t __iomem *)SEC_ADDR(sec_idx);
 	struct rng4tst __iomem *rng =
 			(struct rng4tst __iomem *)&sec->rng;
 	u32 inst_handles;
 
+	gen_sk = !(sec_in32(&rng->rdsta) & RDSTA_SKVN);
 	do {
 		inst_handles = sec_in32(&rng->rdsta) & RNG_STATE_HANDLE_MASK;
 
@@ -574,7 +575,7 @@ static int rng_init(uint8_t sec_idx)
 		 * interval, leading to a sucessful initialization of
 		 * the RNG.
 		 */
-		ret = instantiate_rng(sec_idx);
+		ret = instantiate_rng(sec_idx, gen_sk);
 	} while ((ret == -1) && (ent_delay < RTSDCTL_ENT_DLY_MAX));
 	if (ret) {
 		printf("SEC%u:  Failed to instantiate RNG\n", sec_idx);
diff --git a/include/fsl_sec.h b/include/fsl_sec.h
index 8dce0bbb1b..64b8751f2d 100644
--- a/include/fsl_sec.h
+++ b/include/fsl_sec.h
@@ -69,6 +69,7 @@ struct rng4tst {
 #define RNG_STATE1_HANDLE_INSTANTIATED	0x00000002
 #define RNG_STATE_HANDLE_MASK	\
 	(RNG_STATE0_HANDLE_INSTANTIATED | RNG_STATE1_HANDLE_INSTANTIATED)
+#define RDSTA_SKVN 0x40000000
 	u32 rdsta;		/*RNG DRNG Status Register*/
 	u32 rsvd2[15];
 };
-- 
2.20.1

[PATCH v2 5/6] crypto/fsl: instantiate the RNG with prediciton resistance

[Michael Walle]

If it is already instantiated tear it down first and then reinstanciate
it again with prediction resistance.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/crypto/fsl/desc.h    |  2 ++
 drivers/crypto/fsl/jobdesc.c | 12 ++++++-
 drivers/crypto/fsl/jobdesc.h |  2 ++
 drivers/crypto/fsl/jr.c      | 66 ++++++++++++++++++++++++++++++++----
 include/fsl_sec.h            |  7 ++--
 5 files changed, 78 insertions(+), 11 deletions(-)

diff --git a/drivers/crypto/fsl/desc.h b/drivers/crypto/fsl/desc.h
index 11ad506829..3589e6ea02 100644
--- a/drivers/crypto/fsl/desc.h
+++ b/drivers/crypto/fsl/desc.h
@@ -520,6 +520,8 @@
 #define OP_ALG_ICV_OFF		(0 << OP_ALG_ICV_SHIFT)
 #define OP_ALG_ICV_ON		(1 << OP_ALG_ICV_SHIFT)
 
+#define OP_ALG_PR_ON		0x02
+
 #define OP_ALG_DIR_SHIFT	0
 #define OP_ALG_DIR_MASK		1
 #define OP_ALG_DECRYPT		0
diff --git a/drivers/crypto/fsl/jobdesc.c b/drivers/crypto/fsl/jobdesc.c
index 6102e9c06b..d9554c550b 100644
--- a/drivers/crypto/fsl/jobdesc.c
+++ b/drivers/crypto/fsl/jobdesc.c
@@ -266,7 +266,8 @@ void inline_cnstr_jobdesc_rng_instantiation(u32 *desc, int handle, int do_sk)
 
 	/* INIT RNG in non-test mode */
 	append_operation(desc, OP_TYPE_CLASS1_ALG | OP_ALG_ALGSEL_RNG |
-			(handle << OP_ALG_AAI_SHIFT) | OP_ALG_AS_INIT);
+			 (handle << OP_ALG_AAI_SHIFT) | OP_ALG_AS_INIT |
+			 OP_ALG_PR_ON);
 
 	/* For SH0, Secure Keys must be generated as well */
 	if (!handle && do_sk) {
@@ -286,6 +287,15 @@ void inline_cnstr_jobdesc_rng_instantiation(u32 *desc, int handle, int do_sk)
 	}
 }
 
+/* Descriptor for deinstantiation of the RNG block. */
+void inline_cnstr_jobdesc_rng_deinstantiation(u32 *desc, int handle)
+{
+	init_job_desc(desc, 0);
+
+	append_operation(desc, OP_TYPE_CLASS1_ALG | OP_ALG_ALGSEL_RNG |
+			 (handle << OP_ALG_AAI_SHIFT) | OP_ALG_AS_INITFINAL);
+}
+
 /* Change key size to bytes form bits in calling function*/
 void inline_cnstr_jobdesc_pkha_rsaexp(uint32_t *desc,
 				      struct pk_in_params *pkin, uint8_t *out,
diff --git a/drivers/crypto/fsl/jobdesc.h b/drivers/crypto/fsl/jobdesc.h
index 14b2a119d7..5185ddd535 100644
--- a/drivers/crypto/fsl/jobdesc.h
+++ b/drivers/crypto/fsl/jobdesc.h
@@ -41,6 +41,8 @@ void inline_cnstr_jobdesc_blob_decap(uint32_t *desc, uint8_t *key_idnfr,
 
 void inline_cnstr_jobdesc_rng_instantiation(u32 *desc, int handle, int do_sk);
 
+void inline_cnstr_jobdesc_rng_deinstantiation(u32 *desc, int handle);
+
 void inline_cnstr_jobdesc_pkha_rsaexp(uint32_t *desc,
 				      struct pk_in_params *pkin, uint8_t *out,
 				      uint32_t out_siz);
diff --git a/drivers/crypto/fsl/jr.c b/drivers/crypto/fsl/jr.c
index 42865a6cd7..14f9227b37 100644
--- a/drivers/crypto/fsl/jr.c
+++ b/drivers/crypto/fsl/jr.c
@@ -446,6 +446,51 @@ int sec_reset(void)
 	return sec_reset_idx(0);
 }
 #ifndef CONFIG_SPL_BUILD
+static int deinstantiate_rng(u8 sec_idx, int state_handle_mask)
+{
+	u32 *desc;
+	int sh_idx, ret = 0;
+	int desc_size = sizeof(u32) * 3;
+
+	desc = memalign(ARCH_DMA_MINALIGN, desc_size);
+	if (!desc) {
+		debug("cannot allocate RNG init descriptor memory\n");
+		return -ENOMEM;
+	}
+
+	for (sh_idx = 0; sh_idx < RNG4_MAX_HANDLES; sh_idx++) {
+		/*
+		 * If the corresponding bit is set, then it means the state
+		 * handle was initialized by us, and thus it needs to be
+		 * deinitialized as well
+		 */
+
+		if (state_handle_mask & RDSTA_IF(sh_idx)) {
+			/*
+			 * Create the descriptor for deinstantating this state
+			 * handle.
+			 */
+			inline_cnstr_jobdesc_rng_deinstantiation(desc, sh_idx);
+			flush_dcache_range((unsigned long)desc,
+					   (unsigned long)desc + desc_size);
+
+			ret = run_descriptor_jr_idx(desc, sec_idx);
+			if (ret) {
+				printf("SEC%u:  RNG4 SH%d deinstantiation failed with error 0x%x\n",
+				       sec_idx, sh_idx, ret);
+				ret = -EIO;
+				break;
+			}
+
+			printf("SEC%u:  Deinstantiated RNG4 SH%d\n",
+			       sec_idx, sh_idx);
+		}
+	}
+
+	free(desc);
+	return ret;
+}
+
 static int instantiate_rng(u8 sec_idx, int gen_sk)
 {
 	u32 *desc;
@@ -466,9 +511,18 @@ static int instantiate_rng(u8 sec_idx, int gen_sk)
 		 * If the corresponding bit is set, this state handle
 		 * was initialized by somebody else, so it's left alone.
 		 */
-		rdsta_val = sec_in32(&rng->rdsta) & RNG_STATE_HANDLE_MASK;
-		if (rdsta_val & (1 << sh_idx))
-			continue;
+		rdsta_val = sec_in32(&rng->rdsta);
+		if (rdsta_val & (RDSTA_IF(sh_idx) | RDSTA_PR(sh_idx))) {
+			if (rdsta_val & RDSTA_PR(sh_idx))
+				continue;
+
+			printf("SEC%u:  RNG4 SH%d was instantiated w/o prediction resistance. Tearing it down\n",
+			       sec_idx, sh_idx);
+
+			ret = deinstantiate_rng(sec_idx, RDSTA_IF(sh_idx));
+			if (ret)
+				break;
+		}
 
 		inline_cnstr_jobdesc_rng_instantiation(desc, sh_idx, gen_sk);
 		size = roundup(sizeof(uint32_t) * 6, ARCH_DMA_MINALIGN);
@@ -481,8 +535,8 @@ static int instantiate_rng(u8 sec_idx, int gen_sk)
 			printf("SEC%u:  RNG4 SH%d instantiation failed with error 0x%x\n",
 			       sec_idx, sh_idx, ret);
 
-		rdsta_val = sec_in32(&rng->rdsta) & RNG_STATE_HANDLE_MASK;
-		if (!(rdsta_val & (1 << sh_idx))) {
+		rdsta_val = sec_in32(&rng->rdsta);
+		if (!(rdsta_val & RDSTA_IF(sh_idx))) {
 			free(desc);
 			return -1;
 		}
@@ -554,7 +608,7 @@ static int rng_init(uint8_t sec_idx)
 
 	gen_sk = !(sec_in32(&rng->rdsta) & RDSTA_SKVN);
 	do {
-		inst_handles = sec_in32(&rng->rdsta) & RNG_STATE_HANDLE_MASK;
+		inst_handles = sec_in32(&rng->rdsta) & RDSTA_MASK;
 
 		/*
 		 * If either of the SH's were instantiated by somebody else
diff --git a/include/fsl_sec.h b/include/fsl_sec.h
index 64b8751f2d..1c6f1eb23e 100644
--- a/include/fsl_sec.h
+++ b/include/fsl_sec.h
@@ -65,10 +65,9 @@ struct rng4tst {
 		u32 rtfreqcnt;	/* PRGM=0: freq. count register */
 	};
 	u32 rsvd1[40];
-#define RNG_STATE0_HANDLE_INSTANTIATED	0x00000001
-#define RNG_STATE1_HANDLE_INSTANTIATED	0x00000002
-#define RNG_STATE_HANDLE_MASK	\
-	(RNG_STATE0_HANDLE_INSTANTIATED | RNG_STATE1_HANDLE_INSTANTIATED)
+#define RDSTA_IF(idx) (0x00000001 << (idx))
+#define RDSTA_PR(idx) (0x00000010 << (idx))
+#define RDSTA_MASK (RDSTA_PR(1) | RDSTA_PR(0) | RDSTA_IF(1) | RDSTA_IF(0))
 #define RDSTA_SKVN 0x40000000
 	u32 rdsta;		/*RNG DRNG Status Register*/
 	u32 rsvd2[15];
-- 
2.20.1

[PATCH v2 6/6] crypto/fsl: add RNG support

[Michael Walle]

Register the random number generator with the rng subsystem in u-boot.
This way it can be used by EFI as well as for the 'rng' command.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/crypto/fsl/Kconfig   | 14 ++++++
 drivers/crypto/fsl/Makefile  |  1 +
 drivers/crypto/fsl/jobdesc.c | 10 +++++
 drivers/crypto/fsl/jobdesc.h |  3 ++
 drivers/crypto/fsl/jr.c      |  9 ++++
 drivers/crypto/fsl/rng.c     | 84 ++++++++++++++++++++++++++++++++++++
 6 files changed, 121 insertions(+)
 create mode 100644 drivers/crypto/fsl/rng.c

diff --git a/drivers/crypto/fsl/Kconfig b/drivers/crypto/fsl/Kconfig
index 181a1e5e99..5ed6140da3 100644
--- a/drivers/crypto/fsl/Kconfig
+++ b/drivers/crypto/fsl/Kconfig
@@ -45,3 +45,17 @@ config SYS_FSL_SEC_COMPAT
 
 config SYS_FSL_SEC_LE
 	bool "Little-endian access to Freescale Secure Boot"
+
+if FSL_CAAM
+
+config FSL_CAAM_RNG
+	bool "Enable Random Number Generator support"
+	depends on DM_RNG
+	default y
+	help
+	  Enable support for the hardware based random number generator
+	  module of the CAAM. The random data is fetched from the DRGB
+	  using the prediction resistance flag which means the DRGB is
+	  reseeded from the TRNG every time random data is generated.
+
+endif
diff --git a/drivers/crypto/fsl/Makefile b/drivers/crypto/fsl/Makefile
index cfb36f3bb9..a5e8d38e38 100644
--- a/drivers/crypto/fsl/Makefile
+++ b/drivers/crypto/fsl/Makefile
@@ -7,3 +7,4 @@ obj-$(CONFIG_FSL_CAAM) += jr.o fsl_hash.o jobdesc.o error.o
 obj-$(CONFIG_CMD_BLOB) += fsl_blob.o
 obj-$(CONFIG_CMD_DEKBLOB) += fsl_blob.o
 obj-$(CONFIG_RSA_FREESCALE_EXP) += fsl_rsa.o
+obj-$(CONFIG_FSL_CAAM_RNG) += rng.o
diff --git a/drivers/crypto/fsl/jobdesc.c b/drivers/crypto/fsl/jobdesc.c
index d9554c550b..decde64078 100644
--- a/drivers/crypto/fsl/jobdesc.c
+++ b/drivers/crypto/fsl/jobdesc.c
@@ -296,6 +296,16 @@ void inline_cnstr_jobdesc_rng_deinstantiation(u32 *desc, int handle)
 			 (handle << OP_ALG_AAI_SHIFT) | OP_ALG_AS_INITFINAL);
 }
 
+void inline_cnstr_jobdesc_rng(u32 *desc, void *data_out, u32 size)
+{
+	dma_addr_t dma_data_out = virt_to_phys(data_out);
+
+	init_job_desc(desc, 0);
+	append_operation(desc, OP_ALG_ALGSEL_RNG | OP_TYPE_CLASS1_ALG |
+			 OP_ALG_PR_ON);
+	append_fifo_store(desc, dma_data_out, size, FIFOST_TYPE_RNGSTORE);
+}
+
 /* Change key size to bytes form bits in calling function*/
 void inline_cnstr_jobdesc_pkha_rsaexp(uint32_t *desc,
 				      struct pk_in_params *pkin, uint8_t *out,
diff --git a/drivers/crypto/fsl/jobdesc.h b/drivers/crypto/fsl/jobdesc.h
index 5185ddd535..c4501abd26 100644
--- a/drivers/crypto/fsl/jobdesc.h
+++ b/drivers/crypto/fsl/jobdesc.h
@@ -43,7 +43,10 @@ void inline_cnstr_jobdesc_rng_instantiation(u32 *desc, int handle, int do_sk);
 
 void inline_cnstr_jobdesc_rng_deinstantiation(u32 *desc, int handle);
 
+void inline_cnstr_jobdesc_rng(u32 *desc, void *data_out, u32 size);
+
 void inline_cnstr_jobdesc_pkha_rsaexp(uint32_t *desc,
 				      struct pk_in_params *pkin, uint8_t *out,
 				      uint32_t out_siz);
+
 #endif
diff --git a/drivers/crypto/fsl/jr.c b/drivers/crypto/fsl/jr.c
index 14f9227b37..629a0f30db 100644
--- a/drivers/crypto/fsl/jr.c
+++ b/drivers/crypto/fsl/jr.c
@@ -19,6 +19,7 @@
 #include <asm/cache.h>
 #include <asm/fsl_pamu.h>
 #endif
+#include <dm/lists.h>
 
 #define CIRC_CNT(head, tail, size)	(((head) - (tail)) & (size - 1))
 #define CIRC_SPACE(head, tail, size)	CIRC_CNT((tail), (head) + 1, (size))
@@ -720,6 +721,14 @@ int sec_init_idx(uint8_t sec_idx)
 			printf("SEC%u:  RNG instantiation failed\n", sec_idx);
 			return -1;
 		}
+
+		if (IS_ENABLED(CONFIG_DM_RNG)) {
+			ret = device_bind_driver(NULL, "caam-rng", "caam-rng",
+						 NULL);
+			if (ret)
+				printf("Couldn't bind rng driver (%d)\n", ret);
+		}
+
 		printf("SEC%u:  RNG instantiated\n", sec_idx);
 	}
 #endif
diff --git a/drivers/crypto/fsl/rng.c b/drivers/crypto/fsl/rng.c
new file mode 100644
index 0000000000..3da318d767
--- /dev/null
+++ b/drivers/crypto/fsl/rng.c
@@ -0,0 +1,84 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2020 Michael Walle <michael@walle.cc>
+ *
+ * Driver for Freescale Cryptographic Accelerator and Assurance
+ * Module (CAAM) hardware random number generator.
+ */
+
+#include <asm/cache.h>
+#include <common.h>
+#include <cpu_func.h>
+#include <dm.h>
+#include <rng.h>
+#include "desc_constr.h"
+#include "jobdesc.h"
+#include "jr.h"
+
+#define CAAM_RNG_MAX_FIFO_STORE_SIZE 16
+#define CAAM_RNG_DESC_LEN (3 * CAAM_CMD_SZ + CAAM_PTR_SZ)
+
+struct caam_rng_platdata {
+	u32 desc[CAAM_RNG_DESC_LEN / 4];
+	u8 data[CAAM_RNG_MAX_FIFO_STORE_SIZE] __aligned(ARCH_DMA_MINALIGN);
+};
+
+static int caam_rng_read_one(struct caam_rng_platdata *pdata)
+{
+	int size = CAAM_RNG_MAX_FIFO_STORE_SIZE;
+	int ret;
+
+	ret = run_descriptor_jr(pdata->desc);
+	if (ret < 0)
+		return -EIO;
+
+	invalidate_dcache_range((unsigned long)pdata->data,
+				(unsigned long)pdata->data + size);
+
+	return 0;
+}
+
+static int caam_rng_read(struct udevice *dev, void *data, size_t len)
+{
+	struct caam_rng_platdata *pdata = dev_get_platdata(dev);
+	u8 *buffer = data;
+	size_t size;
+	int ret;
+
+	while (len) {
+		ret = caam_rng_read_one(pdata);
+		if (ret)
+			return ret;
+
+		size = min(len, (size_t)CAAM_RNG_MAX_FIFO_STORE_SIZE);
+
+		memcpy(buffer, pdata->data, len);
+		buffer += size;
+		len -= size;
+	}
+
+	return 0;
+}
+
+static int caam_rng_probe(struct udevice *dev)
+{
+	struct caam_rng_platdata *pdata = dev_get_platdata(dev);
+
+	inline_cnstr_jobdesc_rng(pdata->desc, pdata->data,
+				 CAAM_RNG_MAX_FIFO_STORE_SIZE);
+
+	return 0;
+}
+
+static const struct dm_rng_ops caam_rng_ops = {
+	.read = caam_rng_read,
+};
+
+U_BOOT_DRIVER(caam_rng) = {
+	.name = "caam-rng",
+	.id = UCLASS_RNG,
+	.ops = &caam_rng_ops,
+	.probe = caam_rng_probe,
+	.platdata_auto_alloc_size = sizeof(struct caam_rng_platdata),
+	.flags = DM_FLAG_ALLOC_PRIV_DMA,
+};
-- 
2.20.1

[PATCH v2 5/6] crypto/fsl: instantiate the RNG with prediciton resistance

[Michael Walle]

Am 2020-06-04 17:46, schrieb Michael Walle:
> If it is already instantiated tear it down first and then reinstanciate
> it again with prediction resistance.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/crypto/fsl/desc.h    |  2 ++
>  drivers/crypto/fsl/jobdesc.c | 12 ++++++-
>  drivers/crypto/fsl/jobdesc.h |  2 ++
>  drivers/crypto/fsl/jr.c      | 66 ++++++++++++++++++++++++++++++++----
>  include/fsl_sec.h            |  7 ++--
>  5 files changed, 78 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/crypto/fsl/desc.h b/drivers/crypto/fsl/desc.h
> index 11ad506829..3589e6ea02 100644
> --- a/drivers/crypto/fsl/desc.h
> +++ b/drivers/crypto/fsl/desc.h
> @@ -520,6 +520,8 @@
>  #define OP_ALG_ICV_OFF		(0 << OP_ALG_ICV_SHIFT)
>  #define OP_ALG_ICV_ON		(1 << OP_ALG_ICV_SHIFT)
> 
> +#define OP_ALG_PR_ON		0x02
> +
>  #define OP_ALG_DIR_SHIFT	0
>  #define OP_ALG_DIR_MASK		1
>  #define OP_ALG_DECRYPT		0
> diff --git a/drivers/crypto/fsl/jobdesc.c 
> b/drivers/crypto/fsl/jobdesc.c
> index 6102e9c06b..d9554c550b 100644
> --- a/drivers/crypto/fsl/jobdesc.c
> +++ b/drivers/crypto/fsl/jobdesc.c
> @@ -266,7 +266,8 @@ void inline_cnstr_jobdesc_rng_instantiation(u32
> *desc, int handle, int do_sk)
> 
>  	/* INIT RNG in non-test mode */
>  	append_operation(desc, OP_TYPE_CLASS1_ALG | OP_ALG_ALGSEL_RNG |
> -			(handle << OP_ALG_AAI_SHIFT) | OP_ALG_AS_INIT);
> +			 (handle << OP_ALG_AAI_SHIFT) | OP_ALG_AS_INIT |
> +			 OP_ALG_PR_ON);
> 
>  	/* For SH0, Secure Keys must be generated as well */
>  	if (!handle && do_sk) {
> @@ -286,6 +287,15 @@ void inline_cnstr_jobdesc_rng_instantiation(u32
> *desc, int handle, int do_sk)
>  	}
>  }
> 
> +/* Descriptor for deinstantiation of the RNG block. */
> +void inline_cnstr_jobdesc_rng_deinstantiation(u32 *desc, int handle)
> +{
> +	init_job_desc(desc, 0);
> +
> +	append_operation(desc, OP_TYPE_CLASS1_ALG | OP_ALG_ALGSEL_RNG |
> +			 (handle << OP_ALG_AAI_SHIFT) | OP_ALG_AS_INITFINAL);
> +}
> +
>  /* Change key size to bytes form bits in calling function*/
>  void inline_cnstr_jobdesc_pkha_rsaexp(uint32_t *desc,
>  				      struct pk_in_params *pkin, uint8_t *out,
> diff --git a/drivers/crypto/fsl/jobdesc.h 
> b/drivers/crypto/fsl/jobdesc.h
> index 14b2a119d7..5185ddd535 100644
> --- a/drivers/crypto/fsl/jobdesc.h
> +++ b/drivers/crypto/fsl/jobdesc.h
> @@ -41,6 +41,8 @@ void inline_cnstr_jobdesc_blob_decap(uint32_t *desc,
> uint8_t *key_idnfr,
> 
>  void inline_cnstr_jobdesc_rng_instantiation(u32 *desc, int handle, int 
> do_sk);
> 
> +void inline_cnstr_jobdesc_rng_deinstantiation(u32 *desc, int handle);
> +
>  void inline_cnstr_jobdesc_pkha_rsaexp(uint32_t *desc,
>  				      struct pk_in_params *pkin, uint8_t *out,
>  				      uint32_t out_siz);
> diff --git a/drivers/crypto/fsl/jr.c b/drivers/crypto/fsl/jr.c
> index 42865a6cd7..14f9227b37 100644
> --- a/drivers/crypto/fsl/jr.c
> +++ b/drivers/crypto/fsl/jr.c
> @@ -446,6 +446,51 @@ int sec_reset(void)
>  	return sec_reset_idx(0);
>  }
>  #ifndef CONFIG_SPL_BUILD
> +static int deinstantiate_rng(u8 sec_idx, int state_handle_mask)
> +{
> +	u32 *desc;
> +	int sh_idx, ret = 0;
> +	int desc_size = sizeof(u32) * 3;

This should be 2.

> +
> +	desc = memalign(ARCH_DMA_MINALIGN, desc_size);
> +	if (!desc) {
> +		debug("cannot allocate RNG init descriptor memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	for (sh_idx = 0; sh_idx < RNG4_MAX_HANDLES; sh_idx++) {
> +		/*
> +		 * If the corresponding bit is set, then it means the state
> +		 * handle was initialized by us, and thus it needs to be
> +		 * deinitialized as well
> +		 */
> +
> +		if (state_handle_mask & RDSTA_IF(sh_idx)) {
> +			/*
> +			 * Create the descriptor for deinstantating this state
> +			 * handle.
> +			 */
> +			inline_cnstr_jobdesc_rng_deinstantiation(desc, sh_idx);
> +			flush_dcache_range((unsigned long)desc,
> +					   (unsigned long)desc + desc_size);
> +
> +			ret = run_descriptor_jr_idx(desc, sec_idx);
> +			if (ret) {
> +				printf("SEC%u:  RNG4 SH%d deinstantiation failed with error 
> 0x%x\n",
> +				       sec_idx, sh_idx, ret);
> +				ret = -EIO;
> +				break;
> +			}
> +
> +			printf("SEC%u:  Deinstantiated RNG4 SH%d\n",
> +			       sec_idx, sh_idx);
> +		}
> +	}
> +
> +	free(desc);
> +	return ret;
> +}
> +
>  static int instantiate_rng(u8 sec_idx, int gen_sk)
>  {
>  	u32 *desc;
> @@ -466,9 +511,18 @@ static int instantiate_rng(u8 sec_idx, int gen_sk)
>  		 * If the corresponding bit is set, this state handle
>  		 * was initialized by somebody else, so it's left alone.
>  		 */
> -		rdsta_val = sec_in32(&rng->rdsta) & RNG_STATE_HANDLE_MASK;
> -		if (rdsta_val & (1 << sh_idx))
> -			continue;
> +		rdsta_val = sec_in32(&rng->rdsta);
> +		if (rdsta_val & (RDSTA_IF(sh_idx) | RDSTA_PR(sh_idx))) {
> +			if (rdsta_val & RDSTA_PR(sh_idx))
> +				continue;
> +
> +			printf("SEC%u:  RNG4 SH%d was instantiated w/o prediction
> resistance. Tearing it down\n",
> +			       sec_idx, sh_idx);
> +
> +			ret = deinstantiate_rng(sec_idx, RDSTA_IF(sh_idx));
> +			if (ret)
> +				break;
> +		}
> 
>  		inline_cnstr_jobdesc_rng_instantiation(desc, sh_idx, gen_sk);
>  		size = roundup(sizeof(uint32_t) * 6, ARCH_DMA_MINALIGN);
> @@ -481,8 +535,8 @@ static int instantiate_rng(u8 sec_idx, int gen_sk)
>  			printf("SEC%u:  RNG4 SH%d instantiation failed with error 0x%x\n",
>  			       sec_idx, sh_idx, ret);
> 
> -		rdsta_val = sec_in32(&rng->rdsta) & RNG_STATE_HANDLE_MASK;
> -		if (!(rdsta_val & (1 << sh_idx))) {
> +		rdsta_val = sec_in32(&rng->rdsta);
> +		if (!(rdsta_val & RDSTA_IF(sh_idx))) {
>  			free(desc);
>  			return -1;
>  		}
> @@ -554,7 +608,7 @@ static int rng_init(uint8_t sec_idx)
> 
>  	gen_sk = !(sec_in32(&rng->rdsta) & RDSTA_SKVN);
>  	do {
> -		inst_handles = sec_in32(&rng->rdsta) & RNG_STATE_HANDLE_MASK;
> +		inst_handles = sec_in32(&rng->rdsta) & RDSTA_MASK;
> 
>  		/*
>  		 * If either of the SH's were instantiated by somebody else
> diff --git a/include/fsl_sec.h b/include/fsl_sec.h
> index 64b8751f2d..1c6f1eb23e 100644
> --- a/include/fsl_sec.h
> +++ b/include/fsl_sec.h
> @@ -65,10 +65,9 @@ struct rng4tst {
>  		u32 rtfreqcnt;	/* PRGM=0: freq. count register */
>  	};
>  	u32 rsvd1[40];
> -#define RNG_STATE0_HANDLE_INSTANTIATED	0x00000001
> -#define RNG_STATE1_HANDLE_INSTANTIATED	0x00000002
> -#define RNG_STATE_HANDLE_MASK	\
> -	(RNG_STATE0_HANDLE_INSTANTIATED | RNG_STATE1_HANDLE_INSTANTIATED)
> +#define RDSTA_IF(idx) (0x00000001 << (idx))
> +#define RDSTA_PR(idx) (0x00000010 << (idx))
> +#define RDSTA_MASK (RDSTA_PR(1) | RDSTA_PR(0) | RDSTA_IF(1) | 
> RDSTA_IF(0))
>  #define RDSTA_SKVN 0x40000000
>  	u32 rdsta;		/*RNG DRNG Status Register*/
>  	u32 rsvd2[15];

-- 
-michael

[PATCH v2 1/6] crypto/fsl: make SEC%u status line consistent

[Horia Geantă]

On 6/4/2020 6:48 PM, Michael Walle wrote:
> Align the status line with all the other output in U-Boot.
> 
> Before the change:
> DDR    3.9 GiB (DDR3, 32-bit, CL=11, ECC on)
> SEC0: RNG instantiated
> WDT:   Started with servicing (60s timeout)
> 
> After the change:
> DDR    3.9 GiB (DDR3, 32-bit, CL=11, ECC on)
> SEC0:  RNG instantiated
> WDT:   Started with servicing (60s timeout)
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
Reviewed-by: Horia Geant? <horia.geanta@nxp.com>

Thanks,
Horia

[PATCH v2 4/6] crypto/fsl: don't regenerate secure keys

[Horia Geantă]

On 6/4/2020 6:47 PM, Michael Walle wrote:
> The secure keys (TDKEK, JDKEK, TDSK) can only be generated once after a
> POR. Otherwise the RNG4 will throw an error.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
Reviewed-by: Horia Geant? <horia.geanta@nxp.com>

Did you meet this problem in practice?

Horia

[PATCH v2 5/6] crypto/fsl: instantiate the RNG with prediciton resistance

[Horia Geantă]

On 6/4/2020 6:48 PM, Michael Walle wrote:
> +static int deinstantiate_rng(u8 sec_idx, int state_handle_mask)
> +{
> +	u32 *desc;
> +	int sh_idx, ret = 0;
> +	int desc_size = sizeof(u32) * 3;
As you mentioned, descriptor size should be sizeof(u32) * 2.

> +
> +	desc = memalign(ARCH_DMA_MINALIGN, desc_size);
> +	if (!desc) {
> +		debug("cannot allocate RNG init descriptor memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	for (sh_idx = 0; sh_idx < RNG4_MAX_HANDLES; sh_idx++) {
> +		/*
> +		 * If the corresponding bit is set, then it means the state
> +		 * handle was initialized by us, and thus it needs to be
> +		 * deinitialized as well
> +		 */
> +
> +		if (state_handle_mask & RDSTA_IF(sh_idx)) {
> +			/*
> +			 * Create the descriptor for deinstantating this state
> +			 * handle.
> +			 */
> +			inline_cnstr_jobdesc_rng_deinstantiation(desc, sh_idx);
> +			flush_dcache_range((unsigned long)desc,
> +					   (unsigned long)desc + desc_size);
Shouldn't this be roundup(desc_size, ARCH_DMA_MINALIGN) instead of desc_size?

> @@ -466,9 +511,18 @@ static int instantiate_rng(u8 sec_idx, int gen_sk)
>  		 * If the corresponding bit is set, this state handle
>  		 * was initialized by somebody else, so it's left alone.
>  		 */
> -		rdsta_val = sec_in32(&rng->rdsta) & RNG_STATE_HANDLE_MASK;
> -		if (rdsta_val & (1 << sh_idx))
> -			continue;
> +		rdsta_val = sec_in32(&rng->rdsta);
> +		if (rdsta_val & (RDSTA_IF(sh_idx) | RDSTA_PR(sh_idx))) {
Adding RDSTA_PR(sh_idx) to the mask is not needed,
PR bit is meaningless without IF bit set.

> +			if (rdsta_val & RDSTA_PR(sh_idx))
> +				continue;
Could combine the condition here with the outer if condition:
		if (rdsta_val & RDSTA_IF(sh_idx) && !(rdsta_val & RDSTA_PR(sh_idx))) {

> +
> +			printf("SEC%u:  RNG4 SH%d was instantiated w/o prediction resistance. Tearing it down\n",
> +			       sec_idx, sh_idx);
> +
> +			ret = deinstantiate_rng(sec_idx, RDSTA_IF(sh_idx));
> +			if (ret)
> +				break;
> +		}

Horia

[PATCH v2 6/6] crypto/fsl: add RNG support

[Horia Geantă]

On 6/4/2020 6:48 PM, Michael Walle wrote:
> +static int caam_rng_read(struct udevice *dev, void *data, size_t len)
> +{
> +	struct caam_rng_platdata *pdata = dev_get_platdata(dev);
> +	u8 *buffer = data;
> +	size_t size;
> +	int ret;
> +
> +	while (len) {
> +		ret = caam_rng_read_one(pdata);
> +		if (ret)
> +			return ret;
> +
> +		size = min(len, (size_t)CAAM_RNG_MAX_FIFO_STORE_SIZE);
> +
> +		memcpy(buffer, pdata->data, len);
size instead of len.

> +		buffer += size;
> +		len -= size;
> +	}
> +
> +	return 0;
> +}
> +
> +static int caam_rng_probe(struct udevice *dev)
> +{
> +	struct caam_rng_platdata *pdata = dev_get_platdata(dev);
> +
> +	inline_cnstr_jobdesc_rng(pdata->desc, pdata->data,
> +				 CAAM_RNG_MAX_FIFO_STORE_SIZE);
flush_dcache_range() on pdata->desc would be needed, no?

Horia

[PATCH v2 4/6] crypto/fsl: don't regenerate secure keys

[Michael Walle]

Am 2020-06-17 20:03, schrieb Horia Geant?:
> On 6/4/2020 6:47 PM, Michael Walle wrote:
>> The secure keys (TDKEK, JDKEK, TDSK) can only be generated once after 
>> a
>> POR. Otherwise the RNG4 will throw an error.
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
> Reviewed-by: Horia Geant? <horia.geanta@nxp.com>
> 
> Did you meet this problem in practice?

Only when I tested the reinstantiation.

-michael

[PATCH v2 5/6] crypto/fsl: instantiate the RNG with prediciton resistance

[Michael Walle]

Am 2020-06-17 21:15, schrieb Horia Geant?:
> On 6/4/2020 6:48 PM, Michael Walle wrote:
>> +static int deinstantiate_rng(u8 sec_idx, int state_handle_mask)
>> +{
>> +	u32 *desc;
>> +	int sh_idx, ret = 0;
>> +	int desc_size = sizeof(u32) * 3;
> As you mentioned, descriptor size should be sizeof(u32) * 2.
> 
>> +
>> +	desc = memalign(ARCH_DMA_MINALIGN, desc_size);
>> +	if (!desc) {
>> +		debug("cannot allocate RNG init descriptor memory\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	for (sh_idx = 0; sh_idx < RNG4_MAX_HANDLES; sh_idx++) {
>> +		/*
>> +		 * If the corresponding bit is set, then it means the state
>> +		 * handle was initialized by us, and thus it needs to be
>> +		 * deinitialized as well
>> +		 */
>> +
>> +		if (state_handle_mask & RDSTA_IF(sh_idx)) {
>> +			/*
>> +			 * Create the descriptor for deinstantating this state
>> +			 * handle.
>> +			 */
>> +			inline_cnstr_jobdesc_rng_deinstantiation(desc, sh_idx);
>> +			flush_dcache_range((unsigned long)desc,
>> +					   (unsigned long)desc + desc_size);
> Shouldn't this be roundup(desc_size, ARCH_DMA_MINALIGN) instead of 
> desc_size?

I've seen the same idioms sometimes, but it wasn't clear to me why that 
would
be needed; the hardware only uses the desc_size, right?

>> @@ -466,9 +511,18 @@ static int instantiate_rng(u8 sec_idx, int 
>> gen_sk)
>>  		 * If the corresponding bit is set, this state handle
>>  		 * was initialized by somebody else, so it's left alone.
>>  		 */
>> -		rdsta_val = sec_in32(&rng->rdsta) & RNG_STATE_HANDLE_MASK;
>> -		if (rdsta_val & (1 << sh_idx))
>> -			continue;
>> +		rdsta_val = sec_in32(&rng->rdsta);
>> +		if (rdsta_val & (RDSTA_IF(sh_idx) | RDSTA_PR(sh_idx))) {
> Adding RDSTA_PR(sh_idx) to the mask is not needed,
> PR bit is meaningless without IF bit set.

Ok.

> 
>> +			if (rdsta_val & RDSTA_PR(sh_idx))
>> +				continue;
> Could combine the condition here with the outer if condition:
> 		if (rdsta_val & RDSTA_IF(sh_idx) && !(rdsta_val & RDSTA_PR(sh_idx))) 
> {

If we keep that it is consistent with the linux driver. So I'd vote to
keep it. Also the continue statement would be missing and thus the
rng would be instantiated again. Or am I missing something?

-michael

[PATCH v2 6/6] crypto/fsl: add RNG support

[Michael Walle]

Am 2020-06-17 22:09, schrieb Horia Geant?:
> On 6/4/2020 6:48 PM, Michael Walle wrote:
>> +static int caam_rng_read(struct udevice *dev, void *data, size_t len)
>> +{
>> +	struct caam_rng_platdata *pdata = dev_get_platdata(dev);
>> +	u8 *buffer = data;
>> +	size_t size;
>> +	int ret;
>> +
>> +	while (len) {
>> +		ret = caam_rng_read_one(pdata);
>> +		if (ret)
>> +			return ret;
>> +
>> +		size = min(len, (size_t)CAAM_RNG_MAX_FIFO_STORE_SIZE);
>> +
>> +		memcpy(buffer, pdata->data, len);
> size instead of len.

nice catch.

Thanks for the review!

-michael

[PATCH v2 5/6] crypto/fsl: instantiate the RNG with prediciton resistance

[Horia Geantă]

On 6/17/2020 11:48 PM, Michael Walle wrote:
> Am 2020-06-17 21:15, schrieb Horia Geant?:
>> On 6/4/2020 6:48 PM, Michael Walle wrote:
>>> +
>>> +	desc = memalign(ARCH_DMA_MINALIGN, desc_size);
>>> +	if (!desc) {
>>> +		debug("cannot allocate RNG init descriptor memory\n");
>>> +		return -ENOMEM;
>>> +	}
>>> +
>>> +	for (sh_idx = 0; sh_idx < RNG4_MAX_HANDLES; sh_idx++) {
>>> +		/*
>>> +		 * If the corresponding bit is set, then it means the state
>>> +		 * handle was initialized by us, and thus it needs to be
>>> +		 * deinitialized as well
>>> +		 */
>>> +
>>> +		if (state_handle_mask & RDSTA_IF(sh_idx)) {
>>> +			/*
>>> +			 * Create the descriptor for deinstantating this state
>>> +			 * handle.
>>> +			 */
>>> +			inline_cnstr_jobdesc_rng_deinstantiation(desc, sh_idx);
>>> +			flush_dcache_range((unsigned long)desc,
>>> +					   (unsigned long)desc + desc_size);
>> Shouldn't this be roundup(desc_size, ARCH_DMA_MINALIGN) instead of 
>> desc_size?
> 
> I've seen the same idioms sometimes, but it wasn't clear to me why that 
> would
> be needed; the hardware only uses the desc_size, right?
> 
Yes, HW will use only [desc, desc + desc_size].

I think this is needed to avoid cacheline sharing issues
on non-coherent platforms: CPU needs to make sure a larger area
is written back to memory and corresponding cache lines are invalidated.

Looking at flush_dcache_range() implementation, it does its own rounding,
based on CTR_EL0[DminLine] - "smallest data cache line size".
I guess this value might be smaller than ARCH_DMA_MINALIGN,
hence the explicit rounding to ARCH_DMA_MINALIGN is needed.

>>> @@ -466,9 +511,18 @@ static int instantiate_rng(u8 sec_idx, int 
>>> gen_sk)
>>>  		 * If the corresponding bit is set, this state handle
>>>  		 * was initialized by somebody else, so it's left alone.
>>>  		 */
>>> -		rdsta_val = sec_in32(&rng->rdsta) & RNG_STATE_HANDLE_MASK;
>>> -		if (rdsta_val & (1 << sh_idx))
>>> -			continue;
>>> +		rdsta_val = sec_in32(&rng->rdsta);
>>> +		if (rdsta_val & (RDSTA_IF(sh_idx) | RDSTA_PR(sh_idx))) {
>> Adding RDSTA_PR(sh_idx) to the mask is not needed,
>> PR bit is meaningless without IF bit set.
> 
> Ok.
> 
>>
>>> +			if (rdsta_val & RDSTA_PR(sh_idx))
>>> +				continue;
>> Could combine the condition here with the outer if condition:
>> 		if (rdsta_val & RDSTA_IF(sh_idx) && !(rdsta_val & RDSTA_PR(sh_idx))) 
>> {
> 
> If we keep that it is consistent with the linux driver. So I'd vote to
> keep it. Also the continue statement would be missing and thus the
> rng would be instantiated again. Or am I missing something?
> 
You are correct, let's keep this as is.

Thanks,
Horia

[PATCH v2 5/6] crypto/fsl: instantiate the RNG with prediciton resistance

[Horia Geantă]

On 6/19/2020 7:37 PM, Horia Geanta wrote:
> On 6/17/2020 11:48 PM, Michael Walle wrote:
>> Am 2020-06-17 21:15, schrieb Horia Geant?:
>>> On 6/4/2020 6:48 PM, Michael Walle wrote:
>>>> +
>>>> +	desc = memalign(ARCH_DMA_MINALIGN, desc_size);
>>>> +	if (!desc) {
>>>> +		debug("cannot allocate RNG init descriptor memory\n");
>>>> +		return -ENOMEM;
>>>> +	}
>>>> +
>>>> +	for (sh_idx = 0; sh_idx < RNG4_MAX_HANDLES; sh_idx++) {
>>>> +		/*
>>>> +		 * If the corresponding bit is set, then it means the state
>>>> +		 * handle was initialized by us, and thus it needs to be
>>>> +		 * deinitialized as well
>>>> +		 */
>>>> +
>>>> +		if (state_handle_mask & RDSTA_IF(sh_idx)) {
>>>> +			/*
>>>> +			 * Create the descriptor for deinstantating this state
>>>> +			 * handle.
>>>> +			 */
>>>> +			inline_cnstr_jobdesc_rng_deinstantiation(desc, sh_idx);
>>>> +			flush_dcache_range((unsigned long)desc,
>>>> +					   (unsigned long)desc + desc_size);
>>> Shouldn't this be roundup(desc_size, ARCH_DMA_MINALIGN) instead of 
>>> desc_size?
>>
>> I've seen the same idioms sometimes, but it wasn't clear to me why that 
>> would
>> be needed; the hardware only uses the desc_size, right?
>>
> Yes, HW will use only [desc, desc + desc_size].
> 
> I think this is needed to avoid cacheline sharing issues
> on non-coherent platforms: CPU needs to make sure a larger area
> is written back to memory and corresponding cache lines are invalidated.
> 
> Looking at flush_dcache_range() implementation, it does its own rounding,
> based on CTR_EL0[DminLine] - "smallest data cache line size".
> I guess this value might be smaller than ARCH_DMA_MINALIGN,
> hence the explicit rounding to ARCH_DMA_MINALIGN is needed.
> 
Btw, I think
	desc = memalign(ARCH_DMA_MINALIGN, desc_size);
needs to be replaced with
	desc = malloc_cache_aligned(desc_size);

Horia

[PATCH v2 5/6] crypto/fsl: instantiate the RNG with prediciton resistance

[Michael Walle]

Am 2020-06-19 18:54, schrieb Horia Geant?:
> On 6/19/2020 7:37 PM, Horia Geanta wrote:
>> On 6/17/2020 11:48 PM, Michael Walle wrote:
>>> Am 2020-06-17 21:15, schrieb Horia Geant?:
>>>> On 6/4/2020 6:48 PM, Michael Walle wrote:
>>>>> +
>>>>> +	desc = memalign(ARCH_DMA_MINALIGN, desc_size);
>>>>> +	if (!desc) {
>>>>> +		debug("cannot allocate RNG init descriptor memory\n");
>>>>> +		return -ENOMEM;
>>>>> +	}
>>>>> +
>>>>> +	for (sh_idx = 0; sh_idx < RNG4_MAX_HANDLES; sh_idx++) {
>>>>> +		/*
>>>>> +		 * If the corresponding bit is set, then it means the state
>>>>> +		 * handle was initialized by us, and thus it needs to be
>>>>> +		 * deinitialized as well
>>>>> +		 */
>>>>> +
>>>>> +		if (state_handle_mask & RDSTA_IF(sh_idx)) {
>>>>> +			/*
>>>>> +			 * Create the descriptor for deinstantating this state
>>>>> +			 * handle.
>>>>> +			 */
>>>>> +			inline_cnstr_jobdesc_rng_deinstantiation(desc, sh_idx);
>>>>> +			flush_dcache_range((unsigned long)desc,
>>>>> +					   (unsigned long)desc + desc_size);
>>>> Shouldn't this be roundup(desc_size, ARCH_DMA_MINALIGN) instead of
>>>> desc_size?
>>> 
>>> I've seen the same idioms sometimes, but it wasn't clear to me why 
>>> that
>>> would
>>> be needed; the hardware only uses the desc_size, right?
>>> 
>> Yes, HW will use only [desc, desc + desc_size].
>> 
>> I think this is needed to avoid cacheline sharing issues
>> on non-coherent platforms: CPU needs to make sure a larger area
>> is written back to memory and corresponding cache lines are 
>> invalidated.
>> 
>> Looking at flush_dcache_range() implementation, it does its own 
>> rounding,
>> based on CTR_EL0[DminLine] - "smallest data cache line size".
>> I guess this value might be smaller than ARCH_DMA_MINALIGN,
>> hence the explicit rounding to ARCH_DMA_MINALIGN is needed.
>> 
> Btw, I think
> 	desc = memalign(ARCH_DMA_MINALIGN, desc_size);
> needs to be replaced with
> 	desc = malloc_cache_aligned(desc_size);

But then the rounding is not needed, right? I mean there can't
be any other malloc() which might allocate memory in the same
cache line.

-michael

[PATCH v2 5/6] crypto/fsl: instantiate the RNG with prediciton resistance

[Horia Geantă]

On 6/19/2020 10:02 PM, Michael Walle wrote:
> Am 2020-06-19 18:54, schrieb Horia Geant?:
>> On 6/19/2020 7:37 PM, Horia Geanta wrote:
>>> On 6/17/2020 11:48 PM, Michael Walle wrote:
>>>> Am 2020-06-17 21:15, schrieb Horia Geant?:
>>>>> On 6/4/2020 6:48 PM, Michael Walle wrote:
>>>>>> +
>>>>>> +	desc = memalign(ARCH_DMA_MINALIGN, desc_size);
>>>>>> +	if (!desc) {
>>>>>> +		debug("cannot allocate RNG init descriptor memory\n");
>>>>>> +		return -ENOMEM;
>>>>>> +	}
>>>>>> +
>>>>>> +	for (sh_idx = 0; sh_idx < RNG4_MAX_HANDLES; sh_idx++) {
>>>>>> +		/*
>>>>>> +		 * If the corresponding bit is set, then it means the state
>>>>>> +		 * handle was initialized by us, and thus it needs to be
>>>>>> +		 * deinitialized as well
>>>>>> +		 */
>>>>>> +
>>>>>> +		if (state_handle_mask & RDSTA_IF(sh_idx)) {
>>>>>> +			/*
>>>>>> +			 * Create the descriptor for deinstantating this state
>>>>>> +			 * handle.
>>>>>> +			 */
>>>>>> +			inline_cnstr_jobdesc_rng_deinstantiation(desc, sh_idx);
>>>>>> +			flush_dcache_range((unsigned long)desc,
>>>>>> +					   (unsigned long)desc + desc_size);
>>>>> Shouldn't this be roundup(desc_size, ARCH_DMA_MINALIGN) instead of
>>>>> desc_size?
>>>>
>>>> I've seen the same idioms sometimes, but it wasn't clear to me why 
>>>> that
>>>> would
>>>> be needed; the hardware only uses the desc_size, right?
>>>>
>>> Yes, HW will use only [desc, desc + desc_size].
>>>
>>> I think this is needed to avoid cacheline sharing issues
>>> on non-coherent platforms: CPU needs to make sure a larger area
>>> is written back to memory and corresponding cache lines are 
>>> invalidated.
>>>
>>> Looking at flush_dcache_range() implementation, it does its own 
>>> rounding,
>>> based on CTR_EL0[DminLine] - "smallest data cache line size".
>>> I guess this value might be smaller than ARCH_DMA_MINALIGN,
>>> hence the explicit rounding to ARCH_DMA_MINALIGN is needed.
>>>
>> Btw, I think
>> 	desc = memalign(ARCH_DMA_MINALIGN, desc_size);
>> needs to be replaced with
>> 	desc = malloc_cache_aligned(desc_size);
> 
> But then the rounding is not needed, right? I mean there can't
> be any other malloc() which might allocate memory in the same
> cache line.
> 
Yes, in this case the rounding is no longer needed.

Horia