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

[Jeff Hugo <jeff.hugo@oss.qualcomm.com>]

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