Re: [PATCH v19 2/4] crypto: spacc - Add SPAcc ahash support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] The driver fails to set the cryptographic key on the software fallback TFM for HMAC algorithms, causing fallback operations to compute MACs using a zeroed (unkeyed) state or fail entirely.
- [High] Watchdog timer configuration is wiped out by `memset` during initialization, programming the hardware watchdog to 0.
- [High] Secret key material is leaked on the stack in the error path of `spacc_hash_setkey`.
- [High] Expanded XCBC key material is not zeroed before freeing in the error path.
- [High] The error cleanup path accesses an uninitialized mutex and list_head if `dma_pool_create()` fails during hash algorithm probe.
- [High] The driver unconditionally rejects requests with fragmented scatterlists when the hardware is configured for linear DMA (`SPACC_DMA_LINEAR`).
- [Medium] Improper mixing of `goto`-based error cleanup and scope-based `guard()` declaration in `spacc_hash_do_one_request`.
--

--- Patch [2]: [PATCH v19 2/4] crypto: spacc - Add SPAcc ahash support ---
commit 58138bfcd81212e5b689e0ba4bfea94bf1880728
Author: Pavitrakumar Managutte <pavitrakumarm@vayavyalabs.com>

crypto: spacc - Add SPAcc ahash support

Add ahash support to SPAcc driver.
Below are the hash algos supported:
- cmac(aes), xcbc(aes), cmac(sm4), xcbc(sm4), hmac(md5), md5,
hmac(sha1), sha1, sha224, sha256, sha384, sha512, hmac(sha224),
hmac(sha256), hmac(sha384), hmac(sha512), sha3-224, sha3-256,
sha3-384, sha3-512.

> diff --git a/drivers/crypto/dwc-spacc/spacc_ahash.c b/drivers/crypto/dwc-spacc/spacc_ahash.c
> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/spacc_ahash.c

[ ... ]

