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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Teardown race in `spacc_crypto_remove()` allows the crypto engine to dispatch requests to the hardware after interrupts are disabled and DMA pools are destroyed.
- [High] Deadlock and missed wakeup due to late `atomic_inc` of `dma_ref_cnt`.
- [Medium] Silent failure if `spacc_xof_stringsize_autodetect` fails.
- [Medium] The `aad_copy` flag is silently dropped for buffered jobs.
--

commit 20cca9a22d8fa57ea670b157f7fd94bce9ee94e2
Author: Pavitrakumar Managutte <pavitrakumarm@vayavyalabs.com>
crypto: spacc - Add SPAcc ahash support

This commit adds ahash support to the SPAcc driver. It introduces multiple
hashing algorithms, including AES-CMAC, SM4-CMAC, and various SHA/MD5 variants.
It also sets up DMA pools and adds the corresponding hardware interaction
routines.

> 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
> @@ -539,26 +539,24 @@ static int spacc_hash_do_one_request(struct crypto_engine *engine, void *areq)
[ ... ]
>  	rc = spacc_packet_enqueue_ddt(&priv->spacc, ctx->acb.new_handle,
>  			&ctx->src, &ctx->dst, req->nbytes,
>  			0, req->nbytes, 0, 0, 0, false);
>  	if (rc < 0) {
[ ... ]
>  		return 0;
>  	}
>  
>  	/*
>  	 * Job accepted by hardware. Balanced by the atomic_dec in
>  	 * spacc_digest_cb() when the completion callback has run and the
>  	 * job's DMA buffers are safe to reclaim.
>  	 */
>  	atomic_inc(&priv->dma_ref_cnt);

[Severity: High]
Could the atomic_inc() of dma_ref_cnt here race with the interrupt handler and
cause a missed wakeup? Since spacc_packet_enqueue_ddt() drops the spinlock
before returning, a fast hardware completion could trigger spacc_digest_cb()
and its atomic_dec_and_test() before the atomic_inc() runs here. This would
decrement the counter from 0 to -1 (missing the wake_up), and the subsequent
atomic_inc() would return it to 0 without waking up any threads waiting in
spacc_crypto_remove().

> 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
> @@ -775,12 +775,10 @@ int spacc_init(void __iomem *baseaddr, struct spacc_device *spacc,
[ ... ]
>  	/* autodetect and set string size setting */
>  	if (spacc->config.version == 0x61 || spacc->config.version >= 0x65)
>  		spacc_xof_stringsize_autodetect(spacc);
>  
>  	return 0;

[Severity: Medium]
What happens if spacc_xof_stringsize_autodetect() fails here? It appears the
error code is ignored, meaning that on an allocation failure,
spacc->config.string_size could remain 0, which would leave XOF hashing
operations misconfigured. Should this check the return code and handle the
error?

> @@ -1277,13 +1277,10 @@ int spacc_packet_enqueue_ddt_ex(struct spacc_device *spacc, int use_jb,
[ ... ]
>  	if (_spacc_fifo_full(spacc, prio)) {
>  		if (use_jb)
>  			goto fifo_full;
>  		else
>  			return -EBUSY;
>  	}

[Severity: Medium]
Will the aad_copy flag be lost for jobs that get buffered? In
spacc_packet_enqueue_ddt_ex(), if the hardware FIFO is full, it jumps to
fifo_full before setting the aad_copy flag in the control register. The
spacc_job_buffer struct doesn't have a field to store this flag.

> diff --git a/drivers/crypto/dwc-spacc/spacc_device.c b/drivers/crypto/dwc-spacc/spacc_device.c
> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/spacc_device.c
> @@ -242,15 +242,16 @@ static void spacc_crypto_remove(struct platform_device *pdev)
[ ... ]
>  	wait_event(priv->drain_wq, !atomic_read(&priv->dma_ref_cnt));
>  
>  	spin_lock_irqsave(&priv->spacc.lock, flags);
>  	spacc_irq_glbl_disable(&priv->spacc);
>  	spin_unlock_irqrestore(&priv->spacc.lock, flags);
>  
>  	devm_free_irq(&pdev->dev, priv->irq, &pdev->dev);
>  
>  	if (priv->spacc_wq)
>  		destroy_workqueue(priv->spacc_wq);
>  
>  	if (priv->engine)
>  		crypto_engine_exit(priv->engine);

[Severity: Critical]
Is it safe to exit the crypto engine after waiting for the inflight hardware
jobs to drain? In spacc_crypto_remove(), wait_event() will succeed when there
are no jobs actively running on the hardware (dma_ref_cnt == 0). However, the
engine may still have jobs queued in software. Because crypto_engine_exit() is
called after interrupts are disabled and DMA pools are destroyed, couldn't the
crypto engine dispatch those pending software requests to the hardware during
teardown, leading to memory corruption or a device hang?

> diff --git a/drivers/crypto/dwc-spacc/spacc_manager.c b/drivers/crypto/dwc-spacc/spacc_manager.c
> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/spacc_manager.c
> @@ -274,13 +274,10 @@ int spacc_process_jb(struct spacc_device *spacc, unsigned long *lock_flag)
[ ... ]
>  		if (spacc->job_buffer[tail].active) {
>  			ret = spacc_packet_enqueue_ddt_ex
>  			      (spacc, 0, spacc->job_buffer[tail].job_idx,
>  			       spacc->job_buffer[tail].src,
>  			       spacc->job_buffer[tail].dst,
>  			       spacc->job_buffer[tail].proc_sz,
>  			       spacc->job_buffer[tail].aad_offset,
>  			       spacc->job_buffer[tail].pre_aad_sz,
>  			       spacc->job_buffer[tail].post_aad_sz,
>  			       spacc->job_buffer[tail].iv_offset,
>  			       spacc->job_buffer[tail].prio, false, lock_flag);

[Severity: Medium]
Additionally, when spacc_process_jb() unbuffers the job, it calls
spacc_packet_enqueue_ddt_ex() with aad_copy hardcoded to false. Doesn't this
corrupt data for any crypto operations that originally required aad_copy?

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