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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 13 potential issue(s) to consider:
- [High] Crypto engine worker thread deadlocks due to context exhaustion.
- [High] Use-After-Free during device teardown due to premature algorithm deallocation.
- [High] Returning a negative error code from `do_one_request` stalls the crypto engine indefinitely.
- [High] SM4-CMAC and SM4-XCBC skip software fallback initialization and falsely advertise `CRYPTO_ALG_OPTIONAL_KEY`, leading to kernel panic.
- [High] Incorrect teardown order in `spacc_crypto_remove` causes Use-After-Free during workqueue execution.
- [High] Global DMA pools break multi-device support, causing IOMMU faults and Use-After-Free.
- [High] Shared IRQ is not freed in `spacc_init_device` error paths, causing a destroyed workqueue to execute.
- [High] Unbalanced semaphore release in `spacc_open` inflates context capacity and breaks bounds logic.
- [High] Concurrent lockless mutation of `spacc_crypto_ctx` during `setkey` causes NULL pointer dereferences in active hardware requests.
- [High] `spacc_job_release` leaves a stale job mapping, causing delayed hardware responses to falsely execute the callback of unrelated requests.
- [High] Jobs are silently discarded if buffered queue processing encounters a validation failure, permanently hanging the crypto engine.
- [Medium] Uninitialized memory read of `ctx->acb.new_handle` leading to active job corruption.
- [Medium] Software fallback truncates data and returns false success upon scatterlist length mismatch.
--

commit 5c2742d7183ca1c1ee4b0693c617c98d1a483ad4
Author: Pavitrakumar Managutte <pavitrakumarm@vayavyalabs.com>
crypto: spacc - Add SPAcc ahash support

Add ahash support to SPAcc driver.

