Re: [PATCH v27 4/4] crypto: spacc - Add SPAcc Kconfig and Makefile

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] crypto: spacc - CRYPTO_AHASH_ALG_BLOCK_ONLY inflates statesize past limit
- [High] crypto: spacc - use-after-free in spacc_hash_exit_tfm() and spacc_hash_setkey()
--

commit c650f028323b3118ec1e7adfa8aafa6c190ddd1d
Author: Pavitrakumar Managutte <pavitrakumarm@vayavyalabs.com>

crypto: spacc - Add SPAcc Kconfig and Makefile

This commit adds the Kconfig and Makefile for the Synopsys DesignWare SPAcc
driver and hooks them into drivers/crypto/.

[ ... ]

> diff --git a/drivers/crypto/dwc-spacc/Makefile b/drivers/crypto/dwc-spacc/Makefile
> new file mode 100644
> index 0000000000000..b7125a7797ba0
> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/Makefile
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_CRYPTO_DEV_SPACC) += snps-spacc.o
> +
> +snps-spacc-y := spacc_hal.o spacc_core.o \
> +spacc_manager.o spacc_interrupt.o spacc_device.o
> +
> +snps-spacc-$(CONFIG_CRYPTO_DEV_SPACC_HASH) += spacc_ahash.o

[Severity: High]
As reported by Sashiko on v26 [1], enabling spacc_ahash.o builds
spacc_hash_template and spacc_register_hash() in
drivers/crypto/dwc-spacc/spacc_ahash.c with both CRYPTO_AHASH_ALG_BLOCK_ONLY
and statesize set to HASH_MAX_STATESIZE:

drivers/crypto/dwc-spacc/spacc_ahash.c:spacc_hash_template:
		.halg.base = {
			...
			.cra_flags	= CRYPTO_ALG_TYPE_AHASH		|
					  CRYPTO_ALG_ASYNC		|
					  CRYPTO_AHASH_ALG_BLOCK_ONLY
		},

drivers/crypto/dwc-spacc/spacc_ahash.c:spacc_register_hash() {
	...
	salg->alg.hash.base.halg.digestsize = salg->mode->hashlen;
	salg->alg.hash.base.halg.statesize = HASH_MAX_STATESIZE;

	rc = crypto_engine_register_ahash(&salg->alg.hash);
}

When crypto_engine_register_ahash() -> crypto_register_ahash() ->
ahash_prepare_alg() sees CRYPTO_AHASH_ALG_BLOCK_ONLY, it adds
cra_blocksize + 1 to statesize:

crypto/ahash.c:ahash_prepare_alg() {
	...
	if (base->cra_flags & CRYPTO_AHASH_ALG_BLOCK_ONLY) {
		...
		alg->halg.statesize += base->cra_blocksize + 1;
	}
}

This inflates crypto_ahash_statesize(tfm) to HASH_MAX_STATESIZE +
cra_blocksize + 1 for every registered SPAcc hash algorithm.

As a result, any virtual-address request reaching ahash_do_req_chain() (such
as the generic hmac template wrapping an unkeyed SPAcc hash like sha3-256)
hits:

	if (crypto_ahash_statesize(tfm) > HASH_MAX_STATESIZE)
		return -ENOSYS;

and fails with -ENOSYS. In addition, callers allocating a buffer of size
HASH_MAX_STATESIZE and calling crypto_ahash_export() will overflow that
buffer by cra_blocksize + 1 bytes when crypto_ahash_export() appends the
block-only partial block buffer at out + ss - plen.

Furthermore, spacc_hash_init(), spacc_hash_update(), spacc_hash_final(), and
spacc_hash_finup() delegate directly to the software fallback fbreq rather
than performing block-only hardware hashing:

drivers/crypto/dwc-spacc/spacc_ahash.c:spacc_hash_update() {
	...
	rc = crypto_ahash_import(fbreq, ctx->state_buffer);
	if (!rc)
		rc = crypto_ahash_update(fbreq);
	if (!rc)
		rc = crypto_ahash_export(fbreq, ctx->state_buffer);

	HASH_REQUEST_ZERO(fbreq);

	return rc;
}

Because spacc_hash_update() consumes trailing bytes into ctx->state_buffer
via the fallback and returns 0 instead of returning the unprocessed tail
length expected by ahash_update_finish(), partial blocks are split between
the core block-only buffer and ctx->state_buffer.

Should CRYPTO_AHASH_ALG_BLOCK_ONLY be dropped from spacc_hash_template?

[Severity: High]
Can spacc_hash_exit_tfm() and spacc_hash_setkey() race with
spacc_crypto_remove() and trigger a use-after-free on priv->spacc?

In drivers/crypto/dwc-spacc/spacc_device.c, spacc_crypto_remove() notes that
sysfs unbind does not wait for existing transforms to be freed. At the end of
spacc_crypto_remove(), spacc_remove() frees spacc->ctx and spacc->job via
spacc_fini() before clearing drvdata:

drivers/crypto/dwc-spacc/spacc_device.c:spacc_crypto_remove() {
	...
	spacc_remove(pdev);
	platform_set_drvdata(pdev, NULL);
}

However, in drivers/crypto/dwc-spacc/spacc_ahash.c, neither
spacc_hash_exit_tfm() nor spacc_hash_setkey() -> spacc_hash_commit_key()
checks priv->removing or serializes against spacc_crypto_remove():

drivers/crypto/dwc-spacc/spacc_ahash.c:spacc_hash_exit_tfm() {
	struct spacc_crypto_ctx *tctx = crypto_ahash_ctx(tfm);
	struct spacc_priv *priv = dev_get_drvdata(tctx->dev);

	if (priv && tctx->handle >= 0)
		spacc_close(&priv->spacc, tctx->handle);
	...
}

drivers/crypto/dwc-spacc/spacc_ahash.c:spacc_hash_setkey() {
	...
	struct spacc_priv *priv = dev_get_drvdata(salg->dev);
	...
	if (!spacc_is_mode_keysize_supported(&priv->spacc, salg->mode->id,
				klen, 1)) {
	...
	ret = spacc_hash_commit_key(tfm, priv, key, keylen, keybuf, klen);
}

If an existing transform calls crypto_free_ahash() or crypto_ahash_setkey()
concurrently with sysfs unbind and reads priv before
platform_set_drvdata(pdev, NULL), won't spacc_close() or
spacc_is_mode_keysize_supported() access spacc->job, spacc->ctx, and
devm-managed priv after they have been freed?

[1] https://lore.kernel.org/r/20261019162457.1000115-5-pavitrakumarm@vayavyalabs.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030172456.274744-1-pavitrakumarm@vayavyalabs.com?part=4