Re: [PATCH v2 10/19] crypto: cmh - add ChaCha20-Poly1305

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Failure to reset `total_len` during an error in `cmh_poly_update` causes `cmh_poly_final` to leak uninitialized kernel heap memory.
- [High] Unbounded kernel memory exhaustion (DoS) in Poly1305 driver due to missing request destructor and improper re-initialization.
- [Medium] Using `dma_addr_t` as a boolean check in the error path of `cmh_poly_final` leaks a DMA mapping if the mapped address is 0.
--

--- Patch [10]: [PATCH v2 10/19] crypto: cmh - add ChaCha20-Poly1305 ---
commit 23c814e3e61faada221bb1fbcd27c935e6800b9b
Author: Alex Ousherovitch <aousherovitch@rambus.com>

crypto: cmh - add ChaCha20-Poly1305

Register ChaCha20-Poly1305 AEAD and ChaCha20 skcipher algorithms
using the CMH CCP core (core ID 0x18).  Also registers the Poly1305
ahash for standalone use.

> diff --git a/drivers/crypto/cmh/cmh_ccp_poly.c b/drivers/crypto/cmh/cmh_ccp_poly.c
> new file mode 100644
> index 0000000000000..020a98fbe6070
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_ccp_poly.c

[ ... ]

> +static int cmh_poly_init(struct ahash_request *req)
> +{
> +	struct cmh_poly_reqctx *rctx = ahash_request_ctx(req);
> +
> +	memset(rctx, 0, sizeof(*rctx));

[Severity: High]
Does this code leak dynamically allocated chunks? If a user-space application
creates an AF_ALG socket, accumulates data, and then either closes the request
or re-initializes it by calling cmh_poly_init() again, the request context is
zeroed here, losing the pointers to the chunks while they remain on the TFM
list.

The ahash API does not have a freereq callback to handle early closure
cleanups.

> +	INIT_LIST_HEAD(&rctx->chunks);
> +	return 0;
> +}

[ ... ]

> +err_free_chunks:
> +	/*
> +	 * Terminal error -- free all previously accumulated chunks.
> +	 * Callers may not call .final() on error, so they would leak.
> +	 */
> +	cmh_poly_free_chunks(rctx, tctx);
> +	return ret;

[Severity: Critical]
Could this leak uninitialized kernel memory? When cmh_poly_update() encounters
an error and frees the chunks here, it does not reset rctx->total_len to 0.

If cmh_poly_final() is subsequently called, it allocates an uninitialized
kmalloc buffer of size total_len, skips the memcpy loop because the chunk list
is empty, and maps it for DMA.

An attacker providing the key could potentially recover the plaintext of the
uninitialized kernel heap data by reversing the resulting MAC.

> +}

[ ... ]

> +static int cmh_poly_final(struct ahash_request *req)
> +{
> +	struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
> +	struct cmh_poly_tfm_ctx *tctx = crypto_ahash_ctx(tfm);
> +	struct cmh_poly_reqctx *rctx = ahash_request_ctx(req);
> +	struct vcq_cmd cmds[CMH_POLY_MAX_PAYLOAD];
> +	struct core_dispatch d;
> +	s32 target_mbx;
> +	u32 core_id;
> +	u32 idx;
> +	int ret;
> +	gfp_t gfp;

[ ... ]

> +out_unmap_in:
> +	if (rctx->total_len > 0 && rctx->in_dma)
> +		cmh_dma_unmap_single(rctx->in_dma, rctx->total_len,
> +				     DMA_TO_DEVICE);

[Severity: Medium]
Does this code inadvertently leak a DMA mapping on architectures where 0 is a
valid DMA address? Checking if a dma_addr_t is non-zero will fail when the
mapped address happens to be exactly 0, skipping the unmap call.

> +out_unmap_tag:
> +	cmh_dma_unmap_single(rctx->tag_dma, POLY1305_DIGEST_SIZE,
> +			     DMA_FROM_DEVICE);

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