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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] The driver allocates up to 32MB of contiguous memory using kmalloc for skcipher requests, which exceeds KMALLOC_MAX_SIZE and triggers page allocator warnings.
- [High] The ahash export() function fails with -ENOSPC for valid hash accumulations, breaking the crypto API contract.
- [High] The ahash update() error path fails to reset total_len, allowing uninitialized kernel memory to be hashed and exposed as a MAC oracle.
- [High] Incorrect IV chaining in cmh_sm4_update_iv for CFB mode when req->cryptlen is not a multiple of the block size.
- [High] Unbounded memory leak of accumulated MAC data chunks when ahash requests are aborted, re-initialized, or imported.
- [High] The cmh_sm4_mac_import() function unconditionally uses GFP_KERNEL, which can cause a sleep in atomic context.
- [Medium] Failed key setups in AEAD and MAC algorithms lead to a torn state where the software fallback cipher and the hardware context use different keys.
--

--- Patch [9]: [PATCH v2 09/19] crypto: cmh - add SM4 skcipher/aead/cmac/xcbc ---
commit f6f59427e5bb8538a2126c7ca1459a18ff4882e0
Author: Alex Ousherovitch <aousherovitch@rambus.com>

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

Registers SM4 algorithms using the CMH SM4 core (core ID 0x04),
including skcipher, aead, and ahash variants.

> diff --git a/drivers/crypto/cmh/cmh_sm4_aead.c b/drivers/crypto/cmh/cmh_sm4_aead.c
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_sm4_aead.c
[ ... ]
> +static int cmh_sm4_aead_setkey(struct crypto_aead *tfm, const u8 *key,
> +			       unsigned int keylen)
> +{
> +	struct cmh_sm4_aead_tfm_ctx *tctx = crypto_aead_ctx(tfm);
> +	/* SM4 always uses 128-bit keys */
> +	if (keylen != CMH_SM4_KEY_SIZE)
> +		return -EINVAL;
> +
> +	if (tctx->sw_cipher) {
> +		int ret;
> +
> +		ret = crypto_cipher_setkey(tctx->sw_cipher, key, keylen);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return cmh_key_setkey_raw(&tctx->key, key, keylen, CORE_ID_SM4);
> +}

[Severity: Medium]
Could this leave the transform context in a torn state if the hardware key
setup fails?

If cmh_key_setkey_raw() returns an error, the software cipher has already 
been updated with the new key. Empty inputs might then be processed by the 
software fallback using the new key, while non-empty inputs could be 
processed by the hardware using the old key. 

Would it be safer to only update the software cipher after the hardware 
key setup succeeds?

> diff --git a/drivers/crypto/cmh/cmh_sm4_cmac.c b/drivers/crypto/cmh/cmh_sm4_cmac.c
> --- /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 leak accumulated chunks if the request is re-initialized?

The ahash cmh_sm4_mac_init() function simply zero-fills the request
context, which abandons any chunks previously allocated and appended to
tctx->all_chunks. Since these chunks are only freed during the teardown
in cmh_sm4_mac_exit_tfm(), could repeated initializations cause an
unbounded memory leak leading to an out of memory condition?

[ ... ]
> +err_free_chunks:
> +	/*
> +	 * Terminal error -- free all previously accumulated chunks.
> +	 * callers may not call .final() on error, so they would leak.
> +	 */
> +	cmh_sm4_mac_free_chunks(rctx, tctx);
> +	return ret;
> +}

[Severity: High]
Can this error path in cmh_sm4_mac_update() lead to an uninitialized
memory leak to user-space?

If an error occurs and it jumps to err_free_chunks, it frees the 
accumulated chunks but does not reset rctx->total_len. 
    
If a caller subsequently invokes final(), the code uses this stale 
rctx->total_len to allocate the buffer:

rctx->buf = kmalloc(rctx->total_len, gfp);

Since the chunk list is empty, nothing is copied into the buffer, leaving 
it fully uninitialized. This uninitialized kernel memory is then DMA-mapped 
and passed to the hardware for MAC tag generation, which could expose 
sensitive kernel memory when the tag is returned.

