Re: [PATCH v4 04/19] crypto: cmh - add SHA-2/SHA-3/SHAKE ahash

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] DMA mapping the inline `rctx->checkpoint` buffer violates DMA API rules and causes memory corruption due to cache-line sharing or stack mapping.
- [High] The driver linearizes arbitrary-length hash updates into a single kmalloc buffer, causing valid large updates to predictably fail with -ENOMEM.
- [High] Misusing the crypto API `cra_blocksize` for hardware alignment breaks SHAKE holdback logic, leading to hardware state corruption and potential buffer overflows.
- [High] Accessing `req->src` and `rctx` after submitting an asynchronous hardware request causes a Use-After-Free.
- [Medium] The commit message claims zero-copy support for virtual buffers, but the implementation unconditionally copies the data.
--

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

crypto: cmh - add SHA-2/SHA-3/SHAKE ahash

This commit registers various ahash algorithms (SHA-2, SHA-3, SHAKE) using
the CMH hash core. It implements the necessary operations for incremental
updates, request cloning, and handles hardware submissions.

>     Supports incremental update/finup/final, init/export/import for
>     request cloning, and the CRYPTO_AHASH_REQ_VIRT flag for zero-copy
>     from virtual buffers.

[Severity: Medium]
Does the driver actually perform zero-copy DMA for virtual buffers?

Looking at cmh_hash_update() and cmh_hash_finup(), the implementation
appears to unconditionally allocate a bounce buffer with kmalloc() and
then executes a memcpy() from req->svirt into the bounce buffer. Is there
a code path that avoids the allocation and directly maps the virtual buffer
to DMA, or should the commit message be updated to reflect the copy?

> diff --git a/drivers/crypto/cmh/cmh_hash.c b/drivers/crypto/cmh/cmh_hash.c
> new file mode 100644
> index 0000000000000..72d09c5b0c91f
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_hash.c
[ ... ]
> +	{
> +		.hc_algo     = HC_ALGO_SHAKE128,
> +		.digest_size = CMH_SHAKE128_DIGEST_SIZE,
> +		.block_size  = 1,    /* XOF: no meaningful block for crypto API */
> +		.alg_name    = "shake128",
> +		.drv_name    = "rambus-cmh-shake128",
> +	},
[ ... ]
> +static int cmh_hash_update(struct ahash_request *req)
> +{
[ ... ]
> +	/* Not enough for a full block -- just buffer */
> +	if (total_avail < block_size) {
> +		if (req->base.flags & CRYPTO_AHASH_REQ_VIRT)
> +			memcpy(rctx->buf + rctx->buf_len,
> +			       req->svirt, req->nbytes);

[Severity: High]
Does this logic bypass the software holdback for SHAKE algorithms?

Because block_size is explicitly defined as 1 to indicate byte-oriented
operation for the crypto API, the condition total_avail < block_size is
practically never met (only when total_avail is 0).

The hardware Keccak cores require intermediate updates to be multiples of
the algorithm's block rate (e.g., 168 bytes for SHAKE-128) to correctly
save the state. Submitting unaligned arbitrary byte fragments might
corrupt the hardware state. Additionally, CMH_HASH_MAX_BLOCK is 144, which
would not be able to hold back a true SHAKE-128 block of 168 bytes without
overflowing the array.

[ ... ]
> +	/*
> +	 * Linearise: holdback prefix + full blocks from scatterlist.
> +	 * full_len is user-controlled and can exceed KMALLOC_MAX_SIZE for a
> +	 * single large update; __GFP_NOWARN avoids splatting the page
> +	 * allocator on an oversized request.
> +	 */
> +	rctx->data_buf = kmalloc(full_len, gfp | __GFP_NOWARN);
> +	if (!rctx->data_buf)
> +		return -ENOMEM;

[Severity: High]
Will legitimate large hash requests deterministically fail with -ENOMEM
here?

The ahash API does not enforce a maximum size on req->nbytes, and clients
like IPsec, IMA, or AF_ALG can submit very large scatterlists. Because
kmalloc() is strictly limited to KMALLOC_MAX_SIZE and can fail earlier due
to fragmentation, dynamically allocating a contiguous buffer for an
arbitrary-length user-provided req->nbytes causes large updates to predictably
fail with -ENOMEM.

[ ... ]
> +	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 DMA mapping the inline rctx->checkpoint violate the DMA API rules?

The checkpoint array is embedded inline in struct cmh_hash_reqctx without
being aligned to the cacheline. If the request context is allocated on the
stack (e.g., via AHASH_REQUEST_ON_STACK), this maps stack memory for DMA,
which is prohibited by the DMA API and may trigger a kernel panic.

Even if heap-allocated, the unaligned array shares a cache line with
adjacent members like rctx->packed. Modifying these adjacent fields while
DMA is active could corrupt the checkpoint data (or vice versa) on
non-coherent architectures. This same mapping also happens in
cmh_hash_submit_final().

[ ... ]
> +	ret = cmh_vcq_pack_and_submit_async(cmds, idx, rctx->packed,
> +					    CMH_HASH_MAX_PACKED,
> +					    d.mbx_idx,
> +					    cmh_hash_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]
Can accessing req->src and rctx after submitting an asynchronous hardware
request result in a use-after-free?

Once cmh_vcq_pack_and_submit_async() submits the request to the hardware,
the completion callback cmh_hash_update_complete() could run immediately
on another CPU and signal completion to the client. The client is then
free to release the ahash_request (which contains rctx) and the source
scatterlist (req->src).

If the original thread continues execution and attempts to copy the tail
data into rctx->buf using req->src, it will access freed memory.

[ ... ]
> +static int cmh_hash_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: High]
Similar to the allocation in cmh_hash_update(), could this unbounded
contiguous allocation fail deterministically for valid large requests?

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