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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Persistent datastore objects are permanently leaked on success and error paths, allowing unprivileged DoS of the hardware datastore.
- [High] Missing `PKE_SWAP_FLAGS` in `cmh_mgmt_sm2_ecdh_hash` causes cryptographic corruption.
- [High] Treating DMA address 0 as "absent" causes silent cryptographic degradation and mapping leaks.
- [High] Unprotected concurrent access to `SYS_REF_TEMP` allows cross-process crypto key theft and corruption.
- [High] RSA buffer lengths are calculated via truncation, causing hardware DMA overflows on unaligned key sizes.
- [Medium] Cryptographic randomness buffers are freed with `kfree()` instead of `kfree_sensitive()`, leaking secrets.
- [Medium] `KEY_READ` fails to apply the reverse byte-swap for PKE keys, returning obfuscated keys to userspace.
--

commit 67f85ec0363d5636a036b63752d2d0407ae36205
Author: Alex Ousherovitch <aousherovitch@rambus.com>

crypto: cmh - add key provisioning and management

The in-kernel crypto API cannot express the CMH management plane, so
this patch adds a character device (/dev/cmh_mgmt) for it. The ioctl
commands fall into two groups, enabling datastore key lifecycle
management, KIC derivation, and asymmetric/PQC operations that keep
the private key hardware-resident.

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

[ ... ]

> @@ -274,6 +274,10 @@ static int cmh_mgmt_key_read(void __user *argp)
>  		return -ENOMEM;
>  	}
>  
>  	vcq_set_header(&vcq[0], MGMT_VCQ_CMDS);
>  	vcq_add_sys_read(&vcq[1], req.ref, dma_addr, req.wrap_key, req.len);
>  	vcq_add_sys_flush(&vcq[2]);

[Severity: Medium]
Does this command need to conditionally reverse the PKE byte swap?

If a user writes an RSA or ECC key via KEY_WRITE, the driver applies
PKE_SWAP_FLAGS to store it in the internal big-endian representation.

Since the flags are not reversed here, could this return obfuscated, swapped
keys back to userspace?

[ ... ]

> @@ -661,6 +661,10 @@ static int cmh_mgmt_kic_hkdf1(void __user *argp)
>  out_ref:
>  	if (ref_buf) {
>  		cmh_dma_unmap_single(ref_dma, sizeof(*ref_buf),
>  				     DMA_FROM_DEVICE);
>  		kfree(ref_buf);
>  	}
>  	return ret;
>  }

[Severity: High]
Could this error path leak a persistent datastore object?

If cmh_tm_submit_sync_mbx() fails after the SYS_CMD_NEW command has already
executed and allocated the slot, this path does not call cmh_mgmt_ds_scrub().

This appears to leave an orphaned slot in the hardware datastore. Could a
caller repeatedly trigger timeouts or failures here to exhaust the finite
datastore capacity?

[ ... ]

> @@ -1094,6 +1094,10 @@ static int cmh_mgmt_kic_aes_cmac_kdf(void __user *argp)
>  out_unmap_label_cmac:
>  	if (label_buf && !cmh_dma_map_error(label_dma) && label_dma)
>  		cmh_dma_unmap_single(label_dma, req.label_len, DMA_TO_DEVICE);

[Severity: High]
Is it safe to treat DMA address 0 as an invalid mapping here?

On many architectures, 0 is a valid IOVA. The && label_dma check means the
mapping will be leaked if the label happens to map to address 0.

[ ... ]

> @@ -1470,6 +1470,12 @@ 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);

[Severity: High]
Is the per-mailbox SYS_REF_TEMP state fully protected here?

The lockless atomic_xchg combined with the lack of an ioctl-level mutex
appears to leave the temporary key store vulnerable to concurrent clobbering.

If two processes interleave calls (e.g., one doing KIC_HKDF1 and another
doing DS_EXPORT), could temporary keys derived by one process be exported or
overwritten by the other?

