Re: [PATCH v2 03/19] crypto: cmh - add key provisioning and management

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
- [Critical] Unprotected cross-VCQ access to the shared per-mailbox `SYS_REF_TEMP` datastore creates a race condition that can leak or corrupt raw cryptographic keys.
- [High] Optional DMA pointers default to `DMA_MAPPING_ERROR` instead of `0`, causing the hardware to DMA from an invalid address.
- [High] The ECDH ioctl ignores the CMH_PKE_FLAG_DS_RESULT flag, causing an unconditional out-of-bounds write to userspace memory.
- [High] Persistent hardware datastore slots for intermediate results are leaked during normal execution of certain ioctls, leading to datastore exhaustion.
- [High] Applying `PKE_SWAP_FLAGS` to `SYS_CMD_NEW` scrambles the returned datastore reference handle, causing subsequent operations to fail.
- [High] ECDSA signatures are computed over the wrong hash value because the digest is padded with trailing zeros instead of leading zeros.
- [High] Hardware datastore key slots become permanently orphaned if the final `copy_to_user()` fails to deliver the reference to user space.
- [High] Unsynchronized hardware mailbox flush creates a denial of service (DoS) vector for concurrent crypto operations.
- [High] Premature exposure of the `/dev/cmh_mgmt` misc device before driver initialization is complete leads to a NULL pointer dereference.
- [Medium] Valid DMA addresses mapped at `0` are leaked due to unsafe boolean evaluation.
- [Medium] Potential out-of-bounds kernel memory read in `cmh_mgmt_ds_export` if `req.len` is smaller than the wrapper header size.
- [Medium] Uninitialized kernel heap memory is potentially copied to user space in `cmh_mgmt_eac_read`.
--

--- Patch [3]: [PATCH v2 03/19] crypto: cmh - add key provisioning and management ---
commit fdf4a06222c37879ad3a1f641550a83825a212f0
Author: Alex Ousherovitch <aousherovitch@rambus.com>

crypto: cmh - add key provisioning and management

This commit adds the CMH key management subsystem, including key provisioning,
system object management, and a management ioctl interface (/dev/cmh_mgmt).
It exposes key lifecycle, KIC key derivation, PKE operations, PQC operations,
and SM2 multi-step protocols.

> diff --git a/drivers/crypto/cmh/cmh_main.c b/drivers/crypto/cmh/cmh_main.c
> index de065a425a2cf..7673ed3e58618 100644
> --- a/drivers/crypto/cmh/cmh_main.c
> +++ b/drivers/crypto/cmh/cmh_main.c
> @@ -196,12 +197,19 @@ static int cmh_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_rh_init;
>  
> +	/* Register key management device (/dev/cmh_mgmt) */
> +	ret = cmh_mgmt_register();
> +	if (ret)
> +		goto err_mgmt_register;
> +
>  	g_cmh_dev = dev;

[Severity: High]
Is it safe to register the misc device via cmh_mgmt_register() before setting
the global g_cmh_dev pointer? If userspace opens the device and executes an
ioctl immediately, could the ioctl handler's use of cmh_dev() trigger a NULL
pointer dereference?

