Re: [PATCH v3 16/19] crypto: cmh - add SLH-DSA/LMS/XMSS (HCQ)

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] DMA write-after-free vulnerability on hardware timeout.
- [Medium] Improper rejection of valid 0-length messages for cryptographic operations.
--

--- Patch [16]: [PATCH v3 16/19] crypto: cmh - add SLH-DSA/LMS/XMSS (HCQ) ---
commit 36b1570b3180e707d91edf00a1da1258b01e7320
Author: Alex Ousherovitch <[email protected]>

crypto: cmh - add SLH-DSA/LMS/XMSS (HCQ)

This commit registers the SLH-DSA, LMS, and XMSS signature algorithms using
the CMH HCQ core. SLH-DSA supports sign and verify operations, while LMS
and XMSS are verify-only.

> diff --git a/drivers/crypto/cmh/cmh_pqc_lms.c b/drivers/crypto/cmh/cmh_pqc_lms.c
> new file mode 100644
> index 0000000000000..3a17ca4786603
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_pqc_lms.c
> @@ -0,0 +1,238 @@
> [ ... ]
> +static int cmh_lms_verify(struct crypto_sig *tfm,
> +			  const void *src, unsigned int slen,
> +			  const void *digest, unsigned int dlen)
> +{
> [ ... ]
> +	if (!dlen || dlen > LMS_MAX_MSG_LEN)
> +		return -EINVAL;

[Severity: Medium]
Does this code improperly reject valid 0-length messages? Cryptographic
algorithms natively support empty message inputs, but this check causes them
to fail with -EINVAL.

> +
> +	sig_buf = kmemdup(src, slen, GFP_KERNEL);
> [ ... ]
> +	/* LMS verify traverses Merkle hash chains -- inherently slow */
> +	ret = cmh_tm_submit_sync_tmo(vcq, LMS_VCQ_CMDS, 1, d.mbx_idx,
> +				     cmh_tm_slow_op_timeout_jiffies());
> +
> +out_unmap:
> +	if (!cmh_dma_map_error(pk_dma))
> +		cmh_dma_unmap_single(pk_dma, ctx->pub_key_len, DMA_TO_DEVICE);
> +	if (!cmh_dma_map_error(m_dma))
> +		cmh_dma_unmap_single(m_dma, dlen, DMA_TO_DEVICE);
> +	if (!cmh_dma_map_error(sig_dma))
> +		cmh_dma_unmap_single(sig_dma, slen, DMA_TO_DEVICE);
> +
> +out_free:
> +	kfree(pk_buf);
> +	kfree(m_buf);
> +	kfree(sig_buf);
> +	return ret;
> +}

[Severity: High]
Can this lead to a DMA write-after-free if the hardware times out? If
cmh_tm_submit_sync_tmo() times out and returns -ETIMEDOUT, the caller-owned
heap buffers are unconditionally unmapped and freed. If the hardware later
recovers and completes the transaction, won't it perform an asynchronous DMA
write into memory that has already been returned to the slab allocator?

> diff --git a/drivers/crypto/cmh/cmh_pqc_slhdsa.c b/drivers/crypto/cmh/cmh_pqc_slhdsa.c
> new file mode 100644
> index 0000000000000..4bec4278cd9c6
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_pqc_slhdsa.c
> @@ -0,0 +1,377 @@
> [ ... ]
> +static int cmh_slhdsa_sign(struct crypto_sig *tfm,
> +			   const void *src, unsigned int slen,
> +			   void *dst, unsigned int dlen)
> +{
> [ ... ]
> +	if (!slen || slen > SLHDSA_MAX_MSG_LEN)
> +		return -EINVAL;

[Severity: Medium]
Could this explicit rejection of slen == 0 break API conformance? It looks
like it prevents valid empty-message signing requests from being processed.

> +
> +	m_buf = kmemdup(src, slen, GFP_KERNEL);
> [ ... ]
> +	ret = cmh_tm_submit_sync_tmo(vcq, vcq_count, 1, d.mbx_idx,
> +				     cmh_tm_slow_op_timeout_jiffies());
> +
> +	if (!ret) {
> +		/* Sync bounce buffer so CPU sees the DMA-written signature */
> +		cmh_dma_sync_for_cpu(sig_dma, sig_sz, DMA_FROM_DEVICE);
> +		memcpy(dst, sig_buf, sig_sz);
> +		ret = sig_sz;
> +	}
> +
> +out_unmap:
> +	if (sk_buf) {
> +		if (!cmh_dma_map_error(sk_dma))
> +			cmh_dma_unmap_single(sk_dma, sk_sz, DMA_TO_DEVICE);
> +		kfree_sensitive(sk_buf);
> +	}
> +	if (!cmh_dma_map_error(sig_dma))
> +		cmh_dma_unmap_single(sig_dma, sig_sz, DMA_FROM_DEVICE);
> +	if (!cmh_dma_map_error(m_dma))
> +		cmh_dma_unmap_single(m_dma, slen, DMA_TO_DEVICE);
> +
> +out_free:
> +	kfree(sig_buf);
> +	kfree(m_buf);
> +	return ret;
> +}

