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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] cmh_mgmt_pke_ecdh() unconditionally writes raw shared secret bytes to user memory, ignoring the CMH_PKE_FLAG_DS_RESULT flag.
- [High] Persistent datastore slots leak in success and error paths, permanently exhausting hardware datastore memory.
- [High] Optional pointer validation fails to reject NULL pointers when parameters are logically required, passing DMA_MAPPING_ERROR to hardware.
- [High] Shared mailbox and temp storage lack concurrency control, causing state corruption and cross-process data leaks.
- [High] PKE_SWAP_FLAGS incorrectly applied to SYS_CMD_NEW, corrupting SM2 datastore reference handles.
- [High] RSA ioctls calculate modulus lengths using unrounded division, causing out-of-bounds DMA writes.
- [High] Uninitialized DMA address sig_dma is passed to cmh_dma_unmap_single in the error path.
- [High] The KEY_NEW and KEY_NEW_RANDOM ioctls fail to enforce an upper bound on req.len.
- [Medium] Spurious DMA unmap of address 0 in error paths.
- [Medium] Truthiness check on DMA addresses in cmh_mgmt_pke_rsa_keygen silently leaks mappings on platforms where 0 is a valid DMA address.
--

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

crypto: cmh - add key provisioning and management

This commit adds the CMH key management subsystem. It introduces key
provisioning, derivation, and various PKE/PQC operations via a management
ioctl interface (/dev/cmh_mgmt). It also includes the SM2 ioctl handlers.

> diff --git a/drivers/crypto/cmh/cmh_mgmt.c b/drivers/crypto/cmh/cmh_mgmt.c
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_mgmt.c
[ ... ]
> +static int cmh_mgmt_key_new(void __user *argp)
> +{
> +	struct cmh_ioctl_key_new req;
> +	struct vcq_cmd vcq[MGMT_VCQ_CMDS];
> +	u64 *ref_buf;
> +	dma_addr_t ref_dma;
> +	int ret;
> +
> +	if (copy_from_user(&req, argp, sizeof(req)))
> +		return -EFAULT;
> +
> +	if (req.version != CMH_MGMT_V1)
> +		return -EINVAL;
> +	if (!req.len)
> +		return -EINVAL;

[Severity: High]
Could this code overflow when passing an unbounded length to the firmware?

If a user specifies a very large req.len (e.g., 0xFFFFFFFF) for
CMH_IOCTL_KEY_NEW or CMH_IOCTL_KEY_NEW_RANDOM, it is passed directly
to the CMH eSW. 

If the firmware adds object header sizes to this, it may integer overflow,
resulting in a tiny allocation followed by out-of-bounds writes into datastore
memory. Should we add a check like req.len > CMH_MGMT_MAX_DATA_LEN here?

[ ... ]
> +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]
Does this shared mailbox and temp storage logic allow concurrent processes
to corrupt the SYS_REF_TEMP state?

If cmh_tm_flush_mbx() fails with -EBUSY, the mgmt_temp_dirty flag is
already cleared. Subsequent ioctls will then fail to flush the temp
storage, potentially exhausting memory.

Also, since multiple processes can call this ioctl concurrently on the
same mailbox, can one process's temp key be wiped out or overwritten
by another process's key before it gets exported?

> diff --git a/drivers/crypto/cmh/cmh_mgmt_pke.c b/drivers/crypto/cmh/cmh_mgmt_pke.c
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_mgmt_pke.c
[ ... ]
> +int cmh_mgmt_pke_rsa_enc(void __user *argp)
> +{
> +	u32 pke_cid = cmh_core_default_id(CMH_CORE_PKE);
> +
> +	struct cmh_ioctl_pke_rsa_enc req;
> +	struct vcq_cmd vcq[PKE_VCQ_CMDS_MIN];
> +	u32 n_len, e_padded;
> +	u8 *e_buf, *n_buf, *m_buf, *c_buf;
> +	dma_addr_t e_dma, n_dma, m_dma, c_dma;
> +	int ret;
> +
> +	if (copy_from_user(&req, argp, sizeof(req)))
> +		return -EFAULT;
> +	if (req.version != CMH_MGMT_V1)
> +		return -EINVAL;
> +	if (req.__reserved)
> +		return -EINVAL;
> +	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]
Can this modulus length calculation result in an undersized DMA buffer?

If a user specifies an unaligned bit length like req.bits = 1025,
n_len becomes 128 bytes. However, the hardware will perform a 1025-bit
RSA operation and write 129 bytes to the mapped buffer c_buf, overflowing
it and corrupting adjacent slab memory.

