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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] CTR and CFB modes declare 1-byte block size but fail to buffer partial blocks, leading to stream corruption and keystream reuse.
- [High] Unbounded accumulation of dynamically allocated `cmh_sm4_mac_chunk` structures on the TFM-global list (`tctx->all_chunks`), leading to Denial of Service (OOM) via long-lived TFMs.
- [Medium] High-order kmalloc() is used for DMA linearisation of large payload buffers without __GFP_NOWARN, causing kernel splats and predictable allocation failures.
--

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

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

This commit registers SM4 algorithms using the CMH SM4 core, including
skcipher, aead, and ahash variants.

[ ... ]
> diff --git a/drivers/crypto/cmh/cmh_sm4_aead.c b/drivers/crypto/cmh/cmh_sm4_aead.c
[ ... ]
> @@ -567,8 +567,6 @@ static int cmh_sm4_aead_crypt(struct aead_request *req, u32 sm4_op)
>  	rctx->authsize = authsize;
>  	rctx->encrypting = (sm4_op == SM4_OP_ENCRYPT);
>  
>  	/* Linearise AAD */
>  	if (req->assoclen > 0) {
>  		rctx->aad_buf = kmalloc(req->assoclen, gfp);

[Severity: Medium]
Will this high-order allocation trigger kernel splats if it fails under memory
fragmentation? The maximum payload size is checked against 1MB (SZ_1M), which
can require an order-8 contiguous page allocation. Because unprivileged users
can trigger this via AF_ALG, adding the __GFP_NOWARN flag here and for the
input buffer allocation below might be necessary to avoid spamming dmesg.

>  		if (!rctx->aad_buf)
>  			return -ENOMEM;
[ ... ]
> diff --git a/drivers/crypto/cmh/cmh_sm4_cmac.c b/drivers/crypto/cmh/cmh_sm4_cmac.c
[ ... ]
> @@ -279,7 +279,6 @@ 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 zero-initialization leak chunks that were previously allocated and
added to the context? If a user-space application opens an AF_ALG socket,
calls update() multiple times (which allocates chunks and links them to both
rctx->chunks and tctx->all_chunks), and then calls accept() to reset the hash
state, init() will be called again.

Because the old chunks are abandoned on the global tctx->all_chunks list
without being freed here, can this result in an unbounded TFM-global memory
leak and a denial of service (OOM)? The same pattern occurs in
cmh_sm4_mac_import().

[ ... ]
> diff --git a/drivers/crypto/cmh/cmh_sm4_skcipher.c b/drivers/crypto/cmh/cmh_sm4_skcipher.c
[ ... ]
> @@ -167,16 +167,11 @@ 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_CBC:
[ ... ]
>  	case SM4_MODE_CTR:
>  		/* Arithmetic big-endian 128-bit counter increment */
>  		nblocks = DIV_ROUND_UP(req->cryptlen, bs);

[Severity: Critical]
For CTR mode, since the block size is declared as 1 byte in
cmh_sm4_register(), is it possible for callers to provide partial blocks
(e.g., 5 bytes)?

If so, DIV_ROUND_UP will advance the counter to the next full block (nblocks=1)
even though the block was only partially consumed. On the next request, the
encryption will start at the next counter block instead of continuing the
keystream from the correct offset, leading to corrupted streams and keystream
reuse.

>  		{
>  			u8 *iv = req->iv;
[ ... ]
>  	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) {

[Severity: Critical]
Similarly for CFB mode, if a caller issues a sub-block request, the IV update
is skipped entirely. Will the next sub-block request then reuse the exact same
IV and keystream, breaking cryptographic confidentiality? Because the algorithm
declares a 1-byte block size, partial block updates need to be properly
buffered instead of skipping the IV update.

>  			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;
[ ... ]
> @@ -379,10 +379,12 @@ static int cmh_sm4_crypt(struct skcipher_request *req, u32 sm4_op)
>  
>  	/*
>  	 * cryptlen is user-controlled up to CMH_SM4_MAX_CRYPTLEN (well above
>  	 * KMALLOC_MAX_SIZE), so use __GFP_NOWARN: an oversized request fails
>  	 * cleanly with -ENOMEM instead of splatting the page allocator.
>  	 */
>  	rctx->in_buf = kmalloc(req->cryptlen, gfp | __GFP_NOWARN);

[Severity: Medium]
Will this reliably fail with -ENOMEM for valid inputs? The cryptlen limit
CMH_SM4_MAX_CRYPTLEN is set to 32MB, but kmalloc cannot allocate memory beyond
KMALLOC_MAX_SIZE (which is generally much smaller, typically 4MB or 8MB).

>  	if (!rctx->in_buf)
>  		return -ENOMEM;

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