Re: [PATCH v4 08/19] crypto: cmh - add AES skcipher/aead/cmac

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Aborted or re-initialized `cmac(aes)` ahash requests leak accumulated dynamically allocated chunks until TFM destruction, allowing trivial kernel memory exhaustion (OOM).
- [High] Incorrect IV state update in `cmh_aes_update_iv` for AES-CFB mode with partial blocks breaks cipher chaining.
- [Medium] The skcipher driver attempts to kmalloc up to 32MB for input/output buffers, deterministically failing for valid large requests.
--

commit 0ec57ce28ce08fa19d032fb79fd5650db5f53a75
Author: Alex Ousherovitch <aousherovitch@rambus.com>
Subject: crypto: cmh - add AES skcipher/aead/cmac

This commit registers AES algorithms using the CMH AES core for skcipher,
aead, and ahash interfaces. It handles key management, context setup, and
VCQ command sequencing for encryption and authentication.

> diff --git a/drivers/crypto/cmh/cmh_aes.c b/drivers/crypto/cmh/cmh_aes.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_aes.c
[ ... ]
> +static void cmh_aes_update_iv(struct skcipher_request *req, u32 mode,
> +			      u32 op, const u8 *in_buf, const u8 *out_buf)
> +{
> +	u32 bs = CMH_AES_BLOCK_SIZE;
> +	u32 nblocks;
> +
> +	switch (mode) {
[ ... ]
> +	case AES_MODE_CFB:
> +		/*
> +		 * CFB-128 chains on the last ciphertext block.  On encrypt,
> +		 * that is out_buf; on decrypt, it is in_buf.
> +		 *
> +		 * 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 == AES_OP_ENCRYPT)
> +				memcpy(req->iv, out_buf + req->cryptlen - bs,
> +				       bs);
> +			else
> +				memcpy(req->iv, in_buf + req->cryptlen - bs,
> +				       bs);
> +		}
> +		break;

[Severity: High]
When dealing with partial blocks in CFB mode where req->cryptlen is not a 
multiple of the block size, will the offset calculation (req->cryptlen - bs)
point to an unaligned offset rather than the last full block boundary? The 
next operation in a chained CFB stream requires the last full ciphertext 
block for the chaining IV, so could this break the cipher chaining by 
copying the wrong bytes?

[ ... ]
> +#define CMH_AES_MAX_CRYPTLEN	SZ_32M
[ ... ]
> +static int cmh_aes_crypt(struct skcipher_request *req, u32 aes_op)
> +{
[ ... ]
> +	if (req->cryptlen > CMH_AES_MAX_CRYPTLEN)
> +		return -EINVAL;
[ ... ]
> +	/*
> +	 * Linearise input from scatterlist.  cryptlen is user-controlled up
> +	 * to CMH_AES_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);
> +	if (!rctx->in_buf)
> +		return -ENOMEM;

[Severity: Medium]
Does this driver attempt to allocate up to 32MB of physically contiguous 
memory via kmalloc() for the input and output buffers? While the __GFP_NOWARN 
flag ensures a clean failure, any valid request larger than KMALLOC_MAX_SIZE 
(typically 4MB or 8MB) will deterministically fail with -ENOMEM. Would it be 
better to cap the maximum length to KMALLOC_MAX_SIZE instead of 32MB, or 
process the buffers in smaller chunks?

> diff --git a/drivers/crypto/cmh/cmh_aes_cmac.c b/drivers/crypto/cmh/cmh_aes_cmac.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_aes_cmac.c
[ ... ]
> +static int cmh_aes_cmac_init(struct ahash_request *req)
> +{
> +	struct cmh_aes_cmac_reqctx *rctx = ahash_request_ctx(req);
> +
> +	memset(rctx, 0, sizeof(*rctx));
> +	INIT_LIST_HEAD(&rctx->chunks);
> +	return 0;
> +}

[Severity: High]
If a user opens an AF_ALG socket, accumulates data by calling 
crypto_ahash_update(), and then calls crypto_ahash_init() to reset the hash 
state, won't this memset() clear the rctx->chunks list head and leak the 
dynamically allocated chunks? The orphaned chunks would remain on 
tctx->all_chunks indefinitely until the long-lived TFM is destroyed, which 
could allow unprivileged userspace to easily exhaust kernel memory.

[ ... ]
> +static int cmh_aes_cmac_import(struct ahash_request *req, const void *in)
> +{
> +	struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
> +	struct cmh_aes_cmac_tfm_ctx *tctx = crypto_ahash_ctx(tfm);
> +	struct cmh_aes_cmac_reqctx *rctx = ahash_request_ctx(req);
> +	const struct cmh_aes_cmac_export_state *state = in;
> +	struct cmh_aes_cmac_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]
Similarly, if a caller performs a state import over a request that has 
already accumulated data in its own chunk list, doesn't this memset() also 
strand any previously allocated chunks?

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