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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Hardcoded PKE_SWAP_FLAGS breaks X25519 (Curve25519) coordinate endianness.
- [High] The Phase 1 completion handler unconditionally allows backlogging for Phase 2, ignoring the caller's CRYPTO_TFM_REQ_MAY_BACKLOG flag.
- [High] Hardware Datastore (DS) object leak when Phase 2 submission fails.
- [Medium] Unnecessary kmemdup and double DMA-mapping of the private key violates DMA API rules.
- [Medium] Failure to update req->dst_len on insufficient output buffer.
--

--- Patch [14]: [PATCH v2 14/19] crypto: cmh - add ECDH/X25519 kpp ---
commit c7f55fee9fddee53ac321aab50b8d12f127bfd39
Author: Alex Ousherovitch <aousherovitch@rambus.com>
Date:   Thu Jul 9 20:30:32 2026 +0000

    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..d8b821cc4217c
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_pke_ecdh.c

[ ... ]

> +static int cmh_ecdh_generate_public_key(struct kpp_request *req)
> +{
> +	struct crypto_kpp *tfm = crypto_kpp_reqtfm(req);
> +	struct cmh_ecdh_tfm_ctx *ctx = cmh_ecdh_ctx(tfm);
> +	struct cmh_ecdh_reqctx *rctx = kpp_request_ctx(req);
> +	u32 clen = ctx->clen;
> +	bool is_25519 = (ctx->curve == PKE_CURVE_25519);
> +	u32 out_len = is_25519 ? clen : 2 * clen;
> +	struct vcq_cmd vcq[PKE_VCQ_CMDS_MAX];
> +	struct core_dispatch dd;
> +	u32 swap, dma_swap;
> +	int ret, idx;
> +	gfp_t gfp;
> +
> +	if (ctx->key.mode != CMH_KEY_RAW)
> +		return -EINVAL;
[Severity: Medium]
Will this return -EINVAL without setting req->dst_len to out_len first?
The KPP API typically expects the driver to update req->dst_len with the
required size before returning an error so the caller can allocate a large
enough buffer.

> +	if (req->dst_len < out_len)
> +		return -EINVAL;
> +
> +	gfp = req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP ?
> +	      GFP_KERNEL : GFP_ATOMIC;

[ ... ]

> +	swap = PKE_SWAP_FLAGS;
> +	dma_swap = pke_swap_flags(ctx->curve);
> +
> +	dd = cmh_core_select_instance(CMH_CORE_PKE);
> +
[Severity: Medium]
Does this code unnecessarily copy and double-map the private key?
The ctx->key.raw.data buffer is already DMA-mapped as DMA_TO_DEVICE in
cmh_key_setkey_raw(). Reading this buffer via kmemdup() without a prior
dma_sync_single_for_cpu() violates DMA API rules, and the duplication
adds overhead. Could ctx->key.raw.dma be passed directly to the hardware?

> +	rctx->sk_buf = kmemdup(ctx->key.raw.data, ctx->key.raw.len, gfp);
> +	if (!rctx->sk_buf) {
> +		ret = -ENOMEM;
> +		goto out_unmap;
> +	}
> +	rctx->sk_dma = cmh_dma_map_single(rctx->sk_buf, ctx->key.raw.len,
> +					  DMA_TO_DEVICE);
> +	if (cmh_dma_map_error(rctx->sk_dma)) {
> +		ret = -ENOMEM;
> +		goto out_unmap;
> +	}
> +
> +	vcq_set_header(&vcq[0], PKE_VCQ_CMDS_MAX);
> +	idx = 1;
> +	vcq_add_sys_write(&vcq[idx], SYS_REF_TEMP, rctx->sk_dma,
> +			  SYS_REF_NONE, ctx->key.raw.len,
> +			  ctx->key.raw.sys_type);
> +	vcq[idx].id |= dma_swap;
> +	idx++;
[Severity: High]
Does hardcoding swap to PKE_SWAP_FLAGS break coordinate endianness for Curve25519?
Since pke_swap_flags(ctx->curve) returns 0 for X25519 but swap unconditionally
applies PKE_SWAP_FLAGS, won't this force incorrect hardware byteswapping on
inputs and outputs for Curve25519? Should this be using dma_swap instead?

> +	if (is_25519)
> +		vcq_add_pke_ecdh_keygen(&vcq[idx++], dd.core_id, ctx->curve,
> +					clen, rctx->pk_dma, SYS_REF_TEMP,
> +					swap);
> +	else

[ ... ]

> +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]);
> +
[Severity: High]
Does passing true unconditionally for backlog_ok ignore the caller's requested flags?
If the caller did not set CRYPTO_TFM_REQ_MAY_BACKLOG in req->base.flags,
a backlogged Phase 2 could trigger an unexpected -EINPROGRESS completion callback.
Could this lead to the caller prematurely freeing the request structure, resulting
in a use-after-free when the final completion executes?

