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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] DMA Use-After-Free (UAF) memory corruption during hardware timeout in synchronous ECDSA operations.
- [Medium] Latent stack buffer overflow in ecdsa_be_to_vli() due to flawed bounds checking logic.
--

--- Patch [13]: [PATCH v3 13/19] crypto: cmh - add ECDSA/SM2 sig ---
commit 93e934df7eae63fe0a90fc7b8a6f5ff276f60f86
Author: Alex Ousherovitch <[email protected]>

crypto: cmh - add ECDSA/SM2 sig

Register ECDSA and SM2 sig algorithms using the CMH PKE core.
Supports P-256, P-384, P-521, and SM2 curves for sign and verify
operations. 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..6d6b8aec6c884
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_pke_ecdsa.c

[ ... ]

> +static void ecdsa_be_to_vli(const u8 *in, unsigned int in_len,
> +			    u64 *vli, unsigned int max_digits)
> +{
> +	unsigned int full_len = max_digits * sizeof(u64);
> +	u8 tmp[ECC_MAX_BYTES];
> +	unsigned int i;
> +
> +	if (WARN_ON_ONCE(max_digits > ECC_MAX_DIGITS))
> +		max_digits = ECC_MAX_DIGITS;
> +
> +	memset(tmp, 0, full_len);

[Severity: Medium]
Does this code calculate full_len before clamping max_digits?

If a caller passes a large max_digits, full_len could exceed ECC_MAX_BYTES
before the clamping happens, potentially causing a stack buffer overflow
in the memset. Would it be safer to calculate full_len after the check?

[ ... ]

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

[ ... ]

> +	ret = cmh_tm_submit_sync_mbx(vcq, PKE_VCQ_CMDS_MIN, 1, d.mbx_idx);
> +
> +out_unmap:
> +	if (!cmh_dma_map_error(rp_dma))
> +		cmh_dma_unmap_single(rp_dma, clen, DMA_FROM_DEVICE);
> +	if (!cmh_dma_map_error(sig_dma))
> +		cmh_dma_unmap_single(sig_dma, sig_raw_len, DMA_TO_DEVICE);
> +	if (!cmh_dma_map_error(dig_dma))
> +		cmh_dma_unmap_single(dig_dma, clen, DMA_TO_DEVICE);
> +	if (!cmh_dma_map_error(pk_dma))
> +		cmh_dma_unmap_single(pk_dma, ctx->pub_key_len, DMA_TO_DEVICE);
> +
> +out_free:
> +	kfree(rp_buf);

[Severity: High]
Is it possible for the hardware to still write to rp_buf if
cmh_tm_submit_sync_mbx() times out?

If a timeout occurs, this path unmaps and frees rp_buf back to the slab
allocator. Without a mechanism to abort the in-flight hardware operation,
could the hardware later complete its DMA write and corrupt the reallocated
memory?

> +	kfree(pk_buf);
> +	kfree(sig_raw);
> +	kfree(dig_buf);
> +	return ret;
> +}

[ ... ]

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

[ ... ]

> +	ret = cmh_tm_submit_sync_mbx(vcq, idx, 1, dd.mbx_idx);
> +	if (!ret) {
> +		/* Sync bounce buffer so CPU sees the DMA-written signature */
> +		cmh_dma_sync_for_cpu(sig_dma, sig_raw_len, DMA_FROM_DEVICE);
> +
> +		/* Encode raw (r||s) into VLI ecdsa_raw_sig for kernel API */
> +		ret = ecdsa_raw_to_sig(sig_buf, clen, dst, dlen);
> +	}
> +
> +out_unmap:
> +	if (!cmh_dma_map_error(sig_dma))
> +		cmh_dma_unmap_single(sig_dma, sig_raw_len, DMA_FROM_DEVICE);
> +	if (!cmh_dma_map_error(dig_dma))
> +		cmh_dma_unmap_single(dig_dma, clen, DMA_TO_DEVICE);
> +
> +out_free:
> +	kfree(sig_buf);

[Severity: High]
Similarly, does this free sig_buf while the hardware might still attempt a
DMA write upon a late completion?

Like the verify path, a timeout from cmh_tm_submit_sync_mbx could lead to
the hardware overwriting freed memory if the transaction is not successfully
aborted on the device.

> +	kfree(dig_buf);
> +	return ret;
> +}

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