Re: [PATCH] dmaengine: qcom: gpi: correct channel name in error path

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The error path in `gpi_ch_init()` frees the event ring DMA memory without sending the required hardware deallocation command (`GPI_EV_CMD_DEALLOC`), resulting in DMA Use-After-Free.
- [High] The software state machine (`gchan->pm_state`) is not reverted when `gpi_ch_init()` fails, leading to a double-teardown hardware timeout when the client later releases the successfully allocated sibling channel.
- [High] The event processing tasklet is not killed before freeing the event ring memory in the error path of `gpi_ch_init()`, creating a Use-After-Free vulnerability.
--

commit 40116dd1267be01863d674224210915823326daa
Author: Brian Masney <bmasney@redhat.com>

dmaengine: qcom: gpi: correct channel name in error path

This commit fixes an issue where the wrong channel was passed to
gpi_send_cmd() during the error handling path of gpi_ch_init(). Correcting
this resolves command completion timeouts when hardware cleanup occurs.

> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
> index a5055a6273af6..3f390b5821abb 100644
> --- a/drivers/dma/qcom/gpi.c
> +++ b/drivers/dma/qcom/gpi.c
> @@ -1965,12 +1965,12 @@ static int gpi_ch_init(struct gchan *gchan)
>  error_start_chan:
>  	for (i = i - 1; i >= 0; i--) {
>  		gpi_stop_chan(&gpii->gchan[i]);
> -		gpi_send_cmd(gpii, gchan, GPI_CH_CMD_RESET);
> +		gpi_send_cmd(gpii, &gpii->gchan[i], GPI_CH_CMD_RESET);
>  	}
>  	i = 2;
>  error_alloc_chan:
>  	for (i = i - 1; i >= 0; i--)
> -		gpi_reset_chan(gchan, GPI_CH_CMD_DE_ALLOC);
> +		gpi_reset_chan(&gpii->gchan[i], GPI_CH_CMD_DE_ALLOC);

[Severity: High]
This is a pre-existing issue, but could this error path leave the software
pm_state in an incorrect state?

If gpi_ch_init() fails, the hardware channels are deallocated, but their
pm_state remains as ACTIVE_STATE or CONFIG_STATE. When the client handles the
failure by releasing the previously successful sibling channel via
gpi_free_chan_resources(), it reads the stale state:

drivers/dma/qcom/gpi.c:gpi_free_chan_resources() {
        ...
        if (cur_state == ACTIVE_STATE) {
                gpi_stop_chan(gchan);
                ret = gpi_send_cmd(gpii, gchan, GPI_CH_CMD_RESET);
        ...
}

Would this cause the driver to attempt a graceful shutdown on a channel that
has already been deallocated in hardware, resulting in the same completion
timeouts this patch aims to fix?

>  error_alloc_ev_ring:
>  	gpi_disable_interrupts(gpii);
>  error_config_int:

[Severity: High]
This is a pre-existing issue, but should the event processing tasklet be
canceled in this error path?

gpi_config_interrupts() registers the IRQ, which can fire and schedule
gpii->ev_task. If initialization fails here, gpi_disable_interrupts() frees
the IRQ but does not wait for or cancel scheduled tasklets.

Could this allow the asynchronous tasklet to execute against the freed ev_ring
memory when gpi_process_events() runs?

[Severity: Critical]
This is a pre-existing issue, but is the event ring DMA memory freed without
sending the GPI_EV_CMD_DEALLOC command to the hardware?

Just below this in gpi_ch_init(), gpi_free_ring(&gpii->ev_ring, gpii) is
called. If the hardware retains the physical address of the event ring after
the driver has freed its backing DMA memory, could it silently write into
freed memory if an event is generated later, causing DMA memory corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625-qcom-gpi-err-fix-v1-1-5ca3f00fe2e3@redhat.com?part=1