Re: [PATCH 2/3] spi: amlogic: spisg: Fix the incorrect keep_ss of the last descriptor

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

Pre-existing issues:
- [Critical] Missing hardware abort on SPI transfer timeout leads to DMA Use-After-Free and memory corruption.
- [High] 32-bit integer overflow in `spi_delay_to_sclk` leads to severely truncated SPI delays.
- [High] `SPISG_BLOCK_MAX` exceeds the hardware mask size, causing maximum-length transfers to be processed as zero-length.
- [High] Hardware limits for `cs_setup` and `cs_hold` delays are not clamped before being packed into register fields, causing long delays to silently wrap around to short values.
--

--- Patch [2]: [PATCH 2/3] spi: amlogic: spisg: Fix the incorrect keep_ss of the last descriptor ---
commit 07f3c7893cd8bdd39413002b5f617a22e93e0842
Author: Sunny Luo <[email protected]>

spi: amlogic: spisg: Fix the incorrect keep_ss of the last descriptor

This patch fixes an issue where the CFG_KEEP_SS flag is unconditionally
cleared on the last SPI descriptor. It records the cs_change value of
the last transfer and properly programs CFG_KEEP_SS on the final descriptor.
When a null descriptor is used for cs-hold, the flag is kept on the
preceding descriptor and the recorded value is applied at the end.

> diff --git a/drivers/spi/spi-amlogic-spisg.c b/drivers/spi/spi-amlogic-spisg.c
> index afc8af04638d3..0f026d3e43e0b 100644
> --- a/drivers/spi/spi-amlogic-spisg.c
> +++ b/drivers/spi/spi-amlogic-spisg.c
> @@ -529,9 +530,11 @@ static int aml_spisg_transfer_one_message(struct spi_controller *ctlr,
>  				spi_delay_to_sclk(xfer->effective_speed_hz, &msg->spi->cs_setup));

[Severity: High]
This isn't a bug introduced by this patch, but can the 32-bit multiplication
in spi_delay_to_sclk() overflow here?

If a requested delay in nanoseconds and the SPI clock frequency in Hz are
multiplied together, it evaluates as a 32-bit unsigned operation before being
cast to 64-bit for the division:

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

For example, at a 100MHz clock, a delay greater than 42.9us will wrap around,
causing hardware setup or hold delays to become extremely short or zero.

[Severity: High]
This is also a pre-existing issue, but are the hardware limits for cs_setup
and cs_hold delays clamped before being packed into the register fields?

CFG_CS_SETUP is a 4-bit field (max 15). If spi_delay_to_sclk() returns
a value greater than 15, FIELD_PREP() will silently truncate the upper
bits, resulting in an incorrect, drastically shorter delay.

>  		/* calculate cs-hold delay with the last xfer speed */
> -		if (list_is_last(&xfer->transfer_list, &msg->transfers))
> +		if (list_is_last(&xfer->transfer_list, &msg->transfers)) {
>  			cs_hold_in_sclk =
>  				spi_delay_to_sclk(xfer->effective_speed_hz, &msg->spi->cs_hold);
> +			last_xfer_keep_ss = xfer->cs_change;
> +		}

[ ... ]

[Severity: Critical]
This is a pre-existing issue, but does the error handling path for timeouts in
aml_spisg_transfer_one_message() lack a hardware abort?

If a hardware transfer times out, the driver unmaps DMA buffers and frees the
transfer descriptors without stopping the SPI controller hardware:

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:
    ...
}

Could this lead to the hardware continuing to write to or read from freed memory
pages (DMA Use-After-Free) since the hardware isn't stopped?

[Severity: High]
This is a pre-existing issue, but can the maximum blocks count exceed the
hardware mask size in aml_spisg_setup_transfer()?

The blocks count is capped at SPISG_BLOCK_MAX (0x100000):

drivers/spi/spi-amlogic-spisg.c:aml_spisg_setup_transfer() {
    ...
		blocks = min_t(int, blocks, SPISG_BLOCK_MAX);
		desc->cfg_start |= FIELD_PREP(CFG_BLOCK_SIZE, block_size & 0x7) |
				   FIELD_PREP(CFG_BLOCK_NUM, blocks);
    ...
}

Since CFG_BLOCK_NUM uses a 20-bit mask (GENMASK(19, 0)), a transfer bounded
exactly at SPISG_BLOCK_MAX (0x100000) will be silently zeroed out when masked,
causing it to be processed as a zero-length transfer.

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

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