Re: [PATCH v1] dmaengine: qcom: gpi: Fix channel cleanup in unwind path

[sashiko-bot@kernel.org]

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

New issues:
- [High] Incomplete Error Cleanup / Missing State Restoration causes 750ms kernel stalls per channel.
- [High] Missing state reversion combined with ring freeing causes IOMMU hardware faults (base address 0x0) on reallocation.
- [High] Hardware state leak and DMA Use-After-Free of the event ring.

Pre-existing issues:
- [High] Dangling pointer in `gchan->config` leads to Use-After-Free upon channel reallocation.
- [High] Deferred shared hardware initialization leaves a dangling channel vulnerable to MMIO bus faults.
--

--- Patch [1]: [PATCH v1] dmaengine: qcom: gpi: Fix channel cleanup in unwind path ---
commit a2ee269fe4b77d9be025b81af8f6b123417e0e5c
Author: Aniket Randive <aniket.randive@oss.qualcomm.com>

dmaengine: qcom: gpi: Fix channel cleanup in unwind path

This commit attempts to fix channel cleanup in the gpi_ch_init() error
path. It corrects the channel indices used in the unwind loops, restores
pm_state to DISABLE_STATE, and frees ch_ring on initialization failure.

> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
> index a5055a6273af6..c41bfac65ddf3 100644
> --- a/drivers/dma/qcom/gpi.c
> +++ b/drivers/dma/qcom/gpi.c
> @@ -1965,16 +1965,19 @@ 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);

[Severity: High]
If gpi_start_chan() fails for channel 1, this error path stops and resets
channel 0, but does it leave gpii->gchan[0].pm_state as ACTIVE_STATE?

If gpii->pm_state is forced to DISABLE_STATE later in this unwind path, when
the client releases channel 0, gpi_free_chan_resources() might see
gchan[0].pm_state == ACTIVE_STATE and attempt to send STOP, RESET, and DE_ALLOC
commands.

Because gpii->pm_state is DISABLE_STATE, won't the interrupt handler drop
completion interrupts, causing these commands to time out and stall for 750ms?

>  	}
>  	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);
>  error_alloc_ev_ring:
>  	gpi_disable_interrupts(gpii);
>  error_config_int:
>  	gpi_free_ring(&gpii->ev_ring, gpii);
> +	write_lock_irq(&gpii->pm_lock);
> +	gpii->pm_state = DISABLE_STATE;
> +	write_unlock_irq(&gpii->pm_lock);

[Severity: High]
If initialization fails after the hardware event channel is allocated, we free
the event ring DMA memory here and force gpii->pm_state to DISABLE_STATE.

Later, when gpi_free_chan_resources() is called for the active channel, will
it skip sending GPI_EV_CMD_DEALLOC because cur_state is no longer ACTIVE_STATE?

Could this leave the hardware event channel active, leading to a DMA
use-after-free on the freed event ring memory?

>  exit_gpi_init:
>  	return ret;
>  }
> @@ -2065,6 +2068,8 @@ static int gpi_alloc_chan_resources(struct dma_chan *chan)
>  		goto xfer_alloc_err;
>  
>  	ret = gpi_ch_init(gchan);
> +	if (ret)
> +		gpi_free_ring(&gchan->ch_ring, gpii);

[Severity: High]
When gpi_ch_init() fails, it appears gchan->pm_state remains set to
CONFIG_STATE.

If we free and zero the ch_ring here without reverting gchan->pm_state, what
happens if a DMA client retries channel allocation?

Would a subsequent call to gpi_ch_init() bypass hardware initialization
because pm_state is already CONFIG_STATE, and program the hardware with a
zeroed physical address from the freed ring, leading to an IOMMU fault?

>  
>  	mutex_unlock(&gpii->ctrl_lock);
>  

[Severity: High]
This is a pre-existing issue, but I noticed a potential dangling pointer when
a DMA channel is released.

In drivers/dma/qcom/gpi.c:gpi_free_chan_resources():

        vchan_free_chan_resources(&gchan->vc);
        kfree(gchan->config);

Since gchan persists across clients, if a new client allocates the same channel
and calls dmaengine_slave_config(), this executes:

drivers/dma/qcom/gpi.c:gpi_peripheral_config():

        new_config = krealloc(gchan->config, config->peripheral_size, GFP_NOWAIT);

Does this pass a freed, non-null pointer into krealloc() if gchan->config wasn't
set to NULL after kfree()?

[Severity: High]
This is a pre-existing issue, but could deferred hardware initialization crash
the system if a client only requests one channel?

In drivers/dma/qcom/gpi.c:gpi_ch_init():

        for (i = 0; i < MAX_CHANNELS_PER_GPII; i++)
                if (gpii->gchan[i].pm_state != CONFIG_STATE)
                        goto exit_gpi_init;

If the first channel reaches CONFIG_STATE, it returns success immediately.
If a client then issues a transfer without initializing a second channel:

drivers/dma/qcom/gpi.c:gpi_issue_pending():

        gpi_desc->db = ch_ring->wp;
        gpi_write_ch_db(gchan, &gchan->ch_ring, gpi_desc->db);

Won't writing to the MMIO doorbell of an uninitialized (and potentially
unclocked) block cause a bus fault?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803120316.2703956-1-aniket.randive@oss.qualcomm.com?part=1