linux-kernel - Re: [PATCH v4 1/1] crypto: caam/rng: Add support for PRNG

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8878ca42-6660-2d02-5bcd-2b4be4373914@nxp.com>
Date:   Fri, 29 Apr 2022 14:01:33 +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>
Subject: Re: [PATCH v4 1/1] crypto: caam/rng: Add support for PRNG

On 4/29/2022 11:45 AM, 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
> 
> Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@....com>

There are a few nitpicks below, if you can address them great,
either way here's my
Reviewed-by: Horia Geantă <horia.geanta@....com>

> + * Length of used descriptors, see caam_init_desc()
caam_init_desc() does not exist in this file.

> + */
> +#define CAAM_PRNG_DESC_LEN (CAAM_CMD_SZ +				\
> +			    CAAM_CMD_SZ +				\
> +			    CAAM_CMD_SZ + CAAM_PTR_SZ_MAX)
The reseed descriptor length is now smaller.
Memmory allocated by kmalloc() will probably be the same,
but code would be more readable.
If you want to use the same length for both descriptors,
I suggest renaming the define to CAAM_PRNG_MAX_DESC_LEN.

> +static u32 *caam_init_reseed_desc(u32 *desc)
> +{
> +	init_job_desc(desc, 0);	/* + 1 cmd_sz */
> +	/* Generate random bytes: + 1 cmd_sz */
This is a "reseed" operation, not "generate".

> +	append_operation(desc, OP_TYPE_CLASS1_ALG | OP_ALG_ALGSEL_RNG |
> +			OP_ALG_AS_FINALIZE);
> +
[...]
> +static int caam_prng_generate(struct crypto_rng *tfm,
> +			     const u8 *src, unsigned int slen,
> +			     u8 *dst, unsigned int dlen)
> +{
[...]
> +	ret = caam_jr_enqueue(jrdev,
> +			      caam_init_prng_desc(desc, dst_dma, dlen),
> +			      caam_prng_done, &ctx);
> +
> +	if (ret == -EINPROGRESS) {
> +		wait_for_completion(&ctx.done);
> +		ret = ctx.err;
> +	}
> +
> +	dma_unmap_single(jrdev, dst_dma, dlen, DMA_FROM_DEVICE);
> +
> +	memcpy(dst, buf, dlen);
memcpy() should be performed only when the HW executed the operation
successfully.

Thanks,
Horia