Re: [PATCH v4 07/19] crypto: cmh - add SM3 ahash

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Deterministic failure on large hash updates due to a single large kmalloc linearization.
- [High] Race condition leading to Use-After-Free or state corruption because the request context and input buffers are modified after asynchronous submission.
- [High] DMA mapping of an unaligned embedded structure member violates the DMA API, causes cache coherency corruption, and risks kernel panics if mapped from a vmap stack.
--

commit db71afa324a1c32abd2a50db9da6330962bdae16
Author: Alex Ousherovitch [off-list ref]

crypto: cmh - add SM3 ahash

Register the SM3 ahash algorithm using the CMH SM3 core. Supports
incremental update, finup, final, and export/import functionality.

[ ... ]

+struct cmh_sm3_reqctx {
+	int    error;
+	u32    hw_started;
+	u32    buf_len;
+	u32    has_checkpoint;
+	u8     checkpoint[SM3_CONTEXT_SIZE]; /* SM3 context from last SAVE */
+	/* DMA state for current async operation */

[ ... ]

+static int cmh_sm3_update(struct ahash_request *req)
+{

[ ... ]

+	/*
+	 * 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]
Is it safe to assume the entire remaining data can be linearized into a
single kmalloc buffer? If a caller attempts to hash a large input (e.g.,
several megabytes) using the ahash API via AF_ALG, this allocation will
predictably fail for sizes exceeding KMALLOC_MAX_SIZE or under memory
fragmentation, which breaks the API contract.

[ ... ]

+	rctx->ckpt_dma = DMA_MAPPING_ERROR;
+	if (rctx->has_checkpoint) {
+		rctx->ckpt_dma = cmh_dma_map_single(rctx->checkpoint,
+						    SM3_CONTEXT_SIZE,
+						     DMA_TO_DEVICE);

[Severity: High]
Does mapping this inline checkpoint buffer violate the DMA API?

The checkpoint array is embedded directly inside struct cmh_sm3_reqctx without
ARCH_DMA_MINALIGN alignment, which can lead to cacheline corruption on
non-coherent architectures when adjacent fields are modified.

Additionally, if the request context is allocated on the stack (for instance
via AHASH_REQUEST_ON_STACK) on a system with CONFIG_VMAP_STACK enabled, mapping
a stack address results in a bogus physical address and can cause kernel
panics.

[ ... ]

+	ret = cmh_vcq_pack_and_submit_async(cmds, idx, rctx->packed,
+					    CMH_SM3_MAX_PACKED,
+					    d.mbx_idx,
+					    cmh_sm3_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]
Is there a race condition here that could lead to a use-after-free or state
corruption?

This block modifies the holdback buffer (rctx->buf) and reads from the request
input (req->svirt or req->src) after ownership has already been yielded to the
hardware asynchronously via cmh_vcq_pack_and_submit_async().

If the hardware completes the operation quickly, the interrupt handler could
execute cmh_sm3_update_complete() on another CPU and signal completion to the
caller. The caller could then free or reuse the request and its scatterlists
while this thread is still copying the tail bytes from the freed memory.

[ ... ]

+static int cmh_sm3_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,
+						    SM3_CONTEXT_SIZE,
+						     DMA_TO_DEVICE);

[Severity: High]
Similarly, in the finalization stage, can mapping this embedded inline buffer
cause DMA API violations or kernel panics on vmap stacks?

[ ... ]

+static int cmh_sm3_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]
Does this finup path have the same issue with large inputs?

Because data_len could be very large depending on the caller, this single
kmalloc attempt could deterministically fail if it exceeds KMALLOC_MAX_SIZE,
similar to the issue in the update path.

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