Re: [PATCH v3 14/19] crypto: cmh - add ECDH/X25519 kpp

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Hardware datastore slots allocated for the ECDH shared secret are permanently leaked on success and error paths.
- [Medium] Phase 2 of shared secret computation violates the async crypto API contract by unconditionally enabling backlog and bubbling spurious `-EINPROGRESS` notifications.
- [Medium] The driver bypasses FIPS 186-5 validation for caller-provided private scalars on NIST curves, allowing degenerate or out-of-bounds keys.
--

--- Patch [14]: [PATCH v3 14/19] crypto: cmh - add ECDH/X25519 kpp ---
commit 9f2cb0033e85cf322ca1c6ba96a2932f498d8824
Author: Alex Ousherovitch <[email protected]>

crypto: cmh - add ECDH/X25519 kpp

Register ECDH and X25519 kpp algorithms using the CMH PKE core.
Supports P-256, P-384, and Curve25519 for key agreement.

> diff --git a/drivers/crypto/cmh/cmh_pke_ecdh.c b/drivers/crypto/cmh/cmh_pke_ecdh.c
> new file mode 100644
> index 0000000000000..1b49dd060e52c
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_pke_ecdh.c

[ ... ]

> +static int cmh_ecdh_set_secret_nist(struct crypto_kpp *tfm,
> +				    const void *buf, unsigned int len)
> +{

[ ... ]

> +	if (key_size == 0) {
> +		/*
> +		 * key_size == 0: generate a validated random private key.
> +		 * Uses the kernel ECC library (FIPS 186-5 A.2.2) to ensure
> +		 * the scalar is in the valid range [2, n-3] for the curve.
> +		 */

[ ... ]

> +		ret = cmh_key_setkey_raw(&ctx->key, rnd, ctx->clen,
> +					 CORE_ID_PKE);
> +		kfree_sensitive(rnd);
> +		return ret;
> +	}
> +
> +	if (key_size != ctx->clen)
> +		return -EINVAL;
> +
> +	if (secret.len != ECDH_KPP_SECRET_MIN_SIZE + key_size)
> +		return -EINVAL;
> +
> +	return cmh_key_setkey_raw(&ctx->key, ptr, key_size, CORE_ID_PKE);

[Severity: Medium]
Are we missing a validity check for caller-provided private scalars?

When key_size != 0, we write the raw private key directly into the key
context without calling ecc_is_key_valid() to verify it falls within the
valid range [1, n-1] for the NIST curve.

> +}

[ ... ]

> +static void cmh_ecdh_ss_phase1_complete(void *data, int error)
> +{

[ ... ]

> +	/* Read the DS reference written by Phase 1 */
> +	cmh_dma_sync_for_cpu(rctx->ref_dma, sizeof(u64), DMA_FROM_DEVICE);
> +	cmh_dma_unmap_single(rctx->ref_dma, sizeof(u64), DMA_FROM_DEVICE);
> +	rctx->ref_dma = DMA_MAPPING_ERROR;
> +
> +	/* Phase 2: extract shared secret from DS */
> +	vcq_set_header(&vcq[0], 3);
> +	vcq_add_sys_data(&vcq[1], *rctx->ref_buf, rctx->ss_dma,
> +			 rctx->clen);
> +	vcq[1].id |= rctx->dma_swap;
> +	vcq_add_sys_flush(&vcq[2]);
> +
> +	ret = cmh_tm_submit_async(vcq, 3, 1, rctx->mbx_idx,
> +				  cmh_ecdh_ss_phase2_complete, req,
> +				  true, 0);

[Severity: Medium]
Can this violate the async crypto API contract by bubbling spurious
-EINPROGRESS notifications?

Phase 2 submission hardcodes the backlog_ok parameter to true, which ignores
the user's req->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG setting.

If the queue is full and Phase 2 is backlogged, cmh_ecdh_ss_phase2_complete()
will pass -EINPROGRESS to cmh_complete(), even if the caller did not allow
backlogging.

> +	if (ret == -EBUSY || !ret)
> +		return;
> +
> +	error = ret;
> +
> +out_cleanup:
> +	if (!cmh_dma_map_error(rctx->ref_dma))
> +		cmh_dma_unmap_single(rctx->ref_dma, sizeof(u64),
> +				     DMA_FROM_DEVICE);
> +	if (!cmh_dma_map_error(rctx->ss_dma))
> +		cmh_dma_unmap_single(rctx->ss_dma, rctx->clen,
> +				     DMA_FROM_DEVICE);
> +	kfree(rctx->ref_buf);
> +	rctx->ref_buf = NULL;
> +	kfree_sensitive(rctx->ss_buf);
> +	rctx->ss_buf = NULL;
> +	cmh_complete(&req->base, error);
> +}

[ ... ]

> +static int cmh_ecdh_compute_shared_secret(struct kpp_request *req)
> +{

[ ... ]

> +	dd = cmh_core_select_instance(CMH_CORE_PKE);
> +	rctx->mbx_idx = dd.mbx_idx;
> +
> +	vcq_set_header(&vcq[0], 5);
> +	idx = 1;
> +	vcq_add_sys_write(&vcq[idx], SYS_REF_TEMP, ctx->key.raw.dma,
> +			  SYS_REF_NONE, ctx->key.raw.len,
> +			  ctx->key.raw.sys_type);
> +	vcq[idx].id |= dma_swap;
> +	idx++;
> +	vcq_add_sys_new(&vcq[idx++], 0, rctx->ref_dma, clen);

[Severity: High]
Does this leak the hardware datastore slot?

We allocate a DS slot using SYS_CMD_NEW, but neither the success path in
cmh_ecdh_ss_phase2_complete() nor the error path in
cmh_ecdh_ss_phase1_complete() appear to issue a SYS_CMD_GRANT with 0
permissions to delete the object.

Could this eventually exhaust the hardware datastore memory on repeated
operations?

> +	vcq_add_pke_ecdh(&vcq[idx++], dd.core_id, ctx->curve, clen,
> +			 clen, ss_type, rctx->peer_dma,
> +			 SYS_REF_TEMP, SYS_REF_LAST, swap);
> +	vcq_add_pke_flush(&vcq[idx++], dd.core_id);
> +
> +	ret = cmh_tm_submit_async(vcq, 5, 1, dd.mbx_idx,
> +				  cmh_ecdh_ss_phase1_complete, req,
> +				  !!(req->base.flags &
> +				     CRYPTO_TFM_REQ_MAY_BACKLOG), 0);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=14