Re: [PATCH v4 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:
- [High] Unbounded memory leak of dynamically allocated Poly1305 chunks (`struct cmh_poly_chunk`) when an `ahash_request` is re-initialized or abandoned, due to missing cleanup.
- [High] The driver linearizes arbitrary-length skcipher and AEAD requests into large contiguous kmalloc buffers (up to 1MB), which is highly prone to allocation failures and violates API contracts.
- [Medium] Mapping an unaligned structure member for DMA violates DMA API constraints and causes cacheline sharing issues.
--

commit bb2ec11c7c7c49e89d1a8aef0363955e68578689
Author: Alex Ousherovitch <aousherovitch@rambus.com>
Subject: crypto: cmh - add ChaCha20-Poly1305

This commit registers the ChaCha20-Poly1305 AEAD, ChaCha20 skcipher, and
Poly1305 ahash algorithms using the CMH CCP core. It introduces VCQ
transaction handling for hardware operations while accumulating data in
memory for arbitrary-length poly1305 requests.

> diff --git a/drivers/crypto/cmh/cmh_ccp.c b/drivers/crypto/cmh/cmh_ccp.c
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_ccp.c
[ ... ]
> +static int cmh_ccp_crypt(struct skcipher_request *req, u32 ccp_op)
> +{
[ ... ]
> +	/* Limit linearisation buffers to avoid large allocations. */
> +	if (req->cryptlen > SZ_1M)
> +		return -EINVAL;
> +
> +	gfp = req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP ?
> +	      GFP_KERNEL : GFP_ATOMIC;
> +
> +	memset(rctx, 0, sizeof(*rctx));
> +	rctx->cryptlen = req->cryptlen;
> +
> +	/* Linearise input from scatterlist */
> +	rctx->in_buf = kmalloc(req->cryptlen, gfp | __GFP_NOWARN);

[Severity: High]
Does this code linearize the entire payload into a large contiguous buffer?
Allocating up to 1MB of contiguous physical memory via kmalloc (especially
with GFP_ATOMIC) is extremely prone to failure under memory fragmentation.

Also, returning -EINVAL for requests larger than 1MB breaks the crypto API
contract, which expects support for arbitrary payload sizes. Could this
cause unpredictable failures for subsystems like IPsec?

> diff --git a/drivers/crypto/cmh/cmh_ccp_aead.c b/drivers/crypto/cmh/cmh_ccp_aead.c
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_ccp_aead.c
[ ... ]
> +static int cmh_ccp_aead_crypt(struct aead_request *req, u32 ccp_op)
> +{
[ ... ]
> +	if ((u64)cryptlen + req->assoclen > SZ_1M)
> +		return -EINVAL;
[ ... ]
> +	/* Linearise input */
> +	if (cryptlen > 0) {
> +		rctx->in_buf = kmalloc(cryptlen, gfp | __GFP_NOWARN);

[Severity: High]
Does this AEAD implementation suffer from the same limitation?
Using kmalloc for large contiguous buffers (up to 1MB) can easily fail
under memory pressure, and rejecting requests larger than this size
violates the expected behavior of the crypto API. Can this be reworked to
use scatterlists directly or handle chunks iteratively?

> diff --git a/drivers/crypto/cmh/cmh_ccp_poly.c b/drivers/crypto/cmh/cmh_ccp_poly.c
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_ccp_poly.c
[ ... ]
> +static int cmh_poly_setkey(struct crypto_ahash *tfm, const u8 *key,
> +			   unsigned int keylen)
> +{
[ ... ]
> +	tctx->skey_dma = cmh_dma_map_single(tctx->key + CCP_POLY_KEY_SIZE,
> +					    CCP_POLY_KEY_SIZE,
> +					     DMA_TO_DEVICE);

[Severity: Medium]
Are we mapping a structure member that is not cacheline aligned for DMA?
Mapping an inner offset of the tctx->key array for DMA violates the DMA API
constraints and can cause cacheline sharing issues with surrounding fields
(like tctx->chunk_lock) on non-coherent architectures. Should this use a
separately allocated buffer instead?

[ ... ]
> +static int cmh_poly_init(struct ahash_request *req)
> +{
> +	struct cmh_poly_reqctx *rctx = ahash_request_ctx(req);
> +
> +	memset(rctx, 0, sizeof(*rctx));
> +	INIT_LIST_HEAD(&rctx->chunks);
> +	return 0;
> +}

[Severity: High]
Does this unconditionally overwrite the request context without cleaning up
previously allocated chunks?

If a user re-initializes a request (e.g. via crypto_ahash_init), any chunks
currently linked to rctx->chunks are abandoned here. Since the crypto
framework provides no request destructor, this leads to an unbounded memory
leak.

[ ... ]
> +static int cmh_poly_update(struct ahash_request *req)
> +{
[ ... ]
> +	chunk = kmalloc(sizeof(*chunk) + req->nbytes, gfp);
> +	if (!chunk) {
> +		ret = -ENOMEM;
> +		goto err_free_chunks;
> +	}
> +
> +	chunk->len = req->nbytes;
[ ... ]
> +	list_add_tail(&chunk->list, &rctx->chunks);
> +	spin_lock_bh(&tctx->chunk_lock);
> +	list_add_tail(&chunk->tfm_node, &tctx->all_chunks);
> +	spin_unlock_bh(&tctx->chunk_lock);

[Severity: High]
Is this where the leaked chunks are accumulated?

Chunks allocated here are added to both the request list and the global
transform list. If the user abandons the request (like closing an AF_ALG
socket), the chunks remain permanently orphaned in tctx->all_chunks until
the parent transform is destroyed.

[ ... ]
> +static int cmh_poly_import(struct ahash_request *req, const void *in)
> +{
> +	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);
> +	const struct cmh_poly_export_state *state = in;
> +	struct cmh_poly_chunk *chunk;
> +
> +	memset(rctx, 0, sizeof(*rctx));
> +	INIT_LIST_HEAD(&rctx->chunks);

[Severity: High]
Does this zeroing of the request context cause the same leak as
cmh_poly_init?

Any existing chunks attached to the request before the import will be
abandoned and remain orphaned in the global transform list.

[ ... ]
> +static void cmh_poly_exit_tfm(struct crypto_ahash *tfm)
> +{
> +	struct cmh_poly_tfm_ctx *tctx = crypto_ahash_ctx(tfm);
> +	struct cmh_poly_chunk *c, *tmp;
> +
> +	/* Free any orphaned chunks (e.g. testmgr export/reimport poison) */
> +	spin_lock_bh(&tctx->chunk_lock);
> +	list_for_each_entry_safe(c, tmp, &tctx->all_chunks, tfm_node) {
> +		list_del(&c->tfm_node);
> +		kfree_sensitive(c);
> +	}
> +	spin_unlock_bh(&tctx->chunk_lock);

[Severity: High]
Does this cleanup step confirm the accumulation of orphaned chunks?

The fact that chunks must be freed here during transform destruction
verifies that abandoned requests leave their chunks permanently stranded in
tctx->all_chunks throughout the lifetime of the transform, providing an
easy path for a local denial of service.

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