| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <f91ef72b-dfd0-0e86-59e5-f0d0ff3a959c@nxp.com>
Date: Tue, 5 Apr 2022 15:31:42 +0300
From: Horia Geantă <horia.geanta@....com>
To: Meenakshi Aggarwal <meenakshi.aggarwal@....com>,
Pankaj Gupta <pankaj.gupta@....com>,
Gaurav Jain <gaurav.jain@....com>,
Varun Sethi <V.Sethi@....com>,
Herbert Xu <herbert@...dor.apana.org.au>,
"David S . Miller" <davem@...emloft.net>
Cc: "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
dl-linux-imx <linux-imx@....com>,
kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v2 1/1] crypto: caam/rng: Add support for PRNG
On 3/16/2022 8:02 PM, Meenakshi Aggarwal wrote:
> From: Meenakshi Aggarwal <meenakshi.aggarwal@....com>
>
> Add support for random number generation using PRNG
> mode of CAAM and expose the interface through crypto API.
>
According to the RM, the HW implementation of the DRBG follows
NIST SP 800-90A specification for DRBG_Hash SHA-256 function
(https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-90Ar1.pdf).
This should be mentioned in the commit message at minimum.
> Reported-by: kernel test robot <lkp@...el.com>
This isn't required and doesn't make sense once you've squashed the fix.
> +/* prng per-device context */
> +struct caam_prng_ctx {
> + struct device *jrdev;
jrdev doesn't have to be saved in this struct, it's lifetime is very limited.
> + struct completion done;
> +};
> +
> +struct caam_prng_alg {
> + struct rng_alg rng;
> + bool registered;
> +};
> +
> +static void caam_prng_done(struct device *jrdev, u32 *desc, u32 err,
> + void *context)
> +{
> + struct caam_prng_ctx *jctx = context;
> +
> + if (err)
> + caam_jr_strstatus(jrdev, err);
The error returned by caam_jr_strstatus() should be propagated back to
who initially enqueued the corresponding.
For this purpose, struct caam_prng_ctx could be extended with an "err" member.
> +
> + complete(&jctx->done);
> +}
> +
> +static int caam_prng_generate(struct crypto_rng *tfm,
> + const u8 *src, unsigned int slen,
> + u8 *dst, unsigned int dlen)
> +{
> + struct caam_prng_ctx ctx;
> + dma_addr_t dst_dma;
> + u32 *desc;
> + u8 *buf;
> + int ret;
> +
> + buf = kzalloc(dlen, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + ctx.jrdev = caam_jr_alloc();
> + ret = PTR_ERR_OR_ZERO(ctx.jrdev);
> + if (ret) {
> + pr_err("Job Ring Device allocation failed\n");
> + kfree(buf);
> + return ret;
> + }
> +
> + desc = kzalloc(CAAM_PRNG_DESC_LEN, GFP_KERNEL | GFP_DMA);
> + if (!desc) {
> + caam_jr_free(ctx.jrdev);
> + kfree(buf);
> + return -ENOMEM;
Please fix the error handling to reuse the free code
at the end of the function.
> + }
> +
> + dst_dma = dma_map_single(ctx.jrdev, buf, dlen, DMA_FROM_DEVICE);
> + if (dma_mapping_error(ctx.jrdev, dst_dma)) {
> + dev_err(ctx.jrdev, "Failed to map destination buffer memory\n");
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + init_completion(&ctx.done);
> + ret = caam_jr_enqueue(ctx.jrdev,
> + caam_init_prng_desc(desc, dst_dma, dlen),
> + caam_prng_done, &ctx);
> +
> + if (ret == -EINPROGRESS) {
> + wait_for_completion(&ctx.done);
> + ret = 0;
> + }
> +
> + dma_unmap_single(ctx.jrdev, dst_dma, dlen, DMA_FROM_DEVICE);
> +
> + memcpy(dst, buf, dlen);
I am a bit worried wrt. performance, considering the memory allocations
and the memcpy on the hotpath.
Previous version of CAAM PRNG driver was getting ~ 200 MB/s on LS
and 50 MB/s on i.MX8.
How does the current version compare?
Given that there's no prefetch buffer and there are memory allocation, copy
operations on the hotpath, I'd expect a hefty penalty.
> +out:
> + caam_jr_free(ctx.jrdev);
> + kfree(desc);
> + kfree(buf);
> + return ret;
> +}
> +
> +static void caam_prng_exit(struct crypto_tfm *tfm) {}
> +
> +static int caam_prng_init(struct crypto_tfm *tfm)
> +{
> + return 0;
> +}
> +
> +static int caam_prng_seed(struct crypto_rng *tfm,
> + const u8 *seed, unsigned int slen)
> +{
> + struct caam_prng_ctx ctx;
> + dma_addr_t seed_dma;
> + u32 *desc;
> + u8 *buf;
> + int ret = 0;
> +
> + if (seed == NULL) {
> + pr_err("Seed not provided\n");
> + return ret;
> + }
> +
> + buf = kzalloc(slen, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + ctx.jrdev = caam_jr_alloc();
> + ret = PTR_ERR_OR_ZERO(ctx.jrdev);
> + if (ret) {
> + pr_err("Job Ring Device allocation failed\n");
> + kfree(buf);
> + return ret;
> + }
> +
> + desc = kzalloc(CAAM_PRNG_DESC_LEN, GFP_KERNEL | GFP_DMA);
> + if (!desc) {
> + caam_jr_free(ctx.jrdev);
> + kfree(buf);
> + return -ENOMEM;
Same here, error handling at the end of the function should be reused.
> + }
> +
> + memcpy(buf, seed, slen);
> +
> + seed_dma = dma_map_single(ctx.jrdev, buf, slen, DMA_FROM_DEVICE);
> + if (dma_mapping_error(ctx.jrdev, seed_dma)) {
> + dev_err(ctx.jrdev, "Failed to map seed buffer memory\n");
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + init_completion(&ctx.done);
> + ret = caam_jr_enqueue(ctx.jrdev,
> + caam_init_reseed_desc(desc, seed_dma, slen),
> + caam_prng_done, &ctx);
> +
> + if (ret == -EINPROGRESS) {
> + wait_for_completion(&ctx.done);
> + ret = 0;
> + }
> +
> + dma_unmap_single(ctx.jrdev, seed_dma, slen, DMA_FROM_DEVICE);
> +
> +out:
> + caam_jr_free(ctx.jrdev);
> + kfree(desc);
> + kfree(buf);
> + return ret;
> +}
> +
> +static struct caam_prng_alg caam_prng_alg = {
> + .rng = {
> + .generate = caam_prng_generate,
> + .seed = caam_prng_seed,
> + .seedsize = 32,
seedsize should be set to 0, HW does not need an externally-provided seed
since it fetches it internally from TRNG.
> +int caam_prng_register(struct device *ctrldev)
> +{
> + struct caam_drv_private *priv = dev_get_drvdata(ctrldev);
> + u32 rng_inst;
> + int ret = 0;
> +
> + /* Check for available RNG blocks before registration */
> + if (priv->era < 10)
> + rng_inst = (rd_reg32(&priv->jr[0]->perfmon.cha_num_ls) &
> + CHA_ID_LS_RNG_MASK) >> CHA_ID_LS_RNG_SHIFT;
> + else
> + rng_inst = rd_reg32(&priv->jr[0]->vreg.rng) & CHA_VER_NUM_MASK;
> +
> + if (!rng_inst) {
> + dev_dbg(ctrldev, "RNG block is not available... skipping registering algorithm\n");
> + return ret;
> + }
> +
> + ret = crypto_register_rng(&caam_prng_alg.rng);
> + if (ret) {
> + dev_err(ctrldev,
> + "couldn't register rng crypto alg: %d\n",
> + ret);
> + return ret;
> + }
> +
> + caam_prng_alg.registered = true;
> +
> + dev_info(ctrldev,
> + "rng crypto API alg registered %s\n", caam_prng_alg.rng.base.cra_name);
driver_name should be printed, it's more specific / unique.
> diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
> index 7f2b1101f567..11849362f912 100644
> --- a/drivers/crypto/caam/jr.c
> +++ b/drivers/crypto/caam/jr.c
> @@ -39,6 +39,7 @@ static void register_algs(struct caam_drv_private_jr *jrpriv,
> caam_algapi_hash_init(dev);
> caam_pkc_init(dev);
> jrpriv->hwrng = !caam_rng_init(dev);
> + caam_prng_register(dev);
> caam_qi_algapi_init(dev);
>
> algs_unlock:
> @@ -56,6 +57,7 @@ static void unregister_algs(void)
>
> caam_pkc_exit();
> caam_algapi_hash_exit();
> + caam_prng_unregister(NULL);
Unregistering order should be the reverse order of registering.
Horia
Powered by blists - more mailing lists