Re: [PATCH 3/3] spi: amlogic: spisg: Add support for A9 controller features

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The calculated CFG_CS_HOLD delay value lacks bounds checking before being written via FIELD_MODIFY, leading to silent bitwise truncation and incorrectly shortened hold times.

Pre-existing issues:
- [High] The driver fails to abort the hardware operation when a transfer times out, leading to DMA use-after-free and cascading synchronization failures.
- [High] `spi_delay_to_sclk()` suffers from a 32-bit integer overflow when calculating delay cycles, resulting in drastically shorter setup and hold times than requested.
--

--- Patch [3]: [PATCH 3/3] spi: amlogic: spisg: Add support for A9 controller features ---
commit 850f7cb5955f8bd514dfd78b459ee02b15378db4
Author: Xianwei Zhao <[email protected]>

spi: amlogic: spisg: Add support for A9 controller features
    
The Amlogic A9 SPISG controller extends the A4 controller with additional
configuration options. Add SoC-specific capability data and configure
these features when supported by the hardware, while keeping compatibility
with existing A4 controllers.

> diff --git a/drivers/spi/spi-amlogic-spisg.c b/drivers/spi/spi-amlogic-spisg.c
> index 0f026d3e43e0b..845eb81d483a3 100644
> --- a/drivers/spi/spi-amlogic-spisg.c
> +++ b/drivers/spi/spi-amlogic-spisg.c
[ ... ]
> @@ -525,9 +541,17 @@ static int aml_spisg_transfer_one_message(struct spi_controller *ctlr,
>  		}
>  
>  		/* calculate cs-setup delay with the first xfer speed */
> -		if (list_is_first(&xfer->transfer_list, &msg->transfers))
> -			desc->cfg_bus |= FIELD_PREP(CFG_CS_SETUP,
> -				spi_delay_to_sclk(xfer->effective_speed_hz, &msg->spi->cs_setup));
> +		if (list_is_first(&xfer->transfer_list, &msg->transfers)) {
> +			val = spi_delay_to_sclk(xfer->effective_speed_hz, &msg->spi->cs_setup);

[Severity: High]
This is a pre-existing issue, but can spi_delay_to_sclk() suffer from a
32-bit integer overflow when calculating delay cycles?

Looking at spi_delay_to_sclk():

drivers/spi/spi-amlogic-spisg.c:spi_delay_to_sclk() {
    ...
    return DIV_ROUND_UP_ULL(slck_speed_hz * ns, NSEC_PER_SEC);
}

If a client requests a large delay, the multiplication of the two 32-bit
integers (slck_speed_hz and ns) can wrap around before being cast to a
64-bit integer by DIV_ROUND_UP_ULL.

Could this result in drastically shorter setup and hold times than
requested?

> +			if (data && data->cs_setup_extend_ctrl) {
> +				val = min_t(u32, 0xFF, val);
> +				desc->cfg_bus |= FIELD_PREP(CFG_CS_SETUP, val & 0xF);
> +				FIELD_MODIFY(CFG_CS_SETUP_EXTEND, &spisg->cfg_spi, val >> 4);
> +			} else {
> +				val = min_t(u32, 0xF, val);
> +				desc->cfg_bus |= FIELD_PREP(CFG_CS_SETUP, val);
> +			}
> +		}
>  
>  		/* calculate cs-hold delay with the last xfer speed */
>  		if (list_is_last(&xfer->transfer_list, &msg->transfers)) {
> @@ -542,7 +566,12 @@ static int aml_spisg_transfer_one_message(struct spi_controller *ctlr,
>  				       xfer->effective_speed_hz);
>  	}
>  
> -	if (cs_hold_in_sclk) {
> +	if (data && data->cs_hold_ctrl) {
> +		cs_hold_in_sclk = cs_hold_in_sclk ? : 1;
> +		val = cs_hold_in_sclk * (FIELD_GET(CFG_CLK_DIV, spisg->cfg_bus) + 1);
> +		FIELD_MODIFY(CFG_CS_HOLD, &spisg->cfg_spi, val);

[Severity: High]
Does the calculated CFG_CS_HOLD delay value lack bounds checking before
being written via FIELD_MODIFY?

Unlike CFG_CS_SETUP which safely uses min_t(), cs_hold is left unbounded.
Since CFG_CS_HOLD is a 15-bit field, if an SPI client requests a large CS
hold time, the FIELD_MODIFY macro will silently apply a bitwise mask.

Will this cause the delay to wrap around (e.g. 32768 becomes 0) rather
than clamping at the maximum supported hardware limit, leading to hardware
timing violations?

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for the
timeout error path to free DMA-mapped memory without halting the active
hardware?

Looking further down in aml_spisg_transfer_one_message():

drivers/spi/spi-amlogic-spisg.c:aml_spisg_transfer_one_message() {
    ...
    if (wait_for_completion_timeout(&spisg->completion, ...))
        ret = spisg->status ? -EIO : 0;
    else
        ret = -ETIMEDOUT;

    dma_unmap_single(dev, descs_paddr, descs_len, DMA_TO_DEVICE);
end:
    ...
    kfree(descs);
    ...
}

If an SPI transfer times out and wait_for_completion_timeout() expires,
the code proceeds directly to unmap the DMA memory and free the descriptor
array. However, the hardware is never halted or reset (e.g. by writing 0
to the descriptor list register).

Might the hardware still be actively performing DMA, resulting in memory
corruption from a use-after-free? Furthermore, if the hardware eventually
completes, could the delayed interrupt falsely wake up subsequent transfers?

> +		desc--;
> +	} else if (cs_hold_in_sclk) {
>  		/* additional null-descriptor to achieve the cs-hold delay */
>  		aml_spisg_setup_null_desc(spisg, desc, cs_hold_in_sclk);
>  		desc--;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic