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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 20 potential issue(s) to consider:
- [Critical] Hardware DMA Use-After-Free and memory corruption via unguarded `wait_event_interruptible`.
- [Critical] Hardware polling loop executes too quickly, leading to premature timeout and DMA memory corruption.
- [Critical] Copying of `wait_queue_head_t` breaks internal spinlocks and waitqueue lists in `spacc_clone_handle`.
- [Critical] Destroying the workqueue in `spacc_crypto_remove` before disabling interrupts creates a use-after-free window.
- [High] Double free of `ctx->dst` and `ctx->digest_buf` in `spacc_hash_init_dma` error path and uninitialized `ctx->src` unmap.
- [High] Software fallback hashes the entire lengths of scatterlist entries rather than clamping to `req->nbytes`.
- [High] Incorrect DMA unmapping and memory leak of `tmp_sgl` on request enqueue failure.
- [High] Non-cacheline-aligned fields embedded in the crypto context are mapped for DMA, risking memory corruption.
- [High] Double `up()` on `ctx_sem` when releasing hardware contexts breaks concurrency limits.
- [High] Incorrect module unload order leaves crypto engine vulnerable to use-after-free.
- [High] Massive resource leak on probe failure due to missing cleanup in `spacc_crypto_probe`.
- [High] Use-After-Free of DMA pool during hash probe failure.
- [High] The driver uses a static global DMA pool, breaking multi-device isolation and causing crashes on removal.
- [High] AB-BA / Resource Deadlock in `spacc_compute_xcbc_key` due to `spacc_open` taking `ctx_sem` before reusing the context.
- [High] TOCTOU race on `ref_cnt` when releasing contexts via `spacc_close`.
- [High] Stack out-of-bounds read leading to information leak in `spacc_xof_stringsize_autodetect`.
- [High] Out-of-bounds memory accesses in `spacc_hash_export` and `spacc_hash_import`.
- [High] The linear DMA mode incorrectly handles fragmented scatterlists, leading to silent memory corruption.
- [Medium] Undefined unaligned memory accesses when copying stack buffers to device MMIO.
- [Medium] Device structure reference leak during key setting and request initialization.
--

commit 679c537a437ea5581e46701228d530d1fa314722
Author: Pavitrakumar Managutte <pavitrakumarm@vayavyalabs.com>
Subject: crypto: spacc - Add SPAcc ahash support

Add ahash support to SPAcc driver.
Below are the hash algos supported:
- cmac(aes)
- xcbc(aes)
- cmac(sm4)