[ ... ]
> +int cmh_mgmt_pke_rsa_keygen(void __user *argp)
> +{
[ ... ]
> +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);
> +	if (d_ref_dma && !cmh_dma_map_error(d_ref_dma))
> +		cmh_dma_unmap_single(d_ref_dma, sizeof(u64), DMA_FROM_DEVICE);

[Severity: High]
Are we missing cleanup for the persistent datastore slots in this error path?

If Phase 2 of the key generation fails (for example, due to a timeout from
cmh_tm_submit_sync_tmo), the code jumps directly to out_unmap. The slots
that were allocated in Phase 1 via SYS_CMD_NEW will remain orphaned in
the datastore, leading to permanent hardware memory exhaustion.

[Severity: Medium]
Could the truthiness check on d_ref_dma and crt_ref_dma leak mappings
on architectures where physical address 0 is valid?

If cmh_dma_map_single returns 0, the check evaluates to false, causing
the driver to skip unmapping the address.

[ ... ]
> +int cmh_mgmt_pke_ecdh(void __user *argp)
> +{
[ ... ]
> +	/* 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);
> +	vcq[1].id |= pke_swap_flags(req.curve);
> +	vcq_add_sys_flush(&vcq[2]);
> +
> +	ret = cmh_tm_submit_sync_mbx(vcq, 3, 1, MGMT_MBX);

[Severity: High]
Does this leave the datastore slot orphaned after reading the shared secret?

The slot is allocated using SYS_CMD_NEW in Phase 1, but after reading
the data back with SYS_CMD_DATA, there is no SYS_CMD_GRANT with 0
permissions to delete it. This permanently leaks hardware datastore memory.

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

[Severity: High]
Can this unconditional copy corrupt userspace memory?

If the user calls CMH_IOCTL_PKE_ECDH with the CMH_PKE_FLAG_DS_RESULT flag
set, they expect a 64-bit datastore reference in req.output. Since the
driver ignores this flag entirely, it fetches the raw shared secret and
unconditionally copies clen bytes (e.g., 32 bytes for P-256) into
req.output, overflowing the expected 8-byte variable.

[ ... ]
> +int cmh_mgmt_pke_eddsa_keygen_sca(void __user *argp)
> +{
[ ... ]
> +	if (!ret) {
> +		req.sca_ref = *ref_buf;
> +		if (copy_to_user(argp, &req, sizeof(req)))
> +			ret = -EFAULT;
> +	}

[Severity: High]
Will the datastore slot leak if copy_to_user fails here?

Other functions contain a SYS_CMD_GRANT cleanup block if the copy to
user space fails. Here, it simply returns -EFAULT, leaving the persistent
slot orphaned in hardware memory.

> diff --git a/drivers/crypto/cmh/cmh_mgmt_pqc.c b/drivers/crypto/cmh/cmh_mgmt_pqc.c
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_mgmt_pqc.c
[ ... ]
> +int cmh_mgmt_ml_kem_enc(void __user *argp)
> +{
[ ... ]
> +	if (!hw_rng && req.coin) {
> +		u32 coin_len = masked ? QSE_SEED_LEN_MASKED : QSE_SEED_LEN;
> +
> +		coin_buf = kmalloc(coin_len, GFP_KERNEL);

[Severity: High]
Will this optional pointer validation pass a DMA_MAPPING_ERROR to the
hardware?

If hw_rng is false but req.coin is 0, the mapping block is completely
skipped. However, coin_dma was initialized to DMA_MAPPING_ERROR and will
be passed to vcq_add_qse_ml_kem_enc. The hardware will then attempt a wild
DMA read from the error value.

[ ... ]
> +int cmh_mgmt_ml_dsa_sign(void __user *argp)
> +{
[ ... ]
> +	dma_addr_t m_dma = DMA_MAPPING_ERROR, sig_dma;
> +	dma_addr_t sk_dma = DMA_MAPPING_ERROR;
> +	/*
> +	 * 0 = "no added randomness": the eSW treats a zero rnd pointer as
> +	 * absent.  A garbage DMA_MAPPING_ERROR would be taken as a real
> +	 * address and DMA'd from.
> +	 */
> +	dma_addr_t rnd_dma = 0;
[ ... ]
> +out_unmap:
> +	if (rnd_buf && !cmh_dma_map_error(rnd_dma))
> +		cmh_dma_unmap_single(rnd_dma, rnd_len, DMA_TO_DEVICE);

[Severity: Medium]
Might this code spuriously unmap address 0 on error paths?

Because rnd_dma is initialized to 0, if mapping fails later and the code
jumps to out_unmap, cmh_dma_map_error(0) will evaluate to false. The driver
will then call cmh_dma_unmap_single on address 0.

[ ... ]
> +	if (sk_buf && !cmh_dma_map_error(sk_dma))
> +		cmh_dma_unmap_single(sk_dma, ml_dsa_sk_size[mi],
> +				     DMA_TO_DEVICE);
> +	if (!cmh_dma_map_error(sig_dma))
> +		cmh_dma_unmap_single(sig_dma, sig_size, DMA_FROM_DEVICE);

[Severity: High]
Can the out_unmap error path pass an uninitialized DMA address to
cmh_dma_unmap_single?

The variable sig_dma is declared uninitialized. If m_dma fails to map
and jumps to out_unmap early, the error path evaluates
!cmh_dma_map_error(sig_dma). Because the uninitialized stack value is
highly unlikely to perfectly match DMA_MAPPING_ERROR, it will attempt
to unmap a random address, which could cause an IOMMU fault or kernel panic.

[ ... ]
> +int cmh_mgmt_slhdsa_keygen(void __user *argp)
> +{
[ ... ]
> +	if (!ds_ref)
> +		vcq_add_sys_read(&vcq[idx++], SYS_REF_LAST, sk_dma,
> +				 0, sk_sz + SYS_WRAP_HDR_SIZE);
> +	vcq_add_hcq_flush(&vcq[idx++], hcq_cid);
> +
> +	ret = cmh_tm_submit_sync_tmo(vcq, vcq_cnt, 1, MGMT_MBX,
> +				     cmh_tm_slow_op_timeout_jiffies());

[Severity: High]
Does reading the newly created persistent slot without deleting it cause
a hardware memory leak?

The VCQ allocates a slot with SYS_CMD_NEW and reads it back using
vcq_add_sys_read. However, there is no command to delete it (e.g., via
SYS_CMD_GRANT with 0 permissions) after the read completes.

[ ... ]
> +int cmh_mgmt_slhdsa_sign(void __user *argp)
> +{
[ ... ]
> +	if (req.ctx_len > 0 && req.ctx) {
> +		ctx_buf = kmalloc(req.ctx_len, GFP_KERNEL);
> +		if (!ctx_buf) {
> +			ret = -ENOMEM;
> +			goto out_free;
> +		}
> +		if (copy_from_user(ctx_buf, u64_to_user_ptr(req.ctx),
> +				   req.ctx_len)) {
> +			ret = -EFAULT;
> +			goto out_free;
> +		}
> +	}

[Severity: High]
Would this optional pointer validation pass a DMA_MAPPING_ERROR to the
hardware?

If a user calls this ioctl with req.ctx_len > 0 but req.ctx = 0, this block
is skipped. The variable ctx_dma is left as DMA_MAPPING_ERROR (~0ULL),
and the driver populates the VCQ command with a non-zero ctx_len and
the ~0ULL DMA address, causing the hardware to attempt a wild DMA read.

> diff --git a/drivers/crypto/cmh/cmh_pke_sm2.c b/drivers/crypto/cmh/cmh_pke_sm2.c
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_pke_sm2.c
[ ... ]
> +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 SYS_CMD_NEW corrupt the returned datastore
reference?

The hardware will byte-swap the 64-bit reference ID when DMA-writing it
back to memory. In Phase 2, the driver will read this garbled reference
and pass it to SYS_CMD_DATA, which will fail to find the object.

[ ... ]
> +	if (!keep_ds) {
> +		/* Sync bounce buffer so CPU sees the DMA-written ref */
> +		cmh_dma_sync_for_cpu(ref_dma, sizeof(u64), DMA_FROM_DEVICE);
> +
> +		/* Phase 2: read shared point from DS -> DMA, consuming the slot */
> +		vcq_set_header(&vcq[0], 3);
> +		vcq_add_sys_data(&vcq[1], *ref_buf, sp_dma, SM2_POINT_LEN);
> +		vcq[1].id |= PKE_SWAP_FLAGS;
> +		vcq_add_sys_flush(&vcq[2]);
> +
> +		ret = cmh_tm_submit_sync_mbx(vcq, 3, 1, MGMT_MBX);
> +	}

[Severity: High]
Are we leaving the datastore slot orphaned when !keep_ds is true?

The code reads the shared point from the datastore using SYS_CMD_DATA
but does not issue a command to delete the allocated slot, leading to a
permanent memory leak in the hardware datastore.

[ ... ]
> +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]
Will PKE_SWAP_FLAGS applied to SYS_CMD_NEW here similarly garble the
reference ID?

Similar to cmh_mgmt_sm2_ecdh, the hardware will byte-swap the 64-bit
reference ID when it is written back, causing subsequent lookups to fail.

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