[Severity: High]
Is it possible for a delayed hardware write to corrupt the heap here? In the
event of a timeout from cmh_tm_submit_sync_tmo(), these DMA buffers are
freed. If the hardware is merely delayed and eventually finishes, it might
write the signature into the now-freed sig_buf.

> [ ... ]
> +static int cmh_slhdsa_verify(struct crypto_sig *tfm,
> +			     const void *src, unsigned int slen,
> +			     const void *digest, unsigned int dlen)
> +{
> [ ... ]
> +	if (!dlen || dlen > SLHDSA_MAX_MSG_LEN)
> +		return -EINVAL;

[Severity: Medium]
Does this code block valid 0-length messages from being verified?

> +
> +	sig_buf = kmemdup(src, slen, GFP_KERNEL);
> [ ... ]
> +	/* SLH-DSA verify recomputes hyper-tree hashes -- inherently slow */
> +	ret = cmh_tm_submit_sync_tmo(vcq, HCQ_VCQ_CMDS_MIN, 1, d.mbx_idx,
> +				     cmh_tm_slow_op_timeout_jiffies());
> +
> +out_unmap:
> +	if (!cmh_dma_map_error(pk_dma))
> +		cmh_dma_unmap_single(pk_dma, pk_sz, DMA_TO_DEVICE);
> +	if (!cmh_dma_map_error(m_dma))
> +		cmh_dma_unmap_single(m_dma, dlen, DMA_TO_DEVICE);
> +	if (!cmh_dma_map_error(sig_dma))
> +		cmh_dma_unmap_single(sig_dma, sig_sz, DMA_TO_DEVICE);
> +
> +out_free:
> +	kfree(pk_buf);
> +	kfree(m_buf);
> +	kfree(sig_buf);
> +	return ret;
> +}

[Severity: High]
Can a hardware timeout cause a use-after-free? Freeing m_buf, pk_buf, and
sig_buf immediately after a timeout might allow a delayed hardware DMA
completion to access freed memory.

> diff --git a/drivers/crypto/cmh/cmh_pqc_xmss.c b/drivers/crypto/cmh/cmh_pqc_xmss.c
> new file mode 100644
> index 0000000000000..50de1ef0f488c
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_pqc_xmss.c
> @@ -0,0 +1,238 @@
> [ ... ]
> +static int cmh_xmss_verify(struct crypto_sig *tfm,
> +			   const void *src, unsigned int slen,
> +			   const void *digest, unsigned int dlen)
> +{
> [ ... ]
> +	if (!dlen || dlen > XMSS_MAX_MSG_LEN)
> +		return -EINVAL;

[Severity: Medium]
Will this return -EINVAL for perfectly valid 0-length messages?

> +
> +	sig_buf = kmemdup(src, slen, GFP_KERNEL);
> [ ... ]
> +	/* XMSS verify traverses Merkle hash chains -- inherently slow */
> +	ret = cmh_tm_submit_sync_tmo(vcq, XMSS_VCQ_CMDS, 1, d.mbx_idx,
> +				     cmh_tm_slow_op_timeout_jiffies());
> +
> +out_unmap:
> +	if (!cmh_dma_map_error(pk_dma))
> +		cmh_dma_unmap_single(pk_dma, ctx->pub_key_len, DMA_TO_DEVICE);
> +	if (!cmh_dma_map_error(m_dma))
> +		cmh_dma_unmap_single(m_dma, dlen, DMA_TO_DEVICE);
> +	if (!cmh_dma_map_error(sig_dma))
> +		cmh_dma_unmap_single(sig_dma, slen, DMA_TO_DEVICE);
> +
> +out_free:
> +	kfree(pk_buf);
> +	kfree(m_buf);
> +	kfree(sig_buf);
> +	return ret;
> +}

[Severity: High]
Does this error path introduce a use-after-free risk? If the hardware stalls
and cmh_tm_submit_sync_tmo() returns -ETIMEDOUT, these buffers are freed. A
subsequent hardware recovery would access memory that is no longer allocated.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=16