Re: [PATCH v5 08/19] crypto: cmh - add AES skcipher/aead/cmac

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] crypto: cmh: unbounded memory leak and TFM bricking in cmac(aes) AF_ALG path
--

commit a4044451c1e906d90442c0bef45c7be8c8846c42
Author: Alex Ousherovitch <aousherovitch@rambus.com>

crypto: cmh - add AES skcipher/aead/cmac

This commit registers AES algorithms (skcipher, aead, cmac) using the CMH
AES core. It handles associated data, payloads, and authentication tags
for AEAD and supports 128, 192, and 256-bit keys.

> diff --git a/drivers/crypto/cmh/cmh_aes_cmac.c b/drivers/crypto/cmh/cmh_aes_cmac.c
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_aes_cmac.c

[ ... ]

> +static int cmh_aes_cmac_init(struct ahash_request *req)
> +{
> +	struct cmh_aes_cmac_reqctx *rctx = ahash_request_ctx(req);
> +
> +	memset(rctx, 0, sizeof(*rctx));
> +	INIT_LIST_HEAD(&rctx->chunks);
> +	return 0;
> +}

[Severity: High]
Does this unconditionally wipe the request context? If the request already
has chunks attached to rctx->chunks, it looks like they will be permanently
orphaned and leaked. For example, if a user repeatedly resets an active
request via the AF_ALG interface, this might permanently consume the quota
until the transform is bricked.

[ ... ]

> +static int cmh_aes_cmac_final(struct ahash_request *req)
> +{
[ ... ]
> +	if (ret) {
> +		/*
> +		 * Synchronous rejection (e.g. -EAGAIN: CMQ full, no backlog).
> +		 * Free only the per-submit transients and keep the accumulated
> +		 * chunks intact so the caller can retry the identical final().
> +		 * If no retry comes, cra_exit reclaims the orphaned chunks; the
> +		 * per-tfm buffered-byte cap bounds how much stays pinned.
> +		 */
> +		if (rctx->total_len > 0 && !cmh_dma_map_error(rctx->in_dma))
> +			cmh_dma_unmap_single(rctx->in_dma, rctx->total_len,
> +					     DMA_TO_DEVICE);
> +		cmh_dma_unmap_single(rctx->tag_dma, AES_CMAC_DIGEST_SIZE,
> +				     DMA_FROM_DEVICE);
> +		kfree(rctx->tag_buf);
> +		rctx->tag_buf = NULL;
> +		kfree_sensitive(rctx->buf);
> +		rctx->buf = NULL;
> +		/* Keep chunks + total_len so the retry rebuilds the buffer. */
> +		return ret;

[Severity: High]
Is there a risk of leaking these accumulated chunks on a synchronous error?
The comment mentions keeping them so the caller can retry, but if the caller
ultimately drops the request instead of retrying, these chunks appear to be
left orphaned.

[ ... ]

> +static int cmh_aes_cmac_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);
> +		spin_lock_bh(&tctx->chunk_lock);
> +		list_add_tail(&chunk->list, &rctx->chunks);
> +		list_add_tail(&chunk->tfm_node, &tctx->all_chunks);
> +		tctx->tfm_buffered += chunk->len;
> +		spin_unlock_bh(&tctx->chunk_lock);
> +		rctx->total_len = state->total_len;
> +	}
> +	return 0;
> +}

[Severity: High]
Can an unprivileged user bypass the CMH_AES_CMAC_TFM_MAX_BUFFERED quota
here?

The cmac(aes) driver imposes a 16MB limit on tctx->tfm_buffered during
updates. However, this import function allocates memory and adds to
tctx->tfm_buffered without checking that limit.

If a user creates an ahash socket via AF_ALG, clones the socket repeatedly
with accept() (which triggers import()), and closes the child socket, could
this lead to unbounded kernel memory allocation and an OOM denial of service?

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