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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Unbounded memory accumulation of dynamically allocated chunks in the CMAC ahash driver allows a local attacker to exhaust kernel memory (OOM).
- [High] Missing `chunksize` specification for CTR and CFB stream modes breaks keystream continuity on partial block updates.
- [Medium] CFB-128 mode incorrectly computes the IV offset for non-block-aligned requests, resulting in corrupted IVs.
- [Medium] The skcipher implementation attempts massive physically contiguous allocations via `kmalloc()`, causing deterministic failures for large requests.
- [Medium] Missing AAD (Associated Data) copy for out-of-place AEAD operations.
- [Medium] Large user-controlled kmalloc allocations without `__GFP_NOWARN` in the AEAD encrypt/decrypt paths can trigger page allocation failure splats.
--

--- Patch [8]: [PATCH v3 08/19] crypto: cmh - add AES skcipher/aead/cmac ---
commit 42f9420f62ca78ab729b2164543a52275ed3c503
Author: Alex Ousherovitch <aousherovitch@rambus.com>

crypto: cmh - add AES skcipher/aead/cmac

Registers AES algorithms using the CMH AES core (core ID 0x03).
Supports skcipher, aead, and ahash with AES. Handles key sizes of
128, 192, and 256 bits.

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

[ ... ]

> +	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: Medium]
Is the IV for CFB-128 correctly calculated here when cryptlen is not a
multiple of the block size?

When req->cryptlen is not block-aligned, copying from
out_buf + req->cryptlen - bs results in an unaligned window that spans the
trailing partial block and the preceding block.

Could this produce a corrupted IV for subsequent chaining?

[ ... ]

> +	/*
> +	 * 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);

[Severity: Medium]
Will this allocation always fail for large valid requests?

Since req->cryptlen can be up to CMH_AES_MAX_CRYPTLEN (32MB), which
significantly exceeds the maximum page allocator order (KMALLOC_MAX_SIZE,
usually 4MB), a contiguous kmalloc will deterministically fail for sizes above
that limit.

[ ... ]

> +		alg->base.cra_priority  = 300;
> +		alg->base.cra_flags     = CRYPTO_ALG_KERN_DRIVER_ONLY |
> +					  CRYPTO_ALG_ASYNC;
> +		alg->base.cra_blocksize = aes_is_stream_mode(info->aes_mode)
> +					  ? 1 : CMH_AES_BLOCK_SIZE;
> +		alg->base.cra_ctxsize  = sizeof(struct cmh_aes_tfm_ctx);

[Severity: High]
Is there a missing chunksize specification for CTR and CFB stream modes?

The cra_blocksize is set to 1, but chunksize is omitted, defaulting to 1.
For hardware engines that process in 16-byte blocks and do not save unconsumed
keystream between requests, won't submitting partial block updates cause the
hardware to discard remaining keystream bytes and break continuity on the
next update? Setting chunksize to the hardware block size would force
framework buffering.

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

[ ... ]

> +	if (!error) {
> +		/* GCM empty-input decrypt: compare computed tag with expected */
> +		if (rctx->empty_gcm_fallback && !rctx->encrypting) {
> +			if (crypto_memneq(rctx->tag_buf, rctx->in_buf,
> +					  rctx->authsize))
> +				error = -EBADMSG;
> +		}
> +		if (!error && rctx->cryptlen > 0)
> +			scatterwalk_map_and_copy(rctx->out_buf, req->dst,
> +						 req->assoclen,
> +						rctx->cryptlen, 1);
> +		if (!error && rctx->encrypting)
> +			scatterwalk_map_and_copy(rctx->tag_buf, req->dst,
> +						 req->assoclen +
> +						rctx->cryptlen,
> +						rctx->authsize, 1);
> +	}

[Severity: Medium]
Does this completion handler copy the associated data (AAD) to the destination
buffer?

For out-of-place AEAD operations where req->src != req->dst, the crypto API
requires the driver to copy the AAD from source to destination. The payload
and tag are copied starting at req->assoclen, but the AAD region from 0 to
req->assoclen - 1 appears to be left uninitialized.

[ ... ]

> +	memset(rctx, 0, sizeof(*rctx));
> +	rctx->cryptlen = cryptlen;
> +	rctx->assoclen = req->assoclen;
> +	rctx->authsize = authsize;
> +	rctx->encrypting = (aes_op == AES_OP_ENCRYPT);
> +
> +	/* Linearise AAD */
> +	if (req->assoclen > 0) {
> +		rctx->aad_buf = kmalloc(req->assoclen, gfp);
> +		if (!rctx->aad_buf)
> +			return -ENOMEM;

[Severity: Medium]
Can these kmalloc calls trigger page allocation failure splats in the kernel
log?

The sizes are derived from user-controlled req->cryptlen and req->assoclen
(up to 1MB). When executed under memory pressure or with GFP_ATOMIC, large
contiguous allocations can fail loudly without the __GFP_NOWARN flag.

> diff --git a/drivers/crypto/cmh/cmh_aes_cmac.c b/drivers/crypto/cmh/cmh_aes_cmac.c
> new file mode 100644
> index 0000000000000..4cb3832780575
> --- /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]
Can this result in orphaned chunks being leaked into the TFM list?

Any previously accumulated chunks linked from rctx->chunks will lose their
list head when rctx is zeroed, while remaining tracked in the long-lived
tctx->all_chunks list.

[ ... ]

> +	gfp = req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP ?
> +	      GFP_KERNEL : GFP_ATOMIC;
> +
> +	chunk = kmalloc(sizeof(*chunk) + req->nbytes, gfp);
> +	if (!chunk) {
> +		ret = -ENOMEM;
> +		goto err_free_chunks;
> +	}
> +
> +	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]
Does this permit a local attacker to exhaust kernel memory?

By repeatedly triggering updates and closing the AF_ALG socket before
finalization, chunks can accumulate indefinitely in the parent TFM's
all_chunks list. Since the ahash API has no per-request destructor hook, the
chunks are not freed when the request is dropped.

[ ... ]

> +static void cmh_aes_cmac_exit_tfm(struct crypto_ahash *tfm)
> +{
> +	struct cmh_aes_cmac_tfm_ctx *tctx = crypto_ahash_ctx(tfm);
> +	struct cmh_aes_cmac_chunk *c, *tmp;
> +
> +	/* Free any orphaned chunks (e.g. testmgr export/reimport poison) */
> +	spin_lock_bh(&tctx->chunk_lock);
> +	list_for_each_entry_safe(c, tmp, &tctx->all_chunks, tfm_node) {
> +		list_del(&c->tfm_node);
> +		kfree_sensitive(c);
> +	}

[Severity: High]
Since the chunks are only physically freed when the TFM is destroyed, couldn't
a long-lived socket retain the leaked memory indefinitely?

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