Re: [PATCH v29 4/4] crypto: spacc - Add SPAcc Kconfig and Makefile

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] crypto: spacc: salg leak or priv->ref UAF in
  spacc_probe_hashes()
- [Medium] crypto: spacc: free of active DMA buffer in
  spacc_xof_stringsize_autodetect()
--

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

crypto: spacc - Add SPAcc Kconfig and Makefile

This commit adds the Kconfig and Makefile entries to build the Synopsys
DesignWare SPAcc driver.

> diff --git a/drivers/crypto/dwc-spacc/Makefile b/drivers/crypto/dwc-spacc/Makefile
> new file mode 100644
> index 0000000000000..b7125a7797ba0
> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/Makefile
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_CRYPTO_DEV_SPACC) += snps-spacc.o
> +
> +snps-spacc-y := spacc_hal.o spacc_core.o \
> +spacc_manager.o spacc_interrupt.o spacc_device.o

[Severity: Medium]
Can spacc_xof_stringsize_autodetect() free DMA buffers while a timed-out
hardware job is still in flight?

drivers/crypto/dwc-spacc/spacc_core.c:spacc_wait_for_job_completion() {
	if (readl_poll_timeout(spacc->regmap + SPACC_REG_FIFO_STAT, rbuf,
			       !(rbuf & SPACC_FIFO_STAT_STAT_EMPTY),
			       10, 10000))
		return false;
...
}

drivers/crypto/dwc-spacc/spacc_core.c:spacc_xof_stringsize_autodetect() {
...
			writel(spacc_ctrl[alg], spacc->regmap + SPACC_REG_CTRL);

			if (spacc_wait_for_job_completion(spacc, virt, md[alg]))
				spacc->config.string_size = (16 << ss);
		}
	}
...
	pdu_ddt_free(&ddt);
	dma_free_coherent(spacc->dptr, SPACC_TEST_DMA_BUFF_SIZE,
			  virt, dma);
}

If the 10ms poll times out, spacc_wait_for_job_completion() returns
false just as it does on a digest mismatch.
spacc_xof_stringsize_autodetect() then submits further jobs and frees
ddt and virt while the uncancelable DMA operation may still be accessing
them, and spacc_init() treats the failure as non-fatal and continues
probing.

> +
> +snps-spacc-$(CONFIG_CRYPTO_DEV_SPACC_HASH) += spacc_ahash.o

[Severity: High]
Does spacc_probe_hashes() leak salg or drop priv->ref twice when
spacc_register_hash() fails?

drivers/crypto/dwc-spacc/spacc_ahash.c:spacc_probe_hashes() {
...
			salg = kmalloc_obj(*salg, GFP_KERNEL);
			if (!salg) {
				rc = -ENOMEM;
				goto err_unregister;
			}

			salg->mode = &possible_hashes[index];
			salg->dev = &spacc_pdev->dev;
			salg->priv = priv;
			kref_get(&priv->ref);

			rc = spacc_register_hash(salg);
			if (rc < 0) {
				kref_put(&priv->ref, spacc_priv_release);
				continue;
			}
}

spacc_register_hash() sets cra_destroy to spacc_hash_alg_destroy(),
which calls kfree(salg) and kref_put(&priv->ref, spacc_priv_release).

If spacc_register_hash() fails before __crypto_register_alg() (for
example, when crypto_alloc_ahash() fails), cra_destroy never runs and
salg is leaked.  Conversely, if __crypto_register_alg() fails inside
crypto_engine_register_ahash() (for example, with -EEXIST when a second
SPAcc instance probes with the same cra_driver_name),
crypto_register_alg() calls crypto_alg_put() and runs
spacc_hash_alg_destroy(), so the error branch in spacc_probe_hashes()
drops priv->ref a second time and frees priv while probe continues
using it.

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