Re: [PATCH v16 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:
- [Critical] Missing hardware job cancellation on timeout or interrupt during SPAcc autodetect causes DMA use-after-free, stack use-after-free, and data corruption.
- [High] Abandoned jobs from autodetect cause `spacc_pop_packets_ex` to abort, hanging the STAT FIFO and blocking subsequent jobs.
- [Medium] Autodetect test vector failures do not fully clear the 'hash flag' or override specific ciphers, causing broken algorithms to be falsely registered.
--

--- Patch [3]: [PATCH v16 3/4] crypto: spacc - Add SPAcc AUTODETECT Support ---
commit ecbce57497637f8381b32aed9f07d6c23e608ed7
Author: Pavitrakumar Managutte <pavitrakumarm@vayavyalabs.com>
crypto: spacc - Add SPAcc AUTODETECT Support

This commit introduces an autodetect configuration mode for the SPAcc
crypto driver to dynamically discover supported algorithms.

> 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

[ ... ]

> +	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;

[Severity: High]
When a job times out, it is abandoned and spacc_close() marks its slot as
SPACC_JOB_IDX_UNUSED. If the hardware eventually completes this abandoned job
and the STAT IRQ fires, will spacc_pop_packets_ex() encounter this unused
slot, execute goto ERR, and skip processing the rest of the completed jobs
in the STAT FIFO? Could this cause remaining jobs to get permanently stuck?

> +		} else if (ret == -ERESTARTSYS) {
> +			/* load interrupted; caller aborts and fails probe */
> +			spacc_close(spacc, rc);
> +			return -EINTR;

[Severity: Critical]
If the wait is interrupted (e.g. via a signal) or times out, the software
state is closed and the DMA buffers are immediately freed by the caller in
spacc_autodetect().

Because the hardware crypto engine is not explicitly instructed to abort
the job during spacc_close(), will it continue executing and asynchronously
write its result to the newly freed DMA memory?

> +		}
> +
> +		job->job_done = 0;
> +		err = job->job_err;
> +	}
> +
> +	output_zero_len = !testdata[algo_mode].test[keysz_idx].outlen;
> +	output_mismatch = memcmp(testdata[algo_mode].test[keysz_idx].data, virt,
> +			testdata[algo_mode].test[keysz_idx].outlen);
> +
> +	if (err != 0 || output_zero_len || output_mismatch)
> +		spacc->config.modes[algo_mode] &= ~(1 << keysz_idx);

[Severity: Medium]
When an autodetect test fails, this bitwise operation only clears the
specific keysize bits (0-6). Does this leave bit 7 (the hash flag, 128)
set?

If bit 7 remains set, or if the algorithm uses a hardcoded check like
CRYPTO_MODE_AES_XTS, will spacc_is_mode_keysize_supported() bypass this
failure and falsely register algorithms that the hardware does not support?

> +
> +	spacc_close(spacc, rc);
> +
> +	return 0;
> +}

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