Re: [PATCH v7 4/5] 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:
- [Critical] crypto: cmh: stack overflow via missing tfm->fb in cmh_aes_cmac_init_tfm()
- [High] crypto: cmh: spurious -EINPROGRESS in cmh_aes_complete() for CTR chunk 2
- [Medium] crypto: cmh - missing CCM payload length check for L=2 in cmh_aes_aead_crypt()
--

commit c8ce92a98ef076c53f07cde35dc9d8f4e53da1f5
Author: Alex Ousherovitch <aousherovitch@rambus.com>

crypto: cmh - add AES skcipher/aead/cmac

This commit registers AES skcipher (ECB, CBC, CTR, CFB, XTS), AEAD (GCM,
CCM), and ahash (CMAC) algorithms using the CMH AES core, along with
software fallbacks for unsupported hardware corner cases and incremental
CMAC operations.

> diff --git a/drivers/crypto/cmh/cmh_aes.c b/drivers/crypto/cmh/cmh_aes.c
> new file mode 100644
> index 0000000000000..d4352caf771f6
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_aes.c
[ ... ]
> +static int cmh_aes_ctr_submit_chunk2(struct skcipher_request *req)
> +{
> +	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
> +	struct cmh_aes_tfm_ctx *tctx = crypto_skcipher_ctx(tfm);
> +	struct cmh_aes_reqctx *rctx = skcipher_request_ctx(req);
> +	struct vcq_cmd cmds[CMH_AES_MAX_PAYLOAD];
> +	u32 chunk1 = rctx->ctr_chunk1_len;
> +	u32 chunk2 = rctx->cryptlen - chunk1;
> +	u64 key_ref;
> +	u32 keylen;
> +	u32 idx = 0;
> +
> +	/* Clear split flag so next completion is final */
> +	rctx->ctr_chunk1_len = 0;
> +
> +	vcq_add_sys_write(&cmds[idx++], SYS_REF_TEMP,
> +			  (u64)rctx->key_dma, SYS_REF_NONE,
> +			  tctx->key.raw.len,
> +			  tctx->key.raw.sys_type);
> +	key_ref = SYS_REF_TEMP;
> +	keylen = tctx->key.raw.len;
> +
> +	vcq_add_aes_init(&cmds[idx++], rctx->core_id, key_ref,
> +			 (u64)rctx->iv2_dma, keylen, rctx->ivsize,
> +			 rctx->aes_mode, rctx->aes_op, 0);
> +	vcq_add_aes_final(&cmds[idx++], rctx->core_id,
> +			  (u64)(rctx->in_dma + chunk1),
> +			  (u64)(rctx->out_dma + chunk1), chunk2);
> +	vcq_add_flush(&cmds[idx++], rctx->core_id);
> +
> +	return cmh_vcq_pack_and_submit_async(cmds, idx, rctx->packed,
> +					     CMH_AES_MAX_PACKED,
> +					     rctx->target_mbx,
> +					     cmh_aes_complete, req,
> +					     !!(req->base.flags &
> +						CRYPTO_TFM_REQ_MAY_BACKLOG),
> +					     cmh_tm_async_timeout_jiffies());
> +}
> +
> +/*
> + * Async completion callback -- fires from RH threaded IRQ context.
> + *
> + * Unmaps DMA buffers, copies output to req->dst scatterlist,
> + * updates the IV state, frees temporaries, and completes the request.
> + *
> + * For CTR counter-wrap splits, the first chunk completion chains
> + * into a second VCQ submission rather than finalizing immediately.
> + */
> +static void cmh_aes_complete(void *data, int error)
> +{
> +	struct skcipher_request *req = data;
> +	struct cmh_aes_reqctx *rctx = skcipher_request_ctx(req);
> +
> +	if (error == -EINPROGRESS) {
> +		cmh_complete(&req->base, error);
> +		return;
> +	}
> +
> +	/*
> +	 * CTR counter-wrap: first chunk completed, submit second.
> +	 * DMA mappings remain valid (they cover the full buffer).
> +	 *
> +	 * Recursion depth bounded: chunk2 clears ctr_chunk1_len before
> +	 * submission, so the second cmh_aes_complete invocation sees 0
> +	 * and finalizes (max depth = 2).
> +	 */
> +	if (rctx->ctr_chunk1_len && !error) {
> +		int ret;
> +
> +		ret = cmh_aes_ctr_submit_chunk2(req);
> +
> +		if (!ret || ret == -EBUSY)
> +			return;
> +		/* Submission failed; clean up below */
> +		error = ret;
> +	}

[Severity: High]
Can this deliver a spurious or duplicate -EINPROGRESS notification to the
outer skcipher_request caller when CTR chunk 2 is backlogged?

When an AES-CTR request with CRYPTO_TFM_REQ_MAY_BACKLOG crosses the 64-bit
counter wrap boundary in cmh_aes_crypt(), chunk 1 is submitted first. At
that point, either cmh_aes_crypt() returned -EINPROGRESS directly, or chunk
1 was backlogged (-EBUSY) and already triggered the single allowed
-EINPROGRESS callback when cmh_tm_thread() promoted it.

When chunk 1 finishes in hardware, cmh_aes_complete() calls
cmh_aes_ctr_submit_chunk2(), which submits chunk 2 with backlog_ok enabled
and cmh_aes_complete as the callback. If tm.cmq is full when chunk 2 is
submitted, cmh_tm_post_command() places chunk 2 on tm.backlog and returns
-EBUSY.

When cmh_tm_thread() later promotes chunk 2 from tm.backlog to tm.cmq:

drivers/crypto/cmh/cmh_txn.c:cmh_tm_thread() {
    ...
		if (bl_promoted && bl_promoted->complete)
			bl_promoted->complete(bl_promoted->completion_data,
					      -EINPROGRESS);
    ...
}

cmh_aes_complete() receives -EINPROGRESS for chunk 2 and forwards it to
cmh_complete(&req->base, -EINPROGRESS), invoking req->base.complete() with
-EINPROGRESS even though the initial submission returned -EINPROGRESS (or
already fired -EINPROGRESS once for chunk 1).

> diff --git a/drivers/crypto/cmh/cmh_aes_aead.c b/drivers/crypto/cmh/cmh_aes_aead.c
> new file mode 100644
> index 0000000000000..ed2a0b9449bc0
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_aes_aead.c
[ ... ]
> +static int cmh_aes_aead_crypt(struct aead_request *req, u32 aes_op)
> +{
[ ... ]
> +	if (aes_op == AES_OP_ENCRYPT) {
> +		cryptlen = req->cryptlen;
> +	} else {
> +		if (req->cryptlen < authsize)
> +			return -EINVAL;
> +		cryptlen = req->cryptlen - authsize;
> +	}
> +
> +	/*
> +	 * Validate CCM IV format early -- the empty-input fallback and
> +	 * nonce extraction both depend on iv[0] being in range [1,7].
> +	 */
> +	if (info->type == CMH_AES_AEAD_CCM) {
> +		if (req->iv[0] < 1 || req->iv[0] > 7)
> +			return -EINVAL;
> +	}
> +
> +	/*
> +	 * The CMH eSW rejects GCM/CCM when both aadlen and iolen are zero.
> +	 * For GCM, the tag is simply E(K, J0) -- handle with ECB fallback.
> +	 * For CCM, compute tag = E(K,B0) XOR E(K,A0) in software.
> +	 */
> +	if (cryptlen == 0 && req->assoclen == 0) {
> +		if (info->type == CMH_AES_AEAD_GCM)
> +			return cmh_aes_gcm_empty(req, aes_op);
> +		return cmh_aes_ccm_empty(req, aes_op);
> +	}
> +
> +	/*
> +	 * HW does not support authsize=10 for CCM.  Forward the entire
> +	 * request to the generic CCM implementation.
> +	 */
> +	if (info->type == CMH_AES_AEAD_CCM && authsize == 10)
> +		return cmh_aes_ccm_fallback(req, aes_op);
> +
> +	/*
> +	 * HW uses a proprietary LLI scatter-gather format that is
> +	 * incompatible with struct scatterlist, so the payload is
> +	 * linearised into contiguous buffers for DMA.  Cap total
> +	 * size to prevent excessive memory consumption.
> +	 */
> +	if ((u64)cryptlen + req->assoclen > SZ_1M)
> +		return -EINVAL;

[Severity: Medium]
Should cmh_aes_aead_crypt() check that cryptlen fits within L = req->iv[0] + 1
bytes for CCM, returning -EOVERFLOW when req->iv[0] == 1 and cryptlen > 0xffff?

Per RFC 3610 section 2.1, when req->iv[0] == 1 (L = 2, 13-byte nonce), the
maximum representable payload length is 65535 bytes (as checked in
crypto/ccm.c:set_msg_len() and lib/crypto/aes.c:aes_ccm_init()). Because
cmh_aes_aead_crypt() allows cryptlen up to SZ_1M without checking whether it
exceeds 0xffff for L = 2, an oversized L = 2 request is sent to the hardware
instead of failing with -EOVERFLOW.

If the eSW rejects the command, cmh_aes_aead_complete() returns -EIO on
encrypt and maps -EIO to -EBADMSG on decrypt instead of -EOVERFLOW; on 64K
page systems with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS enabled,
test_aead_vs_generic_impl() generates vectors above 64 KiB and expects
-EOVERFLOW to match the generic CCM implementation.

> diff --git a/drivers/crypto/cmh/cmh_aes_cmac.c b/drivers/crypto/cmh/cmh_aes_cmac.c
> new file mode 100644
> index 0000000000000..586150505d017
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_aes_cmac.c
[ ... ]
> +/*
> + * .digest() -- compute the CMAC over the whole message in a single
> + * hardware transaction:
> + *   SYS_CMD_WRITE(key) + AES_CMD_INIT(CMAC) + AES_CMD_AAD_FINAL_AUTH +
> + *   FLUSH.
> + * The message is linearised into one DMA-mapped buffer.  The crypto core
> + * routes virtual-address requests to the software fallback before this
> + * op runs, so req->src is an ordinary scatterlist here.
> + */
> +static int cmh_aes_cmac_digest(struct ahash_request *req)
> +{
[ ... ]
> +	if (data_len) {
> +		rctx->buf = kmalloc(data_len, gfp | __GFP_NOWARN);
> +		if (!rctx->buf)
> +			return -ENOMEM;
> +		if (req->base.flags & CRYPTO_AHASH_REQ_VIRT)
> +			memcpy(rctx->buf, req->svirt, data_len);
> +		else
> +			scatterwalk_map_and_copy(rctx->buf, req->src, 0,
> +						 data_len, 0);
> +		rctx->data_len = data_len;
> +	}
[ ... ]
> +static int cmh_aes_cmac_init_tfm(struct crypto_ahash *tfm)
> +{
> +	struct cmh_aes_cmac_tfm_ctx *tctx = crypto_ahash_ctx(tfm);
> +	struct crypto_ahash *fb;
> +
> +	tctx->key.mode = CMH_KEY_NONE;
> +
> +	/*
> +	 * The generic cmac(aes) shash has no export_core, so the crypto
> +	 * core cannot wrap it as crypto_ahash_fb().  Allocate it here
> +	 * instead; masking out CRYPTO_ALG_ASYNC excludes this (async)
> +	 * driver so the allocator picks the generic, synchronous transform,
> +	 * and its request is embedded after the reqctx.
> +	 */
> +	fb = crypto_alloc_ahash(crypto_ahash_alg_name(tfm), 0,
> +				CRYPTO_ALG_ASYNC);
> +	if (IS_ERR(fb))
> +		return PTR_ERR(fb);
> +	tctx->fb = fb;
> +
> +	crypto_ahash_set_reqsize(tfm,
> +				 sizeof(struct cmh_aes_cmac_reqctx) +
> +				 crypto_tfm_ctx_alignment() +
> +				 sizeof(struct ahash_request) +
> +				 crypto_ahash_reqsize(fb));
> +	crypto_ahash_set_statesize(tfm, crypto_ahash_statesize(fb));
> +	return 0;
> +}

[Severity: Critical]
Does leaving tfm->fb pointing to the asynchronous CMH transform in
cmh_aes_cmac_init_tfm() overflow the stack buffer in crypto_hash_digest()?

In crypto_create_tfm_node(), tfm->fb is initialized to point to tfm itself.
Because cmh_aes_cmac_alg sets CRYPTO_ALG_NO_FALLBACK, crypto_ahash_init_tfm()
does not allocate a fallback transform or update tfm->fb, and
cmh_aes_cmac_init_tfm() stores the fallback only in tctx->fb while setting
tfm's reqsize to:

  sizeof(struct cmh_aes_cmac_reqctx) + crypto_tfm_ctx_alignment() +
  sizeof(struct ahash_request) + crypto_ahash_reqsize(fb)

Since crypto_ahash_init_tfm() raises crypto_ahash_reqsize(fb) to at least
HASH_MAX_DESCSIZE, and MAX_SYNC_HASH_REQSIZE is sizeof(struct ahash_request) +
HASH_MAX_DESCSIZE, crypto_ahash_reqsize(tfm) exceeds MAX_SYNC_HASH_REQSIZE by
sizeof(struct cmh_aes_cmac_reqctx) + crypto_tfm_ctx_alignment() (over 560
bytes).

When a caller invokes crypto_hash_digest(tfm, data, len, out) (or uses
HASH_FBREQ_ON_STACK() or HASH_REQUEST_CLONE() when falling back on allocation
failure):

crypto/ahash.c:crypto_hash_digest() {
	HASH_REQUEST_ON_STACK(req, crypto_ahash_fb(tfm));
	int err;

	ahash_request_set_callback(req, 0, NULL, NULL);
	ahash_request_set_virt(req, data, out, len);
	err = crypto_ahash_digest(req);

	ahash_request_zero(req);

	return err;
}

HASH_REQUEST_ON_STACK() allocates __req_req on the stack with size
sizeof(struct ahash_request) + MAX_SYNC_HASH_REQSIZE and binds req to
crypto_ahash_fb(tfm), which is still the asynchronous CMH tfm.
crypto_ahash_digest(req) then fails with -EAGAIN (because
ahash_req_on_stack(req) && ahash_is_async(tfm) is true), and
ahash_request_zero(req) zeroes sizeof(*req) + crypto_ahash_reqsize(tfm) bytes,
overwriting more than 560 bytes past the end of __req_req on the kernel stack.

Also, the commit message states that virtual-address (CRYPTO_AHASH_REQ_VIRT)
paths are forwarded to the generic cmac(aes) transform, and the comment above
cmh_aes_cmac_digest() notes that the crypto core routes virtual-address
requests to the software fallback before .digest() runs. However, because
cmh_aes_cmac_alg sets CRYPTO_ALG_REQ_VIRT, ahash_do_req_chain() calls
cmh_aes_cmac_digest() directly for CRYPTO_AHASH_REQ_VIRT requests, which copies
req->svirt and submits the request asynchronously to hardware instead of
forwarding to tctx->fb.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-cmh-b4-a-v7-v7-0-500a83faf500@rambus.com?part=4