linux-kernel - Re: [PATCH] bus: mhi: ep: Fix chained transfer handling in read path

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7c833565-0e7b-4004-b691-37bd07ce6abe@quicinc.com>
Date: Thu, 17 Jul 2025 22:18:54 +0530
From: Akhil Vinod <quic_akhvin@...cinc.com>
To: Manivannan Sadhasivam <mani@...nel.org>,
        Sumit Kumar
	<quic_sumk@...cinc.com>
CC: Alex Elder <elder@...nel.org>,
        Greg Kroah-Hartman
	<gregkh@...uxfoundation.org>,
        <mhi@...ts.linux.dev>, <linux-arm-msm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <quic_krichai@...cinc.com>,
        <quic_skananth@...cinc.com>, <quic_vbadigan@...cinc.com>,
        Sumit Kumar <sumk@....qualcomm.com>, <stable@...r.kernel.org>,
        Akhil Vinod <akhvin@....qualcomm.com>
Subject: Re: [PATCH] bus: mhi: ep: Fix chained transfer handling in read path


On 7/16/2025 12:10 PM, Manivannan Sadhasivam wrote:
> On Wed, Jul 09, 2025 at 04:03:17PM GMT, Sumit Kumar wrote:
>> From: Sumit Kumar <sumk@....qualcomm.com>
>>
>> The current implementation of mhi_ep_read_channel, in case of chained
>> transactions, assumes the End of Transfer(EOT) bit is received with the
>> doorbell. As a result, it may incorrectly advance mhi_chan->rd_offset
>> beyond wr_offset during host-to-device transfers when EOT has not yet
>> arrived. This can lead to access of unmapped host memory, causing
>> IOMMU faults and processing of stale TREs.
>>
>> This change modifies the loop condition to ensure rd_offset remains behind
>> wr_offset, allowing the function to process only valid TREs up to the
>> current write pointer. This prevents premature reads and ensures safe
>> traversal of chained TREs.
>>
>> Fixes: 5301258899773 ("bus: mhi: ep: Add support for reading from the host")
>> Cc: stable@...r.kernel.org
>> Co-developed-by: Akhil Vinod <akhvin@....qualcomm.com>
>> Signed-off-by: Akhil Vinod <akhvin@....qualcomm.com>
>> Signed-off-by: Sumit Kumar <sumk@....qualcomm.com>
>> ---
>>   drivers/bus/mhi/ep/main.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c
>> index b3eafcf2a2c50d95e3efd3afb27038ecf55552a5..2e134f44952d1070c62c24aeca9effc7fd325860 100644
>> --- a/drivers/bus/mhi/ep/main.c
>> +++ b/drivers/bus/mhi/ep/main.c
>> @@ -468,7 +468,7 @@ static int mhi_ep_read_channel(struct mhi_ep_cntrl *mhi_cntrl,
>>   
>>   			mhi_chan->rd_offset = (mhi_chan->rd_offset + 1) % ring->ring_size;
>>   		}
>> -	} while (buf_left && !tr_done);
>> +	} while (buf_left && !tr_done && mhi_chan->rd_offset != ring->wr_offset);
> You should use mhi_ep_queue_is_empty() for checking the available elements to
> process. And with this check in place, the existing check in
> mhi_ep_process_ch_ring() becomes redundant.
>
> - Mani

Yes, agreed that the check can be replaced with the mhi_ep_queue_is_empty, but the existing
check in mhi_ep_process_ch_ring() is still necessary because there can be a case where
there are multiple chained transactions in the ring.

Example: The ring at the time mhi_ep_read_channel is executing may look like:
chained | chained |  EOT#1 | chained | chained | EOT#2
  
If we remove the check from mhi_ep_process_ch_ring, we bail out of the first transaction itself
and the remaining packets won't be processed. mhi_ep_read_channel in its current form is designed
for a single MHI packet only.

- Akhil