> diff --git a/drivers/crypto/dwc-spacc/spacc_ahash.c b/drivers/crypto/dwc-spacc/spacc_ahash.c
> index 0000000000000..a4a7776323606
> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/spacc_ahash.c
[ ... ]
> +static int spacc_ctx_clone_handle(struct ahash_request *req)
> +{
> +	struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
> +	struct spacc_crypto_ctx *tctx = crypto_ahash_ctx(tfm);
> +	struct spacc_crypto_reqctx *ctx = ahash_request_ctx(req);
> +	struct spacc_priv *priv = dev_get_drvdata(tctx->dev);
> +
> +	if (tctx->handle < 0)
> +		return -EINVAL;
> +
> +	ctx->acb.new_handle = spacc_clone_handle(&priv->spacc, tctx->handle,
> +						 &ctx->acb);
> +
> +	if (ctx->acb.new_handle < 0) {
> +		spacc_hash_cleanup_dma(tctx->dev, req);
> +		return -ENOMEM;
> +	}

[Severity: High]
Does this code introduce a double-free on the error path?

When spacc_ctx_clone_handle() fails, it calls spacc_hash_cleanup_dma() which
frees both ctx->digest_buf and ctx->dst. It then returns -ENOMEM to 
spacc_hash_init_dma(), which branches to err_free_dst and err_free_digest,
freeing those same pointers a second time and corrupting the DMA pool.

Also, when this failure occurs, ctx->src is not yet mapped or initialized.
Will the cleanup function attempt to execute a bogus dma_unmap_sg() and
pdu_ddt_free() on uninitialized data?

[ ... ]
> +static int spacc_hash_setkey(struct crypto_ahash *tfm, const u8 *key,
> +		unsigned int keylen)
> +{
[ ... ]
> +	/* Invalidate any cached HW context — key has changed. */
> +	if (tctx->handle >= 0) {
> +		spacc_close(&priv->spacc, tctx->handle);
> +		put_device(tctx->dev);
> +		tctx->handle = -1;
> +		tctx->dev = NULL;
> +	}
> +
> +	tctx->ctx_valid = false;
> +	tctx->dev = get_device(salg->dev);
> +
> +	return 0;
> +}

[Severity: Medium]
Will this leak the device structure reference when tctx->handle is less than 0?

In spacc_hash_init_tfm(), tctx->dev is initialized and tctx->handle is set
to -1. Later, in spacc_hash_setkey(), the driver only calls
put_device(tctx->dev) if tctx->handle >= 0. Because the handle is initially
negative, it skips the put_device() but unconditionally overwrites tctx->dev
with a newly acquired reference from get_device().

[ ... ]
> +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);
> +	if (rc < 0) {
> +		spacc_hash_cleanup_dma(tctx->dev, req);
> +
> +		if (ctx->acb.new_handle >= 0) {
> +			spacc_close(&priv->spacc, ctx->acb.new_handle);
> +			ctx->acb.new_handle = -1;
> +		}
> +
> +		if (rc == -EBUSY) {
> +			dev_dbg(salg->dev, "HW full, engine retry\n");
> +			return -ENOSPC;
> +		}
> +
> +		return rc;
> +
> +	}

[Severity: High]
Does this error path incorrectly unmap DMA and leak memory?

For zero-length requests, spacc_hash_init_dma() allocates and maps
tctx->tmp_sgl. If spacc_packet_enqueue_ddt() fails, this error path calls
the generic spacc_hash_cleanup_dma(). That generic helper blindly unmaps
req->src rather than properly checking if tctx->tmp_sgl was the scatterlist
actually mapped.

This can result in unmapping an unmapped scatterlist, while permanently
leaking the actual DMA mapping. The error path also returns early without
freeing the array, permanently leaking the tctx->tmp_sgl allocation.

