Re: [PATCH v4 09/19] crypto: cmh - add SM4 skcipher/aead/cmac/xcbc

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unbounded kernel memory leak (OOM DoS) in SM4 MAC algorithms because dynamically allocated chunks are orphaned during request abortion or reuse, accumulating indefinitely on the TFM.
--

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

crypto: cmh - add SM4 skcipher/aead/cmac/xcbc

This commit registers SM4 algorithms using the CMH SM4 core. It adds support
for various skcipher, aead, and ahash (CMAC and XCBC) operations.

> diff --git a/drivers/crypto/cmh/cmh_sm4_cmac.c b/drivers/crypto/cmh/cmh_sm4_cmac.c
> new file mode 100644
> index 0000000000000..acf654d98b0b4
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_sm4_cmac.c

[ ... ]

> +static int cmh_sm4_mac_init(struct ahash_request *req)
> +{
> +	struct cmh_sm4_mac_reqctx *rctx = ahash_request_ctx(req);
> +
> +	memset(rctx, 0, sizeof(*rctx));
> +	INIT_LIST_HEAD(&rctx->chunks);
> +	return 0;
> +}

[Severity: High]
Does this code leak cmh_sm4_mac_chunk structures if a request is reused?

By clearing rctx without freeing pre-existing chunks, it appears those chunks
become permanently stranded on the tctx->all_chunks list until the entire TFM
is destroyed.

[ ... ]

> +static int cmh_sm4_mac_update(struct ahash_request *req)
> +{
> +	struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
> +	struct cmh_sm4_mac_tfm_ctx *tctx = crypto_ahash_ctx(tfm);
> +	struct cmh_sm4_mac_reqctx *rctx = ahash_request_ctx(req);
> +	struct cmh_sm4_mac_chunk *chunk;

[ ... ]

> +	chunk->len = req->nbytes;
> +	if (req->base.flags & CRYPTO_AHASH_REQ_VIRT)
> +		memcpy(chunk->data, req->svirt, req->nbytes);
> +	else
> +		scatterwalk_map_and_copy(chunk->data, req->src,
> +					 0, req->nbytes, 0);
> +	list_add_tail(&chunk->list, &rctx->chunks);
> +	spin_lock_bh(&tctx->chunk_lock);
> +	list_add_tail(&chunk->tfm_node, &tctx->all_chunks);
> +	spin_unlock_bh(&tctx->chunk_lock);
> +	rctx->total_len += req->nbytes;
> +	return 0;

[Severity: High]
Can this lead to an unbounded memory leak and subsequent kernel OOM if a user
creates an AF_ALG socket, streams data via cmh_sm4_mac_update(), and then
closes the socket without calling final?

Since the dynamically allocated chunks are tied to the TFM's lifetime on
tctx->all_chunks and the crypto API destroys the request without calling a
driver cleanup callback, an unprivileged local user could potentially trigger
this accumulation indefinitely, causing a denial of service.

[ ... ]

> +static int cmh_sm4_mac_import(struct ahash_request *req, const void *in)
> +{
> +	struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
> +	struct cmh_sm4_mac_tfm_ctx *tctx = crypto_ahash_ctx(tfm);
> +	struct cmh_sm4_mac_reqctx *rctx = ahash_request_ctx(req);
> +	const struct cmh_sm4_mac_export_state *state = in;
> +	struct cmh_sm4_mac_chunk *chunk;
> +
> +	/*
> +	 * Do NOT call free_chunks() here: the crypto API does not
> +	 * guarantee the request context is in a valid state before
> +	 * import(), so the list pointers may be stale or invalid.
> +	 * Re-initialize from scratch instead.  Any pre-existing chunks
> +	 * are tracked on tctx->all_chunks and freed in exit_tfm.
> +	 */
> +	memset(rctx, 0, sizeof(*rctx));
> +	INIT_LIST_HEAD(&rctx->chunks);

[Severity: High]
Is it safe to intentionally strand previous chunks here?

Because they are only cleaned up in exit_tfm instead of when the request
state is replaced, a rapid sequence of export and import operations on the
same request could quickly exhaust kernel memory.

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