> @@ -288,14 +288,10 @@ static int spacc_hash_setkey(struct crypto_ahash *tfm, const u8 *key,
>  		memcpy(keybuf, key, keylen);
>  		klen = keylen;
>  	}
>  
>  	if (!spacc_is_mode_keysize_supported(&priv->spacc, salg->mode->id,
>  				klen, 1)) {
>  		dev_dbg(salg->dev, "keylen %u not supported for mode %d\n",
>  				klen, salg->mode->id);
>  		return -EINVAL;

[Severity: High]
Could this early return leave secret key material exposed on the stack?
The key is copied into the stack-allocated keybuf array earlier in this
function. If the keysize is unsupported, the function returns -EINVAL and
bypasses the memzero_explicit cleanup at the end of the function.

>  	}
>  
>  	{
>  		guard(mutex)(&tctx->lock);
>  
>  		/*
>  		 * For CMAC/XCBC the shash fallback also needs the key so that
>  		 * the software path produces correct MACs when the HW
>  		 * context is not yet available.
>  		 */
>  		if (tctx->shash_fb) {
>  			int rc = crypto_shash_setkey(tctx->shash_fb, key,
>  						     keylen);

[Severity: High]
Is the software fallback TFM for HMAC missing its key setup here?
The key is forwarded to tctx->shash_fb (used for CMAC/XCBC), but the ahash
fallback (tfm->base.fb) used by HMAC does not receive it. Would this cause
fallback operations to fail with -ENOKEY or compute MACs with an unkeyed
state for large messages?

[ ... ]

> @@ -358,13 +358,11 @@ static int spacc_hash_do_one_request(struct crypto_engine *engine, void *areq)
>  	tctx->tmp_sgl = kmalloc_array(2, sizeof(*tctx->tmp_sgl), GFP_KERNEL);
>  
>  	if (!tctx->tmp_sgl)
>  		goto fallback;
>  
>  	sg_init_table(tctx->tmp_sgl, 2);
>  	tctx->tmp_sgl[0].length = 0;
>  
>  	{
>  		guard(mutex)(&tctx->lock);
>  
>  		if (tctx->handle < 0 || !tctx->ctx_valid) {

[Severity: Medium]
Is it safe to mix the scope-based guard with goto-based error cleanup?
The function acquires tctx->lock using guard(), but later uses goto
fallback to clean up tctx->tmp_sgl. The kernel cleanup guidelines strongly
recommend against mixing __free/guard declarations with goto labels to
prevent unexpected ownership and lock state issues.

[ ... ]

> @@ -950,11 +950,10 @@ int spacc_probe_hashes(struct platform_device *spacc_pdev)
>  	priv->hash_pool = dma_pool_create("spacc-digest", &spacc_pdev->dev,
>  			SPACC_MAX_DIGEST_SIZE,
>  			SPACC_DMA_ALIGN, SPACC_DMA_BOUNDARY);
>  
>  	if (!priv->hash_pool)
>  		return -ENOMEM;
>  
>  	INIT_LIST_HEAD(&priv->hash_alg_list);
>  	mutex_init(&priv->hash_alg_mutex);

[Severity: High]
Can this early return cause a crash during the error cleanup path?
If dma_pool_create() fails, it returns before initializing hash_alg_mutex
and hash_alg_list. The probe function's error handler will eventually call
spacc_unregister_hash_algs(), which blindly attempts to lock the
uninitialized mutex and iterate the uninitialized list.

> diff --git a/drivers/crypto/dwc-spacc/spacc_core.c b/drivers/crypto/dwc-spacc/spacc_core.c
> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/spacc_core.c

[ ... ]

> @@ -881,11 +881,10 @@ int spacc_init(void __iomem *baseaddr, struct spacc_device *spacc,
>  #ifdef CONFIG_CRYPTO_DEV_SPACC_CONFIG_DEBUG
>  	unsigned long id;
>  	char version_string[3][16]  = { "SPACC", "SPACC-PDU" };
>  	char idx_string[2][16]      = { "(Normal Port)", "(Secure Port)" };
>  	char dma_type_string[4][16] = { "Unknown", "Scattergather", "Linear",
>  					"Unknown" };
>  #endif
>  
>  	memset(spacc, 0, sizeof(*spacc));

[Severity: High]
Does this unconditional memset clear the watchdog timer configuration?
In spacc_init_device(), the timer is initialized before calling
spacc_init(). Since this memset clears the entire spacc structure, it
appears the user-configured timer value is overwritten. Will this cause
spacc_set_wd_count() to program the hardware watchdog to 0 later?

[ ... ]

> @@ -1189,11 +1189,10 @@ int spacc_compute_xcbc_key(struct spacc_device *spacc, int mode_id,
>  		if (err != 0)
>  			goto xcbc_err;
>  	}
>  
>  	/* now we can copy the key */
>  	memcpy(xcbc_out, buf, 48);
>  	memzero_explicit(buf, 64);
>  
>  xcbc_err:
>  	dma_free_coherent(spacc->dptr, 64, buf, bufphys);

[Severity: High]
Could the expanded XCBC key material in buf be leaked to the DMA pool?
If an error occurs after the keys are derived, the buffer is freed via
dma_free_coherent() without a prior memzero_explicit() call. This might
expose sensitive key material to other devices reusing the DMA memory pool.

[ ... ]

> @@ -1336,10 +1336,9 @@ int spacc_packet_enqueue_ddt_ex(struct spacc_device *spacc, int use_jb,
>  	if (spacc->config.dma_type == SPACC_DMA_DDT) {
>  		pdu_io_cached_write(spacc->dptr, spacc->regmap +
>  				SPACC_REG_SRC_PTR, (uint32_t)src_ddt->phys,
>  				&spacc->cache.src_ptr);
>  		pdu_io_cached_write(spacc->dptr, spacc->regmap +
>  				SPACC_REG_DST_PTR, (uint32_t)dst_ddt->phys,
>  				&spacc->cache.dst_ptr);
>  	} else if (spacc->config.dma_type == SPACC_DMA_LINEAR) {
>  		if (src_ddt->idx != 1 || dst_ddt->idx != 1)
>  			return -EINVAL;

[Severity: High]
Will this cause unexpected failures for callers providing fragmented memory?
The Crypto API requires drivers to handle standard scatterlists, but this
returns -EINVAL for multi-segment lists when using linear DMA. Should this
use a bounce buffer or drop to a software fallback instead of failing
dynamically?

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