Re: [PATCH v3 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:
- [Critical] Unbounded memory allocation in `cmh_cshake_update` allows trivial kernel memory exhaustion (OOM).
- [High] Accumulated dynamically allocated chunks leak when ahash requests are re-initialized, imported, or abandoned.
- [High] `cmh_cshake_export` sleeps in atomic context by using `GFP_KERNEL` and performing a synchronous hardware submission.
- [High] Direct DMA mapping of a stack-allocated request context variable.
- [High] Lack of synchronization in `setkey` allows concurrent hardware access to freed key memory via DMA.
- [High] Integer overflow vulnerability during chunk allocation leads to heap buffer overflow.
- [Medium] Unaligned structure members are directly mapped for DMA, breaking cacheline coherency.
--

--- Patch [6]: [PATCH v3 06/19] crypto: cmh - add CSHAKE/KMAC ahash ---
commit 6138eb846ff49a783a8916ba0affae21d8744139
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. It adds support for incremental
updates and export/import for cSHAKE, and imposes a 64KB data cap for
KMAC.

> 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
> @@ -0,0 +1,814 @@
[ ... ]
> +struct cmh_cshake_chunk {
> +	struct list_head  list;
> +	struct list_head  tfm_node; /* per-tfm orphan tracking */
> +	u32               len;
> +	u8                data[];
> +};

[Severity: Medium]
Will this structure layout cause data corruption on cache-incoherent
architectures? The data array follows list pointers and a u32 length field,
placing it at an unaligned offset. When this flexible array member is
directly mapped for DMA, it violates the requirement that dynamically mapped
buffers must be aligned to ARCH_DMA_MINALIGN to prevent cacheline sharing.

[ ... ]
> +static int cmh_cshake_setkey(struct crypto_ahash *tfm, const u8 *key,
> +			     unsigned int keylen)
> +{
[ ... ]
> +	/* Free previous N and S */
> +	kfree(tctx->func_name);
> +	kfree(tctx->custom);

[Severity: High]
Could this lockless memory free race with an ongoing final() operation?
If user space calls setsockopt() to set a new key concurrently with
socket I/O, the active custom string's memory might be unmapped and freed
while the hardware is actively accessing it via DMA, leading to a
use-after-free and potential memory corruption.

[ ... ]
> +static int cmh_cshake_init(struct ahash_request *req)
> +{
> +	struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
> +	struct cmh_cshake_reqctx *rctx = ahash_request_ctx(req);
> +
> +	rctx->info = cmh_cshake_get_info(tfm);
> +	rctx->error = 0;
> +	INIT_LIST_HEAD(&rctx->chunks);
> +	rctx->num_chunks = 0;

[Severity: High]
Does this cause a memory leak when a request is re-initialized or abandoned?
If a caller issues crypto_ahash_init() on an existing request that already
accumulated data chunks, the chunk list is reset via INIT_LIST_HEAD()
without freeing the existing chunks. The chunks remain indefinitely tracked
in the transform context list, which could allow unbounded memory growth
over the lifetime of a long-lived transform.

[ ... ]
> +static int cmh_cshake_update(struct ahash_request *req)
> +{
[ ... ]
> +	if (!req->nbytes)
> +		return 0;
> +
> +	chunk = kmalloc(sizeof(*chunk) + req->nbytes,
> +			req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP ?
> +			GFP_KERNEL : GFP_ATOMIC);

[Severity: Critical]
Can the arithmetic operation here wrap around on 32-bit platforms?
Since req->nbytes is an unsigned int controlled by the caller, providing
an extremely large value could overflow the addition, allocating a tiny
buffer. The subsequent data copy would then overwrite kernel heap memory.

[Severity: Critical]
Does this allow an unprivileged attacker to exhaust kernel memory?
Unlike the KMAC implementation which enforces a 64KB maximum limit, the
cSHAKE implementation unconditionally buffers all incoming data chunks
without any bounds check on the total length or number of chunks.

[ ... ]
> +static int cmh_cshake_final(struct ahash_request *req)
> +{
[ ... ]
> +	/* Map checkpoint buffer if present (CMH eSW reads it) */
> +	if (rctx->has_checkpoint) {
> +		ckpt_dma = cmh_dma_map_single(rctx->checkpoint,
> +					      HC_CONTEXT_SIZE, DMA_TO_DEVICE);

[Severity: High]
Is it safe to map this request context variable directly for DMA?
The driver maps rctx->checkpoint via cmh_dma_map_single(), but the ahash
request context is frequently placed on the kernel stack by callers.
DMA mapping stack memory causes cacheline sharing corruption on non-coherent
architectures and immediately panics if DMA API debugging is enabled.

[ ... ]
> +static int cmh_cshake_export(struct ahash_request *req, void *out)
> +{
[ ... ]
> +	if (rctx->num_chunks > 0) {
> +		sgm = cmh_cshake_build_sg(rctx, GFP_KERNEL);
> +		if (!sgm)
> +			return -ENOMEM;
> +	}
> +
> +	save_buf = kzalloc(HC_CONTEXT_SIZE, GFP_KERNEL);
> +	if (!save_buf) {
[ ... ]
> +	ret = cmh_vcq_pack_and_submit(cmds, idx, rctx->packed, CMH_CSHAKE_MAX_PACKED,
> +				      d.mbx_idx);

[Severity: High]
Will this sequence sleep in atomic context? 
The crypto_ahash_export() API must be non-blocking because it is called
from atomic contexts like IPsec softirq. Allocating memory with GFP_KERNEL
and performing a synchronous, blocking hardware submission will trigger a
bug or deadlock when called from these contexts.

[ ... ]
> +static int cmh_cshake_import(struct ahash_request *req, const void *in)
> +{
[ ... ]
> +	rctx->error = 0;
> +	INIT_LIST_HEAD(&rctx->chunks);
> +	rctx->num_chunks = 0;

[Severity: High]
Does this cause a memory leak when a request is imported?
Similar to cmh_cshake_init(), this resets the chunk list without freeing
the previously accumulated chunks.

> 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
> @@ -0,0 +1,630 @@
[ ... ]
> +struct cmh_kmac_chunk {
> +	struct list_head  list;
> +	struct list_head  tfm_node; /* per-tfm orphan tracking */
> +	u32               len;
> +	u8                data[];
> +};

[Severity: Medium]
Will this structure layout cause data corruption on cache-incoherent
architectures? Similar to the cSHAKE implementation, placing the flexible
array member at an unaligned offset and mapping it directly for DMA breaks
cacheline coherency.

[ ... ]
> +static int cmh_kmac_setkey(struct crypto_ahash *tfm, const u8 *key,
> +			   unsigned int keylen)
> +{
[ ... ]
> +		/* Store raw key */
> +		{
> +			int ret = cmh_key_setkey_raw(&tctx->key, ptr,
> +						     raw_keylen, CORE_ID_HC);
> +			if (ret)
> +				return ret;
> +		}

[Severity: High]
Could this lockless memory replacement race with an ongoing final() function?
The cmh_key_setkey_raw() function destroys the active key and unmaps its DMA
address. If this happens concurrently with hardware accessing the key memory
via DMA during a request, it could lead to a use-after-free and memory
corruption.

[ ... ]
> +static int cmh_kmac_init(struct ahash_request *req)
> +{
[ ... ]
> +	rctx->error = 0;
> +	INIT_LIST_HEAD(&rctx->chunks);
> +	rctx->num_chunks = 0;

[Severity: High]
Does this cause a memory leak when a request is re-initialized?
Like in the cSHAKE driver, the chunk list is reset without freeing the
previously accumulated chunks, leaking kernel memory.

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

[Severity: Critical]
Can the arithmetic operation here wrap around on 32-bit platforms?
Since req->nbytes is controlled by the caller, providing an extremely large
value could overflow the addition and allocate a tiny buffer. The subsequent
data copy would then overwrite kernel heap memory.

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