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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] crypto: cmh: cshake arbitrary length kmalloc causes ENOMEM/EMSGSIZE
- [High] crypto: cmh: kmac init on active request leaks chunks causing TFM DoS
- [High] crypto: cmh: kmac finup duplicates data on EAGAIN retry
- [Medium] crypto: cmh: kmac final error path allows empty MAC generation on retry
--

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

crypto: cmh - add CSHAKE/KMAC ahash

This commit registers ahash algorithms for cSHAKE128, cSHAKE256, KMAC128, and
KMAC256 using the CMH hash core. CSHAKE supports incremental update and
export/import, while KMAC accumulates the input for a single finalize-time
submission.

> diff --git a/drivers/crypto/cmh/cmh_cshake.c b/drivers/crypto/cmh/cmh_cshake.c
> new file mode 100644
> index 0000000000000..56a6453a0e4fe
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_cshake.c
> @@ -0,0 +1,873 @@

[ ... ]

> +static int cmh_cshake_update(struct ahash_request *req)
> +{

[ ... ]

> +	/*
> +	 * Reject a single update whose linearisation would exceed the largest
> +	 * kmalloc: return a permanent -EMSGSIZE ("message too long") rather
> +	 * than a transient -ENOMEM the client would keep retrying.
> +	 */
> +	if (full_len > KMALLOC_MAX_SIZE)
> +		return -EMSGSIZE;
> +
> +	rctx->data_buf = kmalloc(full_len, gfp | __GFP_NOWARN);
> +	if (!rctx->data_buf)
> +		return -ENOMEM;

[Severity: High]
Does this code artificially limit standard ahash capabilities? The ahash API
requires drivers to support arbitrary-length inputs provided as scatterlists.

By linearizing the entire input scatterlist into a single contiguous kmalloc
buffer, inputs larger than KMALLOC_MAX_SIZE are rejected with -EMSGSIZE.

Even for large inputs below this limit, relying on high-order contiguous
allocations is prone to -ENOMEM failures under memory fragmentation.

[ ... ]

> +static int cmh_cshake_finup(struct ahash_request *req)
> +{

[ ... ]

> +	/* Reject an oversized linearisation with a permanent -EMSGSIZE. */
> +	if (data_len > KMALLOC_MAX_SIZE)
> +		return -EMSGSIZE;
> +
> +	gfp = req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP ?
> +	      GFP_KERNEL : GFP_ATOMIC;
> +
> +	data_buf = kmalloc(data_len, gfp | __GFP_NOWARN);

[Severity: High]
Is this the same single-buffer allocation pattern repeated here?

This appears to also break standard API usage by failing on large inputs.

> diff --git a/drivers/crypto/cmh/cmh_kmac.c b/drivers/crypto/cmh/cmh_kmac.c
> new file mode 100644
> index 0000000000000..97ff36af13a07
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_kmac.c
> @@ -0,0 +1,690 @@

[ ... ]

> +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);
> +	rctx->num_chunks = 0;
> +	rctx->total_len = 0;
> +
> +	return 0;
> +}

[Severity: High]
Could this leak previously accumulated chunks on an active request?

Re-initializing an active request is permitted by the kernel crypto API to
abort a stream and restart. This code resets the list with INIT_LIST_HEAD
without calling cmh_kmac_free_chunks() to free any existing data.

The chunks would remain orphaned on tctx->all_chunks, permanently consuming
the per-TFM memory cap. Repeated aborts/re-inits will exhaust
CMH_KMAC_TFM_MAX_BUFFERED, breaking all subsequent operations on the TFM
with -ENOMEM.

[ ... ]

> +static int cmh_kmac_final(struct ahash_request *req)
> +{

[ ... ]

> +out_free:
> +	cmh_kmac_free_chunks(rctx, tctx);
> +	return ret;
> +}

[Severity: Medium]
Will this error path fail to prevent subsequent calls from silently hashing
an empty buffer?

If a synchronous failure occurs and the code jumps to out_free, it destroys
the accumulated input state but doesn't set rctx->error = ret.

If the caller retries the failed operation, the driver will see 0 chunks
and no error, and compute a MAC for an empty string instead of returning
the failure.

[ ... ]

> +static int cmh_kmac_finup(struct ahash_request *req)
> +{
> +	int ret;
> +
> +	ret = cmh_kmac_update(req);
> +	if (ret)
> +		return ret;
> +
> +	return cmh_kmac_final(req);
> +}

[Severity: High]
Does this logic duplicate data if the operation is retried?

If cmh_kmac_final() fails with -EAGAIN or -EBUSY, it intentionally retains
accumulated chunks so the caller can retry the submission. However, if the
original operation was crypto_ahash_finup(), the caller will retry finup.

This will call cmh_kmac_update() again, unconditionally allocating and
appending the input data to rctx->chunks, which duplicates the input data
and causes the retried operation to output an incorrect MAC.

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