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] [day] [month] [year] [list]

Message-ID: <2biqjalas76dxqpho5745tljtxnohgmc2vywd5aavfd4zkqnj3@4eeukkbtxzcj>
Date: Fri, 22 Aug 2025 16:17:15 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Sumit Kumar <quic_sumk@...cinc.com>
Cc: Akhil Vinod <quic_akhvin@...cinc.com>, 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 Fri, Aug 22, 2025 at 02:20:29PM GMT, Sumit Kumar wrote:
> 
> 
> On 7/17/2025 10:44 PM, Manivannan Sadhasivam wrote:
> > On Thu, Jul 17, 2025 at 10:18:54PM GMT, Akhil Vinod wrote:
> > > 
> > > 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.
> > > 
> > 
> > Then you should ignore the EOT flag by removing '!tr_done' check and just check
> > for buf_left and mhi_ep_process_ch_ring(). Having the same check in caller and
> > callee doesn't make sense.
> > 
> > - Mani
> > 
> Agreed, we can remove the tr_done check from the while loop, then all the
> elements of the ring will be processed in read_channel.
> Additionally, the purpose of buf_left is to process a TRE if
> DEFAULT_MTU_SIZE of endpoint is less than host, but the buf_left will become
> 0 after processing a part of TRE and will not process the remaining data.
> 
> Therefore will remove the buf_left too from read_channel otherwise it will
> exit the loop after processing one TRE or just a part of it.
> 

Right. If we are removing the ring empty check from mhi_ep_process_ch_ring(),
then we do need to remove the buf_left check from mhi_ep_read_channel().

- Mani

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