Re: [PATCH v5 05/19] crypto: cmh - add HMAC ahash

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] crypto: cmh: missing bounds check in cmh_hmac_import allows unbounded memory allocation
- [High] crypto: cmh: under-accounting of chunk struct overhead allows extreme memory exhaustion
- [Medium] crypto: cmh: freeing millions of chunks under spin_lock_bh can trigger a softlockup
- [Medium] crypto: cmh: DMA buffer shares a cache line with CPU-modified variables
--

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

crypto: cmh - add HMAC ahash

This commit registers HMAC ahash algorithms (HMAC-SHA-2 and HMAC-SHA-3) using
the CMH hash core. It implements a self-contained transaction model using
accumulated inputs, with a generic software fallback for payloads above 64KB.

> diff --git a/drivers/crypto/cmh/cmh_hmac.c b/drivers/crypto/cmh/cmh_hmac.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_hmac.c
[ ... ]
> @@ -136,7 +136,7 @@
> +/* Per-Request State */
> +
> +struct cmh_hmac_chunk {
> +	struct list_head  list;
> +	struct list_head  tfm_node; /* per-tfm orphan tracking */
> +	u32               len;
> +	u8                data[];
> +};

[Severity: Medium]
Does this structure layout place the data buffer on the same cache line as
the list heads?

Since the data buffer is mapped for DMA, could concurrent list
modifications in cmh_hmac_update() dirty the cache line while a transfer
is in progress, potentially violating mapping rules on non-coherent
architectures?

[ ... ]
> +static int cmh_hmac_update(struct ahash_request *req)
> +{
[ ... ]
> +	chunk = kmalloc(sizeof(*chunk) + req->nbytes,
> +			req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP ?
> +			GFP_KERNEL : GFP_ATOMIC);
> +	if (!chunk) {
> +		rctx->error = -ENOMEM;
> +		goto err_free_chunks;
> +	}
> +
> +	chunk->len = req->nbytes;
[ ... ]
> +	spin_lock_bh(&tctx->chunk_lock);
> +	if (tctx->tfm_buffered + chunk->len > CMH_HMAC_TFM_MAX_BUFFERED) {
> +		spin_unlock_bh(&tctx->chunk_lock);
> +		kfree_sensitive(chunk);
> +		rctx->error = -ENOMEM;
> +		goto err_free_chunks;
> +	}
> +	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);

[Severity: High]
Are we only accounting for the payload length here, instead of the full
size of the allocated struct?

Since the chunk header is 36 bytes (often rounded up to 64 bytes by the
slab allocator), could repeated 1-byte updates allow pinning roughly 1GB
of kernel memory per transform socket without exceeding the 16MB budget?

[ ... ]
> +static int cmh_hmac_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);

[Severity: High]
Should we verify if the budget is exceeded before appending the imported
chunk and adding to tctx->tfm_buffered?

Without a check against CMH_HMAC_TFM_MAX_BUFFERED, could an unprivileged
user repeatedly call accept() on a socket to clone its state and cause
unbounded kernel memory allocation?

[ ... ]
> +static void cmh_hmac_cra_exit(struct crypto_tfm *tfm)
> +{
> +	struct cmh_hmac_tfm_ctx *tctx = crypto_tfm_ctx(tfm);
> +	struct cmh_hmac_chunk *chunk, *tmp;
> +
> +	/* Free any orphaned chunks (e.g. testmgr export/reimport poison) */
> +	spin_lock_bh(&tctx->chunk_lock);
> +	list_for_each_entry_safe(chunk, tmp, &tctx->all_chunks, tfm_node) {
> +		list_del(&chunk->tfm_node);
> +		tctx->tfm_buffered -= chunk->len;
> +		kfree_sensitive(chunk);
> +	}
> +	spin_unlock_bh(&tctx->chunk_lock);

[Severity: Medium]
Is it safe to free potentially large numbers of chunks while holding the
spinlock with softirqs disabled?

If the 16MB budget contains millions of small chunks (due to the accounting
size issue mentioned earlier), could this loop take significant CPU time
and risk triggering a soft or hard lockup panic?

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