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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Invalid SPAcc hardware error codes silently inherit the error code of the previous job.
- [High] The driver destroys `priv->hash_pool` before flushing `priv->spacc_wq`, causing a use-after-free and NULL pointer dereference.
- [High] Device unbind does not abort active hardware DMA operations before freeing memory pools.
- [High] The bounds check for `max_msg_size` evaluates a stale `job->post_aad_sz` before it is initialized for the current request.
- [Low] Mixing `goto`-based error handling with `guard()` cleanup declarations in the same function.
--

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

crypto: spacc - Add SPAcc ahash support

This commit adds ahash support to the SPAcc driver. It introduces
implementations for various hash algorithms including cmac, xcbc, hmac, md5,
sha1, sha2, and sha3 families.

> diff --git a/drivers/crypto/dwc-spacc/spacc_ahash.c b/drivers/crypto/dwc-spacc/spacc_ahash.c
> index 0000000000000..4dfe1fd979ede 100644
> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/spacc_ahash.c
> @@ -201,16 +201,16 @@ static int spacc_hash_setkey(struct crypto_ahash *tfm, const u8 *key,
>  	}
>  
>  	{
>  		guard(mutex)(&tctx->lock);
>  
>  		/*
>  		 * Key the software fallback that this mode actually uses:
>  		 *  - CMAC/XCBC use tctx->shash_fb (allocated in init_tfm).
>  		 *  - Plain HMAC uses the ahash fallback tfm->fb (provisioned
>  		 *    by the crypto core via NEED_FALLBACK) through
>  		 *    HASH_FBREQ_ON_STACK. It must be keyed here too, or a
>  		 *    request routed to the fallback (e.g. a message larger
>  		 *    than the SPAcc max_msg_size) would run unkeyed and
>  		 *    produce -ENOKEY or an incorrect MAC.
>  		 */
>  		if (tctx->shash_fb) {
>  			ret = crypto_shash_setkey(tctx->shash_fb, key, keylen);
>  			if (ret)
>  				goto out;

[Severity: Low]
Can mixing goto-based error handling with guard cleanup declarations in the
same function create confusing ownership semantics? The subsystem patterns
indicate functions should convert all resources to scope-based cleanup or
convert none of them.

[ ... ]

> @@ -264,16 +264,16 @@ 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) {
>  			/*
>  			 * spacc_open() acquires ctx_sem which may sleep;
>  			 * do_one_request runs in the crypto_engine workqueue
>  			 * which is a sleepable context, so this is safe.
>  			 */
>  			tctx->handle = spacc_open(&priv->spacc,
>  					CRYPTO_MODE_NULL, salg->mode->id,
>  					-1, 0, spacc_digest_cb, reqtfm);
>  			if (tctx->handle < 0) {
>  				dev_dbg(salg->dev,
>  					"context open failed, fallback\n");
>  				goto fallback;

[Severity: Low]
Does jumping to a fallback label outside the guard scope bypass the intended
cleanup mechanism? Similar to the previous instance, mixing goto cleanups and
guard blocks in the same function breaks expected cleanup ordering.

> diff --git a/drivers/crypto/dwc-spacc/spacc_core.c b/drivers/crypto/dwc-spacc/spacc_core.c
> index 0000000000000..0c7d58ef9bd9c 100644
> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/spacc_core.c
> @@ -1296,16 +1296,16 @@ int spacc_packet_enqueue_ddt_ex(struct spacc_device *spacc, int use_jb,
>  	/*
>  	 * Defensive backstop only: the hash path routes over-size requests
>  	 * to the software fallback before reaching here (see
>  	 * spacc_hash_do_one_request()), so this should not fire for hashes.
>  	 * It remains to stop any caller from silently truncating a length
>  	 * that does not fit PROC_LEN (bit 31 also aliases SPACC_AADCOPY_FLAG
>  	 * in pre_aad_sz, so an oversized pre_aad_sz must be rejected too).
>  	 */
>  	if ((u32)(proc_len - job->post_aad_sz) > spacc->config.max_msg_size)
>  		return -EMSGSIZE;

[Severity: High]
Does this bounds check evaluate a stale job->post_aad_sz before it is
initialized for the current request? Since spacc->job slots are reused,
could an oversized payload bypass the check if the stale value happens
to be small?

>  
>  	if ((pre_aad_sz & ~SPACC_AADCOPY_FLAG) > spacc->config.max_msg_size)
>  		return -EMSGSIZE;
>  
>  	if (pre_aad_sz & SPACC_AADCOPY_FLAG) {
>  		job->ctrl  |= SPACC_CTRL_MASK(SPACC_CTRL_AAD_COPY);
>  		pre_aad_sz &= ~(SPACC_AADCOPY_FLAG);
>  	} else
>  		job->ctrl  &= ~SPACC_CTRL_MASK(SPACC_CTRL_AAD_COPY);
>  
>  	job->pre_aad_sz  = pre_aad_sz;
>  	job->post_aad_sz = post_aad_sz;

> diff --git a/drivers/crypto/dwc-spacc/spacc_device.c b/drivers/crypto/dwc-spacc/spacc_device.c
> index 0000000000000..628b9c81283fa 100644
> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/spacc_device.c
> @@ -201,16 +201,16 @@ static void spacc_crypto_remove(struct platform_device *pdev)
>  {
>  	struct spacc_priv *priv = platform_get_drvdata(pdev);
>  
>  	spacc_unregister_algs(priv);
>  
>  	if (priv->engine)
>  		crypto_engine_exit(priv->engine);
>  
>  	spacc_irq_glbl_disable(&priv->spacc);
>  	devm_free_irq(&pdev->dev, priv->irq, &pdev->dev);
>  

[Severity: High]
Does this unbind path need to abort active hardware DMA operations before
freeing memory pools? Since crypto_engine_exit() does not block on active
hardware, could the hardware continue asynchronously performing DMA operations
into memory that the kernel has already freed?

>  	dma_pool_destroy(priv->hash_pool);
>  	priv->hash_pool = NULL;
>  
>  	if (priv->spacc_wq)
>  		destroy_workqueue(priv->spacc_wq);

[Severity: High]
Can destroying priv->hash_pool before flushing priv->spacc_wq cause a
use-after-free? If there are pending items in spacc_wq during teardown,
destroy_workqueue() will flush them, executing spacc_digest_cb() which
unconditionally calls dma_pool_free() on the destroyed pool:

spacc_digest_cb()
  ...
  dma_pool_free(priv->hash_pool, cb->ctx->digest_buf, cb->ctx->digest_dma);

> diff --git a/drivers/crypto/dwc-spacc/spacc_interrupt.c b/drivers/crypto/dwc-spacc/spacc_interrupt.c
> index 0000000000000..7916728c95374 100644
> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/spacc_interrupt.c
> @@ -52,16 +52,16 @@ static int spacc_pop_packets_ex(struct spacc_device *spacc, int *num_popped,
>  			case SPACC_ICVFAIL:
>  				ret = -EBADMSG;
>  				break;
>  			case SPACC_MEMERR:
>  				ret = -EINVAL;
>  				break;
>  			case SPACC_BLOCKERR:
>  				ret = -EINVAL;
>  				break;
>  			case SPACC_SECERR:
>  				ret = -EIO;
>  				break;
>  			case SPACC_OK:
>  				ret = 0;
>  				break;
>  			default:
>  				dev_err(spacc->dptr, "Invalid SPAcc Error\n");
>  			}
>  
>  			job->job_err = ret;

[Severity: High]
Can an invalid hardware error code silently inherit the error code of the
previous job here? If a hardware error is not recognized in the switch block,
ret is not updated in the default case. If the previous loop iteration
succeeded, would the currently failed operation be mistakenly treated as a
success?

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