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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing hardware reset in the removal path allows DMA writes to freed memory
- [High] Use-after-free of `priv->engine` during device teardown due to incorrect cleanup ordering
- [Medium] Data corruption due to overloading `pre_aad_sz` parameter with `SPACC_AADCOPY_FLAG`
--

--- Patch [2]: [PATCH v22 2/4] crypto: spacc - Add SPAcc ahash support ---
commit 78512023fe2c29d98b9094823066da614d7046ac
Author: Pavitrakumar Managutte <[email protected]>

crypto: spacc - Add SPAcc ahash support

This commit introduces asynchronous hash support for the SPAcc driver,
covering algorithms like CMAC, XCBC, MD5, SHA1, SHA2, and SHA3. It implements
the core driver structure, crypto engine callbacks, and the hardware control
logic for these hash operations.

> diff --git a/drivers/crypto/dwc-spacc/spacc_ahash.c b/drivers/crypto/dwc-spacc/spacc_ahash.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/spacc_ahash.c
[ ... ]
> +static int spacc_hash_do_one_request(struct crypto_engine *engine, void *areq)
> +{
[ ... ]
> +	if (spacc_hash_try_hw(tctx, priv, salg, reqtfm, req) < 0)
> +		goto fallback;
> +
> +	rc = spacc_packet_enqueue_ddt(&priv->spacc, ctx->acb.new_handle,
> +			&ctx->src, &ctx->dst, req->nbytes,
> +			0, req->nbytes, 0, 0, 0);

[Severity: Medium]
Here in spacc_hash_do_one_request(), the length req->nbytes is passed
directly as the pre_aad_sz argument to spacc_packet_enqueue_ddt().

If a caller submits a hash request with a buffer size >= 2GB (where the
MSB is set), doesn't this trigger the SPACC_AADCOPY_FLAG check inside
spacc_packet_enqueue_ddt_ex()?

spacc_packet_enqueue_ddt_ex() implements:

    if (pre_aad_sz & SPACC_AADCOPY_FLAG) {
        job->ctrl  |= SPACC_CTRL_MASK(SPACC_CTRL_AAD_COPY);
        pre_aad_sz &= ~(SPACC_AADCOPY_FLAG);
    }

Could this lead to the hardware receiving an incorrectly truncated payload
size and an unexpected SPACC_CTRL_AAD_COPY control flag, causing an
incorrect hash output or hardware malfunction?

[ ... ]
> diff --git a/drivers/crypto/dwc-spacc/spacc_device.c b/drivers/crypto/dwc-spacc/spacc_device.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/spacc_device.c
[ ... ]
> +static void spacc_crypto_remove(struct platform_device *pdev)
> +{
> +	struct spacc_priv *priv = platform_get_drvdata(pdev);
> +	unsigned long flags;
> +
> +	spacc_unregister_algs(priv);
> +
> +	if (priv->engine)
> +		crypto_engine_exit(priv->engine);

[Severity: High]
The crypto engine is exited and its structure is freed here, but the
interrupt handler and workqueue are still active.

If a hardware job completes and raises an interrupt immediately after
crypto_engine_exit() returns, the IRQ handler queues the work. The
worker then executes spacc_digest_cb(), which eventually calls:

    crypto_finalize_hash_request(priv->engine, cb->req, err);

Does this create a race condition where the active callbacks dereference
the freed priv->engine state, leading to a use-after-free?

> +
> +	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);
> +
> +	dma_pool_destroy(priv->hash_pool);

[Severity: High]
The hash_pool (and later the ddt pools via spacc_remove()) are
destroyed here.

However, the removal path never issues a hardware abort or reset command
to the SPAcc engine to halt any ongoing asynchronous operations. If the
module is unloaded while a crypto hardware request is actively processing,
the hardware will continue executing the job.

Could this allow the hardware to eventually complete the job and perform a
DMA write of the digest or ciphertext into the destination buffer allocated
from the now-destroyed pools, silently corrupting whatever system memory
has claimed that physical address?

> +	priv->hash_pool = NULL;
> +
> +	spacc_remove(pdev);
> +	platform_set_drvdata(pdev, NULL);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2