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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] crypto: cmh: missing tfm_buffered increment in cmh_poly_import() causes underflow
--

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

This commit registers the ChaCha20-Poly1305 AEAD and ChaCha20 skcipher
algorithms using the CMH CCP core. It 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..d8482dd96d12c
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_ccp_poly.c
[ ... ]
> +static void cmh_poly_free_chunks(struct cmh_poly_reqctx *rctx,
> +				 struct cmh_poly_tfm_ctx *tctx)
> +{
> +	struct cmh_poly_chunk *c, *tmp;
> +
> +	spin_lock_bh(&tctx->chunk_lock);
> +	list_for_each_entry_safe(c, tmp, &rctx->chunks, list) {
> +		list_del(&c->list);
> +		list_del(&c->tfm_node);
> +		tctx->tfm_buffered -= c->len;

[Severity: Medium]
Can this subtraction underflow if the chunk was created during an import?

This is where the underflow actually occurs when the request is freed,
because the corresponding increment was missing in cmh_poly_import.

> +		kfree_sensitive(c);
> +	}
> +	spin_unlock_bh(&tctx->chunk_lock);
> +	rctx->total_len = 0;
> +}
[ ... ]
> +static int cmh_poly_import(struct ahash_request *req, const void *in)
> +{
[ ... ]
> +	if (state->total_len) {
> +		chunk = kmalloc(sizeof(*chunk) + state->total_len,
> +				req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP ?
> +				GFP_KERNEL : GFP_ATOMIC);
> +		if (!chunk)
> +			return -ENOMEM;
> +		chunk->len = state->total_len;
> +		memcpy(chunk->data, state->data, state->total_len);
> +		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: Medium]
Should tctx->tfm_buffered be incremented here?

Since the newly allocated chunk is being added to the TFM tracking list,
omitting the increment causes tctx->tfm_buffered to become unbalanced.

When a user creates an AF_ALG hash socket, writes data to it, and clones it
via accept(), cmh_poly_import is called for the clone. When the sockets are
closed, cmh_poly_free_chunks will subtract the chunk lengths from
tctx->tfm_buffered twice (once for the parent, once for the clone).

Because the clone never added its length during import, the counter
underflows to a massive positive value, permanently breaking the instance by
causing subsequent update calls to spuriously fail with -ENOMEM.

> +		rctx->total_len = state->total_len;
> +	}
> +	return 0;
> +}

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