> +	ret = cmh_tm_submit_async(vcq, 3, 1, rctx->mbx_idx,
> +				  cmh_ecdh_ss_phase2_complete, req,
> +				  true, 0);
[Severity: High]
If cmh_tm_submit_async() fails here, do we leak the hardware Datastore (DS) object?
Phase 1 created a DS object in hardware memory. If we hit an error here and jump
to out_cleanup, it looks like we exit without sending a SYS_CMD_FLUSH command
to the hardware, which would strand the object in the mailbox memory permanently.

> +	if (ret == -EBUSY || !ret)
> +		return;
> +
> +	error = ret;
> +
> +out_cleanup:
> +	if (!cmh_dma_map_error(rctx->ref_dma))

[ ... ]

> +static int cmh_ecdh_compute_shared_secret(struct kpp_request *req)
> +{
> +	struct crypto_kpp *tfm = crypto_kpp_reqtfm(req);
> +	struct cmh_ecdh_tfm_ctx *ctx = cmh_ecdh_ctx(tfm);
> +	struct cmh_ecdh_reqctx *rctx = kpp_request_ctx(req);
> +	u32 clen = ctx->clen;
> +	bool is_25519 = (ctx->curve == PKE_CURVE_25519);
> +	u32 peer_len = is_25519 ? clen : 2 * clen;
> +	u32 ss_type = SYS_TYPE_SET(SYS_TYPE_FLAG_PT, CORE_ID_PKE);
> +	struct vcq_cmd vcq[5];
> +	struct core_dispatch dd;
> +	u32 swap, dma_swap;
> +	int ret, idx, nents;
> +	gfp_t gfp;
> +
> +	if (ctx->key.mode != CMH_KEY_RAW)
> +		return -EINVAL;
[Severity: Medium]
Will this return -EINVAL without setting req->dst_len to clen first?
Similar to cmh_ecdh_generate_public_key(), the caller needs to know the required
size if the destination buffer is too small.

> +	if (req->src_len < peer_len || req->dst_len < clen)
> +		return -EINVAL;
> +
> +	gfp = req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP ?
> +	      GFP_KERNEL : GFP_ATOMIC;

[ ... ]

> +	if (cmh_dma_map_error(rctx->peer_dma) ||
> +	    cmh_dma_map_error(rctx->ss_dma) ||
> +	    cmh_dma_map_error(rctx->ref_dma)) {
> +		ret = -ENOMEM;
> +		goto out_unmap;
> +	}
> +
> +	swap = PKE_SWAP_FLAGS;
> +	dma_swap = pke_swap_flags(ctx->curve);
> +	rctx->dma_swap = dma_swap;
> +
> +	dd = cmh_core_select_instance(CMH_CORE_PKE);
> +	rctx->mbx_idx = dd.mbx_idx;
> +
[Severity: Medium]
Does this code unnecessarily copy and double-map the private key?
The ctx->key.raw.data buffer is already DMA-mapped as DMA_TO_DEVICE in
cmh_key_setkey_raw(). Reading this buffer via kmemdup() without a prior
dma_sync_single_for_cpu() violates DMA API rules, and the duplication
adds overhead. Could ctx->key.raw.dma be passed directly to the hardware?

> +	rctx->sk_buf = kmemdup(ctx->key.raw.data, ctx->key.raw.len, gfp);
> +	if (!rctx->sk_buf) {
> +		ret = -ENOMEM;
> +		goto out_unmap;
> +	}
> +	rctx->sk_dma = cmh_dma_map_single(rctx->sk_buf, ctx->key.raw.len,
> +					  DMA_TO_DEVICE);
> +	if (cmh_dma_map_error(rctx->sk_dma)) {
> +		ret = -ENOMEM;
> +		goto out_unmap;
> +	}
> +
> +	vcq_set_header(&vcq[0], 5);
> +	idx = 1;
> +	vcq_add_sys_write(&vcq[idx], SYS_REF_TEMP, rctx->sk_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 hardcoding swap to PKE_SWAP_FLAGS break coordinate endianness for Curve25519?
Since pke_swap_flags(ctx->curve) returns 0 for X25519 but swap unconditionally
applies PKE_SWAP_FLAGS, won't this force incorrect hardware byteswapping on
inputs and outputs for Curve25519? Should this be using dma_swap instead?

> +	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);
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709203037.1884436-1-skrishnamoorthy@rambus.com?part=14