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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Lack of synchronization in `cmh_ecdh_scratch_free` causes a data race and Use-After-Free/NULL-dereference with active TFMs.
- [High] Async PKE operations directly reference `ctx->key.raw.dma` without taking a reference, leading to a DMA Use-After-Free if `set_secret` is called concurrently.
--

commit 9c7dc04a32751847394f5938e43b1f3fa2bfd7c8
Author: Alex Ousherovitch <aousherovitch@rambus.com>

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..2264fea1facbb
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_pke_ecdh.c
[ ... ]
> +static int cmh_ecdh_generate_public_key(struct kpp_request *req)
> +{
[ ... ]
> +	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);

[Severity: High]
Does this code risk a DMA use-after-free if the key is updated concurrently?

This queues an asynchronous hardware command using the transform's key DMA
address (ctx->key.raw.dma) as the data source, and then returns -EINPROGRESS.
However, the kernel crypto API does not serialize set_secret against in-flight
asynchronous operations.

If set_secret is called concurrently, cmh_ecdh_commit_key() delegates to
cmh_key_setkey_raw(), which destroys the key:

drivers/crypto/cmh/cmh_key.c:cmh_key_destroy() {
	if (ctx->mode == CMH_KEY_RAW && ctx->raw.data) {
		cmh_dma_unmap_single(ctx->raw.dma, ctx->raw.len,
				     DMA_TO_DEVICE);
		kfree_sensitive(ctx->raw.data);
	...
}

This immediately executes cmh_dma_unmap_single() and kfree_sensitive() on the
key material without waiting for pending operations to complete. Could this
allow the hardware to read from unmapped or freed memory?

[ ... ]
> +static void cmh_ecdh_scratch_free(void)
> +{
> +	u32 i;
> +
> +	if (!cmh_ecdh_key_scratch)
> +		return;
> +
> +	for (i = 0; i < cmh_ecdh_scratch_count; i++) {
> +		struct vcq_cmd vcq[3];
> +
> +		if (!cmh_ecdh_key_scratch[i])
> +			continue;
> +
> +		vcq_set_header(&vcq[0], 3);
> +		vcq_add_sys_grant(&vcq[1], cmh_ecdh_key_scratch[i], 0, 0, 0);
> +		vcq_add_sys_flush(&vcq[2]);
> +		cmh_tm_submit_sync_mbx(vcq, 3, 1, (s32)i);
> +		cmh_ecdh_key_scratch[i] = 0;
> +	}
> +
> +	kfree(cmh_ecdh_key_scratch);
> +	cmh_ecdh_key_scratch = NULL;
> +	cmh_ecdh_scratch_count = 0;
> +	cmh_ecdh_scratch_ready = false;
> +}

[Severity: High]
Does this cleanup path safely handle active TFMs?

This function frees the cmh_ecdh_key_scratch array without holding the
cmh_ecdh_scratch_lock mutex and without waiting for active TFMs to be
destroyed.

If the device is unbound (e.g., via sysfs unbind) while a user holds an
active AF_ALG socket, the remove path calls cmh_remove() ->
cmh_pke_ecdh_unregister() -> cmh_ecdh_scratch_free().

If the user submits a request via the active socket concurrently:

drivers/crypto/cmh/cmh_pke_ecdh.c:cmh_ecdh_compute_shared_secret() {
	...
	if (dd.mbx_idx < 0 || (u32)dd.mbx_idx >= cmh_ecdh_scratch_count ||
	    !cmh_ecdh_key_scratch[dd.mbx_idx]) {
		ret = -EIO;
		goto out_unmap;
	}
	...
}

This locklessly accesses cmh_ecdh_key_scratch, leading to a NULL pointer
dereference or reading unmapped memory.

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