Re: [PATCH v2 13/19] crypto: cmh - add ECDSA/SM2 sig

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] `cmh_ecdsa_set_priv_key` incorrectly rejects valid P-521 private keys due to a strict length check.
- [High] A malicious user can provide an oversized ECDSA signature to trigger a WARN_ON_ONCE, potentially causing a kernel panic on systems with panic_on_warn enabled.
- [Medium] Unnecessary copying and DMA mapping of the private key on the fast path during `cmh_ecdsa_sign()`.
--

--- Patch [13]: [PATCH v2 13/19] crypto: cmh - add ECDSA/SM2 sig ---
commit 6d12540edcf70f967d4d40c53ce926233bb16875
Author: Alex Ousherovitch <aousherovitch@rambus.com>

crypto: cmh - add ECDSA/SM2 sig

This commit registers ECDSA and SM2 signature algorithms using the CMH PKE
core. It supports P-256, P-384, P-521, and SM2 curves for signing and
verification. SM2 is registered as verify-only via the crypto API.

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

[ ... ]

> +static void ecdsa_vli_to_be(const u64 *vli, unsigned int ndigits,
> +			    u8 *out, unsigned int out_len)
> +{

[ ... ]

> +	} else {
> +		/* VLI wider than out -- skip leading (zero) bytes */
> +		u8 tmp[ECC_MAX_BYTES];
> +
> +		for (i = 0; i < ndigits; i++)
> +			put_unaligned_be64(vli[ndigits - 1 - i],
> +					   &tmp[i * sizeof(u64)]);
> +		skip = full_len - out_len;
> +		WARN_ON_ONCE(memchr_inv(tmp, 0, skip));

[Severity: High]
Can an unprivileged user trigger this WARN_ON_ONCE by providing an
oversized signature (e.g., via keyctl or AF_ALG)?

If the user passes an r or s array padded with non-zero upper bytes during
signature verification, could this cause a kernel panic on systems with
panic_on_warn enabled?

Would it be safer to return -EINVAL here instead of warning?

> +		memcpy(out, tmp + skip, out_len);
> +	}
> +}

[ ... ]

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

[ ... ]

> +	dig_buf = kzalloc(clen, GFP_KERNEL);
> +	sig_buf = kzalloc(sig_raw_len, GFP_KERNEL);
> +	sk_buf = kmemdup(ctx->key.raw.data, ctx->key.raw.len, GFP_KERNEL);
> +	if (!dig_buf || !sig_buf || !sk_buf) {
> +		ret = -ENOMEM;
> +		goto out_free;
> +	}
> +
> +	/*
> +	 * Truncate or zero-pad digest to clen bytes, right-aligned.
> +	 * Matches ECDSA bits2int: use leftmost min(slen, clen) bytes,
> +	 * zero-pad on the left when slen < clen.
> +	 */
> +	memcpy(dig_buf + (clen - copy_len), src, copy_len);
> +
> +	dig_dma = cmh_dma_map_single(dig_buf, clen, DMA_TO_DEVICE);
> +	sig_dma = cmh_dma_map_single(sig_buf, sig_raw_len, DMA_FROM_DEVICE);
> +	sk_dma = cmh_dma_map_single(sk_buf, ctx->key.raw.len, DMA_TO_DEVICE);

[Severity: Medium]
Is it necessary to allocate and DMA map the private key on the fast path
during cmh_ecdsa_sign()?

Since cmh_key_setkey_raw() already pre-maps the key into ctx->key.raw.dma
for the lifetime of the transform, does duplicating this mapping here
cause unnecessary performance overhead, or unnecessarily expose the private
key material in a transient heap buffer?

> +
> +	if (cmh_dma_map_error(dig_dma) || cmh_dma_map_error(sig_dma) ||
> +	    cmh_dma_map_error(sk_dma)) {
> +		ret = -ENOMEM;

[ ... ]

> +static int cmh_ecdsa_set_priv_key(struct crypto_sig *tfm,
> +				  const void *key, unsigned int keylen)
> +{
> +	struct cmh_ecdsa_tfm_ctx *ctx = cmh_ecdsa_ctx(tfm);
> +
> +	if (keylen != ctx->clen)
> +		return -EINVAL;

[Severity: High]
Does this strict length equality check prevent P-521 signing?

The kernel crypto API passes exactly 66 bytes (ceil(521/8)) for P-521
coordinates, but ctx->clen for P-521 is 68. Would it be better to accept
lengths <= clen and left-pad them with zeroes, similar to how
cmh_ecdsa_set_pub_key() handles it?

> +
> +	return cmh_key_setkey_raw(&ctx->key, key, keylen, CORE_ID_PKE);
> +}

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