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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Linearizing arbitrarily large ahash input buffers via kmalloc restricts request sizes and risks deterministic allocation failures.
- [High] DMA mapping an inline array (rctx->checkpoint) potentially on the stack violates the DMA API, causing cacheline sharing and vmap stack corruption.
- [High] An integer overflow when calculating total available data leads to a massive heap buffer overflow.
- [Medium] cmh_sm3_export leaks uninitialized memory into the exported state buffer.
- [Medium] Missing endianness conversions when populating hardware command descriptors.
--

--- Patch [7]: [PATCH v2 07/19] crypto: cmh - add SM3 ahash ---
commit 28c61ed61efa5f206b21de41a5196e656d04ad75
Author: Alex Ousherovitch <aousherovitch@rambus.com>

crypto: cmh - add SM3 ahash

Register the SM3 ahash algorithm using the CMH SM3 core (core ID
0x05).  Supports incremental update/finup/final and export/import.

> diff --git a/drivers/crypto/cmh/cmh_sm3.c b/drivers/crypto/cmh/cmh_sm3.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_sm3.c
> @@ -0,0 +1,651 @@
[ ... ]
> +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 */

[Severity: High]
Can this structure be allocated on the stack?

The crypto framework often allocates ahash request contexts on the stack
using AHASH_REQUEST_ON_STACK. If CONFIG_VMAP_STACK is enabled, mapping this
inline checkpoint array for DMA via cmh_dma_map_single() will pass a
vmalloc address, which yields an invalid physical address.

Additionally, since the checkpoint array isn't cacheline aligned, could
DMA operations corrupt adjacent fields in the context on non-coherent
architectures?

> +	/* DMA state for current async operation */
> +	dma_addr_t ckpt_dma;
> +	dma_addr_t save_dma;
[ ... ]
> +static void vcq_add_sm3_update(struct vcq_cmd *slot, u32 core_id, u64 input_phys, u32 len)
> +{
> +	memset(slot, 0, sizeof(*slot));
> +	slot->magic = VCQ_CMD_MAGIC;
> +	slot->id = VCQ_CMD_ID(core_id, 0, 1, SM3_CMD_UPDATE);
> +	slot->hwc.sm3.cmd_update.input = input_phys;
> +	slot->hwc.sm3.cmd_update.inlen = len;
> +}

[Severity: Medium]
Does the hardware expect a specific endianness for these command descriptors?

The CPU-native values are assigned directly to the hardware command structure.
If this driver is built on a Big-Endian architecture and the hardware expects
Little-Endian (which is typical for PCIe/SoCs), will these values be
interpreted correctly without explicit conversions like cpu_to_le64()?

[ ... ]
> +static int cmh_sm3_update(struct ahash_request *req)
> +{
> +	struct cmh_sm3_reqctx *rctx = ahash_request_ctx(req);
> +	struct vcq_cmd cmds[CMH_SM3_MAX_PAYLOAD];
> +	struct core_dispatch d;
> +	u32 total_avail, full_len, tail_len, from_src;
> +	u32 idx;
> +	int ret;
> +	gfp_t gfp;
> +
> +	if (rctx->error)
> +		return rctx->error;
> +
> +	if (!req->nbytes)
> +		return 0;
> +
> +	gfp = req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP ?
> +	      GFP_KERNEL : GFP_ATOMIC;
> +
> +	total_avail = rctx->buf_len + req->nbytes;
> +
> +	if (total_avail < CMH_SM3_BLOCK_SIZE) {

[Severity: High]
Could this arithmetic wrap around if a caller provides a very large request?

If req->nbytes is close to U32_MAX, total_avail can overflow and become
less than CMH_SM3_BLOCK_SIZE. The code would then bypass the block size
check and execute:

    scatterwalk_map_and_copy(rctx->buf + rctx->buf_len,
                             req->src, 0,
                             req->nbytes, 0);

Would this attempt to copy gigabytes of data into the 64-byte rctx->buf,
causing a massive buffer overflow?

> +		if (req->base.flags & CRYPTO_AHASH_REQ_VIRT)
> +			memcpy(rctx->buf + rctx->buf_len,
> +			       req->svirt, req->nbytes);
> +		else
> +			scatterwalk_map_and_copy(rctx->buf + rctx->buf_len,
> +						 req->src, 0,
> +						 req->nbytes, 0);
> +		rctx->buf_len = total_avail;
> +		return 0;
> +	}
> +
> +	full_len = total_avail - total_avail % CMH_SM3_BLOCK_SIZE;
> +	tail_len = total_avail - full_len;
> +	from_src = full_len - rctx->buf_len;
> +
> +	rctx->data_buf = kmalloc(full_len, gfp);

[Severity: High]
Is it safe to assume the input data will always fit in a contiguous kmalloc
allocation?

The ahash API contract requires drivers to handle arbitrarily large inputs
(e.g., multi-megabyte requests from IPsec or dm-verity). Bouncing the
entire scatterlist into a single contiguous buffer via kmalloc() will fail
deterministically for sizes exceeding KMALLOC_MAX_SIZE, or earlier due to
memory fragmentation.

Should the driver process the scatterlist in bounded chunks instead of
attempting to linearize it entirely?

[ ... ]
> +static int cmh_sm3_export(struct ahash_request *req, void *out)
> +{
> +	struct cmh_sm3_reqctx *rctx = ahash_request_ctx(req);
> +	struct cmh_sm3_export_state *state = out;
> +
> +	if (rctx->hw_started && rctx->has_checkpoint)
> +		memcpy(state->checkpoint, rctx->checkpoint, SM3_CONTEXT_SIZE);
> +	else
> +		memset(state->checkpoint, 0, SM3_CONTEXT_SIZE);
> +
> +	if (rctx->buf_len > 0)
> +		memcpy(state->buf, rctx->buf, rctx->buf_len);

[Severity: Medium]
Does this leave the remainder of the export buffer uninitialized?

If rctx->buf_len is less than CMH_SM3_BLOCK_SIZE, the trailing bytes in
state->buf are never initialized. Since the state buffer is provided by the
caller (e.g., via crypto_ahash_export) and could be an uninitialized socket
allocation from AF_ALG, could this leak kernel memory if the state is later
exposed to userspace?

> +
> +	state->buf_len = rctx->buf_len;
> +	state->hw_started = rctx->hw_started;
> +
> +	return 0;
> +}

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