[ ... ]
> +static int cmh_sm4_mac_export(struct ahash_request *req, void *out)
> +{
> +	struct cmh_sm4_mac_reqctx *rctx = ahash_request_ctx(req);
> +	struct cmh_sm4_mac_export_state *state = out;
> +	struct cmh_sm4_mac_chunk *chunk;
> +	u32 offset = 0;
> +
> +	if (rctx->total_len > CMH_SM4_MAC_EXPORT_MAX)
> +		return -ENOSPC;

[Severity: High]
Does this break the crypto API contract for ahash export?

The driver allows accumulating up to 64KB in cmh_sm4_mac_update(), but 
cmh_sm4_mac_export() strictly checks against CMH_SM4_MAC_EXPORT_MAX (which 
is slightly under 4KB). If a caller accumulates more than this threshold 
and calls export(), it will fail with ENOSPC. 

Since the API requires export() to succeed when provided a buffer of 
exactly statesize, will this unexpectedly break subsystems like IPsec 
that clone ahash states?

[ ... ]
> +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]
Could this memset abandon existing chunks similar to the init() function?

When the crypto API imports state, the request context might already 
contain active allocations. Zeroing it out directly abandons them in 
tctx->all_chunks, creating another pathway for unbounded memory leaks.

> +
> +	if (state->total_len > CMH_SM4_MAC_EXPORT_MAX)
> +		return -EINVAL;
> +
> +	if (state->total_len) {
> +		chunk = kmalloc(sizeof(*chunk) + state->total_len, GFP_KERNEL);

[Severity: High]
Is it safe to use GFP_KERNEL here?

The crypto API allows import() to be called from atomic or softirq 
contexts, such as when the networking stack restores IPsec states. Using 
GFP_KERNEL here allows direct reclaim, which could result in a scheduling
while atomic bug or deadlock.

> diff --git a/drivers/crypto/cmh/cmh_sm4_skcipher.c b/drivers/crypto/cmh/cmh_sm4_skcipher.c
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_sm4_skcipher.c
[ ... ]
> +static void cmh_sm4_update_iv(struct skcipher_request *req, u32 mode,
> +			      u32 op, const u8 *in_buf, const u8 *out_buf)
> +{
> +	u32 bs = CMH_SM4_BLOCK_SIZE;
> +	u32 nblocks;
> +
> +	switch (mode) {
[ ... ]
> +	case SM4_MODE_CFB:
> +		/*
> +		 * For sub-block requests (cryptlen < 16), there is no
> +		 * complete ciphertext block to chain, so the IV is left
> +		 * unchanged -- CFB-128 has no defined chaining semantic
> +		 * for partial blocks (shift-register CFB-n is a different
> +		 * mode).  Without this guard the pointer arithmetic
> +		 * underflows and reads before the buffer.
> +		 */
> +		if (req->cryptlen >= bs) {
> +			if (op == SM4_OP_ENCRYPT)
> +				memcpy(req->iv, out_buf + req->cryptlen - bs,
> +				       bs);
> +			else
> +				memcpy(req->iv, in_buf + req->cryptlen - bs,
> +				       bs);
> +		}
> +		break;

[Severity: High]
Will this copy an invalid IV for unaligned CFB requests?

If req->cryptlen is not a multiple of the 16-byte block size, this pointer 
arithmetic will copy a mix of bytes from the last full ciphertext block and 
the final partial block. 
    
Since CFB-128 strictly requires full 128-bit blocks for its feedback 
mechanism, this looks like it creates a shift-register style IV. 
Could this result in corrupted ciphertext on chained sub-block requests?

[ ... ]
> +static int cmh_sm4_crypt(struct skcipher_request *req, u32 sm4_op)
> +{
> +	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
> +	struct cmh_sm4_tfm_ctx *tctx = crypto_skcipher_ctx(tfm);
[ ... ]
> +	rctx->in_buf = kmalloc(req->cryptlen, gfp);
> +	if (!rctx->in_buf)
> +		return -ENOMEM;

[Severity: High]
Can this allocation exceed the maximum kmalloc limits?

The req->cryptlen value is user-controlled and can be up to 32MB 
(CMH_SM4_MAX_CRYPTLEN). Passing this directly to kmalloc() exceeds 
the typical maximum page order limit (usually 4MB or 8MB). 

If a large payload is submitted, this will fail and trigger a page 
allocation warning in the kernel log. Could this be exploited by an 
unprivileged user to cause log spam or a denial of service? Would 
a scatter-gather approach or chunking be more appropriate here?

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