> diff --git a/drivers/crypto/dwc-spacc/spacc_ahash.c b/drivers/crypto/dwc-spacc/spacc_ahash.c
> --- a/drivers/crypto/dwc-spacc/spacc_ahash.c
> +++ b/drivers/crypto/dwc-spacc/spacc_ahash.c
[ ... ]
> +static int spacc_hash_init_dma(struct device *dev, struct ahash_request *req)
> +{
[ ... ]
> +err_free_dst:
> +	/*
> +	 * If the handle was already cloned (semaphore count and job slot
> +	 * taken) before this failure, release it.  Clone-failure paths
> +	 * leave new_handle < 0, so the guard skips them.
> +	 */
> +	if (ctx->acb.new_handle >= 0) {
> +		spacc_close(&priv->spacc, ctx->acb.new_handle);
> +		ctx->acb.new_handle = -1;
> +	}

[Severity: Medium]
Can ctx->acb.new_handle be read uninitialized in this error path? 

If this function is called with req->nbytes > 0 but ctx->total_nents == 0
due to a scatterlist mapping failure, both initialization branches are
skipped and the function jumps to err_free_dst. The uninitialized stack or
heap memory of ctx->acb.new_handle would then be evaluated, potentially
closing an unrelated active handle if it evaluates to >= 0.

[ ... ]
> +static int spacc_hash_setkey(struct crypto_ahash *tfm, const u8 *key,
[ ... ]
> +	if (tctx->handle >= 0) {
> +		spacc_close(&priv->spacc, tctx->handle);
> +		tctx->handle = -1;
> +	}
> +
> +	if (tctx->dev) {
> +		put_device(tctx->dev);
> +		tctx->dev = NULL;
> +	}
> +
> +	tctx->ctx_valid = false;
> +	tctx->dev = get_device(salg->dev);

[Severity: High]
Is it safe to modify tctx->dev and other context fields without
synchronization here?

The crypto API permits setkey to run concurrently with do_one_request on the
same tfm. If spacc_hash_do_one_request() reads tctx->dev to extract drvdata
at the exact moment it is set to NULL here, it could result in a NULL
pointer dereference.

[ ... ]
> +static int spacc_hash_do_one_request(struct crypto_engine *engine, void *areq)
> +{
[ ... ]
> +		/*
> +		 * Open the HW context here, not in setkey.  spacc_open()
> +		 * acquires ctx_sem which may sleep; do_one_request runs in
> +		 * the crypto_engine workqueue which is a sleepable context,
> +		 * so this is safe.
> +		 */
> +		tctx->handle = spacc_open(&priv->spacc, CRYPTO_MODE_NULL,
> +				salg->mode->id, -1, 0,
> +				spacc_digest_cb, reqtfm);

[Severity: High]
Could this lead to a deadlock of the crypto engine worker thread?

When spacc_hash_do_one_request() calls spacc_open() with ctxid=-1, it
triggers down_interruptible(&spacc->ctx_sem). If userspace opens more tfm
instances than the hardware limit, the single engine worker thread will block
indefinitely on this semaphore, completely stalling all crypto operations.

[ ... ]
> +	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_free_mems(ctx, tctx, req);
> +
> +		dma_pool_free(priv->hash_pool, ctx->digest_buf,
> +					ctx->digest_dma);
> +		pdu_ddt_free(&ctx->dst);
> +
> +		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 returning an error directly here stall the crypto engine indefinitely?

Since the engine was initialized with retry_support = false, returning a
negative error code directly without calling crypto_finalize_hash_request()
will fail to clear engine->cur_req. The engine pump loop then permanently
aborts because engine->cur_req remains populated.

[ ... ]
> +fallback:
> +	kfree(tctx->tmp_sgl);
> +	tctx->tmp_sgl = NULL;
> +
> +	if (tctx->shash_fb) {
> +		SHASH_DESC_ON_STACK(desc, tctx->shash_fb);
> +
> +		desc->tfm = tctx->shash_fb;
> +
> +		rc = crypto_shash_init(desc);
> +		if (!rc) {
> +			struct sg_mapping_iter miter;
> +			unsigned int remaining = req->nbytes;
> +
> +			sg_miter_start(&miter, req->src,
> +				       sg_nents_for_len(req->src, req->nbytes),
> +				       SG_MITER_ATOMIC | SG_MITER_FROM_SG);

[Severity: Medium]
How does this handle a mismatch in scatterlist lengths? 

If req->nbytes exceeds the actual sum of the scatterlist lengths,
sg_nents_for_len() returns -EINVAL. This negative error code is implicitly
cast to an unsigned integer in sg_miter_start(), causing the miter to
iterate to the end of the available data and the software fallback to
falsely return success for a partial MAC.

[ ... ]
> +static int spacc_hash_init_tfm(struct crypto_ahash *tfm)
> +{
[ ... ]
> +	if (salg->mode->id == CRYPTO_MODE_MAC_CMAC ||
> +	    salg->mode->id == CRYPTO_MODE_MAC_XCBC) {
> +		tctx->shash_fb = crypto_alloc_shash(
> +				salg->alg.hash.base.halg.base.cra_name, 0, 0);

[Severity: High]
Do SM4-CMAC and SM4-XCBC properly initialize their software fallbacks?

This condition only checks for AES MACs, meaning spacc_hash_init_tfm() does
not allocate tctx->shash_fb for SM4 MACs. If an unkeyed MAC is requested,
it falls back to the generic HASH_FBREQ_ON_STACK. Because the fallback TFM is
NULL, this triggers a guaranteed NULL pointer dereference.

[ ... ]
> +static int spacc_register_hash(struct spacc_alg *salg)
> +{
[ ... ]
> +	if (salg->mode->id == CRYPTO_MODE_MAC_CMAC ||
> +	    salg->mode->id == CRYPTO_MODE_MAC_XCBC) {
> +		salg->alg.hash.base.halg.base.cra_flags =
> +			CRYPTO_ALG_TYPE_AHASH | CRYPTO_ALG_ASYNC |
> +			CRYPTO_ALG_NO_FALLBACK;
> +	} else {
> +		salg->alg.hash.base.halg.base.cra_flags =
> +			CRYPTO_ALG_TYPE_AHASH | CRYPTO_ALG_ASYNC |
> +			CRYPTO_ALG_OPTIONAL_KEY;
> +	}

[Severity: High]
Similar to the fallback allocation above, SM4 MACs fall into the else branch
here, falsely advertising CRYPTO_ALG_OPTIONAL_KEY despite requiring a key
and missing their software fallback initialization.

[ ... ]
> +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);
> +		kfree(salg);
> +	}

[Severity: High]
Is it safe to unconditionally kfree(salg) here?

While crypto_engine_unregister_ahash() removes the algorithm from the
registry, any currently open TFMs will continue to retain a pointer to the
freed salg memory. This results in a use-after-free when the device unbinds.

> diff --git a/drivers/crypto/dwc-spacc/spacc_core.c b/drivers/crypto/dwc-spacc/spacc_core.c
> --- a/drivers/crypto/dwc-spacc/spacc_core.c
> +++ b/drivers/crypto/dwc-spacc/spacc_core.c
[ ... ]
> +int spacc_open(struct spacc_device *spacc, int enc, int hash, int ctxid,
> +	       int secure_mode, spacc_callback cb, void *cbdata)
> +{
[ ... ]
> +	if (ctxid == -1) {
> +		if (down_interruptible(&spacc->ctx_sem)) {
> +			dev_dbg(spacc->dptr, "ERR: Interrupted by signal\n");
> +			return -ERESTARTSYS;
> +		}
> +	}
> +
> +	spin_lock_irqsave(&spacc->ctx_lock, flags);
> +	job_idx = spacc_job_request(spacc, ctxid);
> +
> +	if (job_idx < 0) {
> +		spin_unlock_irqrestore(&spacc->ctx_lock, flags);
> +		dev_dbg(spacc->dptr, "Failed to acquire context\n");
> +		ret = -EIO;
> +		goto err_release_sem;
> +	}
[ ... ]
> +err_release_job:
> +	spacc_job_release(spacc, job_idx);
> +err_release_sem:
> +	if (!ctx_reused)
> +		up(&spacc->ctx_sem);
> +
> +	return ret;
> +}

[Severity: High]
Can this error path result in an unbalanced semaphore release?

The spacc->ctx_sem semaphore is only acquired if ctxid == -1, but the
err_release_sem path unconditionally calls up(&spacc->ctx_sem) since
ctx_reused defaults to false. This grants an extra phantom permit to the
semaphore, allowing concurrent callers to bypass context limits and access
out-of-bounds memory.

> diff --git a/drivers/crypto/dwc-spacc/spacc_device.c b/drivers/crypto/dwc-spacc/spacc_device.c
> --- a/drivers/crypto/dwc-spacc/spacc_device.c
> +++ b/drivers/crypto/dwc-spacc/spacc_device.c
[ ... ]
> +static int spacc_init_device(struct platform_device *pdev)
> +{
[ ... ]
> +	irq_ret = devm_request_irq(&pdev->dev, irq_num, spacc_irq_handler,
> +			     IRQF_SHARED, dev_name(&pdev->dev),
> +			     &pdev->dev);
> +	if (irq_ret) {
> +		dev_err(&pdev->dev, "Failed to request IRQ : %d\n", irq_ret);
> +		err = irq_ret;
> +		goto free_spacc_workq;
> +	}
[ ... ]
> +	err = spacc_autodetect(&priv->spacc);
> +	if (err < 0) {
> +		spacc_irq_glbl_disable(&priv->spacc);
> +		goto free_spacc_workq;
> +	}
[ ... ]
> +free_spacc_workq:
> +	destroy_workqueue(priv->spacc_wq);
> +
> +free_engine:
> +	crypto_engine_exit(priv->engine);

[Severity: High]
Does the error path need to manually free the shared interrupt?

The interrupt is registered as IRQF_SHARED via devm, but if initialization
fails (e.g., during spacc_autodetect), the workqueue is destroyed while the
interrupt remains active until devres cleanup runs later. If another device
triggers the shared interrupt line in this window, the handler could try to
queue work to the destroyed workqueue.

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

[Severity: High]
Is the teardown order correct here?

The driver calls spacc_unregister_algs() (which destroys priv->hash_pool)
before flushing and destroying priv->spacc_wq. If there are pending work
items in spacc_wq, spacc_pop_jobs() could run concurrently and attempt to
free memory from the already destroyed pool.

> diff --git a/drivers/crypto/dwc-spacc/spacc_hal.c b/drivers/crypto/dwc-spacc/spacc_hal.c
> --- a/drivers/crypto/dwc-spacc/spacc_hal.c
> +++ b/drivers/crypto/dwc-spacc/spacc_hal.c
[ ... ]
> +static struct dma_pool *ddt_pool, *ddt16_pool, *ddt4_pool;
> +static struct device *ddt_device;
> +
> +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);

[Severity: High]
Could the use of global static variables for the DMA pools cause issues on
systems with multiple SPAcc engines?

If multiple instances probe, they will all share the pool allocated against
the first device's struct, which violates IOMMU isolation. Additionally, if
the first device is removed, it destroys the pool for all active instances,
leading to use-after-free crashes.

> diff --git a/drivers/crypto/dwc-spacc/spacc_manager.c b/drivers/crypto/dwc-spacc/spacc_manager.c
> --- a/drivers/crypto/dwc-spacc/spacc_manager.c
> +++ b/drivers/crypto/dwc-spacc/spacc_manager.c
[ ... ]
> +int spacc_job_release(struct spacc_device *spacc, int job_idx)
> +{
[ ... ]
> +	job	      = &spacc->job[job_idx];
> +	/* release context that goes with job */
> +	ret	      = spacc_ctx_release(spacc, job->ctx_idx);
> +	job->ctx_idx  = SPACC_CTX_IDX_UNUSED;
> +	job->job_used = SPACC_JOB_IDX_UNUSED;
> +	/* disable any callback */
> +	job->cb       = NULL;
> +
> +	/* NOTE: this leaves ctrl data in memory */
> +	spin_unlock_irqrestore(&spacc->lock, lock_flag);
> +
> +	return ret;
> +}

[Severity: High]
Should spacc->job_lookup be cleared for this swid during release?

If a request is software-aborted, the job slot is freed but the swid mapping
remains. When the hardware eventually completes the orphaned job, the IRQ
handler might read this stale mapping and fire the callback of a newly
allocated, completely unrelated request.

[ ... ]
> +int spacc_process_jb(struct spacc_device *spacc)
> +{
[ ... ]
> +		if (spacc->job_buffer[tail].active) {
> +			ret = spacc_packet_enqueue_ddt_ex
> +			      (spacc, 0, spacc->job_buffer[tail].job_idx,
> +			       spacc->job_buffer[tail].src,
> +			       spacc->job_buffer[tail].dst,
> +			       spacc->job_buffer[tail].proc_sz,
> +			       spacc->job_buffer[tail].aad_offset,
> +			       spacc->job_buffer[tail].pre_aad_sz,
> +			       spacc->job_buffer[tail].post_aad_sz,
> +			       spacc->job_buffer[tail].iv_offset,
> +			       spacc->job_buffer[tail].prio);
> +
> +			if (ret != -EBUSY)
> +				spacc->job_buffer[tail].active = 0;
> +			else
> +				return -EBUSY;
> +		}

[Severity: High]
What happens to the pending request if spacc_packet_enqueue_ddt_ex() returns
an error other than -EBUSY (such as -EINVAL for LINEAR mode errors)? 

By setting active to 0 and not invoking the job's callback, this silently
drops the request and leaves the crypto engine waiting forever.

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