diff mbox series

[v6,6/7] crypto: caam - enable prediction resistance in HRWNG

Message ID 20200108154047.12526-7-andrew.smirnov@gmail.com (mailing list archive)
State Changes Requested
Delegated to: Herbert Xu
Headers show
Series enable CAAM's HWRNG as default | expand

Commit Message

Andrey Smirnov Jan. 8, 2020, 3:40 p.m. UTC
Instantiate CAAM RNG with prediction resistance enabled to improve its
quality (with PR on DRNG is forced to reseed from TRNG every time
random data is generated).

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Horia Geantă <horia.geanta@nxp.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Iuliana Prodan <iuliana.prodan@nxp.com>
Cc: linux-crypto@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-imx@nxp.com
---
 drivers/crypto/caam/caamrng.c |  3 ++-
 drivers/crypto/caam/ctrl.c    | 22 ++++++++++++++++++----
 drivers/crypto/caam/desc.h    |  2 ++
 drivers/crypto/caam/regs.h    |  4 +++-
 4 files changed, 25 insertions(+), 6 deletions(-)

Comments

Horia Geanta Jan. 20, 2020, 4:38 p.m. UTC | #1
On 1/8/2020 5:42 PM, Andrey Smirnov wrote:
> @@ -275,12 +276,25 @@ static int instantiate_rng(struct device *ctrldev, int state_handle_mask,
>  		return -ENOMEM;
>  
>  	for (sh_idx = 0; sh_idx < RNG4_MAX_HANDLES; sh_idx++) {
> +		const u32 rdsta_if = RDSTA_IF0 << sh_idx;
> +		const u32 rdsta_pr = RDSTA_PR0 << sh_idx;
> +		const u32 rdsta_mask = rdsta_if | rdsta_pr;
>  		/*
>  		 * If the corresponding bit is set, this state handle
>  		 * was initialized by somebody else, so it's left alone.
>  		 */
> -		if ((1 << sh_idx) & state_handle_mask)
> -			continue;
> +		if (rdsta_if & state_handle_mask) {
> +			if (rdsta_pr & state_handle_mask)
> +				continue;
> +
> +			dev_info(ctrldev,
> +				 "RNG4 SH%d was previously instantiated without prediction resistance. Tearing it down\n",
> +				 sh_idx);
> +
> +			ret = deinstantiate_rng(ctrldev, rdsta_if);
> +			if (ret)
> +				break;
In case state handle 0 is deinstantiated, its reinstantiation with PR support
will have the side effect of re-generating JDKEK, TDKEK, TDSK.
This needs to be avoided, since other SW components (like OP-TEE f/w)
could have black keys in use. Overwriting the KEK registers would no longer
allow CAAM to decrypt them.

Horia
Horia Geanta Jan. 21, 2020, 6:20 a.m. UTC | #2
On 1/20/2020 6:38 PM, Horia Geanta wrote:
> On 1/8/2020 5:42 PM, Andrey Smirnov wrote:
>> @@ -275,12 +276,25 @@ static int instantiate_rng(struct device *ctrldev, int state_handle_mask,
>>  		return -ENOMEM;
>>  
>>  	for (sh_idx = 0; sh_idx < RNG4_MAX_HANDLES; sh_idx++) {
>> +		const u32 rdsta_if = RDSTA_IF0 << sh_idx;
>> +		const u32 rdsta_pr = RDSTA_PR0 << sh_idx;
>> +		const u32 rdsta_mask = rdsta_if | rdsta_pr;
>>  		/*
>>  		 * If the corresponding bit is set, this state handle
>>  		 * was initialized by somebody else, so it's left alone.
>>  		 */
>> -		if ((1 << sh_idx) & state_handle_mask)
>> -			continue;
>> +		if (rdsta_if & state_handle_mask) {
>> +			if (rdsta_pr & state_handle_mask)
>> +				continue;
>> +
>> +			dev_info(ctrldev,
>> +				 "RNG4 SH%d was previously instantiated without prediction resistance. Tearing it down\n",
>> +				 sh_idx);
>> +
>> +			ret = deinstantiate_rng(ctrldev, rdsta_if);
>> +			if (ret)
>> +				break;
> In case state handle 0 is deinstantiated, its reinstantiation with PR support
> will have the side effect of re-generating JDKEK, TDKEK, TDSK.
> This needs to be avoided, since other SW components (like OP-TEE f/w)
> could have black keys in use. Overwriting the KEK registers would no longer
> allow CAAM to decrypt them.
> 
Never mind, looks like there is logic in place to check whether
keys have been generated or not, by looking at RDSTA[SKVN].

Horia
Horia Geanta Jan. 21, 2020, 4:38 p.m. UTC | #3
On 1/8/2020 5:42 PM, Andrey Smirnov wrote:
> @@ -275,12 +276,25 @@ static int instantiate_rng(struct device *ctrldev, int state_handle_mask,
>  		return -ENOMEM;
>  
>  	for (sh_idx = 0; sh_idx < RNG4_MAX_HANDLES; sh_idx++) {
> +		const u32 rdsta_if = RDSTA_IF0 << sh_idx;
> +		const u32 rdsta_pr = RDSTA_PR0 << sh_idx;
> +		const u32 rdsta_mask = rdsta_if | rdsta_pr;
>  		/*
>  		 * If the corresponding bit is set, this state handle
>  		 * was initialized by somebody else, so it's left alone.
>  		 */
> -		if ((1 << sh_idx) & state_handle_mask)
> -			continue;
> +		if (rdsta_if & state_handle_mask) {
> +			if (rdsta_pr & state_handle_mask)
instantiate_rng() is called with
	state_handle_mask = rd_reg32(&ctrl->r4tst[0].rdsta) & RDSTA_IFMASK;
so if (rdsta_pr & state_handle_mask) will always be false,
leading to unneeded state handle re-initialization.

Horia
Horia Geanta Jan. 22, 2020, 1:37 p.m. UTC | #4
On 1/21/2020 6:38 PM, Horia Geanta wrote:
> On 1/8/2020 5:42 PM, Andrey Smirnov wrote:
>> @@ -275,12 +276,25 @@ static int instantiate_rng(struct device *ctrldev, int state_handle_mask,
>>  		return -ENOMEM;
>>  
>>  	for (sh_idx = 0; sh_idx < RNG4_MAX_HANDLES; sh_idx++) {
>> +		const u32 rdsta_if = RDSTA_IF0 << sh_idx;
>> +		const u32 rdsta_pr = RDSTA_PR0 << sh_idx;
>> +		const u32 rdsta_mask = rdsta_if | rdsta_pr;
>>  		/*
>>  		 * If the corresponding bit is set, this state handle
>>  		 * was initialized by somebody else, so it's left alone.
>>  		 */
>> -		if ((1 << sh_idx) & state_handle_mask)
>> -			continue;
>> +		if (rdsta_if & state_handle_mask) {
>> +			if (rdsta_pr & state_handle_mask)
> instantiate_rng() is called with
> 	state_handle_mask = rd_reg32(&ctrl->r4tst[0].rdsta) & RDSTA_IFMASK;
> so if (rdsta_pr & state_handle_mask) will always be false,
> leading to unneeded state handle re-initialization.
> 
Sorry, I missed this change:
-#define RDSTA_IFMASK (RDSTA_IF1 | RDSTA_IF0)
+#define RDSTA_IFMASK (RDSTA_PR1 | RDSTA_PR0 | RDSTA_IF1 | RDSTA_IF0)

which means code is correct (though I must admit not so intuitive).

Horia
Andrey Smirnov Jan. 27, 2020, 1:45 p.m. UTC | #5
On Wed, Jan 22, 2020 at 5:37 AM Horia Geanta <horia.geanta@nxp.com> wrote:
>
> On 1/21/2020 6:38 PM, Horia Geanta wrote:
> > On 1/8/2020 5:42 PM, Andrey Smirnov wrote:
> >> @@ -275,12 +276,25 @@ static int instantiate_rng(struct device *ctrldev, int state_handle_mask,
> >>              return -ENOMEM;
> >>
> >>      for (sh_idx = 0; sh_idx < RNG4_MAX_HANDLES; sh_idx++) {
> >> +            const u32 rdsta_if = RDSTA_IF0 << sh_idx;
> >> +            const u32 rdsta_pr = RDSTA_PR0 << sh_idx;
> >> +            const u32 rdsta_mask = rdsta_if | rdsta_pr;
> >>              /*
> >>               * If the corresponding bit is set, this state handle
> >>               * was initialized by somebody else, so it's left alone.
> >>               */
> >> -            if ((1 << sh_idx) & state_handle_mask)
> >> -                    continue;
> >> +            if (rdsta_if & state_handle_mask) {
> >> +                    if (rdsta_pr & state_handle_mask)
> > instantiate_rng() is called with
> >       state_handle_mask = rd_reg32(&ctrl->r4tst[0].rdsta) & RDSTA_IFMASK;
> > so if (rdsta_pr & state_handle_mask) will always be false,
> > leading to unneeded state handle re-initialization.
> >
> Sorry, I missed this change:
> -#define RDSTA_IFMASK (RDSTA_IF1 | RDSTA_IF0)
> +#define RDSTA_IFMASK (RDSTA_PR1 | RDSTA_PR0 | RDSTA_IF1 | RDSTA_IF0)
>
> which means code is correct (though I must admit not so intuitive).

Renamed this to RDSTA_MASK in v7, to, hopefully make things more clear.

Thanks,
Andrey Smirnov
diff mbox series

Patch

diff --git a/drivers/crypto/caam/caamrng.c b/drivers/crypto/caam/caamrng.c
index 554aafbd4d11..91ccde0240fe 100644
--- a/drivers/crypto/caam/caamrng.c
+++ b/drivers/crypto/caam/caamrng.c
@@ -77,7 +77,8 @@  static u32 *caam_init_desc(u32 *desc, dma_addr_t dst_dma, int len)
 {
 	init_job_desc(desc, 0);	/* + 1 cmd_sz */
 	/* Generate random bytes: + 1 cmd_sz */
-	append_operation(desc, OP_ALG_ALGSEL_RNG | OP_TYPE_CLASS1_ALG);
+	append_operation(desc, OP_ALG_ALGSEL_RNG | OP_TYPE_CLASS1_ALG |
+			 OP_ALG_PR_ON);
 	/* Store bytes */
 	append_fifo_store(desc, dst_dma, len, FIFOST_TYPE_RNGSTORE);
 
diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index 22d8676dd610..85c2e831839a 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -36,7 +36,8 @@  static void build_instantiation_desc(u32 *desc, int handle, int do_sk)
 	init_job_desc(desc, 0);
 
 	op_flags = 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;
 
 	/* INIT RNG in non-test mode */
 	append_operation(desc, op_flags);
@@ -275,12 +276,25 @@  static int instantiate_rng(struct device *ctrldev, int state_handle_mask,
 		return -ENOMEM;
 
 	for (sh_idx = 0; sh_idx < RNG4_MAX_HANDLES; sh_idx++) {
+		const u32 rdsta_if = RDSTA_IF0 << sh_idx;
+		const u32 rdsta_pr = RDSTA_PR0 << sh_idx;
+		const u32 rdsta_mask = rdsta_if | rdsta_pr;
 		/*
 		 * If the corresponding bit is set, this state handle
 		 * was initialized by somebody else, so it's left alone.
 		 */
-		if ((1 << sh_idx) & state_handle_mask)
-			continue;
+		if (rdsta_if & state_handle_mask) {
+			if (rdsta_pr & state_handle_mask)
+				continue;
+
+			dev_info(ctrldev,
+				 "RNG4 SH%d was previously instantiated without prediction resistance. Tearing it down\n",
+				 sh_idx);
+
+			ret = deinstantiate_rng(ctrldev, rdsta_if);
+			if (ret)
+				break;
+		}
 
 		/* Create the descriptor for instantiating RNG State Handle */
 		build_instantiation_desc(desc, sh_idx, gen_sk);
@@ -302,7 +316,7 @@  static int instantiate_rng(struct device *ctrldev, int state_handle_mask,
 
 		rdsta_val = rd_reg32(&ctrl->r4tst[0].rdsta) & RDSTA_IFMASK;
 		if ((status && status != JRSTA_SSRC_JUMP_HALT_CC) ||
-		    !(rdsta_val & (1 << sh_idx))) {
+		    (rdsta_val & rdsta_mask) != rdsta_mask) {
 			ret = -EAGAIN;
 			break;
 		}
diff --git a/drivers/crypto/caam/desc.h b/drivers/crypto/caam/desc.h
index 4b6854bf896a..e796d3cb9be8 100644
--- a/drivers/crypto/caam/desc.h
+++ b/drivers/crypto/caam/desc.h
@@ -1254,6 +1254,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		BIT(1)
+
 #define OP_ALG_DIR_SHIFT	0
 #define OP_ALG_DIR_MASK		1
 #define OP_ALG_DECRYPT		0
diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h
index c191e8fd0fa7..fe1f8c1409fd 100644
--- a/drivers/crypto/caam/regs.h
+++ b/drivers/crypto/caam/regs.h
@@ -524,9 +524,11 @@  struct rng4tst {
 	u32 rsvd1[40];
 #define RDSTA_SKVT 0x80000000
 #define RDSTA_SKVN 0x40000000
+#define RDSTA_PR0 BIT(4)
+#define RDSTA_PR1 BIT(5)
 #define RDSTA_IF0 0x00000001
 #define RDSTA_IF1 0x00000002
-#define RDSTA_IFMASK (RDSTA_IF1 | RDSTA_IF0)
+#define RDSTA_IFMASK (RDSTA_PR1 | RDSTA_PR0 | RDSTA_IF1 | RDSTA_IF0)
 	u32 rdsta;
 	u32 rsvd2[15];
 };