Everyone:
This series is a continuation of original [discussion]. I don't know
if what's in the series is enough to use CAAMs HWRNG system wide, but
I am hoping that with enough iterations and feedback it will be.
Changes since [v1]:
- Original hw_random replaced with the one using output of TRNG directly
- SEC4 DRNG IP block exposed via crypto API
- Small fix regarding use of GFP_DMA added to the series
Chagnes since [v2]:
- msleep in polling loop to avoid wasting CPU cycles
- caam_trng_read() bails out early if 'wait' is set to 'false'
- fixed typo in ZII's name
Changes since [v3]:
- DRNG's .cra_name is now "stdrng"
- collected Reviewd-by tag from Lucas
- typo fixes in commit messages of the series
Changes since [v4]:
- Dropped "crypto: caam - RNG4 TRNG errata" and "crypto: caam -
enable prediction resistance in HRWNG" to limit the scope of the
series. Those two patches are not yet ready and can be submitted
separately later.
- Collected Tested-by from Chris
Changes since [v5]:
- Series is converted back to implementing HWRNG using a job ring
as per feedback from Horia.
Feedback is welcome!
Thanks,
Andrey Smirnov
[discussion] https://patchwork.kernel.org/patch/9850669/
[v1] https://lore.kernel.org/lkml/[email protected]
[v2] https://lore.kernel.org/lkml/[email protected]
[v3] https://lore.kernel.org/lkml/[email protected]
[v4] https://lore.kernel.org/lkml/[email protected]
[v5] https://lore.kernel.org/lkml/[email protected]
Andrey Smirnov (7):
crypto: caam - use struct hwrng's .init for initialization
crypto: caam - drop global context pointer and init_done
crypto: caam - simplify RNG implementation
crypto: caam - check if RNG job failed
crypto: caam - invalidate entropy register during RNG initialization
crypto: caam - enable prediction resistance in HRWNG
crypto: caam - limit single JD RNG output to maximum of 16 bytes
drivers/crypto/caam/caamrng.c | 391 ++++++++++++----------------------
drivers/crypto/caam/ctrl.c | 33 ++-
drivers/crypto/caam/desc.h | 2 +
drivers/crypto/caam/intern.h | 5 -
drivers/crypto/caam/jr.c | 1 -
drivers/crypto/caam/regs.h | 7 +-
6 files changed, 174 insertions(+), 265 deletions(-)
--
2.21.0
Make caamrng code a bit more symmetric by moving initialization code
to .init hook of struct hwrng.
Signed-off-by: Andrey Smirnov <[email protected]>
Cc: Chris Healy <[email protected]>
Cc: Lucas Stach <[email protected]>
Cc: Horia Geantă <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: Iuliana Prodan <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
drivers/crypto/caam/caamrng.c | 47 ++++++++++++++++++-----------------
1 file changed, 24 insertions(+), 23 deletions(-)
diff --git a/drivers/crypto/caam/caamrng.c b/drivers/crypto/caam/caamrng.c
index e8baacaabe07..1ce7fbd29e85 100644
--- a/drivers/crypto/caam/caamrng.c
+++ b/drivers/crypto/caam/caamrng.c
@@ -256,6 +256,7 @@ static void caam_cleanup(struct hwrng *rng)
}
rng_unmap_ctx(rng_ctx);
+ caam_jr_free(rng_ctx->jrdev);
}
static int caam_init_buf(struct caam_rng_ctx *ctx, int buf_id)
@@ -274,28 +275,43 @@ static int caam_init_buf(struct caam_rng_ctx *ctx, int buf_id)
return 0;
}
-static int caam_init_rng(struct caam_rng_ctx *ctx, struct device *jrdev)
+static int caam_init(struct hwrng *rng)
{
+ struct caam_rng_ctx *ctx = rng_ctx;
int err;
- ctx->jrdev = jrdev;
+ ctx->jrdev = caam_jr_alloc();
+ err = PTR_ERR_OR_ZERO(ctx->jrdev);
+ if (err) {
+ pr_err("Job Ring Device allocation for transform failed\n");
+ return err;
+ }
err = rng_create_sh_desc(ctx);
if (err)
- return err;
+ goto free_jrdev;
ctx->current_buf = 0;
ctx->cur_buf_idx = 0;
err = caam_init_buf(ctx, 0);
if (err)
- return err;
+ goto free_jrdev;
+
+ err = caam_init_buf(ctx, 1);
+ if (err)
+ goto free_jrdev;
- return caam_init_buf(ctx, 1);
+ return 0;
+
+free_jrdev:
+ caam_jr_free(ctx->jrdev);
+ return err;
}
static struct hwrng caam_rng = {
.name = "rng-caam",
+ .init = caam_init,
.cleanup = caam_cleanup,
.read = caam_read,
};
@@ -305,14 +321,12 @@ void caam_rng_exit(void)
if (!init_done)
return;
- caam_jr_free(rng_ctx->jrdev);
hwrng_unregister(&caam_rng);
kfree(rng_ctx);
}
int caam_rng_init(struct device *ctrldev)
{
- struct device *dev;
u32 rng_inst;
struct caam_drv_private *priv = dev_get_drvdata(ctrldev);
int err;
@@ -328,21 +342,11 @@ int caam_rng_init(struct device *ctrldev)
if (!rng_inst)
return 0;
- dev = caam_jr_alloc();
- if (IS_ERR(dev)) {
- pr_err("Job Ring Device allocation for transform failed\n");
- return PTR_ERR(dev);
- }
rng_ctx = kmalloc(sizeof(*rng_ctx), GFP_DMA | GFP_KERNEL);
- if (!rng_ctx) {
- err = -ENOMEM;
- goto free_caam_alloc;
- }
- err = caam_init_rng(rng_ctx, dev);
- if (err)
- goto free_rng_ctx;
+ if (!rng_ctx)
+ return -ENOMEM;
- dev_info(dev, "registering rng-caam\n");
+ dev_info(ctrldev, "registering rng-caam\n");
err = hwrng_register(&caam_rng);
if (!err) {
@@ -350,9 +354,6 @@ int caam_rng_init(struct device *ctrldev)
return err;
}
-free_rng_ctx:
kfree(rng_ctx);
-free_caam_alloc:
- caam_jr_free(dev);
return err;
}
--
2.21.0
Leverage devres to get rid of code storing global context as well as
init_done flag.
Signed-off-by: Andrey Smirnov <[email protected]>
Cc: Chris Healy <[email protected]>
Cc: Lucas Stach <[email protected]>
Cc: Horia Geantă <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: Iuliana Prodan <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
drivers/crypto/caam/caamrng.c | 60 ++++++++++++-----------------------
drivers/crypto/caam/intern.h | 5 ---
drivers/crypto/caam/jr.c | 1 -
3 files changed, 20 insertions(+), 46 deletions(-)
diff --git a/drivers/crypto/caam/caamrng.c b/drivers/crypto/caam/caamrng.c
index 1ce7fbd29e85..fe187db91233 100644
--- a/drivers/crypto/caam/caamrng.c
+++ b/drivers/crypto/caam/caamrng.c
@@ -70,6 +70,7 @@ struct buf_data {
/* rng per-device context */
struct caam_rng_ctx {
+ struct hwrng rng;
struct device *jrdev;
dma_addr_t sh_desc_dma;
u32 sh_desc[DESC_RNG_LEN];
@@ -78,13 +79,10 @@ struct caam_rng_ctx {
struct buf_data bufs[2];
};
-static struct caam_rng_ctx *rng_ctx;
-
-/*
- * Variable used to avoid double free of resources in case
- * algorithm registration was unsuccessful
- */
-static bool init_done;
+static struct caam_rng_ctx *to_caam_rng_ctx(struct hwrng *r)
+{
+ return container_of(r, struct caam_rng_ctx, rng);
+}
static inline void rng_unmap_buf(struct device *jrdev, struct buf_data *bd)
{
@@ -143,7 +141,7 @@ static inline int submit_job(struct caam_rng_ctx *ctx, int to_current)
static int caam_read(struct hwrng *rng, void *data, size_t max, bool wait)
{
- struct caam_rng_ctx *ctx = rng_ctx;
+ struct caam_rng_ctx *ctx = to_caam_rng_ctx(rng);
struct buf_data *bd = &ctx->bufs[ctx->current_buf];
int next_buf_idx, copied_idx;
int err;
@@ -246,17 +244,18 @@ static inline int rng_create_job_desc(struct caam_rng_ctx *ctx, int buf_id)
static void caam_cleanup(struct hwrng *rng)
{
+ struct caam_rng_ctx *ctx = to_caam_rng_ctx(rng);
int i;
struct buf_data *bd;
for (i = 0; i < 2; i++) {
- bd = &rng_ctx->bufs[i];
+ bd = &ctx->bufs[i];
if (atomic_read(&bd->empty) == BUF_PENDING)
wait_for_completion(&bd->filled);
}
- rng_unmap_ctx(rng_ctx);
- caam_jr_free(rng_ctx->jrdev);
+ rng_unmap_ctx(ctx);
+ caam_jr_free(ctx->jrdev);
}
static int caam_init_buf(struct caam_rng_ctx *ctx, int buf_id)
@@ -277,7 +276,7 @@ static int caam_init_buf(struct caam_rng_ctx *ctx, int buf_id)
static int caam_init(struct hwrng *rng)
{
- struct caam_rng_ctx *ctx = rng_ctx;
+ struct caam_rng_ctx *ctx = to_caam_rng_ctx(rng);
int err;
ctx->jrdev = caam_jr_alloc();
@@ -309,28 +308,11 @@ static int caam_init(struct hwrng *rng)
return err;
}
-static struct hwrng caam_rng = {
- .name = "rng-caam",
- .init = caam_init,
- .cleanup = caam_cleanup,
- .read = caam_read,
-};
-
-void caam_rng_exit(void)
-{
- if (!init_done)
- return;
-
- hwrng_unregister(&caam_rng);
- kfree(rng_ctx);
-}
-
int caam_rng_init(struct device *ctrldev)
{
+ struct caam_rng_ctx *ctx;
u32 rng_inst;
struct caam_drv_private *priv = dev_get_drvdata(ctrldev);
- int err;
- init_done = false;
/* Check for an instantiated RNG before registration */
if (priv->era < 10)
@@ -342,18 +324,16 @@ int caam_rng_init(struct device *ctrldev)
if (!rng_inst)
return 0;
- rng_ctx = kmalloc(sizeof(*rng_ctx), GFP_DMA | GFP_KERNEL);
- if (!rng_ctx)
+ ctx = devm_kzalloc(ctrldev, sizeof(*ctx), GFP_DMA | GFP_KERNEL);
+ if (!ctx)
return -ENOMEM;
- dev_info(ctrldev, "registering rng-caam\n");
+ ctx->rng.name = "rng-caam";
+ ctx->rng.init = caam_init;
+ ctx->rng.cleanup = caam_cleanup;
+ ctx->rng.read = caam_read;
- err = hwrng_register(&caam_rng);
- if (!err) {
- init_done = true;
- return err;
- }
+ dev_info(ctrldev, "registering rng-caam\n");
- kfree(rng_ctx);
- return err;
+ return devm_hwrng_register(ctrldev, &ctx->rng);
}
diff --git a/drivers/crypto/caam/intern.h b/drivers/crypto/caam/intern.h
index c7c10c90464b..6d64931409eb 100644
--- a/drivers/crypto/caam/intern.h
+++ b/drivers/crypto/caam/intern.h
@@ -161,7 +161,6 @@ static inline void caam_pkc_exit(void)
#ifdef CONFIG_CRYPTO_DEV_FSL_CAAM_RNG_API
int caam_rng_init(struct device *dev);
-void caam_rng_exit(void);
#else
@@ -170,10 +169,6 @@ static inline int caam_rng_init(struct device *dev)
return 0;
}
-static inline void caam_rng_exit(void)
-{
-}
-
#endif /* CONFIG_CRYPTO_DEV_FSL_CAAM_RNG_API */
#ifdef CONFIG_CAAM_QI
diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
index fc97cde27059..f15d0d92c031 100644
--- a/drivers/crypto/caam/jr.c
+++ b/drivers/crypto/caam/jr.c
@@ -53,7 +53,6 @@ static void unregister_algs(void)
caam_qi_algapi_exit();
- caam_rng_exit();
caam_pkc_exit();
caam_algapi_hash_exit();
caam_algapi_exit();
--
2.21.0
Rework CAAM RNG implementation as follows:
- Make use of the fact that HWRNG supports partial reads and will
handle such cases gracefully by removing recursion in caam_read()
- Convert blocking caam_read() codepath to do a single blocking job
read directly into requested buffer, bypassing any intermediary
buffers
- Convert async caam_read() codepath into a simple single
reader/single writer FIFO use-case, thus simplifying concurrency
handling and delegating buffer read/write position management to KFIFO
subsystem.
- Leverage the same low level RNG data extraction code for both async
and blocking caam_read() scenarios, get rid of the shared job
descriptor and make non-shared one as a simple as possible (just
HEADER + ALGORITHM OPERATION + FIFO STORE)
- Split private context from DMA related memory, so that the former
could be allocated without GFP_DMA.
NOTE: On its face value this commit decreased throughput numbers
reported by
dd if=/dev/hwrng of=/dev/null bs=1 count=100K [iflag=nonblock]
by about 15%, however commits that enable prediction resistance and
limit JR total size impact the performance so much and move the
bottleneck such as to make this regression irrelevant.
NOTE: On the bright side, this commit reduces RNG in kernel DMA buffer
memory usage from 2 x RN_BUF_SIZE (~256K) to 32K.
Signed-off-by: Andrey Smirnov <[email protected]>
Cc: Chris Healy <[email protected]>
Cc: Lucas Stach <[email protected]>
Cc: Horia Geantă <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: Iuliana Prodan <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
drivers/crypto/caam/caamrng.c | 325 ++++++++++++----------------------
1 file changed, 112 insertions(+), 213 deletions(-)
diff --git a/drivers/crypto/caam/caamrng.c b/drivers/crypto/caam/caamrng.c
index fe187db91233..3960f5c81c97 100644
--- a/drivers/crypto/caam/caamrng.c
+++ b/drivers/crypto/caam/caamrng.c
@@ -7,35 +7,12 @@
*
* Based on caamalg.c crypto API driver.
*
- * relationship between job descriptors to shared descriptors:
- *
- * --------------- --------------
- * | JobDesc #0 |-------------------->| ShareDesc |
- * | *(buffer 0) | |------------->| (generate) |
- * --------------- | | (move) |
- * | | (store) |
- * --------------- | --------------
- * | JobDesc #1 |------|
- * | *(buffer 1) |
- * ---------------
- *
- * A job desc looks like this:
- *
- * ---------------------
- * | Header |
- * | ShareDesc Pointer |
- * | SEQ_OUT_PTR |
- * | (output buffer) |
- * ---------------------
- *
- * The SharedDesc never changes, and each job descriptor points to one of two
- * buffers for each device, from which the data will be copied into the
- * requested destination
*/
#include <linux/hw_random.h>
#include <linux/completion.h>
#include <linux/atomic.h>
+#include <linux/kfifo.h>
#include "compat.h"
@@ -45,38 +22,34 @@
#include "jr.h"
#include "error.h"
+/* length of descriptors */
+#define CAAM_RNG_MAX_FIFO_STORE_SIZE U16_MAX
+
+#define CAAM_RNG_FIFO_LEN SZ_32K /* Must be a multiple of 2 */
+
/*
- * Maximum buffer size: maximum number of random, cache-aligned bytes that
- * will be generated and moved to seq out ptr (extlen not allowed)
+ * See caam_init_desc()
*/
-#define RN_BUF_SIZE (0xffff / L1_CACHE_BYTES * \
- L1_CACHE_BYTES)
+#define CAAM_RNG_DESC_LEN (CAAM_CMD_SZ + \
+ CAAM_CMD_SZ + \
+ CAAM_CMD_SZ + CAAM_PTR_SZ_MAX)
-/* length of descriptors */
-#define DESC_JOB_O_LEN (CAAM_CMD_SZ * 2 + CAAM_PTR_SZ_MAX * 2)
-#define DESC_RNG_LEN (3 * CAAM_CMD_SZ)
-
-/* Buffer, its dma address and lock */
-struct buf_data {
- u8 buf[RN_BUF_SIZE] ____cacheline_aligned;
- dma_addr_t addr;
- struct completion filled;
- u32 hw_desc[DESC_JOB_O_LEN];
-#define BUF_NOT_EMPTY 0
-#define BUF_EMPTY 1
-#define BUF_PENDING 2 /* Empty, but with job pending --don't submit another */
- atomic_t empty;
+typedef u8 caam_rng_desc[CAAM_RNG_DESC_LEN];
+
+enum {
+ DESC_ASYNC,
+ DESC_SYNC,
+ DESC_NUM,
};
/* rng per-device context */
struct caam_rng_ctx {
struct hwrng rng;
struct device *jrdev;
- dma_addr_t sh_desc_dma;
- u32 sh_desc[DESC_RNG_LEN];
- unsigned int cur_buf_idx;
- int current_buf;
- struct buf_data bufs[2];
+ struct device *ctrldev;
+ caam_rng_desc *desc;
+ struct work_struct worker;
+ struct kfifo fifo;
};
static struct caam_rng_ctx *to_caam_rng_ctx(struct hwrng *r)
@@ -84,228 +57,152 @@ static struct caam_rng_ctx *to_caam_rng_ctx(struct hwrng *r)
return container_of(r, struct caam_rng_ctx, rng);
}
-static inline void rng_unmap_buf(struct device *jrdev, struct buf_data *bd)
+static void caam_rng_done(struct device *jrdev, u32 *desc, u32 err,
+ void *context)
{
- if (bd->addr)
- dma_unmap_single(jrdev, bd->addr, RN_BUF_SIZE,
- DMA_FROM_DEVICE);
-}
-
-static inline void rng_unmap_ctx(struct caam_rng_ctx *ctx)
-{
- struct device *jrdev = ctx->jrdev;
-
- if (ctx->sh_desc_dma)
- dma_unmap_single(jrdev, ctx->sh_desc_dma,
- desc_bytes(ctx->sh_desc), DMA_TO_DEVICE);
- rng_unmap_buf(jrdev, &ctx->bufs[0]);
- rng_unmap_buf(jrdev, &ctx->bufs[1]);
-}
-
-static void rng_done(struct device *jrdev, u32 *desc, u32 err, void *context)
-{
- struct buf_data *bd;
-
- bd = container_of(desc, struct buf_data, hw_desc[0]);
+ struct completion *done = context;
if (err)
caam_jr_strstatus(jrdev, err);
- atomic_set(&bd->empty, BUF_NOT_EMPTY);
- complete(&bd->filled);
-
- /* Buffer refilled, invalidate cache */
- dma_sync_single_for_cpu(jrdev, bd->addr, RN_BUF_SIZE, DMA_FROM_DEVICE);
-
- print_hex_dump_debug("rng refreshed buf@: ", DUMP_PREFIX_ADDRESS, 16, 4,
- bd->buf, RN_BUF_SIZE, 1);
+ complete(done);
}
-static inline int submit_job(struct caam_rng_ctx *ctx, int to_current)
+static u32 *caam_init_desc(u32 *desc, dma_addr_t dst_dma, int len)
{
- struct buf_data *bd = &ctx->bufs[!(to_current ^ ctx->current_buf)];
- struct device *jrdev = ctx->jrdev;
- u32 *desc = bd->hw_desc;
- int err;
+ 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);
+ /* Store bytes */
+ append_fifo_store(desc, dst_dma, len, FIFOST_TYPE_RNGSTORE);
- dev_dbg(jrdev, "submitting job %d\n", !(to_current ^ ctx->current_buf));
- init_completion(&bd->filled);
- err = caam_jr_enqueue(jrdev, desc, rng_done, ctx);
- if (err)
- complete(&bd->filled); /* don't wait on failed job*/
- else
- atomic_inc(&bd->empty); /* note if pending */
+ print_hex_dump_debug("rng job desc@: ", DUMP_PREFIX_ADDRESS,
+ 16, 4, desc, desc_bytes(desc), 1);
- return err;
+ return desc;
}
-static int caam_read(struct hwrng *rng, void *data, size_t max, bool wait)
+static int caam_rng_read_one(struct device *jrdev,
+ void *dst, int len,
+ void *desc,
+ struct completion *done)
{
- struct caam_rng_ctx *ctx = to_caam_rng_ctx(rng);
- struct buf_data *bd = &ctx->bufs[ctx->current_buf];
- int next_buf_idx, copied_idx;
+ dma_addr_t dst_dma;
int err;
- if (atomic_read(&bd->empty)) {
- /* try to submit job if there wasn't one */
- if (atomic_read(&bd->empty) == BUF_EMPTY) {
- err = submit_job(ctx, 1);
- /* if can't submit job, can't even wait */
- if (err)
- return 0;
- }
- /* no immediate data, so exit if not waiting */
- if (!wait)
- return 0;
-
- /* waiting for pending job */
- if (atomic_read(&bd->empty))
- wait_for_completion(&bd->filled);
- }
-
- next_buf_idx = ctx->cur_buf_idx + max;
- dev_dbg(ctx->jrdev, "%s: start reading at buffer %d, idx %d\n",
- __func__, ctx->current_buf, ctx->cur_buf_idx);
+ len = min_t(int, len, CAAM_RNG_MAX_FIFO_STORE_SIZE);
- /* if enough data in current buffer */
- if (next_buf_idx < RN_BUF_SIZE) {
- memcpy(data, bd->buf + ctx->cur_buf_idx, max);
- ctx->cur_buf_idx = next_buf_idx;
- return max;
+ dst_dma = dma_map_single(jrdev, dst, len, DMA_FROM_DEVICE);
+ if (dma_mapping_error(jrdev, dst_dma)) {
+ dev_err(jrdev, "unable to map destination memory\n");
+ return -ENOMEM;
}
- /* else, copy what's left... */
- copied_idx = RN_BUF_SIZE - ctx->cur_buf_idx;
- memcpy(data, bd->buf + ctx->cur_buf_idx, copied_idx);
- ctx->cur_buf_idx = 0;
- atomic_set(&bd->empty, BUF_EMPTY);
-
- /* ...refill... */
- submit_job(ctx, 1);
+ init_completion(done);
+ err = caam_jr_enqueue(jrdev,
+ caam_init_desc(desc, dst_dma, len),
+ caam_rng_done, done);
+ if (!err)
+ wait_for_completion(done);
- /* and use next buffer */
- ctx->current_buf = !ctx->current_buf;
- dev_dbg(ctx->jrdev, "switched to buffer %d\n", ctx->current_buf);
+ dma_unmap_single(jrdev, dst_dma, len, DMA_FROM_DEVICE);
- /* since there already is some data read, don't wait */
- return copied_idx + caam_read(rng, data + copied_idx,
- max - copied_idx, false);
+ return err ?: len;
}
-static inline int rng_create_sh_desc(struct caam_rng_ctx *ctx)
+static void caam_rng_maybe_refill_fifo(struct caam_rng_ctx *ctx)
{
- struct device *jrdev = ctx->jrdev;
- u32 *desc = ctx->sh_desc;
-
- init_sh_desc(desc, HDR_SHARE_SERIAL);
-
- /* Generate random bytes */
- append_operation(desc, OP_ALG_ALGSEL_RNG | OP_TYPE_CLASS1_ALG);
-
- /* Store bytes */
- append_seq_fifo_store(desc, RN_BUF_SIZE, FIFOST_TYPE_RNGSTORE);
-
- ctx->sh_desc_dma = dma_map_single(jrdev, desc, desc_bytes(desc),
- DMA_TO_DEVICE);
- if (dma_mapping_error(jrdev, ctx->sh_desc_dma)) {
- dev_err(jrdev, "unable to map shared descriptor\n");
- return -ENOMEM;
- }
+ if (kfifo_len(&ctx->fifo) <= CAAM_RNG_FIFO_LEN / 2)
+ schedule_work(&ctx->worker);
+}
- print_hex_dump_debug("rng shdesc@: ", DUMP_PREFIX_ADDRESS, 16, 4,
- desc, desc_bytes(desc), 1);
+static void caam_rng_fill_async(struct caam_rng_ctx *ctx)
+{
+ struct scatterlist sg[1];
+ struct completion done;
+ int len, nents;
+
+ sg_init_table(sg, ARRAY_SIZE(sg));
+ nents = kfifo_dma_in_prepare(&ctx->fifo, sg, ARRAY_SIZE(sg),
+ CAAM_RNG_FIFO_LEN);
+ if (!nents)
+ return;
+
+ len = caam_rng_read_one(ctx->jrdev, sg_virt(&sg[0]),
+ sg[0].length,
+ &ctx->desc[DESC_ASYNC],
+ &done);
+ if (len < 0)
+ return;
+
+ kfifo_dma_in_finish(&ctx->fifo, len);
+ caam_rng_maybe_refill_fifo(ctx);
+}
- return 0;
+static void caam_rng_worker(struct work_struct *work)
+{
+ struct caam_rng_ctx *ctx = container_of(work, struct caam_rng_ctx,
+ worker);
+ caam_rng_fill_async(ctx);
}
-static inline int rng_create_job_desc(struct caam_rng_ctx *ctx, int buf_id)
+static int caam_read(struct hwrng *rng, void *dst, size_t max, bool wait)
{
- struct device *jrdev = ctx->jrdev;
- struct buf_data *bd = &ctx->bufs[buf_id];
- u32 *desc = bd->hw_desc;
- int sh_len = desc_len(ctx->sh_desc);
+ struct caam_rng_ctx *ctx = to_caam_rng_ctx(rng);
+ int out;
- init_job_desc_shared(desc, ctx->sh_desc_dma, sh_len, HDR_SHARE_DEFER |
- HDR_REVERSE);
+ if (wait) {
+ struct completion done;
- bd->addr = dma_map_single(jrdev, bd->buf, RN_BUF_SIZE, DMA_FROM_DEVICE);
- if (dma_mapping_error(jrdev, bd->addr)) {
- dev_err(jrdev, "unable to map dst\n");
- return -ENOMEM;
+ return caam_rng_read_one(ctx->jrdev, dst, max,
+ ctx->desc[DESC_SYNC], &done);
}
- append_seq_out_ptr_intlen(desc, bd->addr, RN_BUF_SIZE, 0);
+ out = kfifo_out(&ctx->fifo, dst, max);
+ caam_rng_maybe_refill_fifo(ctx);
- print_hex_dump_debug("rng job desc@: ", DUMP_PREFIX_ADDRESS, 16, 4,
- desc, desc_bytes(desc), 1);
-
- return 0;
+ return out;
}
static void caam_cleanup(struct hwrng *rng)
{
struct caam_rng_ctx *ctx = to_caam_rng_ctx(rng);
- int i;
- struct buf_data *bd;
- for (i = 0; i < 2; i++) {
- bd = &ctx->bufs[i];
- if (atomic_read(&bd->empty) == BUF_PENDING)
- wait_for_completion(&bd->filled);
- }
-
- rng_unmap_ctx(ctx);
+ flush_work(&ctx->worker);
caam_jr_free(ctx->jrdev);
+ kfifo_free(&ctx->fifo);
}
-static int caam_init_buf(struct caam_rng_ctx *ctx, int buf_id)
+static int caam_init(struct hwrng *rng)
{
- struct buf_data *bd = &ctx->bufs[buf_id];
+ struct caam_rng_ctx *ctx = to_caam_rng_ctx(rng);
int err;
- err = rng_create_job_desc(ctx, buf_id);
- if (err)
- return err;
-
- atomic_set(&bd->empty, BUF_EMPTY);
- submit_job(ctx, buf_id == ctx->current_buf);
- wait_for_completion(&bd->filled);
+ ctx->desc = devm_kcalloc(ctx->ctrldev, DESC_NUM, sizeof(*ctx->desc),
+ GFP_DMA | GFP_KERNEL);
+ if (!ctx->desc)
+ return -ENOMEM;
- return 0;
-}
+ if (kfifo_alloc(&ctx->fifo, CAAM_RNG_FIFO_LEN, GFP_DMA | GFP_KERNEL))
+ return -ENOMEM;
-static int caam_init(struct hwrng *rng)
-{
- struct caam_rng_ctx *ctx = to_caam_rng_ctx(rng);
- int err;
+ INIT_WORK(&ctx->worker, caam_rng_worker);
ctx->jrdev = caam_jr_alloc();
err = PTR_ERR_OR_ZERO(ctx->jrdev);
if (err) {
+ kfifo_free(&ctx->fifo);
pr_err("Job Ring Device allocation for transform failed\n");
return err;
}
- err = rng_create_sh_desc(ctx);
- if (err)
- goto free_jrdev;
-
- ctx->current_buf = 0;
- ctx->cur_buf_idx = 0;
-
- err = caam_init_buf(ctx, 0);
- if (err)
- goto free_jrdev;
-
- err = caam_init_buf(ctx, 1);
- if (err)
- goto free_jrdev;
+ /*
+ * Fill async buffer to have early randomness data for
+ * hw_random
+ */
+ caam_rng_fill_async(ctx);
return 0;
-
-free_jrdev:
- caam_jr_free(ctx->jrdev);
- return err;
}
int caam_rng_init(struct device *ctrldev)
@@ -324,10 +221,12 @@ int caam_rng_init(struct device *ctrldev)
if (!rng_inst)
return 0;
- ctx = devm_kzalloc(ctrldev, sizeof(*ctx), GFP_DMA | GFP_KERNEL);
+ ctx = devm_kzalloc(ctrldev, sizeof(*ctx), GFP_KERNEL);
if (!ctx)
return -ENOMEM;
+ ctx->ctrldev = ctrldev;
+
ctx->rng.name = "rng-caam";
ctx->rng.init = caam_init;
ctx->rng.cleanup = caam_cleanup;
--
2.21.0
We shouldn't stay silent if RNG job fails. Add appropriate code to
check for that case and propagate error code up appropriately.
Signed-off-by: Andrey Smirnov <[email protected]>
Cc: Chris Healy <[email protected]>
Cc: Lucas Stach <[email protected]>
Cc: Horia Geantă <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: Iuliana Prodan <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
drivers/crypto/caam/caamrng.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/crypto/caam/caamrng.c b/drivers/crypto/caam/caamrng.c
index 3960f5c81c97..554aafbd4d11 100644
--- a/drivers/crypto/caam/caamrng.c
+++ b/drivers/crypto/caam/caamrng.c
@@ -52,6 +52,11 @@ struct caam_rng_ctx {
struct kfifo fifo;
};
+struct caam_rng_job_ctx {
+ struct completion *done;
+ int *err;
+};
+
static struct caam_rng_ctx *to_caam_rng_ctx(struct hwrng *r)
{
return container_of(r, struct caam_rng_ctx, rng);
@@ -60,12 +65,12 @@ static struct caam_rng_ctx *to_caam_rng_ctx(struct hwrng *r)
static void caam_rng_done(struct device *jrdev, u32 *desc, u32 err,
void *context)
{
- struct completion *done = context;
+ struct caam_rng_job_ctx *jctx = context;
if (err)
- caam_jr_strstatus(jrdev, err);
+ *jctx->err = caam_jr_strstatus(jrdev, err);
- complete(done);
+ complete(jctx->done);
}
static u32 *caam_init_desc(u32 *desc, dma_addr_t dst_dma, int len)
@@ -89,6 +94,10 @@ static int caam_rng_read_one(struct device *jrdev,
{
dma_addr_t dst_dma;
int err;
+ struct caam_rng_job_ctx jctx = {
+ .done = done,
+ .err = &err,
+ };
len = min_t(int, len, CAAM_RNG_MAX_FIFO_STORE_SIZE);
@@ -101,7 +110,7 @@ static int caam_rng_read_one(struct device *jrdev,
init_completion(done);
err = caam_jr_enqueue(jrdev,
caam_init_desc(desc, dst_dma, len),
- caam_rng_done, done);
+ caam_rng_done, &jctx);
if (!err)
wait_for_completion(done);
--
2.21.0
In order to follow recommendation in SP800-90C (section "9.4 The
Oversampling-NRBG Construction") limit the output of "generate" JD
submitted to CAAM. See
https://lore.kernel.org/linux-crypto/VI1PR0402MB3485EF10976A4A69F90E5B0F98580@VI1PR0402MB3485.eurprd04.prod.outlook.com/
for more details.
This change should make CAAM's hwrng driver good enough to have 999
quality rating.
Signed-off-by: Andrey Smirnov <[email protected]>
Cc: Chris Healy <[email protected]>
Cc: Lucas Stach <[email protected]>
Cc: Horia Geantă <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: Iuliana Prodan <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
drivers/crypto/caam/caamrng.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/crypto/caam/caamrng.c b/drivers/crypto/caam/caamrng.c
index 91ccde0240fe..2b75ffffcac9 100644
--- a/drivers/crypto/caam/caamrng.c
+++ b/drivers/crypto/caam/caamrng.c
@@ -23,7 +23,7 @@
#include "error.h"
/* length of descriptors */
-#define CAAM_RNG_MAX_FIFO_STORE_SIZE U16_MAX
+#define CAAM_RNG_MAX_FIFO_STORE_SIZE 16
#define CAAM_RNG_FIFO_LEN SZ_32K /* Must be a multiple of 2 */
@@ -134,7 +134,7 @@ static void caam_rng_fill_async(struct caam_rng_ctx *ctx)
sg_init_table(sg, ARRAY_SIZE(sg));
nents = kfifo_dma_in_prepare(&ctx->fifo, sg, ARRAY_SIZE(sg),
- CAAM_RNG_FIFO_LEN);
+ CAAM_RNG_MAX_FIFO_STORE_SIZE);
if (!nents)
return;
@@ -241,6 +241,7 @@ int caam_rng_init(struct device *ctrldev)
ctx->rng.init = caam_init;
ctx->rng.cleanup = caam_cleanup;
ctx->rng.read = caam_read;
+ ctx->rng.quality = 999;
dev_info(ctrldev, "registering rng-caam\n");
--
2.21.0
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 <[email protected]>
Cc: Chris Healy <[email protected]>
Cc: Lucas Stach <[email protected]>
Cc: Horia Geantă <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: Iuliana Prodan <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
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(-)
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];
};
--
2.21.0
In order to make sure that we always use non-stale entropy data, change
the code to invalidate entropy register during RNG initialization.
Signed-off-by: Aymen Sghaier <[email protected]>
Signed-off-by: Vipul Kumar <[email protected]>
[[email protected] ported to upstream kernel, rewrote commit msg]
Signed-off-by: Andrey Smirnov <[email protected]>
Cc: Chris Healy <[email protected]>
Cc: Lucas Stach <[email protected]>
Cc: Horia Geantă <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: Iuliana Prodan <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
drivers/crypto/caam/ctrl.c | 11 ++++++++---
drivers/crypto/caam/regs.h | 3 ++-
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index c99a6a3b22de..22d8676dd610 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -338,8 +338,12 @@ static void kick_trng(struct platform_device *pdev, int ent_delay)
ctrl = (struct caam_ctrl __iomem *)ctrlpriv->ctrl;
r4tst = &ctrl->r4tst[0];
- /* put RNG4 into program mode */
- clrsetbits_32(&r4tst->rtmctl, 0, RTMCTL_PRGM);
+ /*
+ * Setting both RTMCTL:PRGM and RTMCTL:TRNG_ACC causes TRNG to
+ * properly invalidate the entropy in the entropy register and
+ * force re-generation.
+ */
+ clrsetbits_32(&r4tst->rtmctl, 0, RTMCTL_PRGM | RTMCTL_ACC);
/*
* Performance-wise, it does not make sense to
@@ -369,7 +373,8 @@ static void kick_trng(struct platform_device *pdev, int ent_delay)
* select raw sampling in both entropy shifter
* and statistical checker; ; put RNG4 into run mode
*/
- clrsetbits_32(&r4tst->rtmctl, RTMCTL_PRGM, RTMCTL_SAMP_MODE_RAW_ES_SC);
+ clrsetbits_32(&r4tst->rtmctl, RTMCTL_PRGM | RTMCTL_ACC,
+ RTMCTL_SAMP_MODE_RAW_ES_SC);
}
static int caam_get_era_from_hw(struct caam_ctrl __iomem *ctrl)
diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h
index 05127b70527d..c191e8fd0fa7 100644
--- a/drivers/crypto/caam/regs.h
+++ b/drivers/crypto/caam/regs.h
@@ -487,7 +487,8 @@ struct rngtst {
/* RNG4 TRNG test registers */
struct rng4tst {
-#define RTMCTL_PRGM 0x00010000 /* 1 -> program mode, 0 -> run mode */
+#define RTMCTL_ACC BIT(5) /* TRNG access mode */
+#define RTMCTL_PRGM BIT(16) /* 1 -> program mode, 0 -> run mode */
#define RTMCTL_SAMP_MODE_VON_NEUMANN_ES_SC 0 /* use von Neumann data in
both entropy shifter and
statistical checker */
--
2.21.0
On 1/8/2020 5:42 PM, Andrey Smirnov wrote:
> @@ -342,18 +324,16 @@ int caam_rng_init(struct device *ctrldev)
> if (!rng_inst)
> return 0;
>
> - rng_ctx = kmalloc(sizeof(*rng_ctx), GFP_DMA | GFP_KERNEL);
> - if (!rng_ctx)
> + ctx = devm_kzalloc(ctrldev, sizeof(*ctx), GFP_DMA | GFP_KERNEL);
> + if (!ctx)
> return -ENOMEM;
>
> - dev_info(ctrldev, "registering rng-caam\n");
> + ctx->rng.name = "rng-caam";
> + ctx->rng.init = caam_init;
> + ctx->rng.cleanup = caam_cleanup;
> + ctx->rng.read = caam_read;
>
> - err = hwrng_register(&caam_rng);
> - if (!err) {
> - init_done = true;
> - return err;
> - }
> + dev_info(ctrldev, "registering rng-caam\n");
>
> - kfree(rng_ctx);
> - return err;
> + return devm_hwrng_register(ctrldev, &ctx->rng);
This means hwrng_unregister() is called only when ctrldev is removed.
OTOH caam_rng_init() could be called multiple times, e.g. if there's only one
jrdev left in the system and it's removed then added back.
This will lead to caam_rng_init() -> hwrng_register() called twice
with the same "rng-caam" name, without a hwrng_unregister() called in-between.
Horia
On 1/8/2020 5:42 PM, Andrey Smirnov wrote:
> In order to follow recommendation in SP800-90C (section "9.4 The
> Oversampling-NRBG Construction") limit the output of "generate" JD
> submitted to CAAM. See
> https://lore.kernel.org/linux-crypto/VI1PR0402MB3485EF10976A4A69F90E5B0F98580@VI1PR0402MB3485.eurprd04.prod.outlook.com/
> for more details.
>
> This change should make CAAM's hwrng driver good enough to have 999
> quality rating.
>
[...]
> @@ -241,6 +241,7 @@ int caam_rng_init(struct device *ctrldev)
> ctx->rng.init = caam_init;
> ctx->rng.cleanup = caam_cleanup;
> ctx->rng.read = caam_read;
> + ctx->rng.quality = 999;
>
AFAICS the maximum value of hwrng.quality is 1024.
Any reason why it's configured to be lower, now that CAAM RNG-based DRBG
is configured to reseed as requested by FIPS spec to behave as a TRNG?
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)
> + 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 Mon, Jan 13, 2020 at 6:10 AM Horia Geanta <[email protected]> wrote:
>
> On 1/8/2020 5:42 PM, Andrey Smirnov wrote:
> > In order to follow recommendation in SP800-90C (section "9.4 The
> > Oversampling-NRBG Construction") limit the output of "generate" JD
> > submitted to CAAM. See
> > https://lore.kernel.org/linux-crypto/VI1PR0402MB3485EF10976A4A69F90E5B0F98580@VI1PR0402MB3485.eurprd04.prod.outlook.com/
> > for more details.
> >
> > This change should make CAAM's hwrng driver good enough to have 999
> > quality rating.
> >
> [...]
> > @@ -241,6 +241,7 @@ int caam_rng_init(struct device *ctrldev)
> > ctx->rng.init = caam_init;
> > ctx->rng.cleanup = caam_cleanup;
> > ctx->rng.read = caam_read;
> > + ctx->rng.quality = 999;
> >
> AFAICS the maximum value of hwrng.quality is 1024.
>
> Any reason why it's configured to be lower, now that CAAM RNG-based DRBG
> is configured to reseed as requested by FIPS spec to behave as a TRNG?
>
Only my reading of the old version of corresponding documentation
which listed this field as being per mil. Will fix in v7.
Thanks,
Andrey Smirnov
On Mon, Jan 13, 2020 at 1:41 AM Horia Geanta <[email protected]> wrote:
>
> On 1/8/2020 5:42 PM, Andrey Smirnov wrote:
> > @@ -342,18 +324,16 @@ int caam_rng_init(struct device *ctrldev)
> > if (!rng_inst)
> > return 0;
> >
> > - rng_ctx = kmalloc(sizeof(*rng_ctx), GFP_DMA | GFP_KERNEL);
> > - if (!rng_ctx)
> > + ctx = devm_kzalloc(ctrldev, sizeof(*ctx), GFP_DMA | GFP_KERNEL);
> > + if (!ctx)
> > return -ENOMEM;
> >
> > - dev_info(ctrldev, "registering rng-caam\n");
> > + ctx->rng.name = "rng-caam";
> > + ctx->rng.init = caam_init;
> > + ctx->rng.cleanup = caam_cleanup;
> > + ctx->rng.read = caam_read;
> >
> > - err = hwrng_register(&caam_rng);
> > - if (!err) {
> > - init_done = true;
> > - return err;
> > - }
> > + dev_info(ctrldev, "registering rng-caam\n");
> >
> > - kfree(rng_ctx);
> > - return err;
> > + return devm_hwrng_register(ctrldev, &ctx->rng);
> This means hwrng_unregister() is called only when ctrldev is removed.
>
> OTOH caam_rng_init() could be called multiple times, e.g. if there's only one
> jrdev left in the system and it's removed then added back.
> This will lead to caam_rng_init() -> hwrng_register() called twice
> with the same "rng-caam" name, without a hwrng_unregister() called in-between.
>
True, but the logic you describe is broken in reality due to circular
reference from HWRNG, which we never fixed. I'll fix both in v7.
Thanks,
Andrey Smirnov