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

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

Pre-existing issues:
- [High] Timeout completion race leading to cross-message corruption and potential DMA UAF.
- [High] PM runtime usage counter leak and missing disable on probe error and module remove.
- [High] Driver erroneously assumes DMA address 0 is invalid for mapping tracking.
- [High] The runtime PM resume callback ignores clock enablement errors.
--

--- Patch [4]: [PATCH v2 4/4] spi: amlogic: spisg: Add support for A9 controller features ---
commit 733f8e986a88161245fdf261af60ee5f16e4648c
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 they are supported by the underlying hardware.

> diff --git a/drivers/spi/spi-amlogic-spisg.c b/drivers/spi/spi-amlogic-spisg.c
> index 2d66f00dccd45..bc9564aef2452 100644
> --- a/drivers/spi/spi-amlogic-spisg.c
> +++ b/drivers/spi/spi-amlogic-spisg.c

[ ... ]

> @@ -542,7 +566,13 @@ 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);
> +		val = min_t(u32, 0x7FFF, val);
> +		FIELD_MODIFY(CFG_CS_HOLD, &spisg->cfg_spi, val);
> +		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--;

[Severity: High]
This is a pre-existing issue, but does aml_spisg_transfer_one_message() have a
timeout completion race that could lead to DMA use-after-free?

If wait_for_completion_timeout() expires, the driver attempts to stop the
transfer by writing 0 to SPISG_REG_DESC_LIST_H:

	if (wait_for_completion_timeout(&spisg->completion, ...)) {
		ret = spisg->status ? -EIO : 0;
	} else {
		/* stop transfer */
		regmap_write(spisg->map, SPISG_REG_DESC_LIST_H, 0);
		ret = -ETIMEDOUT;
	}

Since it fails to disable the hardware interrupt or synchronize with the IRQ
handler via synchronize_irq(), could a delayed hardware interrupt call
complete(&spisg->completion) slightly late?

If this happens during or just after the next SPI message calls
reinit_completion(), it would immediately satisfy the new message's
completion wait, unmapping the DMA buffers while the hardware is actively
using them.

[Severity: High]
This isn't a bug introduced by this patch, but does this driver erroneously
assume DMA address 0 is invalid for mapping tracking?

In aml_spisg_setup_transfer() and aml_spisg_cleanup_transfer(), the code checks
if the mapped DMA address is zero to determine if a mapping exists:

	if (desc->tx_paddr) {
		if (FIELD_GET(CFG_TXD_MODE, desc->cfg_start) == SPISG_DATA_MODE_SG) {
			dma_unmap_single(dev, (dma_addr_t)desc->tx_paddr,
					 exdesc->tx_ccsg_len, DMA_TO_DEVICE);
			kfree(exdesc->tx_ccsg);

Since 0 is a valid DMA address on some platforms, if dma_map_single()
successfully maps a buffer to address 0, would this skip the unmap and kfree
calls, leaking memory and the DMA mapping? Should this use a separate boolean
flag like tx_mapped instead of testing the DMA address value?

[ ... ]

> @@ -773,6 +805,21 @@ static int aml_spisg_probe(struct platform_device *pdev)
>  
>  	spisg->cfg_spi = FIELD_PREP(CFG_SFLASH_WP, 1) |
>  			 FIELD_PREP(CFG_SFLASH_HD, 1);
> +
> +	if (spisg->data && spisg->data->mo_idle_output_ctrl) {
> +		if (!of_property_read_u32(dev->of_node, "amlogic,mo-idle-output", &val))
> +			spisg->cfg_spi |=  FIELD_PREP(CFG_MO_IDLE_OUTPUT, val);
> +		else
> +			spisg->cfg_spi |=  FIELD_PREP(CFG_MO_IDLE_OUTPUT, 0);
> +	}
> +
> +	if (spisg->data && spisg->data->word_gap_ctrl) {
> +		if (!of_property_read_u32(dev->of_node, "amlogic,word-gap", &val))
> +			spisg->cfg_spi |=  FIELD_PREP(CFG_WORD_GAP, val);
> +		else
> +			spisg->cfg_spi |=  FIELD_PREP(CFG_WORD_GAP, 1);
> +	}
> +
>  	if (spi_controller_is_target(ctlr)) {
>  		spisg->cfg_spi |= FIELD_PREP(CFG_SLAVE_EN, 1);
>  		spisg->cfg_bus = FIELD_PREP(CFG_TX_TUNING, 0xf);

[Severity: High]
This isn't a bug introduced by this patch, but does aml_spisg_probe() leak
the PM runtime usage counter on error paths?

The driver enables PM runtime and increments the usage counter:

	pm_runtime_set_active(&spisg->pdev->dev);
	pm_runtime_enable(&spisg->pdev->dev);
	pm_runtime_resume_and_get(&spisg->pdev->dev);

If a subsequent operation like devm_request_irq() fails, the code branches to
out_clk and returns without decrementing the usage counter or disabling PM
runtime. 

Additionally, aml_spisg_remove() does not appear to call pm_runtime_disable(),
which could leave the device dangling in the PM runtime core after module
unload.

[ ... ]

> @@ -859,6 +906,13 @@ static int spisg_resume_runtime(struct device *dev)
>  	return 0;
>  }
>  
> +static const struct aml_spisg_data a9_spisg_data = {
> +	.mo_idle_output_ctrl = true,
> +	.word_gap_ctrl = true,
> +	.cs_hold_ctrl = true,
> +	.cs_setup_extend_ctrl = true,
> +};
> +

[Severity: High]
This is a pre-existing issue, but does spisg_resume_runtime() ignore clock
enablement errors?

In spisg_resume_runtime(), the driver calls:

	clk_prepare_enable(spisg->core);
	clk_prepare_enable(spisg->sclk);

If clock enablement fails, the function still returns 0 (success). Could this
trick the PM runtime core into believing the device is active, causing
subsequent MMIO register accesses to hang the bus or crash the system since
the clock domain is off?

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

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