| 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 |
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
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
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
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
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 --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]; };
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(-)