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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Ignoring the return value of wait_event_interruptible() leads to DMA memory Use-After-Free (UAF) if a signal interrupts the wait.
- [High] A stack out-of-bounds read occurs when configuring hash algorithms with key sizes larger than 64 bytes.
- [High] Hash autodetection unconditionally fails and incorrectly disables all hash algorithms because the hardware is instructed to output a 0-length hash.
- [High] spacc_check_modes() permanently mutates the shared 'key' array for CSHAKE/KMAC modes, corrupting the key for subsequently tested algorithms.
- [High] Unbounded wait_event_interruptible when polling hardware can permanently hang the kernel if the device stalls.
- [High] A race condition in the interrupt handler causes autodetect to read uninitialized error statuses, randomly failing valid algorithms.
- [Low] The testdata array uses sequential initialization instead of designated initializers, making it extremely fragile to enum changes.
--

commit 76505a1d1e0cde015e1c2a3d87751c32da9ef277
Author: Pavitrakumar Managutte <pavitrakumarm@vayavyalabs.com>
crypto: spacc - Add SPAcc AUTODETECT Support

This commit adds an autodetect configuration mode for SPAcc, which probes the
hardware to dynamically detect supported cryptographic algorithms based on a
statically defined list of test vectors.

> 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
> @@ -200,6 +200,882 @@ static const unsigned char template[] = {
>  	[CRYPTO_MODE_MAC_SM4_CMAC]      = 242,
>  };
>  
> +#if IS_ENABLED(CONFIG_CRYPTO_DEV_SPACC_AUTODETECT)
> +static const struct {
> +	unsigned int min_version;
> +	struct {
> +		int outlen;
> +		unsigned char data[64];
> +	} test[7];
> +} testdata[CRYPTO_MODE_LAST] = {
> +	/* NULL */
> +	{ .min_version = 0x65,
> +		.test[0].outlen = 0
> +	},

[Severity: Low]
This isn't a bug, but could this array initialization be made more robust?

Because it uses sequential initialization instead of designated initializers
for the algorithm indices, any changes to the enum order could silently
break the mapping to test vectors.

> [ ... ]
> +static void spacc_check_modes(struct spacc_device *spacc, int algo_mode,
> +			      int keysz_idx, void *virt, char *key,
> +			      struct pdu_ddt *ddt)
> +{
> [ ... ]
> +	/* if this is a hash or mac */
> +	if (template[algo_mode] & 128) {
> +		switch (algo_mode) {
> +		case CRYPTO_MODE_HASH_CSHAKE128:
> +		case CRYPTO_MODE_HASH_CSHAKE256:
> +		case CRYPTO_MODE_MAC_KMAC128:
> +		case CRYPTO_MODE_MAC_KMAC256:
> +		case CRYPTO_MODE_MAC_KMACXOF128:
> +		case CRYPTO_MODE_MAC_KMACXOF256:
> +		       /*
> +			* Special initial bytes to encode
> +			* length for cust strings
> +			*/
> +			key[0] = 0x01;
> +			key[1] = 0x70;

[Severity: High]
Will modifying this array affect subsequent algorithm checks?

Since the key array is allocated once in spacc_autodetect() and passed
by pointer in the loop, mutating it here for CSHAKE and KMAC modes
seems to leave the corrupted key in place for any algorithms tested in
later loop iterations. Could this cause them to fail verification
against their predefined test vectors?

> +			break;
> +		}
> +
> +		spacc_write_context(spacc, rc, SPACC_HASH_OPERATION,
> +				key, keysizes[1][keysz_idx] +
> +				(algo_mode == CRYPTO_MODE_MAC_XCBC ? 32 : 0),
> +				key, 16);

[Severity: High]
Is there a risk of an out-of-bounds stack read here?

The key array is statically sized to 64 bytes in spacc_autodetect().
However, for certain hash modes like CRYPTO_MODE_HASH_SHA512, the
requested read size can be up to 128 bytes. This looks like it might
copy uninitialized stack memory into the hardware context buffer and
cause the check to fail.

> [ ... ]
> +	spacc_set_operation(spacc, rc, OP_ENCRYPT, 0, IP_ICV_APPEND, 0,
> +			0, 0);

[Severity: High]
Does this correctly configure the hardware for hash algorithms?

The icvsz parameter (the 7th argument) is hardcoded to 0, which
configures the hardware to append a 0-length hash.

> [ ... ]
> +	/*
> +	 * 16AAD/16PT or 32AAD/0PT depending on
> +	 * whether we're in a hash or not mode
> +	 */
> +	aadlen  = 16;
> +	proclen = 32;
> +	if (!enc)
> +		aadlen += 16;

[Severity: High]
Additionally, proclen is left as 32 instead of being properly zeroed
for hash operations. Will these values guarantee a mismatch when
comparing the output against the expected test vectors?

> [ ... ]
> +	err = spacc_packet_enqueue_ddt(spacc, rc, ddt, ddt, proclen, 0,
> +			aadlen, 0, 0, 0);
> +
> +	job = &spacc->job[rc];
> +
> +	if (err == 0) {
> +		wait_event_interruptible(job->waitq, job->job_done);

[Severity: High]
Could this lead to a kernel hang or memory corruption if interrupted?

If a signal interrupts the wait, it returns immediately but the return
value is ignored. Once the function returns and spacc_autodetect()
frees the DMA memory, the hardware might still be actively writing to
that freed memory.

Also, without a timeout mechanism, could a hardware stall during probe
cause modprobe to hang indefinitely?

> [ ... ]
> diff --git a/drivers/crypto/dwc-spacc/spacc_interrupt.c b/drivers/crypto/dwc-spacc/spacc_interrupt.c
> --- a/drivers/crypto/dwc-spacc/spacc_interrupt.c
> +++ b/drivers/crypto/dwc-spacc/spacc_interrupt.c
> @@ -54,6 +54,10 @@
>  			/* mark job as done */
>  			if (spacc->autodetect) {
>  				job->job_done = 1;
>  				wake_up_interruptible(&job->waitq);
>  			}
> [ ... ]
> @@ -83,2 +87,2 @@
>  	job->job_err = ret;

[Severity: High]
Can this cause a race condition when waking up the autodetect thread?

The waitqueue is woken up before the hardware status is evaluated and
assigned to job->job_err.

Could the woken thread in spacc_check_modes() resume and read
job->job_err before it is updated, observe the stale -EINPROGRESS
value, and incorrectly register valid tests as failures?

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