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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] System crash (Use-After-Free) on sysfs device unbind due to devres and TFM lifetime mismatch.
- [Medium] Watchdog counter (`wdcnt`) drifts upward when workqueue execution is delayed, permanently degrading interrupt handling performance.
--

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

crypto: spacc - Add SPAcc ahash support

This commit adds ahash support to the SPAcc driver. It introduces
multiple supported hash algorithms including CMAC, XCBC, HMAC, SHA
variants, and SM3/4 variants.

> diff --git a/drivers/crypto/dwc-spacc/spacc_ahash.c b/drivers/crypto/dwc-spacc/spacc_ahash.c
> new file mode 100644
> index 0000000000000..ec038c7774e7c
> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/spacc_ahash.c
[ ... ]
> +static int spacc_hash_digest(struct ahash_request *req)
> +{
> +	struct crypto_ahash *reqtfm = crypto_ahash_reqtfm(req);
> +	struct spacc_crypto_ctx *tctx = crypto_ahash_ctx(reqtfm);
> +	struct spacc_priv *priv = dev_get_drvdata(tctx->dev);
> +
> +	if (!priv || !priv->engine)
> +		return -ENODEV;
> +
> +	return crypto_transfer_hash_request_to_engine(priv->engine, req);
> +}

[Severity: High]
Could this lead to a use-after-free if an administrator unbinds the
driver via sysfs while a user-space process keeps a crypto tfm open?

Since priv is allocated via devres in spacc_init_device(), it is tied
to the driver binding lifetime and will be freed after spacc_crypto_remove()
completes.

If a user process concurrently submits a request that reaches
spacc_hash_digest(), it could check priv and priv->engine just before
they are freed, and then dereference the memory after devres frees it.

Does the driver need to refcount the hardware state or block removal
until all active tfms are destroyed?

[ ... ]
> diff --git a/drivers/crypto/dwc-spacc/spacc_core.c b/drivers/crypto/dwc-spacc/spacc_core.c
> new file mode 100644
> index 0000000000000..910916a1fe000
> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/spacc_core.c
[ ... ]
> +void spacc_pop_jobs(struct work_struct *data)
> +{
> +	int num = 0;
> +	struct spacc_priv *priv = container_of(data, struct spacc_priv,
> +					       pop_jobs);
> +	struct spacc_device *spacc = &priv->spacc;
> +	unsigned long lock_flag;
> +
> +	/*
> +	 * Decrement the WD CNT here since
> +	 * now we're actually going to respond
> +	 * to the IRQ completely
> +	 * (under spacc->lock -- spacc_process_irq() increments/resets
> +	 * this same counter under the same lock from IRQ context)
> +	 */
> +	spin_lock_irqsave(&spacc->lock, lock_flag);
> +	if (spacc->wdcnt)
> +		--(spacc->wdcnt);
> +	spin_unlock_irqrestore(&spacc->lock, lock_flag);

[Severity: Medium]
Will the watchdog counter drift upward permanently when system load delays
the execution of the workqueue?

In spacc_process_irq(), spacc->wdcnt increments for every watchdog
interrupt.

If multiple interrupts coalesce before the workqueue finally
executes, spacc_pop_jobs() only decrements the counter by 1.

Over time, these unmatched increments could accumulate until the counter
artificially hits SPACC_WD_LIMIT, causing the driver to fall back to an
inefficient polling mode.

Is there a way to ensure we decrement by the actual number of handled
events?

[ ... ]
> diff --git a/drivers/crypto/dwc-spacc/spacc_device.c b/drivers/crypto/dwc-spacc/spacc_device.c
> new file mode 100644
> index 0000000000000..f4e4f8b7f3525
> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/spacc_device.c
[ ... ]
> +static int spacc_init_device(struct platform_device *pdev)
> +{
[ ... ]
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);

[Severity: High]
Hardware state is allocated here using devm, tying it directly to the
driver binding. As mentioned in the spacc_hash_digest() comment, this
lifetime mismatch with the extended tfm lifetime creates the race condition
leading to a use-after-free.

[ ... ]
> +static void spacc_crypto_remove(struct platform_device *pdev)
> +{
> +	struct spacc_priv *priv = platform_get_drvdata(pdev);
> +	unsigned long flags;
> +
> +	/* Stop the crypto API from handing out any new tfms. */
> +	spacc_unregister_algs(priv);
> +
> +	/*
> +	 * Must run before IRQ/workqueue teardown as  crypto_engine_stop()
> +	 * waits on engine->busy, which only clears via the IRQ ->
> +	 * workqueue -> callback completion path.
> +	 */
> +	if (priv->engine)
> +		crypto_engine_exit(priv->engine);

[Severity: High]
Can this engine teardown occur while active tfms are still attempting to
submit requests?

Because unbind can occur while tfms are active, tearing down the engine
here before active tfms are tracked and finalized could disrupt in-flight
requests and contribute to the memory corruption issue.

[ ... ]
> diff --git a/drivers/crypto/dwc-spacc/spacc_interrupt.c b/drivers/crypto/dwc-spacc/spacc_interrupt.c
> new file mode 100644
> index 0000000000000..c6febd585ee2a
> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/spacc_interrupt.c
[ ... ]
> +u32 spacc_process_irq(struct spacc_device *spacc)
> +{
[ ... ]
> +	/* watchdog IRQ */
> +	if (spacc->op_mode == SPACC_OP_MODE_WD &&
> +			irq_status & SPACC_IRQ_STAT_STAT_WD) {
> +		if (++spacc->wdcnt == SPACC_WD_LIMIT) {

[Severity: Medium]
This is where the counter is incremented for every watchdog interrupt. As
noted above for spacc_pop_jobs(), multiple fast increments here will outpace
the single decrement in the delayed workqueue, corrupting the logical state.

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