Re: [PATCH v9 2/4] dmaengine: qcom: gpi: Add lock/unlock TREs for multi-owner I2C transfers

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Setting `TRE_FLAGS_IEOB` on LOCK and UNLOCK TREs causes multiple completion events per descriptor, leading to Use-After-Free and queue corruption.
- [High] The driver prematurely drops the hardware lock in Write-then-Read multi-message transfers due to misinterpreting `DMA_PREP_INTERRUPT`.
- [High] Failure to reset `gchan->lock_pending` during channel reset or termination permanently bypasses hardware locking for subsequent transactions.
- [High] `gpi_prep_slave_sg()` blindly hardcodes the maximum TRE requirement to 3, failing to account for the newly added LOCK and UNLOCK TREs, causing ring buffer overflow.

Pre-existing issues:
- [High] Dangling pointer in `gchan->config` leads to Use-After-Free during channel reallocation.
--

--- Patch [2]: [PATCH v9 2/4] dmaengine: qcom: gpi: Add lock/unlock TREs for multi-owner I2C transfers ---
commit b6883d50c999f402ed4f7be119614262119be22d
Author: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>

dmaengine: qcom: gpi: Add lock/unlock TREs for multi-owner I2C transfers

This commit adds support for emitting lock and unlock Transfer Ring Elements
(TREs) around I2C transfers on QUP-based controllers shared with other
processors. The GPI driver autonomously determines the placement of these
TREs based on transaction boundaries and latches a multi-owner flag during
initial configuration.

> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
> index a5055a6273af6..25fe7410bee2e 100644
> --- a/drivers/dma/qcom/gpi.c
> +++ b/drivers/dma/qcom/gpi.c

[ ... ]

> @@ -518,7 +529,7 @@ struct gpii {
>  	bool ieob_set;
>  };
>  
> -#define MAX_TRE 3
> +#define MAX_TRE 5

[Severity: High]
While MAX_TRE is increased to 5 here, does gpi_prep_slave_sg() still
hardcode the maximum TRE requirement to 3 when checking if the ring buffer
has enough elements available via gpi_ring_num_elements_avail()?

If the ring buffer only has 3 or 4 slots available, could this pass the
check but fail to enqueue the remaining elements like the UNLOCK TRE,
potentially locking the hardware bus?

>  struct gpi_desc {

[ ... ]

> @@ -1617,6 +1629,17 @@ gpi_peripheral_config(struct dma_chan *chan, struct dma_slave_config *config)
>  	gchan->config = new_config;
>  	memcpy(gchan->config, config->peripheral_config, config->peripheral_size);
>  

[Severity: High]
This isn't a bug introduced by this patch, but when
gpi_free_chan_resources() frees this memory with kfree(gchan->config), it
leaves gchan->config as a dangling pointer.

Since gchan elements are statically allocated arrays reused across DMA
channel requests, if the channel is requested again, will this call to
krealloc() receive the freed pointer and cause a slab use-after-free?

> +	/*
> +	 * Latch the multi_owner flag from the initial config call so the GPI

[ ... ]

> @@ -1627,10 +1650,33 @@ static int gpi_create_i2c_tre(struct gchan *chan, struct gpi_desc *desc,
> +	/*
> +	 * Insert a LOCK TRE before the first transfer of a multi-owner
> +	 * transaction.  The GPI driver detects the transaction start
> +	 * autonomously: if multi_owner is set and no lock has been issued
> +	 * since the last unlock (lock_pending == false), this is the first
> +	 * descriptor in a new transaction.
> +	 */
> +	if (chan->multi_owner && !chan->lock_pending) {
> +		tre = &desc->tre[tre_idx];
> +		tre_idx++;
> +
> +		tre->dword[0] = 0;
> +		tre->dword[1] = 0;
> +		tre->dword[2] = 0;
> +		tre->dword[3] = u32_encode_bits(1, TRE_LOCK);
> +		tre->dword[3] |= u32_encode_bits(1, TRE_FLAGS_IEOB);

[Severity: Critical]
Adding TRE_FLAGS_IEOB to the LOCK and UNLOCK TREs means a single transaction
produces an EOB event (from Lock), an EOT event (from DMA), and another EOB
event (from Unlock).

In gpi_process_xfer_compl_event(), EOB events fall through to the success
path which pops the active descriptor from gchan->vc and frees it. Since a
single descriptor generates multiple events, could the event handler pop and
free unrelated descriptors from the queue, causing a use-after-free?

> +		tre->dword[3] |= u32_encode_bits(0, TRE_MINOR_TYPE);
> +		tre->dword[3] |= u32_encode_bits(3, TRE_MAJOR_TYPE);
> +
> +		chan->lock_pending = true;

[Severity: High]
If a transaction errors out after a LOCK TRE is generated and the client
calls gpi_terminate_all() to abort the transfer, gchan->lock_pending is not
reset to false.

On the next transaction, will gpi_create_i2c_tre() evaluate lock_pending as
true and skip inserting the necessary LOCK TRE, causing transactions to run
on the shared bus without acquiring the hardware lock?

> +	}
> +
>  	/* first create config tre if applicable */
>  	if (i2c->set_config) {

[ ... ]

> @@ -1690,6 +1736,35 @@ static int gpi_create_i2c_tre(struct gchan *chan, struct gpi_desc *desc,

[ ... ]

> +	/*
> +	 * Insert an UNLOCK TRE after the last write transfer of a multi-owner
> +	 * transaction.  DMA_PREP_INTERRUPT marks the final descriptor in the
> +	 * batch; reads carry their own completion event so the unlock follows
> +	 * the write leg of each read message.
> +	 */
> +	if (chan->multi_owner && is_last && i2c->op != I2C_READ) {
> +		tre = &desc->tre[tre_idx];
> +		tre_idx++;
> +
> +		tre->dword[0] = 0;
> +		tre->dword[1] = 0;
> +		tre->dword[2] = 0;
> +		tre->dword[3] = u32_encode_bits(1, TRE_UNLOCK);

[Severity: High]
If the i2c-qcom-geni client driver executes Write-then-Read sequences by
setting DMA_PREP_INTERRUPT on the write leg to wait for its completion, will
this logic equate the flag with the absolute end of the I2C transaction?

Could this prematurely append an UNLOCK TRE between the write and read,
destroying the I2C bus stretch and allowing other owners to interleave
transactions and corrupt the bus state?

> +		tre->dword[3] |= u32_encode_bits(1, TRE_FLAGS_IEOB);
> +		tre->dword[3] |= u32_encode_bits(1, TRE_MINOR_TYPE);
> +		tre->dword[3] |= u32_encode_bits(3, TRE_MAJOR_TYPE);
> +
> +		chan->lock_pending = false;
>  	}
>  
>  	for (i = 0; i < tre_idx; i++)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813140047.1006478-1-mukesh.savaliya@oss.qualcomm.com?part=2