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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Race condition and ring buffer corruption in `spacc_process_jb` due to unsafe reloading of the tail pointer.
- [High] Use-After-Free of algorithm structures during `spacc_unregister_hash_algs`.
- [High] Device teardown sequence in `spacc_crypto_remove()` causes deadlock, hangs, and DMA Use-After-Free.
- [High] DDT DMA descriptors are written in CPU endianness, causing memory corruption on big-endian architectures.
- [High] The `SPACC_AADCOPY_FLAG` multiplexing causes hash requests of 2GB or more to be silently truncated.
- [Low] Mixing goto-based error handling with guard() cleanup in the same function.
--

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

crypto: spacc - Add SPAcc ahash support

This commit adds ahash support to the SPAcc driver. It implements various
hash algorithms including CMAC, XCBC, MD5, and SHA variants, along with
crypto engine integration, hardware descriptor management, and device
teardown sequences.

> 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
> @@ -311,10 +311,10 @@ static int spacc_hash_do_one_request(struct crypto_engine *engine, void *areq)
> [ ... ]
> +	{
> +		guard(mutex)(&tctx->lock);
> +
> +		if (tctx->handle < 0 || !tctx->ctx_valid) {
> [ ... ]
> +			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 this code violate the cleanup subsystem technical guidelines?

The guidelines explicitly prohibit mixing goto-based error handling and
guard() declarations in the same function to prevent confusing ownership
semantics. Here, the goto fallback jumps out of a block protected by
guard(mutex)(&tctx->lock).

> [ ... ]
> +	}
> +	rc = spacc_packet_enqueue_ddt(&priv->spacc, ctx->acb.new_handle,
> +			&ctx->src, &ctx->dst, req->nbytes,
> +			0, req->nbytes, 0, 0, 0);

[Severity: High]
Could this cause silent truncation for hash requests of 2GB or larger?

In spacc_hash_do_one_request(), req->nbytes is passed as pre_aad_sz
to spacc_packet_enqueue_ddt_ex(). This creates an issue when multiplexing
large lengths with control flags in the lower layers.

> @@ -978,5 +978,5 @@ int spacc_unregister_hash_algs(struct spacc_priv *priv)
> [ ... ]
> +	list_for_each_entry_safe(salg, tmp, &priv->hash_alg_list, list) {
> +		crypto_engine_unregister_ahash(&salg->alg.hash);
> +		list_del(&salg->list);
> +	}

[Severity: High]
Could this lead to a use-after-free during module unload?

When crypto_engine_unregister_ahash() is called on salg->alg.hash, it
drops the final reference to the algorithm, triggering the asynchronous
destructor spacc_hash_alg_destroy() which calls kfree(salg). The
immediate next line executes list_del(&salg->list), dereferencing the
freed salg pointer.

> 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
> @@ -1286,7 +1286,7 @@ int spacc_packet_enqueue_ddt_ex(struct spacc_device *spacc, int use_jb,
> [ ... ]
> +	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;

[Severity: High]
Is the SPACC_AADCOPY_FLAG multiplexing safe for large hash requests?

If req->nbytes from ahash requests is greater than or equal to 0x80000000
(2GB), the driver interprets the MSB as the SPACC_AADCOPY_FLAG here. It
strips the MSB from the length, silently truncating the requested hash length,
and sets the hardware control flag instead of processing the full buffer.

> 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
> @@ -185,8 +185,8 @@ static void spacc_crypto_remove(struct platform_device *pdev)
> [ ... ]
> +	spacc_irq_glbl_disable(&priv->spacc);
> +	spacc_unregister_algs(priv);
> +
> +	if (priv->engine)
> +		crypto_engine_exit(priv->engine);

[Severity: High]
Is the teardown sequence ordered correctly?

During driver removal, spacc_irq_glbl_disable() turns off hardware IRQs. Next,
spacc_unregister_algs() destroys the DMA hash_pool. Then crypto_engine_exit()
tries to stop the engine, waiting up to 10 seconds for engine->busy to clear.

Because IRQs are disabled, inflight requests can never finish. The engine
fails to stop, leaks its worker thread, and the hardware can perform DMA
writes into the destroyed pool, resulting in a use-after-free.

> diff --git a/drivers/crypto/dwc-spacc/spacc_hal.c b/drivers/crypto/dwc-spacc/spacc_hal.c
> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/spacc_hal.c
> @@ -251,8 +251,8 @@ int pdu_ddt_add(struct device *dev, struct pdu_ddt *ddt, dma_addr_t phys,
> [ ... ]
> +	if (ddt->idx == ddt->limit)
> +		return -EINVAL;
> +
> +	ddt->virt[ddt->idx * 2 + 0] = (uint32_t)phys;
> +	ddt->virt[ddt->idx * 2 + 1] = size;
> +	ddt->virt[ddt->idx * 2 + 2] = 0;
> +	ddt->virt[ddt->idx * 2 + 3] = 0;

[Severity: High]
Does this code handle endianness correctly for the DMA descriptors?

The SPAcc hardware uses a little-endian DMA bus interface, but the code
writes the phys address and size parameters directly to ddt->virt
without a cpu_to_le32() conversion. On big-endian architectures, this
results in byteswapped addresses and lengths being read by the hardware,
leading to out-of-bounds DMA accesses and memory corruption.

> 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
> @@ -350,10 +350,10 @@ int spacc_process_jb(struct spacc_device *spacc, unsigned long *lock_flag)
> [ ... ]
> +					spin_unlock_irqrestore(&spacc->lock,
> +							       *lock_flag);
> +					job->cb(spacc, job->cbdata);
> +					spin_lock_irqsave(&spacc->lock,
> +							  *lock_flag);
> +					tail = spacc->jb_tail;
> +				} else if (!spacc->autodetect) {
> +					job->job_done = 1;
> +					wake_up_interruptible(&job->waitq);
> +				}
> +			}
> +		}
> +
> +		tail++;
> +		if (tail == SPACC_MAX_JOB_BUFFERS)
> +			tail = 0;
> +
> +		spacc->jb_tail = tail;

[Severity: High]
Can this sequence cause a race condition and corrupt the ring buffer?

In spacc_process_jb(), the lock is dropped to call job->cb. If another
thread queues a new job and processes it, advancing spacc->jb_tail, the
first thread will wake up, reload the newly advanced jb_tail into tail,
and unconditionally increment it here. This skips the unprocessed job at
the new tail index and corrupts the buffer state.

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