Re: [PATCH v23 3/4] crypto: spacc - add spacc autodetect support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Use of `wait_event_killable` contradicts safety assumptions, allowing fatal signals to cause DMA Use-After-Free
--

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

crypto: spacc - add spacc autodetect support

This commit adds autodetection support to the SPAcc crypto driver. It dynamically 
tests which algorithms the hardware supports by issuing test operations against 
the hardware during initialization.

> 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
[ ... ]
> @@ -579,6 +1480,276 @@ 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)
> +{
[ ... ]
> +	if (err == 0) {
> +		/*
> +		 * No timeout, not interruptible: this only returns once
> +		 * job_done is actually true, i.e. the hardware has
> +		 * genuinely relinquished the job. That guarantees it is
> +		 * always safe to close the job/context and (via the
> +		 * caller) free the DMA buffer immediately below --
> +		 * there is no "unconfirmed" case left to handle.
> +		 *
> +		 * Tradeoff: if the hardware ever genuinely fails to
> +		 * complete a submitted job, this hangs here
> +		 * unkillably (TASK_UNINTERRUPTIBLE, no timeout) for the
> +		 * rest of probe. Accepted deliberately in place of the
> +		 * abandoned-job/UAF risk of giving up early with no
> +		 * hardware cancel available.
> +		 */
> +		wait_event_killable(job->waitq, job->job_done);

[Severity: High]
The comment explicitly states this wait should be "not interruptible" and
uses TASK_UNINTERRUPTIBLE to prevent abandoned-job and use-after-free
risks. However, wait_event_killable() can be interrupted by a fatal
signal.

If a fatal signal (like SIGKILL) is delivered, won't this return early
without checking the return value? Since the hardware is still processing,
does returning here allow the caller to prematurely free the DMA buffers?

> +
> +		job->job_done = 0;
> +		err = job->job_err;
> +	}
[ ... ]
> +int spacc_autodetect(struct spacc_device *spacc)
> +{
[ ... ]
> +		for (y = 0; y < ARRAY_SIZE(keysizes[0]); y++) {
> +			/*
> +			 * spacc_check_modes() now always returns 0 --
> +			 * it only comes back once the job is confirmed
> +			 * done, closes it itself, and disables the mode
> +			 * bit internally on any failure/mismatch. Nothing
> +			 * left for this loop to branch on.
> +			 */
> +			spacc_check_modes(spacc, x, y, virt, key, &ddt);
> +		}
> +
> +		if (!(spacc->config.modes[x] & 0x7F))
> +			spacc->config.modes[x] &= ~128;
> +	}
> +
> +	pdu_ddt_free(&ddt);
> +	dma_free_coherent(spacc->dptr, SPACC_TEST_DMA_BUFF_SIZE, virt, dma);

[Severity: High]
Following the potential early return from wait_event_killable() in
spacc_check_modes() above, these DMA buffers will be freed here.

Will this cause a use-after-free and DMA corruption when the hardware
eventually completes the abandoned job and writes to this freed memory?

> +	spacc->autodetect = false;
> +
> +	return 0;
> +}

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