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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing memory barriers when locklessly communicating job completion state and error code between the interrupt handler and probe thread.
- [Medium] Valid AES algorithms (AES_CFB, AES_OFB, AES_F8) are silently disabled in autodetect mode due to missing test vectors.
--

commit 98356c1a6a36e9a2fc851d26cc76a6db9ee5514b
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 910916a1fe000..5d83ceaec4a56 100644
--- a/drivers/crypto/dwc-spacc/spacc_core.c
+++ b/drivers/crypto/dwc-spacc/spacc_core.c
[ ... ]
> 	if ((template[algo_mode] & (1 << keysz_idx)) == 0)
> 		return 0;
> 
> 	if (!testdata[algo_mode].test[keysz_idx].outlen) {
> 		spacc->config.modes[algo_mode] &= ~(1 << keysz_idx);

[Severity: Medium]
Does this unintentionally drop support for otherwise valid algorithms like
AES_CFB, AES_OFB, and AES_F8?

Because the testdata array explicitly configures these algorithms with
.test[0].outlen = 0:

	[CRYPTO_MODE_AES_CFB] = {
		.min_version = 0x65,
		.test[0].outlen = 0
	},

this check will always evaluate to true and clear their mode bits. This
causes a silent loss of functionality in autodetect mode compared to the
static configuration. Can we add the missing test vectors to prevent them
from being disabled?

> 		return 0;
> 	}
> 
> 	/*
[ ... ]
> 		 * rest of probe. Accepted deliberately in place of the
> 		 * abandoned-job/UAF risk of giving up early with no
> 		 * hardware cancel available.
> 		 */
> 		wait_event(job->waitq, job->job_done);
> 
> 		job->job_done = 0;
> 		err = job->job_err;

[Severity: High]
Does this code need memory barriers when locklessly communicating the job
completion state?

In the interrupt handler, spacc_pop_packets_ex() updates these values without
a write memory barrier:

drivers/crypto/dwc-spacc/spacc_interrupt.c:spacc_pop_packets_ex() {
    ...
			job->job_err = ret;

			if (spacc->autodetect) {
				job->job_done = 1;
				wake_up(&job->waitq);
			}
    ...
}

Because wait_event evaluates job->job_done locklessly on its fast path, can
a weakly ordered CPU (like ARM64) reorder the independent stores such that
this thread observes job_done == 1 but still reads the stale value of
job_err (e.g., -EINPROGRESS)?

Would it be safer to use smp_store_release(&job->job_done, 1) in the
interrupt handler and smp_load_acquire(&job->job_done) in the wait_event
condition?

> 	}
> 
> 	output_zero_len = !testdata[algo_mode].test[keysz_idx].outlen;

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