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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing software fallback for oversized ML-DSA messages and incorrect rejection of 0-length messages.
- [Medium] Redundant DMA mappings and DMA API ownership violation for key buffers.
--

--- Patch [15]: [PATCH v2 15/19] crypto: cmh - add ML-KEM/ML-DSA (QSE) ---
commit c3b137e6f523e1962d3380a121a8e0166757ffb3
Author: Alex Ousherovitch <aousherovitch@rambus.com>

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

This patch registers ML-KEM and ML-DSA algorithms using the CMH QSE core.
It provides ML-DSA as a signature algorithm at priority 5001, overriding
the kernel's software implementation. It includes key generation, signing,
and verification capabilities.

> diff --git a/drivers/crypto/cmh/cmh_pqc_mldsa.c b/drivers/crypto/cmh/cmh_pqc_mldsa.c
> new file mode 100644
> index 0000000000000..cbe63c34a1c80
> --- /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 (ctx->key.mode != CMH_KEY_RAW)
> +		return -EINVAL;
> +	if (dlen < sig_size)
> +		return -EINVAL;
> +	if (!slen || slen > ML_DSA_MAX_MLEN)
> +		return -EINVAL;

[Severity: High]
Does returning -EINVAL here permanently fail valid operations?

FIPS 204 allows 0-length messages, so rejecting slen == 0 might break valid
requests. Additionally, since this driver registers at priority 5001 and
overrides the generic software implementation (priority 5000), returning an
error for messages over ML_DSA_MAX_MLEN (10KB) means the crypto API won't
transparently fall back.

Should the driver internally allocate a fallback software tfm to handle
oversized messages, and properly support 0-length messages?

> +
> +	m_buf = kmemdup(src, slen, GFP_KERNEL);
> +	sig_buf = kzalloc(sig_size, GFP_KERNEL);
> +	if (!m_buf || !sig_buf) {
> +		ret = -ENOMEM;
> +		goto out_free;
> +	}
> +
> +	if (ctx->key.raw.len != sk_size) {
> +		ret = -EINVAL;
> +		goto out_free;
> +	}
> +
> +	sk_buf = kmemdup(ctx->key.raw.data, ctx->key.raw.len, GFP_KERNEL);

[Severity: Medium]
Does this redundantly copy and map the secret key?

The key in ctx->key.raw.data was already mapped as ctx->key.raw.dma in
cmh_key_setkey_raw() for the lifetime of the key. Also, since that buffer
is mapped to the device (DMA_TO_DEVICE), does reading from it here with
kmemdup() violate DMA API ownership rules?

Can this code just pass ctx->key.raw.dma directly to the VCQ command to
avoid the extra allocation and copy on the hot path?

> +	if (!sk_buf) {
> +		ret = -ENOMEM;
> +		goto out_free;
> +	}
[ ... ]
> +static int cmh_mldsa_verify(struct crypto_sig *tfm,
> +			    const void *src, unsigned int slen,
> +			    const void *digest, unsigned int dlen)
> +{
[ ... ]
> +	if (!ctx->pub_key)
> +		return -EINVAL;
> +	if (slen != sig_size)
> +		return -EINVAL;
> +	if (!dlen || dlen > ML_DSA_MAX_MLEN)
> +		return -EINVAL;

[Severity: High]
Similar to the signing path, does this incorrectly reject valid 0-length
messages and oversized messages?

Since the driver overrides the software fallback, it likely needs to handle
dlen == 0 and delegate messages larger than ML_DSA_MAX_MLEN to a software
fallback to maintain API contracts.

> +
> +	sig_buf = kmemdup(src, slen, GFP_KERNEL);
> +	m_buf = kmemdup(digest, dlen, GFP_KERNEL);
> +	pk_buf = kmemdup(ctx->pub_key, pk_size, GFP_KERNEL);

[Severity: Medium]
Does this redundantly allocate and map the public key on every verification?

Instead of copying it here, could the public key be mapped once in
cmh_mldsa_set_pub_key() and reused, avoiding this extra copy overhead
on every verify operation?

> +	if (!sig_buf || !m_buf || !pk_buf) {
> +		ret = -ENOMEM;
> +		goto out_free;
> +	}

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