Re: [PATCH v15 3/4] crypto: spacc - Add SPAcc AUTODETECT Support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Critical] Abandoned autodetect jobs leave the SPAcc hardware engine performing active DMA operations into freed or reused memory buffers.
- [High] spacc_autodetect() ignores fatal error codes (-EINTR, -ETIMEDOUT) and unconditionally returns 0, falsely reporting success on failures.

Pre-existing issues:
- [High] Error paths in `spacc_open` increment `spacc->ctx_sem` incorrectly (sometimes twice), permanently artificially inflating the hardware context count.
--

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

crypto: spacc - Add SPAcc AUTODETECT Support

SPAcc is configurable and it supports the below modes:
1. AUTODETECT configuration - Autodetects the supported algos.
2. Static configuration - The algo support is defined statically.

> diff --git a/drivers/crypto/dwc-spacc/spacc_core.c b/drivers/crypto/dwc-spacc/spacc_core.c
> index 68a309ee10d47..3e50488afeccd 100644
> --- a/drivers/crypto/dwc-spacc/spacc_core.c
> +++ b/drivers/crypto/dwc-spacc/spacc_core.c
[ ... ]
> @@ -557,6 +1460,311 @@ int spacc_close(struct spacc_device *dev, int handle)
[ ... ]
> +static int spacc_check_modes(struct spacc_device *spacc, int algo_mode,
> +			      int keysz_idx, void *virt, char *key,
> +			      struct pdu_ddt *ddt)
> +{
[ ... ]
> +	rc = spacc_open(spacc, enc, hash, -1, 0, NULL, NULL);

[Severity: High]
This is a pre-existing issue, but calling spacc_open() here can trigger a
double semaphore increment on its error path. If spacc_open() encounters an
unsupported mode, it jumps to err_release_job which calls
spacc_job_release(). Since spacc_job_release() internally calls
spacc_ctx_release() and issues an up() on the semaphore, falling through
to err_release_sem will execute a second up() on ctx_sem:

spacc_open() {
    ...
err_release_job:
    spacc_job_release(spacc, job_idx);
err_release_sem:
    if (!ctx_reused)
        up(&spacc->ctx_sem);
}

Could this double increment permanently inflate the context count and
eventually lead to concurrent allocations of the same hardware slot?

> +	if (rc < 0) {
> +		spacc->config.modes[algo_mode] &= ~(1 << keysz_idx);
> +		return 0;
> +	}
[ ... ]
> +	if (err == 0) {
> +		long ret;
> +
> +		ret = wait_event_interruptible_timeout(job->waitq,
> +				job->job_done,
> +				msecs_to_jiffies(SPACC_AUTODETECT_TIMEOUT_MS));
> +		if (ret == 0) {
> +			/* no response; caller disables the whole mode */
> +			spacc_close(spacc, rc);
> +			return -ETIMEDOUT;
> +		} else if (ret == -ERESTARTSYS) {
> +			/* load interrupted; caller aborts and fails probe */
> +			spacc_close(spacc, rc);
> +			return -EINTR;
> +		}

[Severity: Critical]
If wait_event_interruptible_timeout() times out or is interrupted, the
job is abandoned and we return here, but spacc_close() only clears the
software state. Is there a risk that the hardware engine continues
performing DMA into the buffers, leading to memory corruption when the
caller immediately frees the ddt and virt buffers?

> +
> +		job->job_done = 0;
> +		err = job->job_err;
> +	}
[ ... ]
> +int spacc_autodetect(struct spacc_device *spacc)
> +{
[ ... ]
> +			if (ret == -ETIMEDOUT) {
> +				/*
> +				 * No response for this algorithm. Disable the
> +				 * whole mode (never ship a half-validated algo)
> +				 * and move on. Several timeouts in a row mean
> +				 * the engine is dead: fail loudly rather than
> +				 * silently registering a crippled device.
> +				 */
> +				spacc->config.modes[x] = 0;
> +
> +				if (++timeouts
> +					>= SPACC_AUTODETECT_MAX_TIMEOUTS) {
> +					dev_err(spacc->dptr,
> +						"SPAcc unresponsive: timeout");
> +					err = -ETIMEDOUT;
> +					goto out_free_dma;
> +				}
> +
> +				break;	/* skip this mode's other keysizes */
> +			}
> +
> +			/* engine answered — reset the dead-engine streak */
> +			timeouts = 0;
> +		}
> +	}
> +
> +out_free_dma:
> +	pdu_ddt_free(&ddt);
> +	dma_free_coherent(get_ddt_device(),
> +			SPACC_TEST_DMA_BUFF_SIZE, virt, dma);
> +	spacc->autodetect = false;
> +
> +	return 0;
> +}

[Severity: High]
When returning from an error path via the out_free_dma label, does this
unconditionally return 0 and drop the recorded error code? It looks like
failures from spacc_check_modes() might be silently swallowed here,
causing the driver probe to report success even when hardware
initialization fails.

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