Re: [PATCH v17 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:
- [High] Unkillable wait_event() in device probe can permanently hang the system if an error path is taken.
- [High] Missing test vectors for 128-byte HMAC keys silently disable previously supported algorithms.
- [High] testdata for CRYPTO_MODE_HMAC_SM3 contains identical test vectors for 64-byte and 128-byte keys, validating non-compliant hardware behavior.
--

--- Patch [3]: [PATCH v17 3/4] crypto: spacc - Add SPAcc AUTODETECT Support ---
commit e7cc55bd58c9e6a7c0c33295410fbd1d815c4aec
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
> --- a/drivers/crypto/dwc-spacc/spacc_core.c
> +++ b/drivers/crypto/dwc-spacc/spacc_core.c
[ ... ]
> +	[CRYPTO_MODE_HMAC_MD5] = {
> +		.min_version = 0x65,
> +		.test[1].outlen = 16, .test[1].data = { 0xb6, 0x39, 0xc8, 0x73,
> +			0x16, 0x38, 0x61, 0x8b, 0x70, 0x79, 0x72, 0xaa, 0x6e,
> +			0x96, 0xcf, 0x90,  },
> +		.test[4].outlen = 16, .test[4].data = { 0xb7, 0x79, 0x68, 0xea,
> +			0x17, 0x32, 0x1e, 0x32, 0x13, 0x90, 0x6c, 0x2e, 0x9f,
> +			0xd5, 0xc8, 0xb3,  },
> +		.test[5].outlen = 16, .test[5].data = { 0x80, 0x3e, 0x0a, 0x2f,
> +			0x8a, 0xd8, 0x31, 0x8f, 0x8e, 0x12, 0x28, 0x86, 0x22,
> +			0x59, 0x6b, 0x05,  },
> +	},

[Severity: High]
Does this test data structure omit test vectors for 128-byte keys (test[6])?

For algorithms like CRYPTO_MODE_HMAC_MD5, CRYPTO_MODE_HMAC_SHA1,
CRYPTO_MODE_HMAC_SHA224, and CRYPTO_MODE_HMAC_SHA256, the lack of a test[6]
entry means outlen evaluates to 0. 

Later in spacc_check_modes(), an outlen of 0 causes the driver to silently
disable support for that key size. This might inadvertently remove previously
supported 128-byte keys for these algorithms.

[ ... ]
> +	[CRYPTO_MODE_HMAC_SM3] = {
> +		.min_version = 0x65,
> +		.test[1].outlen = 32, .test[1].data = { 0x68, 0xf0, 0x65,
> +			0xd8, 0xd8, 0xc9, 0xc2, 0x0e, 0x10, 0xfd, 0x52, 0x7c,
> +			0xf2, 0xd7, 0x42, 0xd3,
> +			0x08, 0x44, 0x22, 0xbc, 0xf0, 0x9d, 0xcc, 0x34, 0x7b,
> +			0x76, 0x13, 0x91, 0xba, 0xce, 0x4d, 0x17 },
> +		.test[4].outlen = 32, .test[4].data = { 0xd8, 0xab, 0x2a,
> +			0x7b, 0x56, 0x21, 0xb1, 0x59, 0x64, 0xb2, 0xa3, 0xd6,
> +			0x72, 0xb3, 0x95, 0x81,
> +			0xa0, 0xcd, 0x96, 0x47, 0xf0, 0xbc, 0x8c, 0x16, 0x5b,
> +			0x9b, 0x7d, 0x2f, 0x71, 0x3f, 0x23, 0x19 },
> +		.test[5].outlen = 32, .test[5].data = { 0xa0, 0xd1, 0xd5,
> +			0xa0, 0x9e, 0x4c, 0xca, 0x8c, 0x7b, 0xe0, 0x8f, 0x70,
> +			0x92, 0x2e, 0x3f, 0x4c,
> +			0xa0, 0xca, 0xef, 0xa1, 0x86, 0x9d, 0xb2, 0xe1, 0xc5,
> +			0xfa, 0x9d, 0xfa, 0xbc, 0x11, 0xcb, 0x1f },
> +		.test[6].outlen = 32, .test[6].data = { 0xa0, 0xd1, 0xd5,
> +			0xa0, 0x9e, 0x4c, 0xca, 0x8c, 0x7b, 0xe0, 0x8f, 0x70,
> +			0x92, 0x2e, 0x3f, 0x4c,
> +			0xa0, 0xca, 0xef, 0xa1, 0x86, 0x9d, 0xb2, 0xe1, 0xc5,
> +			0xfa, 0x9d, 0xfa, 0xbc, 0x11, 0xcb, 0x1f }
> +	},

[Severity: High]
Are these test vectors for 64-byte (test[5]) and 128-byte (test[6]) keys
expected to produce the exact same MAC output?

Since HMAC_SM3 has a block size of 64 bytes, keys longer than 64 bytes are
supposed to be hashed to 32 bytes before being used. Producing the same MAC
for both might mean the hardware is incorrectly truncating the 128-byte key
instead of hashing it. Passing validation with these vectors could permit
broken cryptographic behavior to be used in production.

[ ... ]
> +	err = spacc_packet_enqueue_ddt(spacc, rc, ddt, ddt, proclen, 0,
> +				       aadlen, 0, 0, 0);
> +	job = &spacc->job[rc];
> +
> +	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(job->waitq, job->job_done);

[Severity: High]
Could this wait_event() cause the system to hang permanently during probe?

The wait_event() macro places the task in TASK_UNINTERRUPTIBLE. However, if
the job is deferred and subsequently fails to be dispatched to hardware
(for example, due to a DMA error in spacc_process_jb()), the error path sets
job_err and calls wake_up_interruptible(&job->waitq).

Since wake_up_interruptible() only wakes tasks in TASK_INTERRUPTIBLE, this
thread would never wake up from the error path, stranding it forever without
a timeout.

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