> diff --git a/drivers/crypto/cmh/cmh_mgmt.c b/drivers/crypto/cmh/cmh_mgmt.c
> index 0000000000000..d228213f7850c 100644
> --- a/drivers/crypto/cmh/cmh_mgmt.c
> +++ b/drivers/crypto/cmh/cmh_mgmt.c
> @@ -89,6 +89,53 @@ static atomic_t mgmt_temp_dirty = ATOMIC_INIT(0);
>  
>  /* -- KEY_NEW -------------------------- */
>  
> +static int cmh_mgmt_key_new(void __user *argp)
> +{
[ ... ]
> +	req.ref = *ref_buf;
> +	kfree(ref_buf);
> +
> +	if (copy_to_user(argp, &req, sizeof(req)))
> +		return -EFAULT;

[Severity: High]
Does this code leak the newly allocated hardware datastore key if copy_to_user
fails? Should it issue a SYS_CMD_GRANT with 0 permissions to delete the object
before returning -EFAULT?

> @@ -351,6 +351,59 @@ static int cmh_mgmt_ds_export(void __user *argp)
> +	dmabuf = kzalloc(req.len, GFP_KERNEL);
> +	if (!dmabuf)
> +		return -ENOMEM;
[ ... ]
> +	/* Parse actual blob size from the eSW-written header */
> +	{
> +		struct sys_wrap_hdr *hdr = (struct sys_wrap_hdr *)dmabuf;
> +		u64 actual;
> +
> +		if (check_add_overflow((u64)sizeof(*hdr), (u64)hdr->wrap,
> +				       &actual) ||

[Severity: Medium]
What happens if the user specifies a very small req.len, such as 4 bytes?
If the hardware unexpectedly returns success for a truncated buffer, could
casting dmabuf to struct sys_wrap_hdr * and reading its fields cause an
out-of-bounds read of kernel memory?

> @@ -493,6 +493,76 @@ static int cmh_mgmt_ds_import(void __user *argp)
> +static int cmh_mgmt_kic_hkdf1(void __user *argp)
> +{
[ ... ]
> +	if (temp) {
> +		/* Flush MBX to reset temp stack -- see KIC section comment */
> +		ret = cmh_tm_flush_mbx(MGMT_MBX);

[Severity: High]
Will unconditionally flushing MGMT_MBX abort any active commands concurrently
executing on that shared mailbox? Does this create a denial of service vector
for other crypto operations?

> +		if (ret)
> +			goto out_unmap_label;
> +
> +		n_cmds = 3;
> +		vcq_set_header(&vcq[0], n_cmds);
> +		vcq_add_kic_hkdf1(&vcq[1], SYS_REF_TEMP, req.base_key,
> +				  label_dma, req.key_len, req.label_len,
> +				   SYS_TYPE_SET(0, CORE_ID_AES));
> +		vcq_add_sys_flush(&vcq[2]);
[ ... ]
> +	ret = cmh_tm_submit_sync_mbx(vcq, n_cmds, 1, MGMT_MBX);

[Severity: Critical]
Since the derived key is stored in SYS_REF_TEMP and the ioctl then returns to
userspace, what prevents a concurrent crypto request on the same MGMT_MBX from
overwriting this temporary slot before the key is later consumed? Could a
subsequent read inadvertently extract the raw key of the unrelated crypto
operation?

> @@ -608,6 +608,8 @@ out_unmap_label:
> +out_unmap_label:
> +	if (label_buf && !cmh_dma_map_error(label_dma) && label_dma)
> +		cmh_dma_unmap_single(label_dma, req.label_len, DMA_TO_DEVICE);

[Severity: Medium]
Does the && label_dma condition mistakenly treat a valid DMA mapping at address
0 as an error? Will this leak the mapping on architectures where 0 is a
legitimate bus address?

> @@ -1185,6 +1185,41 @@ static int cmh_mgmt_key_new_random(void __user *argp)
> +static long cmh_mgmt_eac_read(void __user *argp)
> +{
[ ... ]
> +	rsp = kmalloc_obj(*rsp, GFP_KERNEL);
> +	if (!rsp)
> +		return -ENOMEM;

[Severity: Medium]
Since rsp is allocated without zeroing its contents, what happens if the
hardware performs a partial DMA write? Could the trailing unwritten fields
such as sram_bank_errors retain old kernel heap data that gets leaked when
copied to userspace?

> @@ -1369,6 +1369,87 @@ static inline bool cmh_mgmt_needs_temp_flush(unsigned int cmd)
> +static long cmh_mgmt_ioctl(struct file *file, unsigned int cmd,
> +			   unsigned long arg)
> +{
> +	void __user *argp = (void __user *)arg;
> +	int ret;
> +
> +	if (cmh_mgmt_needs_temp_flush(cmd) &&
> +	    atomic_xchg(&mgmt_temp_dirty, 0)) {
> +		ret = cmh_tm_flush_mbx(MGMT_MBX);
> +		if (ret)
> +			return ret;
> +	}

[Severity: High]
Can this conditional flush of MGMT_MBX abort in-flight commands from concurrent
crypto operations? Should there be a lock to verify no concurrent operations
are executing before flushing the shared mailbox?

> diff --git a/drivers/crypto/cmh/cmh_mgmt_pke.c b/drivers/crypto/cmh/cmh_mgmt_pke.c
> index 0000000000000..6954832fa8ac5 100644
> --- a/drivers/crypto/cmh/cmh_mgmt_pke.c
> +++ b/drivers/crypto/cmh/cmh_mgmt_pke.c
> @@ -319,6 +319,160 @@ int cmh_mgmt_pke_rsa_keygen(void __user *argp)
> +{
[ ... ]
> +		/* Sync DMA so we can read back the opaque refs */
> +		cmh_dma_unmap_single(d_ref_dma, sizeof(u64), DMA_FROM_DEVICE);
> +		cmh_dma_unmap_single(crt_ref_dma, sizeof(u64),
> +				     DMA_FROM_DEVICE);
> +		d_ref_dma = 0;
> +		crt_ref_dma = 0;
[ ... ]
> +out_unmap:
> +	if (crt_ref_dma && !cmh_dma_map_error(crt_ref_dma))
> +		cmh_dma_unmap_single(crt_ref_dma, sizeof(u64),
> +				     DMA_FROM_DEVICE);

[Severity: Medium]
Does this code use 0 as a sentinel for an unmapped address? If the DMA API
returns 0 as a valid mapped address, will this condition skip the unmap step
and leak the mapping?

[ ... ]
> +	if (!ret) {
> +		/* Copy generated modulus and refs back */
> +		if (copy_to_user(u64_to_user_ptr(req.n), n_buf, n_len)) {
> +			ret = -EFAULT;
> +			goto out_free;
> +		}

[Severity: High]
If copy_to_user fails here, does this skip cleaning up the d_ref and crt_ref
datastore objects? Will these hardware key slots become permanently orphaned
since the user never receives the references to free them?

> @@ -513,6 +513,66 @@ int cmh_mgmt_pke_ecdsa_sign(void __user *argp)
> +{
[ ... ]
> +	dig_map_len = max_t(u32, req.digest_len, clen);
> +
> +	dig_buf = kzalloc(dig_map_len, GFP_KERNEL);
> +	sig_buf = kzalloc(sig_len, GFP_KERNEL);
[ ... ]
> +	if (copy_from_user(dig_buf, u64_to_user_ptr(req.digest),
> +			   req.digest_len)) {

[Severity: High]
If the provided req.digest_len is smaller than the curve length clen, does
copying the digest to the start of dig_buf incorrectly pad it with trailing
zeros? Since the buffer is interpreted as a big-endian integer, will this act
as a mathematical left-shift and cause the signature to be computed over the
wrong hash value?

> @@ -594,6 +594,76 @@ int cmh_mgmt_pke_ecdh(void __user *argp)
> +{
[ ... ]
> +	vcq_add_sys_new(&vcq[idx++], 0, ref_dma, clen);
> +	vcq_add_pke_ecdh(&vcq[idx++], pke_cid, req.curve, clen, clen,
> +			 ss_type, peer_dma, req.key_ref,
> +			 SYS_REF_LAST, swap);
[ ... ]
> +	ret = cmh_tm_submit_sync_mbx(vcq, 4, 1, MGMT_MBX);
> +	if (ret)
> +		goto out_unmap;
> +
> +	/* Sync bounce buffer so CPU sees the DMA-written ref */
> +	cmh_dma_sync_for_cpu(ref_dma, sizeof(u64), DMA_FROM_DEVICE);
> +
> +	/* Phase 2: extract shared secret from DS via actual ref */
> +	vcq_set_header(&vcq[0], 3);
> +	vcq_add_sys_data(&vcq[1], *ref_buf, ss_dma, clen);
[ ... ]
> +	ret = cmh_tm_submit_sync_mbx(vcq, 3, 1, MGMT_MBX);

[Severity: High]
Does this code leak the persistent datastore slot for the shared secret? After
the result is extracted to memory, it appears the slot is never deleted via a
SYS_CMD_GRANT with 0 permissions.

[ ... ]
> +	if (!ret) {
> +		if (copy_to_user(u64_to_user_ptr(req.output), ss_buf, clen))
> +			ret = -EFAULT;
> +	}

[Severity: High]
Is the CMH_PKE_FLAG_DS_RESULT flag in req.flags being ignored here? By
unconditionally executing Phase 2 and copying clen bytes to req.output, will
this cause an out-of-bounds write to userspace memory if the caller expected
only an 8-byte reference?

> diff --git a/drivers/crypto/cmh/cmh_mgmt_pqc.c b/drivers/crypto/cmh/cmh_mgmt_pqc.c
> index 0000000000000..db479e80326bc 100644
> --- a/drivers/crypto/cmh/cmh_mgmt_pqc.c
> +++ b/drivers/crypto/cmh/cmh_mgmt_pqc.c
> @@ -356,6 +356,117 @@ int cmh_mgmt_ml_kem_dec(void __user *argp)
> +{
[ ... ]
> +		ret = cmh_tm_submit_sync_mbx(vcq, QSE_VCQ_CMDS_MIN,
> +					     1, MGMT_MBX);
> +		if (ret)
> +			goto out_unmap;
> +
> +		/* Phase 2: extract ss from SYS_REF_TEMP */
> +		vcq_set_header(&vcq[0], QSE_VCQ_CMDS_MIN);
> +		vcq_add_sys_data(&vcq[1], SYS_REF_TEMP, ss_dma,
> +				 ss_out_len);
> +		vcq_add_sys_flush(&vcq[2]);
> +
> +		ret = cmh_tm_submit_sync_mbx(vcq, QSE_VCQ_CMDS_MIN,
> +					     1, MGMT_MBX);

[Severity: Critical]
Because Phase 1 writes to SYS_REF_TEMP and Phase 2 reads from it using
entirely separate submissions, what prevents a concurrent crypto request on
MGMT_MBX from overwriting the temporary slot in between these phases? Does
this lack of synchronization risk leaking raw keys to userspace?

> @@ -844,6 +844,115 @@ int cmh_mgmt_slhdsa_keygen(void __user *argp)
> +{
[ ... ]
> +	} else {
> +		vcq_cnt = HCQ_VCQ_CMDS_MAX; /* hdr+new+write+keygen+read+flush */
> +		vcq_set_header(&vcq[idx++], vcq_cnt);
> +		vcq_add_sys_new(&vcq[idx++], SYS_CID_NONE, ref_dma,
> +				sk_sz);
> +	}
[ ... ]
> +	if (!ds_ref)
> +		vcq_add_sys_read(&vcq[idx++], SYS_REF_LAST, sk_dma,
> +				 0, sk_sz + SYS_WRAP_HDR_SIZE);

[Severity: High]
When ds_ref is false, a persistent slot is created to hold the generated key,
but does it ever get deleted after the key is read back to memory? This seems
to leak the datastore slot.

> @@ -1005,6 +1005,108 @@ int cmh_mgmt_slhdsa_sign(void __user *argp)
> +{
[ ... ]
> +	dma_addr_t sig_dma, rnd_dma = DMA_MAPPING_ERROR;
[ ... ]
> +	if (req.add_random) {
> +		rnd_buf = kmalloc(n_val, GFP_KERNEL);
[ ... ]
> +	if (rnd_buf) {
> +		rnd_dma = cmh_dma_map_single(rnd_buf, n_val, DMA_TO_DEVICE);
[ ... ]
> +	vcq_add_hcq_slhdsa_sign(&vcq[1], hcq_cid, req.parameter_set,
> +				req.msg_len, req.ctx_len,
> +				rnd_dma, msg_dma, ctx_dma,
> +				req.sk, sig_dma);

[Severity: High]
If req.add_random is omitted, rnd_dma remains DMA_MAPPING_ERROR. When this
is passed directly into the hardware VCQ structure, won't the hardware attempt
a DMA read from an invalid physical address and trigger bus errors?

> diff --git a/drivers/crypto/cmh/cmh_pke_sm2.c b/drivers/crypto/cmh/cmh_pke_sm2.c
> index 0000000000000..9a6e30c7f5e59 100644
> --- a/drivers/crypto/cmh/cmh_pke_sm2.c
> +++ b/drivers/crypto/cmh/cmh_pke_sm2.c
> @@ -156,6 +156,128 @@ int cmh_mgmt_sm2_ecdh(void __user *argp)
> +{
[ ... ]
> +	/* Phase 1: sys_new(shared_point_ref) + SM2_ECDH(->SYS_REF_LAST) */
> +	idx = 0;
> +	vcq_set_header(&vcq[idx++], 4);
> +	vcq_add_sys_new(&vcq[idx], 0, ref_dma, SM2_POINT_LEN);
> +	vcq[idx++].id |= PKE_SWAP_FLAGS;

[Severity: High]
Does applying PKE_SWAP_FLAGS to the SYS_CMD_NEW command ID scramble the 64-bit
reference handle returned by the hardware? Will this cause subsequent
operations attempting to use this reference to fail with an ENOENT error?

> @@ -563,6 +563,80 @@ int cmh_mgmt_sm2_ecdh_hash(void __user *argp)
> +{
[ ... ]
> +	/*
> +	 * Phase 1: sys_new(shared_key_ref) + SM2_ECDH_HASH
> +	 * The shared_point_ref from the ECDH step is passed directly
> +	 * as a DS reference -- the eSW hub reads it from DS.
> +	 */
> +	vcq_set_header(&vcq[0], 4);
> +	vcq_add_sys_new(&vcq[1], 0, ref_dma, SM2_SHARED_KEY_LEN);
> +	vcq[1].id |= PKE_SWAP_FLAGS;

[Severity: High]
Similarly here, does applying PKE_SWAP_FLAGS to SYS_CMD_NEW corrupt the
datastore reference handle?

[ ... ]
> +	/* Phase 2: read shared key from DS -> DMA */
> +	vcq_set_header(&vcq[0], 3);
> +	vcq_add_sys_data(&vcq[1], *ref_buf, sk_dma, SM2_SHARED_KEY_LEN);
> +	vcq_add_sys_flush(&vcq[2]);

[Severity: High]
Is the persistent object orphaned and leaked here after extracting its data?
Should it be deleted with SYS_CMD_GRANT using 0 permissions?

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