Re: [PATCH v8 4/5] crypto: cmh - add AES skcipher/aead/cmac

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Critical] crypto: cmh: stack overflow in ahash_request_zero() via
  missing tfm->fb
- [Medium] crypto: cmh: missing CRYPTO_ALG_ASYNC mask in
  cmh_aes_aead_init_tfm()
--

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

crypto: cmh - add AES skcipher/aead/cmac

This commit registers AES skcipher (ECB, CBC, CTR, XTS), AEAD (GCM, CCM),
and ahash (CMAC) algorithms on the CMH AES core, along with the AES core
ABI definitions.

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

[ ... ]

> +static int cmh_aes_aead_init_tfm(struct crypto_aead *tfm)
> +{
> +	struct cmh_aes_aead_tfm_ctx *tctx = crypto_aead_ctx(tfm);
> +	const struct cmh_aes_aead_info *info = cmh_aes_aead_get_info(tfm);
> +
> +	memset(tctx, 0, sizeof(*tctx));
> +	tctx->authsize = info->maxauthsize;
> +
> +	if (info->type == CMH_AES_AEAD_CCM) {
> +		struct crypto_aead *fb;
> +
> +		tctx->ccm = true;
> +		fb = crypto_alloc_aead("ccm(aes)", 0,
> +				       CRYPTO_ALG_NEED_FALLBACK);

[Severity: Medium]
Does this fallback allocation need CRYPTO_ALG_ASYNC in the mask so that
ccm(aes) does not wrap rambus-cmh-ctr-aes?

On architectures where ccm-aes-lib is not registered (such as arm64 and
arm), allocating ccm(aes) instantiates the ccm template via
crypto_ccm_create_common(), which grabs ctr(aes) using the passed mask.
Because cmh_aes_register() has already registered rambus-cmh-ctr-aes at
priority 300, the fallback ends up running CTR on the CMH hardware via
bounce buffers and rejecting requests over 32 MiB with -EINVAL, rather
than using a software implementation with no size limit as documented.

> diff --git a/drivers/crypto/cmh/cmh_aes_cmac.c b/drivers/crypto/cmh/cmh_aes_cmac.c
> new file mode 100644
> index 0000000000000..34a7a5ba07baf
> --- /dev/null
> +++ b/drivers/crypto/cmh/cmh_aes_cmac.c

[ ... ]

> +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]
Can leaving crypto_ahash_tfm(tfm)->fb pointing to the async CMH transform
overflow the kernel stack when a caller uses crypto_hash_digest() or
HASH_FBREQ_ON_STACK()?

Because cmh_aes_cmac_alg sets CRYPTO_ALG_NO_FALLBACK,
crypto_create_tfm_node() leaves tfm->fb pointing to tfm itself, while
cmh_aes_cmac_init_tfm() sets tfm's reqsize to over 800 bytes above
MAX_SYNC_HASH_REQSIZE:

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() reserves only sizeof(struct ahash_request) +
MAX_SYNC_HASH_REQSIZE bytes on the stack and binds req to
crypto_ahash_fb(tfm) (still the CMH tfm).  crypto_ahash_digest() then
returns -EAGAIN because an on-stack request is used with an async
transform, and ahash_request_zero() zeroes sizeof(*req) +
crypto_ahash_reqsize(tfm) bytes, writing over 800 bytes past the stack
buffer.

Pointing crypto_ahash_tfm(tfm)->fb at crypto_ahash_tfm(fb) (and restoring
it before freeing fb in cmh_aes_cmac_exit_tfm()) would direct stack
requests to the synchronous fallback transform.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-cmh-b4-a-v8-v8-0-4cea8ebc1d40@rambus.com?part=4