Re: [PATCH v3 15/19] crypto: cmh - add ML-KEM/ML-DSA (QSE)

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] The driver unconditionally unmaps and frees DMA bounce buffers when hardware transactions time out, risking severe memory corruption if the hardware is wedged.
- [High] The ML-DSA sign and verify operations explicitly reject zero-length messages, violating FIPS 204 and breaking software fallback compatibility.
--

--- Patch [15]: [PATCH v3 15/19] crypto: cmh - add ML-KEM/ML-DSA (QSE) ---
commit 7381e2595560d52e928536b3ed32c1907a27b36c
Author: Alex Ousherovitch <[email protected]>

crypto: cmh - add ML-KEM/ML-DSA (QSE)

Register ML-KEM (Kyber) and ML-DSA (Dilithium) algorithms using
the CMH QSE core (core ID 0x09). ML-KEM is ioctl-only (keygen,
encaps, decaps). ML-DSA is registered as a sig algorithm with
priority 5001 to override the kernel's verify-only mldsa
implementation at priority 5000.

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

[ ... ]

> +static int cmh_mldsa_sign(struct crypto_sig *tfm,
> +			  const void *src, unsigned int slen,
> +			  void *dst, unsigned int dlen)
> +{

[ ... ]

> +	if (dlen < sig_size)
> +		return -EINVAL;
> +	if (!slen || slen > ML_DSA_MAX_MLEN)
> +		return -EINVAL;

[Severity: High]
Does this break compatibility by rejecting zero-length messages? FIPS 204
allows for empty messages, and rejecting them here could prevent the signing of
valid empty messages. Since the hardware driver overrides the software
fallback, this would break the capability kernel-wide.

[ ... ]

> +	ret = cmh_tm_submit_sync_mbx(vcq, QSE_VCQ_CMDS_MIN, 1,
> +				     dd.mbx_idx);
> +	if (!ret) {
> +		/* Sync bounce buffer so CPU sees the DMA-written signature */
> +		cmh_dma_sync_for_cpu(sig_dma, sig_size, DMA_FROM_DEVICE);
> +		memcpy(dst, sig_buf, sig_size);
> +		ret = sig_size;
> +	}
> +
> +out_unmap:
> +	if (!cmh_dma_map_error(sig_dma))
> +		cmh_dma_unmap_single(sig_dma, sig_size, 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: Critical]
If cmh_tm_submit_sync_mbx() times out and returns an error, does this code
unconditionally unmap and free the DMA bounce buffers?

If the hardware is wedged and a timeout occurs, freeing the memory here could
leave the hardware DMA engine with a dangling physical address that it might
still write to later, leading to memory corruption.

[ ... ]

> +static int cmh_mldsa_verify(struct crypto_sig *tfm,
> +			    const void *src, unsigned int slen,
> +			    const void *digest, unsigned int dlen)
> +{

[ ... ]

> +	if (slen != sig_size)
> +		return -EINVAL;
> +	if (!dlen || dlen > ML_DSA_MAX_MLEN)
> +		return -EINVAL;

[Severity: High]
Similar to cmh_mldsa_sign(), does this incorrectly reject valid zero-length
verification requests?

[ ... ]

> +	ret = cmh_tm_submit_sync_mbx(vcq, QSE_VCQ_CMDS_MIN, 1, d.mbx_idx);
> +
> +out_unmap:
> +	if (!cmh_dma_map_error(pk_dma))
> +		cmh_dma_unmap_single(pk_dma, pk_size, 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_size, DMA_TO_DEVICE);
> +
> +out_free:
> +	kfree(pk_buf);
> +	kfree(m_buf);
> +	kfree(sig_buf);
> +	return ret;
> +}

[Severity: Critical]
Could a timeout from cmh_tm_submit_sync_mbx() cause these buffers to be freed
while the hardware might still perform a DMA read from them later?

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