[PATCH] bus: mhi: host: Detect events pointing to unexpected TREs

[PATCH] bus: mhi: host: Detect events pointing to unexpected TREs

[Youssef Samir]

When a remote device sends a completion event to the host, it contains a
pointer to the consumed TRE. The host uses this pointer to process all of
the TREs between it and the host's local copy of the ring's read pointer.
This works when processing completion for chained transactions, but can
lead to nasty results if the device sends an event for a single-element
transaction with a read pointer that is multiple elements ahead of the
host's read pointer.

For instance, if the host accesses an event ring while the device is
updating it, the pointer inside of the event might still point to an old
TRE. If the host uses the channel's xfer_cb() to directly free the buffer
pointed to by the TRE, the buffer will be double-freed.

Validate the pointer inside an event before processing it.

Fixes: 1d3173a3bae7 ("bus: mhi: core: Add support for processing events from client device")
Signed-off-by: Youssef Samir <quic_yabdulra@quicinc.com>

---
 drivers/bus/mhi/host/main.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
index aa8a0ef697c7..57dc9c5c0d84 100644
--- a/drivers/bus/mhi/host/main.c
+++ b/drivers/bus/mhi/host/main.c
@@ -602,7 +602,7 @@ static int parse_xfer_event(struct mhi_controller *mhi_cntrl,
 	{
 		dma_addr_t ptr = MHI_TRE_GET_EV_PTR(event);
 		struct mhi_ring_element *local_rp, *ev_tre;
-		void *dev_rp;
+		void *dev_rp, *next_rp;
 		struct mhi_buf_info *buf_info;
 		u16 xfer_len;
 
@@ -621,6 +621,16 @@ static int parse_xfer_event(struct mhi_controller *mhi_cntrl,
 		result.dir = mhi_chan->dir;
 
 		local_rp = tre_ring->rp;
+
+		next_rp = local_rp + 1;
+		if (next_rp >= tre_ring->base + tre_ring->len)
+			next_rp = tre_ring->base;
+		if (dev_rp != next_rp && !MHI_TRE_DATA_GET_CHAIN(local_rp)) {
+			dev_err(&mhi_cntrl->mhi_dev->dev,
+				"Event element points to an unexpected tre\n");
+			break;
+		}
+
 		while (local_rp != dev_rp) {
 			buf_info = buf_ring->rp;
 			/* If it's the last TRE, get length from the event */
-- 
2.25.1

Re: [PATCH] bus: mhi: host: Detect events pointing to unexpected TREs

[Jeff Hugo]

On 5/21/2025 10:31 AM, Youssef Samir wrote:
> When a remote device sends a completion event to the host, it contains a
> pointer to the consumed TRE. The host uses this pointer to process all of
> the TREs between it and the host's local copy of the ring's read pointer.
> This works when processing completion for chained transactions, but can
> lead to nasty results if the device sends an event for a single-element
> transaction with a read pointer that is multiple elements ahead of the
> host's read pointer.
> 
> For instance, if the host accesses an event ring while the device is
> updating it, the pointer inside of the event might still point to an old
> TRE. If the host uses the channel's xfer_cb() to directly free the buffer
> pointed to by the TRE, the buffer will be double-freed.
> 
> Validate the pointer inside an event before processing it.
> 
> Fixes: 1d3173a3bae7 ("bus: mhi: core: Add support for processing events from client device")
> Signed-off-by: Youssef Samir <quic_yabdulra@quicinc.com>

Reviewed-by: Jeff Hugo <jeff.hugo@oss.qualcomm.com>

Re: [PATCH] bus: mhi: host: Detect events pointing to unexpected TREs

[Manivannan Sadhasivam]

On Wed, May 21, 2025 at 06:31:10PM +0200, Youssef Samir wrote:
> When a remote device sends a completion event to the host, it contains a
> pointer to the consumed TRE. The host uses this pointer to process all of
> the TREs between it and the host's local copy of the ring's read pointer.
> This works when processing completion for chained transactions, but can
> lead to nasty results if the device sends an event for a single-element
> transaction with a read pointer that is multiple elements ahead of the
> host's read pointer.
> 

How can this happen? I cannot relate this with the scenario mentioned below.

> For instance, if the host accesses an event ring while the device is
> updating it, the pointer inside of the event might still point to an old
> TRE.

I cannot interpret this, sorry. The host is supposed to access the ring elements
till the RP. Even if the device updates the RP while host is processing the TREs,
it should not cause any issues for the host which only sees the used ring
elements.

Maybe I'm missing something?

- Mani

> If the host uses the channel's xfer_cb() to directly free the buffer
> pointed to by the TRE, the buffer will be double-freed.
> 
> Validate the pointer inside an event before processing it.
> 
> Fixes: 1d3173a3bae7 ("bus: mhi: core: Add support for processing events from client device")
> Signed-off-by: Youssef Samir <quic_yabdulra@quicinc.com>
> 
> ---
>  drivers/bus/mhi/host/main.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
> index aa8a0ef697c7..57dc9c5c0d84 100644
> --- a/drivers/bus/mhi/host/main.c
> +++ b/drivers/bus/mhi/host/main.c
> @@ -602,7 +602,7 @@ static int parse_xfer_event(struct mhi_controller *mhi_cntrl,
>  	{
>  		dma_addr_t ptr = MHI_TRE_GET_EV_PTR(event);
>  		struct mhi_ring_element *local_rp, *ev_tre;
> -		void *dev_rp;
> +		void *dev_rp, *next_rp;
>  		struct mhi_buf_info *buf_info;
>  		u16 xfer_len;
>  
> @@ -621,6 +621,16 @@ static int parse_xfer_event(struct mhi_controller *mhi_cntrl,
>  		result.dir = mhi_chan->dir;
>  
>  		local_rp = tre_ring->rp;
> +
> +		next_rp = local_rp + 1;
> +		if (next_rp >= tre_ring->base + tre_ring->len)
> +			next_rp = tre_ring->base;
> +		if (dev_rp != next_rp && !MHI_TRE_DATA_GET_CHAIN(local_rp)) {
> +			dev_err(&mhi_cntrl->mhi_dev->dev,
> +				"Event element points to an unexpected tre\n");
> +			break;
> +		}
> +
>  		while (local_rp != dev_rp) {
>  			buf_info = buf_ring->rp;
>  			/* If it's the last TRE, get length from the event */
> -- 
> 2.25.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] bus: mhi: host: Detect events pointing to unexpected TREs

[Youssef Samir]

On 6/17/2025 11:24 AM, Manivannan Sadhasivam wrote:
> On Wed, May 21, 2025 at 06:31:10PM +0200, Youssef Samir wrote:
>> When a remote device sends a completion event to the host, it contains a
>> pointer to the consumed TRE. The host uses this pointer to process all of
>> the TREs between it and the host's local copy of the ring's read pointer.
>> This works when processing completion for chained transactions, but can
>> lead to nasty results if the device sends an event for a single-element
>> transaction with a read pointer that is multiple elements ahead of the
>> host's read pointer.
>>
> 
> How can this happen? I cannot relate this with the scenario mentioned below.
> 
>> For instance, if the host accesses an event ring while the device is
>> updating it, the pointer inside of the event might still point to an old
>> TRE.
> 
> I cannot interpret this, sorry. The host is supposed to access the ring elements
> till the RP. Even if the device updates the RP while host is processing the TREs,
> it should not cause any issues for the host which only sees the used ring
> elements.
> 
> Maybe I'm missing something?

Hi Mani,

This is related to the behavior that 'commit 6f18d174b73d ("bus: mhi: ep: Update read pointer only after buffer is written")'
aimed to fix from the endpoint. The scenario I observed with a device using drivers/bus/mhi/ep/
involved the device sending an MSI to host for an event that hasn't had its data
completely updated. the event could be pointing to a TRE that is not local_rp + 1.
As you mentioned, the host will process all the TREs until the event's rp,
which allows it to access data that were freed or hasn't been written to yet.

An example of this is qrtr_mhi, where qcom_mhi_qrtr_ul_callback() gets
called to consume skbs that were already freed, which led to refcount
and null pointer dereference issues that caused the host to crash.

This happened because the MSI would reach the host and get processed
before the device was done updating the event ring element. Even though
the aforementioned behavior should not occur with 6f18d174b73d, this patch
aims to harden the host implementation by assuming that the host will not
receive an event with an rp that is n elements ahead of its local rp, unless
it is a chained transaction, otherwise, it is treated as an error.

Thanks,
Youssef

> 
> - Mani
> 

Re: [PATCH] bus: mhi: host: Detect events pointing to unexpected TREs

[Manivannan Sadhasivam]

On Wed, Jul 02, 2025 at 04:06:55PM GMT, Youssef Samir wrote:
> 
> 
> On 6/17/2025 11:24 AM, Manivannan Sadhasivam wrote:
> > On Wed, May 21, 2025 at 06:31:10PM +0200, Youssef Samir wrote:
> >> When a remote device sends a completion event to the host, it contains a
> >> pointer to the consumed TRE. The host uses this pointer to process all of
> >> the TREs between it and the host's local copy of the ring's read pointer.
> >> This works when processing completion for chained transactions, but can
> >> lead to nasty results if the device sends an event for a single-element
> >> transaction with a read pointer that is multiple elements ahead of the
> >> host's read pointer.
> >>
> > 
> > How can this happen? I cannot relate this with the scenario mentioned below.
> > 
> >> For instance, if the host accesses an event ring while the device is
> >> updating it, the pointer inside of the event might still point to an old
> >> TRE.
> > 
> > I cannot interpret this, sorry. The host is supposed to access the ring elements
> > till the RP. Even if the device updates the RP while host is processing the TREs,
> > it should not cause any issues for the host which only sees the used ring
> > elements.
> > 
> > Maybe I'm missing something?
> 
> Hi Mani,
> 
> This is related to the behavior that 'commit 6f18d174b73d ("bus: mhi: ep: Update read pointer only after buffer is written")'
> aimed to fix from the endpoint. The scenario I observed with a device using drivers/bus/mhi/ep/
> involved the device sending an MSI to host for an event that hasn't had its data
> completely updated. the event could be pointing to a TRE that is not local_rp + 1.
> As you mentioned, the host will process all the TREs until the event's rp,
> which allows it to access data that were freed or hasn't been written to yet.
> 

So you are saying that mhi_ep_ring_add_element() didn't update the ring pointer
before triggering MSI? If that's the case, we should add a barrier to make sure
that the RP is updated before raising MSI. Though, I thought that the implicit
barrier offered by the mutex_unlock() would be enough to make sure that the RP
is updated before triggering MSI.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] bus: mhi: host: Detect events pointing to unexpected TREs

[Jeff Hugo]

On 7/8/2025 7:26 AM, Manivannan Sadhasivam wrote:
> On Wed, Jul 02, 2025 at 04:06:55PM GMT, Youssef Samir wrote:
>>
>>
>> On 6/17/2025 11:24 AM, Manivannan Sadhasivam wrote:
>>> On Wed, May 21, 2025 at 06:31:10PM +0200, Youssef Samir wrote:
>>>> When a remote device sends a completion event to the host, it contains a
>>>> pointer to the consumed TRE. The host uses this pointer to process all of
>>>> the TREs between it and the host's local copy of the ring's read pointer.
>>>> This works when processing completion for chained transactions, but can
>>>> lead to nasty results if the device sends an event for a single-element
>>>> transaction with a read pointer that is multiple elements ahead of the
>>>> host's read pointer.
>>>>
>>>
>>> How can this happen? I cannot relate this with the scenario mentioned below.
>>>
>>>> For instance, if the host accesses an event ring while the device is
>>>> updating it, the pointer inside of the event might still point to an old
>>>> TRE.
>>>
>>> I cannot interpret this, sorry. The host is supposed to access the ring elements
>>> till the RP. Even if the device updates the RP while host is processing the TREs,
>>> it should not cause any issues for the host which only sees the used ring
>>> elements.
>>>
>>> Maybe I'm missing something?
>>
>> Hi Mani,
>>
>> This is related to the behavior that 'commit 6f18d174b73d ("bus: mhi: ep: Update read pointer only after buffer is written")'
>> aimed to fix from the endpoint. The scenario I observed with a device using drivers/bus/mhi/ep/
>> involved the device sending an MSI to host for an event that hasn't had its data
>> completely updated. the event could be pointing to a TRE that is not local_rp + 1.
>> As you mentioned, the host will process all the TREs until the event's rp,
>> which allows it to access data that were freed or hasn't been written to yet.
>>
> 
> So you are saying that mhi_ep_ring_add_element() didn't update the ring pointer
> before triggering MSI? If that's the case, we should add a barrier to make sure
> that the RP is updated before raising MSI. Though, I thought that the implicit
> barrier offered by the mutex_unlock() would be enough to make sure that the RP
> is updated before triggering MSI.

No, we are saying that an ep without 'commit 6f18d174b73d ("bus: mhi: 
ep: Update read pointer only after buffer is written")' can cause the 
host to crash because the host is trusting the ep, when it shouldn't be. 
  This patch hardens the host by removing that trust and checking that 
the ep didn't do something invalid (to the extend that we can detect 
invalid behavior).

The ep updated the ring pointer before it updated the ring contents. 
Therefore there is a window where the host can see the updated ring 
pointer, but the ring contents won't be updated therefore the host sees 
stale data.  This is identical to the scenario where the ep updates the 
ring contents with invalid data (maybe a FW bug), and then updates the 
ring pointer.  This proposed patch catches both issues and avoids the 
host consuming the invalid data.

-Jeff

Re: [PATCH] bus: mhi: host: Detect events pointing to unexpected TREs

[Manivannan Sadhasivam]

On Tue, Jul 08, 2025 at 11:18:10AM GMT, Jeff Hugo wrote:
> On 7/8/2025 7:26 AM, Manivannan Sadhasivam wrote:
> > On Wed, Jul 02, 2025 at 04:06:55PM GMT, Youssef Samir wrote:
> > > 
> > > 
> > > On 6/17/2025 11:24 AM, Manivannan Sadhasivam wrote:
> > > > On Wed, May 21, 2025 at 06:31:10PM +0200, Youssef Samir wrote:
> > > > > When a remote device sends a completion event to the host, it contains a
> > > > > pointer to the consumed TRE. The host uses this pointer to process all of
> > > > > the TREs between it and the host's local copy of the ring's read pointer.
> > > > > This works when processing completion for chained transactions, but can
> > > > > lead to nasty results if the device sends an event for a single-element
> > > > > transaction with a read pointer that is multiple elements ahead of the
> > > > > host's read pointer.
> > > > > 
> > > > 
> > > > How can this happen? I cannot relate this with the scenario mentioned below.
> > > > 
> > > > > For instance, if the host accesses an event ring while the device is
> > > > > updating it, the pointer inside of the event might still point to an old
> > > > > TRE.
> > > > 
> > > > I cannot interpret this, sorry. The host is supposed to access the ring elements
> > > > till the RP. Even if the device updates the RP while host is processing the TREs,
> > > > it should not cause any issues for the host which only sees the used ring
> > > > elements.
> > > > 
> > > > Maybe I'm missing something?
> > > 
> > > Hi Mani,
> > > 
> > > This is related to the behavior that 'commit 6f18d174b73d ("bus: mhi: ep: Update read pointer only after buffer is written")'
> > > aimed to fix from the endpoint. The scenario I observed with a device using drivers/bus/mhi/ep/
> > > involved the device sending an MSI to host for an event that hasn't had its data
> > > completely updated. the event could be pointing to a TRE that is not local_rp + 1.
> > > As you mentioned, the host will process all the TREs until the event's rp,
> > > which allows it to access data that were freed or hasn't been written to yet.
> > > 
> > 
> > So you are saying that mhi_ep_ring_add_element() didn't update the ring pointer
> > before triggering MSI? If that's the case, we should add a barrier to make sure
> > that the RP is updated before raising MSI. Though, I thought that the implicit
> > barrier offered by the mutex_unlock() would be enough to make sure that the RP
> > is updated before triggering MSI.
> 
> No, we are saying that an ep without 'commit 6f18d174b73d ("bus: mhi: ep:
> Update read pointer only after buffer is written")' can cause the host to
> crash because the host is trusting the ep, when it shouldn't be.  This patch
> hardens the host by removing that trust and checking that the ep didn't do
> something invalid (to the extend that we can detect invalid behavior).
> 
> The ep updated the ring pointer before it updated the ring contents.
> Therefore there is a window where the host can see the updated ring pointer,
> but the ring contents won't be updated therefore the host sees stale data.
> This is identical to the scenario where the ep updates the ring contents
> with invalid data (maybe a FW bug), and then updates the ring pointer.  This
> proposed patch catches both issues and avoids the host consuming the invalid
> data.
> 

Ok, makes sense now. This information (with reference to 6f18d174b73d) should be
present in the commit message to make it self explanatory.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] bus: mhi: host: Detect events pointing to unexpected TREs

[Jeff Hugo]

On 7/9/2025 5:08 AM, Manivannan Sadhasivam wrote:
> On Tue, Jul 08, 2025 at 11:18:10AM GMT, Jeff Hugo wrote:
>> On 7/8/2025 7:26 AM, Manivannan Sadhasivam wrote:
>>> On Wed, Jul 02, 2025 at 04:06:55PM GMT, Youssef Samir wrote:
>>>>
>>>>
>>>> On 6/17/2025 11:24 AM, Manivannan Sadhasivam wrote:
>>>>> On Wed, May 21, 2025 at 06:31:10PM +0200, Youssef Samir wrote:
>>>>>> When a remote device sends a completion event to the host, it contains a
>>>>>> pointer to the consumed TRE. The host uses this pointer to process all of
>>>>>> the TREs between it and the host's local copy of the ring's read pointer.
>>>>>> This works when processing completion for chained transactions, but can
>>>>>> lead to nasty results if the device sends an event for a single-element
>>>>>> transaction with a read pointer that is multiple elements ahead of the
>>>>>> host's read pointer.
>>>>>>
>>>>>
>>>>> How can this happen? I cannot relate this with the scenario mentioned below.
>>>>>
>>>>>> For instance, if the host accesses an event ring while the device is
>>>>>> updating it, the pointer inside of the event might still point to an old
>>>>>> TRE.
>>>>>
>>>>> I cannot interpret this, sorry. The host is supposed to access the ring elements
>>>>> till the RP. Even if the device updates the RP while host is processing the TREs,
>>>>> it should not cause any issues for the host which only sees the used ring
>>>>> elements.
>>>>>
>>>>> Maybe I'm missing something?
>>>>
>>>> Hi Mani,
>>>>
>>>> This is related to the behavior that 'commit 6f18d174b73d ("bus: mhi: ep: Update read pointer only after buffer is written")'
>>>> aimed to fix from the endpoint. The scenario I observed with a device using drivers/bus/mhi/ep/
>>>> involved the device sending an MSI to host for an event that hasn't had its data
>>>> completely updated. the event could be pointing to a TRE that is not local_rp + 1.
>>>> As you mentioned, the host will process all the TREs until the event's rp,
>>>> which allows it to access data that were freed or hasn't been written to yet.
>>>>
>>>
>>> So you are saying that mhi_ep_ring_add_element() didn't update the ring pointer
>>> before triggering MSI? If that's the case, we should add a barrier to make sure
>>> that the RP is updated before raising MSI. Though, I thought that the implicit
>>> barrier offered by the mutex_unlock() would be enough to make sure that the RP
>>> is updated before triggering MSI.
>>
>> No, we are saying that an ep without 'commit 6f18d174b73d ("bus: mhi: ep:
>> Update read pointer only after buffer is written")' can cause the host to
>> crash because the host is trusting the ep, when it shouldn't be.  This patch
>> hardens the host by removing that trust and checking that the ep didn't do
>> something invalid (to the extend that we can detect invalid behavior).
>>
>> The ep updated the ring pointer before it updated the ring contents.
>> Therefore there is a window where the host can see the updated ring pointer,
>> but the ring contents won't be updated therefore the host sees stale data.
>> This is identical to the scenario where the ep updates the ring contents
>> with invalid data (maybe a FW bug), and then updates the ring pointer.  This
>> proposed patch catches both issues and avoids the host consuming the invalid
>> data.
>>
> 
> Ok, makes sense now. This information (with reference to 6f18d174b73d) should be
> present in the commit message to make it self explanatory.

Youssef please send a v2 with this suggestion.

-Jeff