[ ... ]
> +static int spacc_hash_update(struct ahash_request *req)
> +{
[ ... ]
> +	if (tctx->shash_fb) {
> +		SHASH_DESC_ON_STACK(desc, tctx->shash_fb);
> +		struct sg_mapping_iter miter;
> +
> +		desc->tfm = tctx->shash_fb;
> +		rc = crypto_shash_import(desc, ctx->state_buffer);
> +		if (rc)
> +			goto out_shash;
> +
> +		sg_miter_start(&miter, req->src,
> +			       sg_nents_for_len(req->src, req->nbytes),
> +			       SG_MITER_ATOMIC | SG_MITER_FROM_SG);
> +		while (sg_miter_next(&miter)) {
> +			rc = crypto_shash_update(desc,
> +						miter.addr, miter.length);
> +			if (rc)
> +				break;
> +		}
> +		sg_miter_stop(&miter);

[Severity: High]
Does the software fallback improperly hash data outside the logical request?

The code iterates over the source scatterlist using sg_nents_for_len(). If
req->nbytes exceeds the total scatterlist length, sg_nents_for_len() returns
a negative error code, which is cast to a large unsigned integer by
sg_miter_start(), leading to massive out-of-bounds reads.

Even when successful, the loop unconditionally calls crypto_shash_update()
with the full miter.length of each chunk rather than tracking and clamping
to the remaining req->nbytes. This incorrectly hashes trailing out-of-bounds
bytes outside the logical request.

[ ... ]
> +static int spacc_hash_export(struct ahash_request *req, void *out)
> +{
> +	struct spacc_crypto_reqctx *ctx = ahash_request_ctx(req);
> +
> +	memcpy(out, ctx->state_buffer, sizeof(ctx->state_buffer));
> +	return 0;
> +}
> +
> +static int spacc_hash_import(struct ahash_request *req, const void *in)
> +{
> +	struct spacc_crypto_reqctx *ctx = ahash_request_ctx(req);
> +
> +	memcpy(ctx->state_buffer, in, sizeof(ctx->state_buffer));
> +	return 0;
> +}

[Severity: High]
Do these memcpy calls result in out-of-bounds accesses?

The algorithm's state size is explicitly set to HASH_MAX_STATESIZE during
registration. The crypto API bounds and allocates export buffers of exactly
this size.

However, the driver's export and import functions use
sizeof(ctx->state_buffer) as the length parameter. Because state_buffer
is defined internally as u8 state_buffer[HASH_MAX_STATESIZE+16], this
forcibly copies 16 bytes past the bounds of the API-provided buffers,
resulting in a heap buffer overflow on export and an out-of-bounds read
on import.

[ ... ]
> +int spacc_probe_hashes(struct platform_device *spacc_pdev)
> +{
[ ... ]
> +		/* Just check hardware support - no .valid flag needed */
> +		if (spacc_is_mode_keysize_supported(&priv->spacc,
> +				    possible_hashes[index].id & 0xFF,
> +				    possible_hashes[index].hashlen, 1)) {
> +			salg = kmalloc_obj(*salg, GFP_KERNEL);
> +			if (!salg) {
> +				rc = -ENOMEM;
> +				goto err_destroy_pool;
> +			}
> +
> +			salg->mode = &possible_hashes[index];
> +			salg->dev = &spacc_pdev->dev;
> +
> +			rc = spacc_register_hash(salg);
> +			if (rc < 0) {
> +				kfree(salg);
> +				continue;
> +			}
> +
> +			registered++;
> +		}
> +	}
> +
> +	return registered;
> +
> +err_destroy_pool:
> +	dma_pool_destroy(priv->hash_pool);
> +	priv->hash_pool = NULL;
> +	return rc;
> +}

[Severity: High]
Does this error path trigger a use-after-free of the DMA pool?

If kmalloc_obj() fails mid-loop, the code branches to err_destroy_pool and
immediately calls dma_pool_destroy(priv->hash_pool). However, algorithms
successfully registered in earlier loop iterations remain active in the
crypto API until the caller's error path eventually unregisters them.

If a concurrent crypto request is made against these active algorithms in
this window, it will attempt to allocate from the destroyed DMA pool.

> diff --git a/drivers/crypto/dwc-spacc/spacc_core.c b/drivers/crypto/dwc-spacc/spacc_core.c
> index 0000000000000..e0f64f41f4b41
> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/spacc_core.c
[ ... ]
> +int spacc_packet_enqueue_ddt_ex(struct spacc_device *spacc, int use_jb,
> +				int job_idx, struct pdu_ddt *src_ddt,
> +				struct pdu_ddt *dst_ddt, u32 proc_sz,
> +				u32 aad_offset, u32 pre_aad_sz,
> +				u32 post_aad_sz, u32 iv_offset,
> +				u32 prio)
> +{
[ ... ]
> +	if (spacc->config.dma_type == SPACC_DMA_DDT) {
> +		pdu_io_cached_write(spacc->dptr, spacc->regmap +
> +				    SPACC_REG_SRC_PTR, (uint32_t)src_ddt->phys,
> +				    &spacc->cache.src_ptr);
> +		pdu_io_cached_write(spacc->dptr, spacc->regmap +
> +				    SPACC_REG_DST_PTR, (uint32_t)dst_ddt->phys,
> +				    &spacc->cache.dst_ptr);
> +	} else if (spacc->config.dma_type == SPACC_DMA_LINEAR) {
> +		pdu_io_cached_write(spacc->dptr, spacc->regmap +
> +				    SPACC_REG_SRC_PTR,
> +				    (uint32_t)src_ddt->virt[0],
> +				    &spacc->cache.src_ptr);
> +		pdu_io_cached_write(spacc->dptr, spacc->regmap +
> +				    SPACC_REG_DST_PTR,
> +				    (uint32_t)dst_ddt->virt[0],
> +				    &spacc->cache.dst_ptr);
> +	} else
> +		return -EIO;

[Severity: High]
Does the linear DMA mode incorrectly handle fragmented scatterlists?

If spacc->config.dma_type == SPACC_DMA_LINEAR, the code unconditionally
assigns the hardware source pointer to src_ddt->virt[0] (the physical
address of the first scatterlist chunk) and instructs the hardware to read
proc_len bytes contiguously.

It never verifies if the mapped scatterlist actually consists of a single
contiguous block (sg_nents == 1). If a fragmented SG list is provided by
the crypto API, the hardware will read/write past the physical bounds of
the first chunk directly into unrelated physical memory pages.

[ ... ]
> +int spacc_open(struct spacc_device *spacc, int enc, int hash, int ctxid,
> +	       int secure_mode, spacc_callback cb, void *cbdata)
> +{
[ ... ]
> +	/*
> +	 * Acquire the semaphore. This will decrement the count. If the count
> +	 * is already zero (meaning all HW contexts are in use), this call
> +	 * will sleep interruptibly until another thread calls up().
> +	 */
> +	if (down_interruptible(&spacc->ctx_sem)) {
> +		dev_dbg(spacc->dptr, "ERR: Interrupted by signal\n");
> +		return -ERESTARTSYS; /* Woken by a signal */
> +	}
> +
> +	spin_lock_irqsave(&spacc->ctx_lock, flags);
> +	job_idx = spacc_job_request(spacc, ctxid);

[Severity: High]
Can this sequence cause a resource deadlock when reusing a context?

spacc_open() unconditionally blocks on down_interruptible(&spacc->ctx_sem)
before it acquires the spinlock to check if the context is being reused.

During an XCBC request, spacc_hash_do_one_request() calls spacc_open(),
acquiring a token from ctx_sem. It then calls spacc_compute_xcbc_key(),
which calls spacc_open() again to reuse the parent's ctx_idx. If all
hardware context semaphore tokens are currently held by other threads, this
inner spacc_open() will block indefinitely while still holding the parent
token, causing an unrecoverable AB-BA resource deadlock.

[ ... ]
> +int spacc_close(struct spacc_device *dev, int handle)
> +{
> +	int ret;
> +	int ref_cnt_before = 0;
> +	unsigned long flags;
> +
> +	if (handle < 0 || handle >= SPACC_MAX_JOBS)
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(&dev->ctx_lock, flags);
> +
> +	if (dev->job[handle].ctx_idx != SPACC_CTX_IDX_UNUSED &&
> +	    dev->job[handle].ctx_idx < dev->config.num_ctx)
> +		ref_cnt_before = dev->ctx[dev->job[handle].ctx_idx].ref_cnt;
> +
> +	spin_unlock_irqrestore(&dev->ctx_lock, flags);
> +
> +	ret = spacc_job_release(dev, handle);
> +
> +	if (ref_cnt_before == 1)
> +		up(&dev->ctx_sem);
> +
> +	return ret;
> +}

[Severity: High]
Does this introduce a TOCTOU race on ref_cnt?

spacc_close() evaluates ref_cnt_before by reading it while holding
dev->ctx_lock, drops the lock, calls spacc_job_release() (which decreases
ref_cnt), and then uses ref_cnt_before to conditionally call
up(&dev->ctx_sem).

Concurrently, spacc_job_request() modifies ref_cnt holding ONLY spacc->lock,
completely bypassing ctx_lock. A concurrent clone operation can increment
ref_cnt between spacc_close()'s check and its action, causing spacc_close()
to erroneously release the semaphore for a context that was actively grabbed
by the clone.

[Severity: High]
Will this also result in a double up() on the semaphore?

When a context's reference count drops to 0, spacc_job_release() (via
spacc_ctx_release) already correctly releases the ctx_sem semaphore by
calling up(&spacc->ctx_sem). However, spacc_close() explicitly calls
up(&dev->ctx_sem) a second time. These persistent double-increments inflate
the semaphore count far past the physical hardware context limits, eventually
allowing infinite concurrent access and destroying hardware isolation.

[ ... ]
> +static bool spacc_wait_for_job_completion(struct spacc_device *spacc,
> +		void *virt, unsigned char *expected_md)
> +{
> +	int stat;
> +	unsigned long rbuf;
> +
> +	for (int i = 0; i < 20; i++) {
> +		rbuf = readl(spacc->regmap + SPACC_REG_FIFO_STAT) &
> +			SPACC_FIFO_STAT_STAT_EMPTY;
> +		if (rbuf)
> +			continue;
> +
> +		/* Check result */
> +		writel(1, spacc->regmap + SPACC_REG_STAT_POP);

[Severity: Critical]
Does this hardware polling loop execute too quickly and time out prematurely?

The loop polls the hardware FIFO_STAT register 20 times in a tight CPU loop
with no delays (e.g., udelay() or cpu_relax()). On modern CPUs, this executes
in a few dozen nanoseconds, guaranteeing a premature timeout before the
hardware cryptographic operation can complete.

The caller spacc_xof_stringsize_autodetect() interprets this false failure by
immediately freeing the active DMA buffers and resetting the engine while the
hardware is still executing the job, causing it to DMA the crypto results
into freed physical memory.

[ ... ]
> +static int spacc_xof_stringsize_autodetect(struct spacc_device *spacc)
> +{
> +	void *virt;
> +	int ss, alg;
> +	dma_addr_t dma;
> +	struct pdu_ddt	ddt;
> +	unsigned long buflen;
> +	unsigned char buf[256];
[ ... ]
> +			/* write key context */
> +			pdu_to_dev_s(spacc->regmap + SPACC_CTX_HASH_KEY, buf,
> +				     spacc->config.hash_page_size >> 2,
> +				     spacc->config.big_endian);

[Severity: High]
Does this copy result in a stack out-of-bounds read?

The function allocates a fixed 256-byte stack array unsigned char buf[256].
However, it writes this buffer to the hardware using pdu_to_dev_s() with a
size parameter of spacc->config.hash_page_size >> 2 words.

Because the configured hash page size is read dynamically from a 4-bit
hardware capability field that configures contexts up to 32KB, it can vastly
exceed 256 bytes. This reads past the bounds of buf on the stack, exposing
arbitrary uninitialized kernel stack memory to the hardware cryptographic
context.

[ ... ]
> +int spacc_compute_xcbc_key(struct spacc_device *spacc, int mode_id,
> +			   int job_idx, const unsigned char *key,
> +			   int keylen, unsigned char *xcbc_out)
> +{
[ ... ]
> +		/*
> +		 * Do the 48 bytes as a single SPAcc job this is the ideal case
> +		 * but only possible if ECB was enabled in the core
> +		 */
> +		spacc_write_context(spacc, handle, SPACC_CRYPTO_OPERATION,
> +				    key, keylen, iv, 16);
> +		err = spacc_packet_enqueue_ddt(spacc, handle, &ddt, &ddt, 48,
> +					       0, 0, 0, 0, 0);
> +		if (err != 0)
> +			goto xcbc_err;
> +
> +		wait_event_interruptible(job->waitq, job->job_done);
> +		job->job_done = 0;
> +		err = job->job_err;
> +
> +		if (err != 0)
> +			goto xcbc_err;
> +	}

[Severity: Critical]
Can this lead to hardware DMA use-after-free if interrupted by a signal?

The driver waits for a hardware crypto job to finish using
wait_event_interruptible(), but completely ignores its return value.
If interrupted by a signal, the wait aborts, leaving job->job_done as 0
and job->job_err as -EINPROGRESS.

The code interprets this non-zero error as a failure and jumps to xcbc_err,
immediately freeing the buf DMA memory and DDT structures. Because the
hardware operation was never canceled, the device continues writing its
cryptographic results into the freed physical memory, causing severe
asynchronous DMA corruption.

> diff --git a/drivers/crypto/dwc-spacc/spacc_device.c b/drivers/crypto/dwc-spacc/spacc_device.c
> index 0000000000000..a1948d6790a58
> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/spacc_device.c
[ ... ]
> +static int spacc_crypto_probe(struct platform_device *pdev)
> +{
> +	int rc = 0;
> +
> +	rc = spacc_init_device(pdev);
> +	if (rc < 0)
> +		goto err;
> +
> +#if IS_ENABLED(CONFIG_CRYPTO_DEV_SPACC_HASH)
> +	rc = spacc_probe_hashes(pdev);
> +	if (rc < 0)
> +		goto err;
> +#endif
> +
> +	return 0;
> +err:
> +	spacc_unregister_algs(platform_get_drvdata(pdev));
> +
> +	return rc;
> +}

[Severity: High]
Does this error path leak resources on probe failure?

If spacc_probe_hashes() fails, the error path jumps to the err label, which
exclusively unregisters the algorithms. It completely fails to call
crypto_engine_exit(), destroy_workqueue(), or clean up the SPAcc device
hardware context.

This skips tearing down the device structures, leaves the IRQ handler
registered, and permanently leaks the hardware engine thread and workqueue.

[ ... ]
> +static void spacc_crypto_remove(struct platform_device *pdev)
> +{
> +	struct spacc_priv *priv = platform_get_drvdata(pdev);
> +
> +	if (priv->engine)
> +		crypto_engine_exit(priv->engine);
> +
> +	if (priv->spacc_wq)
> +		destroy_workqueue(priv->spacc_wq);
> +
> +	spacc_unregister_algs(priv);
> +	spacc_remove(pdev);
> +}

[Severity: High]
Does this module unload order leave the crypto engine vulnerable to
use-after-free?

crypto_engine_exit() is called before spacc_unregister_algs(). This stops
and destroys the crypto engine thread while the driver's algorithms are
still fully registered and actively exposed to the crypto API.

If a new request arrives in this window, the crypto API routes it to the
driver, which will attempt to queue it to the destroyed priv->engine
pointer, leading to a kernel crash.

[Severity: Critical]
Does destroying the workqueue here create a use-after-free window?

destroy_workqueue(priv->spacc_wq) is called to free the workqueue, but
hardware interrupts (SPACC_REG_IRQ_EN) are not manually disabled. Because
the IRQ handler is managed via devm_request_irq(), it remains fully active
until devres_release_all() runs after remove() completes.

If the hardware raises an interrupt in this window, the active IRQ handler
will call queue_work() on the already destroyed workqueue, leading to an
immediate use-after-free kernel panic.

> diff --git a/drivers/crypto/dwc-spacc/spacc_device.h b/drivers/crypto/dwc-spacc/spacc_device.h
> index 0000000000000..991aef9a29ffb
> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/spacc_device.h
[ ... ]
> +struct spacc_crypto_ctx {
> +	struct device *dev;
> +	unsigned int statesize;
> +	int handle, mode, auth_size, key_len;
> +	unsigned char *cipher_key;
> +
> +	/*
> +	 * Indicates that the H/W context has been setup and can be used for
> +	 * crypto; otherwise, the software fallback will be used.
> +	 */
> +	bool ctx_valid;
> +
> +	/* salt used for rfc3686/givencrypt mode */
> +	unsigned char csalt[16];
> +	u8 ipad[128] __aligned(sizeof(u32));
> +	u8 digest_ctx_buf[128] __aligned(sizeof(u32));
> +	u8 tmp_buffer[128] __aligned(sizeof(u32));

[Severity: High]
Are non-cacheline-aligned fields risking memory corruption during DMA?

tmp_buffer is declared with only __aligned(sizeof(u32)) (4-byte) alignment.
During zero-length requests, this internal buffer is mapped directly for
DMA via sg_set_buf().

Because it lacks ____cacheline_aligned, it shares a CPU cacheline with
adjacent struct fields. On non-coherent architectures that perform cache
maintenance (flush/invalidate) on full cachelines during DMA transfers,
concurrent CPU accesses to adjacent fields will result in silent memory
corruption.

> diff --git a/drivers/crypto/dwc-spacc/spacc_hal.c b/drivers/crypto/dwc-spacc/spacc_hal.c
> index 0000000000000..7dc8139ae9499
> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/spacc_hal.c
[ ... ]
> +int pdu_mem_init(void *device)
> +{
> +	if (ddt_device)
> +		return 0; /* already setup */
> +
> +	/* max of 64 DDT entries */
> +	ddt_device = device;
> +	ddt_pool = dma_pool_create("spaccddt", device,
> +				   DDT_MAX_ENTRIES, 8, 0);
[ ... ]
> +void pdu_mem_deinit(void *device)
> +{
> +	/* for now, just skip deinit except for matching device */
> +	if (device != ddt_device)
> +		return;
> +
> +	dma_pool_destroy(ddt_pool);

[Severity: High]
Do these static global DMA pools break multi-device isolation?

ddt_pool, ddt16_pool, and ddt4_pool are implemented as static global variables.
pdu_mem_init() sets ddt_device to the first device that probes. If a second
SPACC device probes, it reuses the first device's pool, breaking IOMMU and
DMA mask isolation constraints.

If the first device is later unbound, pdu_mem_deinit() unconditionally
destroys the global DMA pools, leaving the second device active but using a
freed/destroyed DMA pool for all subsequent operations.

> diff --git a/drivers/crypto/dwc-spacc/spacc_manager.c b/drivers/crypto/dwc-spacc/spacc_manager.c
> index 0000000000000..518cc49866b54
> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/spacc_manager.c
[ ... ]
> +int spacc_clone_handle(struct spacc_device *spacc, int old_handle,
> +		       void *cbdata)
> +{
> +	int new_handle;
> +
> +	new_handle = spacc_job_request(spacc, spacc->job[old_handle].ctx_idx);
> +	if (new_handle < 0)
> +		return new_handle;
> +
> +	spacc->job[new_handle]          = spacc->job[old_handle];
> +	spacc->job[new_handle].job_used = new_handle;
> +	spacc->job[new_handle].cbdata   = cbdata;
> +
> +	return new_handle;
> +}

[Severity: Critical]
Does copying struct spacc_job directly break internal spinlocks and waitqueues?

spacc_clone_handle() copies the struct spacc_job using a direct struct
assignment. Since struct spacc_job embeds a wait_queue_head_t waitq, this
blindly copies the underlying spinlock_t and list_head pointers.

This violates locking invariants and severely corrupts the waitqueue. If any
thread is waiting or subsequently waits on the child's waitqueue, the system
will panic or hang.

[ ... ]
> +int spacc_write_context(struct spacc_device *spacc, int job_idx, int op,
> +			const unsigned char *key, int ksz,
> +			const unsigned char *iv, int ivsz)
> +{
> +	int buflen;
> +	int ret = 0;
> +	unsigned char buf[300];
[ ... ]
> +		pdu_to_dev_s(ctx->ciph_key, buf, buflen >> 2,
> +			     spacc->config.big_endian);

[Severity: Medium]
Will this unaligned stack buffer trigger unaligned access kernel panics?

A local stack buffer is declared as unsigned char buf[300], providing only
a 1-byte alignment guarantee. This byte array is passed to pdu_to_dev_s()
and pdu_from_dev_s(), which cast the unsigned char * directly to u32 * and
perform raw 32-bit word dereferences (e.g., *((u32 *)src)).

On strict-alignment architectures, this violates alignment requirements and
triggers unaligned access kernel panics.

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