Re: [PATCH v4 06/19] crypto: cmh - add CSHAKE/KMAC ahash

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] The driver illegally maps a request context field (`rctx->checkpoint`) for DMA, which may reside on the stack, causing IOMMU faults or memory corruption.
- [High] Unbounded kernel memory leak (OOM vector) on abandoned KMAC requests because dynamically allocated chunks are orphaned onto a TFM-scoped list.
- [High] Race condition between tail holdback updates and asynchronous completion handler.
- [High] Use of spin_lock_bh() on a lock that can be acquired from hardirq context.
- [Medium] Unbounded `kmalloc` in `cmh_cshake_update` and `cmh_cshake_finup` breaks the API contract for large inputs.
- [Medium] State destruction on retryable synchronous rejection breaks the crypto API contract in cSHAKE and KMAC.
- [Medium] Defining export/import stubs for unsupported algorithms breaks testmgr initialization.
--

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

crypto: cmh - add CSHAKE/KMAC ahash

Registers ahash algorithms for cSHAKE128, cSHAKE256, KMAC128, and KMAC256
using the CMH hash core. cSHAKE supports incremental update and export/import,
while KMAC has a 64KB data cap.

> diff --git a/drivers/crypto/cmh/cmh_cshake.c b/drivers/crypto/cmh/cmh_cshake.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_cshake.c
[ ... ]
> +static int cmh_cshake_update(struct ahash_request *req)
> +{
[ ... ]
> +	rctx->data_buf = kmalloc(full_len, gfp | __GFP_NOWARN);
> +	if (!rctx->data_buf)
> +		return -ENOMEM;

[Severity: Medium]
Can this allocation fail for large inputs? The buffer size scales directly
with the unconstrained user-supplied req->nbytes, which might trigger an
unbounded allocation.

[ ... ]
> +	rctx->ckpt_dma = DMA_MAPPING_ERROR;
> +	if (rctx->has_checkpoint) {
> +		rctx->ckpt_dma = cmh_dma_map_single(rctx->checkpoint,
> +						    HC_CONTEXT_SIZE,
> +						    DMA_TO_DEVICE);

[Severity: High]
Does this map stack memory directly for DMA?

If a caller uses AHASH_REQUEST_ON_STACK to allocate the request, mapping the
inline context field rctx->checkpoint will generate invalid physical addresses
on systems with CONFIG_VMAP_STACK enabled, causing IOMMU faults or memory
corruption.

> +		if (cmh_dma_map_error(rctx->ckpt_dma)) {
> +			ret = -ENOMEM;
> +			goto err_unmap_save;
> +		}
> +	}
[ ... ]
> +	ret = cmh_vcq_pack_and_submit_async(cmds, idx, rctx->packed,
> +					    CMH_CSHAKE_MAX_PACKED,
> +					    d.mbx_idx,
> +					    cmh_cshake_update_complete, req,
> +					    !!(req->base.flags &
> +					       CRYPTO_TFM_REQ_MAY_BACKLOG),
> +					    cmh_tm_async_timeout_jiffies());
> +	if (ret && ret != -EBUSY)
> +		goto err_unmap_ckpt;
> +
> +	/*
> +	 * Submit accepted (in flight or backlogged) -- only now move the tail
> +	 * into the holdback.  A synchronous rejection above (e.g. -EAGAIN)
> +	 * leaves rctx->buf/buf_len untouched so the caller can retry the
> +	 * identical update.
> +	 */
> +	if (tail_len > 0) {
> +		if (req->base.flags & CRYPTO_AHASH_REQ_VIRT)
> +			memcpy(rctx->buf, req->svirt + from_src, tail_len);
> +		else
> +			scatterwalk_map_and_copy(rctx->buf, req->src,
> +						 from_src, tail_len, 0);
> +	}
> +	rctx->buf_len = tail_len;

[Severity: High]
Does updating the tail holdback after submitting the asynchronous hardware
command introduce a race condition?

If the hardware processing completes instantly, the callback executes and
allows the caller to submit a new request while this thread is still modifying
the holdback buffer, leading to state corruption.

> +
> +	if (ret == -EBUSY)
> +		return -EBUSY;
[ ... ]
> +static int cmh_cshake_submit_final(struct ahash_request *req,
> +				   u8 *data_buf, u32 data_len)
> +{
[ ... ]
> +	rctx->ckpt_dma = DMA_MAPPING_ERROR;
> +	if (rctx->has_checkpoint) {
> +		rctx->ckpt_dma = cmh_dma_map_single(rctx->checkpoint,
> +						    HC_CONTEXT_SIZE,
> +						    DMA_TO_DEVICE);

[Severity: High]
Can this map a stack-allocated request context for DMA in the finalization path,
leading to the same memory corruption issue if AHASH_REQUEST_ON_STACK is used?

> +		if (cmh_dma_map_error(rctx->ckpt_dma)) {
> +			ret = -ENOMEM;
> +			goto err_unmap_data;
> +		}
> +	}
[ ... ]
> +err_free_digest:
> +	kfree(rctx->digest_buf);
> +	rctx->digest_buf = NULL;
> +err_free_data:
> +	kfree(data_buf);
> +	rctx->data_buf = NULL;
> +	cmh_cshake_free_reqctx(rctx);
> +	return ret;

[Severity: Medium]
Does clearing has_checkpoint in this error path break the API contract?

If the hardware submission is rejected synchronously (e.g., -EAGAIN or
-ENOSPC), the caller is expected to retry, but the ability to resume on retry
has been destroyed.

> +}
[ ... ]
> +static int cmh_cshake_finup(struct ahash_request *req)
> +{
[ ... ]
> +	gfp = req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP ?
> +	      GFP_KERNEL : GFP_ATOMIC;
> +
> +	data_buf = kmalloc(data_len, gfp | __GFP_NOWARN);
> +	if (!data_buf)
> +		return -ENOMEM;

[Severity: Medium]
Will this unconstrained allocation fail if the caller provides a very large
input for the combined update and final transaction?

[ ... ]
> +	if (req->nbytes > 0) {
> +		if (req->base.flags & CRYPTO_AHASH_REQ_VIRT)
> +			memcpy(data_buf + rctx->buf_len,
> +			       req->svirt, req->nbytes);
> +		else
> +			scatterwalk_map_and_copy(data_buf + rctx->buf_len,
> +						 req->src, 0,
> +						 req->nbytes, 0);
> +	}
> +
> +	rctx->buf_len = 0;
> +	return cmh_cshake_submit_final(req, data_buf, data_len);

[Severity: Medium]
Is it safe to clear rctx->buf_len before knowing if the hardware will
accept the submission?

If cmh_cshake_submit_final() returns -EAGAIN, the caller will retry, but the
holdback tail has already been cleared.

> +}
[ ... ]
> diff --git a/drivers/crypto/cmh/cmh_kmac.c b/drivers/crypto/cmh/cmh_kmac.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_kmac.c
[ ... ]
> +static void cmh_kmac_free_chunks(struct cmh_kmac_reqctx *rctx,
> +				 struct cmh_kmac_tfm_ctx *tctx)
> +{
> +	struct cmh_kmac_chunk *chunk, *tmp;
> +
> +	spin_lock_bh(&tctx->chunk_lock);

[Severity: High]
Could using spin_lock_bh() lead to deadlocks?

The crypto ahash API can be called from a hardirq context. Because
spin_lock_bh() does not disable hardirqs, a hardware interrupt could spin
indefinitely on this lock if it attempts to acquire it while a softirq already
holds it.

> +	list_for_each_entry_safe(chunk, tmp, &rctx->chunks, list) {
> +		list_del(&chunk->list);
> +		list_del(&chunk->tfm_node);
> +		kfree(chunk);
> +	}
> +	spin_unlock_bh(&tctx->chunk_lock);
> +	rctx->num_chunks = 0;
> +	rctx->total_len = 0;
> +}
[ ... ]
> +static int cmh_kmac_init(struct ahash_request *req)
> +{
> +	struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
> +	struct cmh_kmac_reqctx *rctx = ahash_request_ctx(req);
> +
> +	rctx->info = cmh_kmac_get_info(tfm);
> +	rctx->error = 0;
> +	INIT_LIST_HEAD(&rctx->chunks);

[Severity: High]
Does re-initializing the request list drop tracking of previously accumulated
chunks?

If an earlier request was abandoned and not finalized properly, this leaves
the chunks orphaned in the TFM-scoped tctx->all_chunks list, creating a
memory leak.

> +	rctx->num_chunks = 0;
> +	rctx->total_len = 0;
> +
> +	return 0;
> +}
> +
> +static int cmh_kmac_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;
> +	if (req->base.flags & CRYPTO_AHASH_REQ_VIRT) {
> +		memcpy(chunk->data, req->svirt, req->nbytes);
> +	} else {
> +		nents = sg_nents_for_len(req->src, req->nbytes);
> +		if (nents < 0 ||
> +		    sg_copy_to_buffer(req->src, nents,
> +				      chunk->data, req->nbytes) != req->nbytes) {
> +			kfree(chunk);
> +			rctx->error = -EINVAL;
> +			goto err_free_chunks;
> +		}
> +	}
> +
> +	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 there an unbounded kernel memory leak here?

The chunks are dynamically allocated on the heap and tied to the TFM's lifetime
in tctx->all_chunks. If unprivileged userspace repeatedly creates and abandons
requests (e.g. by closing an AF_ALG socket early), does it continuously leak
memory until an OOM panic occurs?

> +	rctx->num_chunks++;
> +	rctx->total_len += req->nbytes;
> +
> +	return 0;
[ ... ]
> +static int cmh_kmac_final(struct ahash_request *req)
> +{
[ ... ]
> +out_free_sg:
> +	cmh_dma_free_sg(sgm);
> +
> +out_free:
> +	cmh_kmac_free_chunks(rctx, tctx);
> +	return ret;

[Severity: Medium]
Will this error path destroy all accumulated KMAC chunks if the hardware
submission is rejected synchronously?

This breaks the ability to retry on transient errors.

> +}
[ ... ]
> +static int cmh_kmac_export(struct ahash_request *req, void *out)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static int cmh_kmac_import(struct ahash_request *req, const void *in)
> +{
> +	return -EOPNOTSUPP;
> +}

[Severity: Medium]
Do these stub implementations cause the testmgr to fail? The testmgr expects
that if export/import function pointers are non-NULL, the feature is supported
and will try to exercise it.

[ ... ]
> +static void cmh_kmac_cra_exit(struct crypto_tfm *tfm)
> +{
> +	struct cmh_kmac_tfm_ctx *tctx = crypto_tfm_ctx(tfm);
> +	struct cmh_kmac_chunk *chunk, *tmp;
> +
> +	/* Free any orphaned chunks (e.g. testmgr export/reimport poison) */
> +	spin_lock_bh(&tctx->chunk_lock);

[Severity: High]
Does acquiring this lock with spin_lock_bh() during TFM destruction present
the same deadlock risk if hardirq accesses are possible?

> +	list_for_each_entry_safe(chunk, tmp, &tctx->all_chunks, tfm_node) {
> +		list_del(&chunk->tfm_node);
> +		kfree(chunk);
> +	}
> +	spin_unlock_bh(&tctx->chunk_lock);

[Severity: High]
Is doing this sweep only at TFM teardown what permits the unbounded memory
growth during the TFM's lifetime for abandoned requests?

[ ... ]
> +	for (i = 0; i < CMH_KMAC_ALG_COUNT; i++) {
> +		const struct cmh_kmac_alg_info *info =
> +			&cmh_kmac_algs_info[i];
> +		struct cmh_kmac_alg_drv *drv = &cmh_kmac_drvs[i];
> +		struct ahash_alg *alg = &drv->alg;
> +
> +		drv->info = info;
> +
> +		alg->init   = cmh_kmac_init;
> +		alg->update = cmh_kmac_update;
> +		alg->final  = cmh_kmac_final;
> +		alg->finup  = cmh_kmac_finup;
> +		alg->digest = cmh_kmac_digest;
> +		alg->export = cmh_kmac_export;
> +		alg->import = cmh_kmac_import;

[Severity: Medium]
Is it problematic to expose these stubs to the crypto API by assigning them
here? The unsupported export/import will cause testmgr initialization to fail.

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