> diff --git a/drivers/crypto/cmh/cmh_mgmt_pke.c b/drivers/crypto/cmh/cmh_mgmt_pke.c
> new file mode 100644
> index 0000000000000..649ad20086579
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_mgmt_pke.c

[ ... ]

> @@ -50,6 +50,10 @@ int cmh_mgmt_pke_rsa_enc(void __user *argp)
>  	if (req.bits < PKE_RSA_MIN_BITS || req.bits > PKE_RSA_MAX_BITS)
>  		return -EINVAL;
>  	if (!req.e_len || req.e_len > PKE_MAX_OPERAND)
>  		return -EINVAL;
>  
>  	n_len = req.bits / 8;

[Severity: High]
If req.bits is not a multiple of 8 (e.g., 2049), does this integer division
drop the fractional byte?

This would underestimate the required DMA buffer size.

Could the hardware then write more bytes than the buffer holds, leading to
memory corruption or an IOMMU fault?

> diff --git a/drivers/crypto/cmh/cmh_mgmt_pqc.c b/drivers/crypto/cmh/cmh_mgmt_pqc.c
> new file mode 100644
> index 0000000000000..83af21eec716e
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_mgmt_pqc.c

[ ... ]

> @@ -852,6 +852,10 @@ int cmh_mgmt_ml_dsa_sign(void __user *argp)
>  out_unmap:
>  	if (rnd_dma && !cmh_dma_map_error(rnd_dma))
>  		cmh_dma_unmap_single(rnd_dma, rnd_len, DMA_TO_DEVICE);

[Severity: High]
Is this if (rnd_dma) check correct for validating the DMA mapping?

As mentioned earlier, DMA address 0 is a valid IOVA on some platforms. If the
mapping returns 0, this check evaluates to false and skips unmapping, which
leaks the mapping.

If the hardware interprets a 0 pointer as absent randomness, could this also
silently drop the randomness from the signature generation, degrading its
security?

[ ... ]

> @@ -869,6 +873,11 @@ int cmh_mgmt_ml_dsa_sign(void __user *argp)
>  out_free:
>  	kfree(rnd_buf);
>  	kfree(sig_buf);
>  	kfree(m_buf);
>  	kfree_sensitive(sk_buf);
>  	return ret;
>  }

[Severity: Medium]
Should rnd_buf be cleared with kfree_sensitive() instead of kfree()?

This buffer holds the user's secret cryptographic randomness.

Freeing it without zeroization leaves sensitive secrets in the kernel heap,
where they might be exposed by a memory leak vulnerability.

> diff --git a/drivers/crypto/cmh/cmh_pke_sm2.c b/drivers/crypto/cmh/cmh_pke_sm2.c
> new file mode 100644
> index 0000000000000..9daeaaa948281
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_pke_sm2.c

[ ... ]

> @@ -622,6 +622,10 @@ int cmh_mgmt_sm2_ecdh_hash(void __user *argp)
>  	/* 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]
Does this missing PKE_SWAP_FLAGS cause cryptographic corruption?

In Phase 1 and in SM2_ECDH, the command is OR'd with PKE_SWAP_FLAGS to
reverse the byte order.

Since vcq[1].id is not modified here, will the shared key be returned to
userspace in the internal big-endian format rather than the host's format?

[ ... ]

> @@ -651,6 +655,8 @@ int cmh_mgmt_sm2_ecdh_hash(void __user *argp)
>  out_unmap:
>  	if (!cmh_dma_map_error(ref_dma))
>  		cmh_dma_unmap_single(ref_dma, sizeof(u64), DMA_FROM_DEVICE);

[Severity: High]
Is it possible that the persistent datastore object referenced by *ref_buf
is permanently leaked here?

The *ref_buf object created by SYS_CMD_NEW is never scrubbed on the success
path or the error path.

Could repeated calls to this ioctl exhaust the finite datastore capacity
and brick the hardware until a reset?

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