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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Unbounded memory leak and DoS via orphaned chunks in `cmh_aes_cmac_init` and `cmh_aes_cmac_import`.
- [High] Memory allocation failure and `WARN` due to oversized `kmalloc` in `cmh_aes_crypt`.
- [High] Cryptographic logic bug causing mathematically incorrect IV generation in CFB-128 partial block chaining.
- [High] Alignment violation resulting in misaligned pointers for generic CCM fallback subrequest.
- [High] Sleeping in atomic context due to hardcoded `GFP_KERNEL` in `cmh_aes_cmac_import`.
--

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

crypto: cmh - add AES skcipher/aead/cmac

Register AES algorithms using the CMH AES core (core ID 0x03): skcipher, aead,
and ahash. Supports 128, 192, and 256-bit keys and handles associated data,
payload, and authentication tag.

> 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)
> +{
[ ... ]
> +	case AES_MODE_CFB:
[ ... ]
> +		if (req->cryptlen >= bs) {
> +			if (op == AES_OP_ENCRYPT)
> +				memcpy(req->iv, out_buf + req->cryptlen - bs,
> +				       bs);

[Severity: High]
Does this extract an unaligned 16-byte slice when the request length is not
block-aligned? Mixing the end of the previous ciphertext block and the start
of the partial block produces an incorrect shift-register state for CFB-128.
If cra_blocksize is set to 1, standard CFB-128 chaining on partial blocks
requires saving the unused keystream in the state between requests instead.

[ ... ]
> +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 */
> +	rctx->in_buf = kmalloc(req->cryptlen, gfp);

[Severity: High]
Does this allow kmalloc to be called with a size up to 32MB? Since
KMALLOC_MAX_SIZE is typically 4MB or 8MB, allocations exceeding this limit
will trigger page allocator warnings and fail. Unprivileged userspace could
potentially trigger this via the AF_ALG skcipher interface.

> diff --git a/drivers/crypto/cmh/cmh_aes_aead.c b/drivers/crypto/cmh/cmh_aes_aead.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_aes_aead.c
[ ... ]
> +static int cmh_aes_aead_init_tfm(struct crypto_aead *tfm)
> +{
[ ... ]
> +		/*
> +		 * Subreq lives at (rctx + 1).  Alignment is guaranteed
> +		 * by the crypto framework's __ctx ALIGN mechanism.
> +		 */
> +		crypto_aead_set_reqsize(tfm,
> +					sizeof(struct cmh_aes_aead_reqctx) +
> +					sizeof(struct aead_request) +
> +					crypto_aead_reqsize(fb));
[ ... ]
> +static int cmh_aes_ccm_fallback(struct aead_request *req, u32 aes_op)
> +{
> +	struct crypto_aead *tfm = crypto_aead_reqtfm(req);
> +	struct cmh_aes_aead_tfm_ctx *tctx = crypto_aead_ctx(tfm);
> +	struct cmh_aes_aead_reqctx *rctx = aead_request_ctx(req);
> +	struct aead_request *subreq = (void *)(rctx + 1);

[Severity: High]
Does this calculate a misaligned pointer for subreq? The crypto framework
aligns the start of the primary request context, but
sizeof(struct cmh_aes_aead_reqctx) is not guaranteed to be a multiple of
CRYPTO_MINALIGN.

Because struct aead_request requires strict alignment for its __ctx member,
this can lead to alignment faults or data corruption when CCM mode falls
back to software. Should the context size wrap the struct size with
ALIGN(..., crypto_tfm_ctx_alignment())?

> 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));
[ ... ]
> +static int cmh_aes_cmac_update(struct ahash_request *req)
> +{
[ ... ]
> +	chunk = kmalloc(sizeof(*chunk) + req->nbytes, gfp);

[Severity: High]
Does zeroing the request context in cmh_aes_cmac_init() orphan the chunks
dynamically allocated during cmh_aes_cmac_update()? If a user repeatedly
calls .update() followed by .init() on the same request via the AF_ALG
socket, it seems total_len is reset and the link to the allocated chunks is
severed. Can this lead to unbounded memory leaks and an out-of-memory
condition?

[ ... ]
> +static int cmh_aes_cmac_import(struct ahash_request *req, const void *in)
> +{
[ ... ]
> +	if (state->total_len) {
> +		chunk = kmalloc(sizeof(*chunk) + state->total_len, GFP_KERNEL);

[Severity: High]
Can this allocation happen in atomic or softirq contexts? The kernel crypto
API allows .import() callbacks to execute during IPsec packet processing.
Using a hardcoded GFP_KERNEL here without checking
req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP could trigger a sleeping in
atomic context bug.

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