Re: [PATCH v6 3/8] bus: mhi: core: Add range check for channel id received in event ring

[Hemant Kumar]

Hi Mani,

On 5/7/20 10:45 PM, Manivannan Sadhasivam wrote:
> On Tue, May 05, 2020 at 03:47:07PM -0700, Bhaumik Bhatt wrote:
> > From: Hemant Kumar <hemantk@xxxxxxxxxxxxxx>
> >
> > MHI data completion handler function reads channel id from event
> > ring element. Value is under the control of MHI devices and can be
> > any value between 0 and 255. In order to prevent out of bound access
> > add a bound check against the max channel supported by controller
> > and skip processing of that event ring element.
> >
> > Signed-off-by: Hemant Kumar <hemantk@xxxxxxxxxxxxxx>
> > Signed-off-by: Bhaumik Bhatt <bbhatt@xxxxxxxxxxxxxx>
> > Reviewed-by: Jeffrey Hugo <jhugo@xxxxxxxxxxxxxx>
> > ---
> >   drivers/bus/mhi/core/main.c | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> > index 605640c..e60ab21 100644
> > --- a/drivers/bus/mhi/core/main.c
> > +++ b/drivers/bus/mhi/core/main.c
> > @@ -776,6 +776,9 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl,
> >   		case MHI_PKT_TYPE_TX_EVENT:
> >   			chan = MHI_TRE_GET_EV_CHID(local_rp);
> >   			mhi_chan = &mhi_cntrl->mhi_chan[chan];
>
> Check should be done before this statement, isn't it?
my bad. thanks for pointing that out.
> > +			if (WARN_ON(chan >= mhi_cntrl->max_chan))
> > +				goto next_event;
> > +
>
> I don't prefer using gotos for non exit paths but I don't have a better solution
> here. But you can try to wrap 'WARN_ON' inside the 'MHI_TRE_GET_EV_CHID'
> definition and the just use:
Instead of moving WARN_ON to macro as it requires mhi_cntrl->max_chan to 
compare, how about just adding WARN_ON statement above if condition like 
this

		WARN_ON(chan >= mhi_cntrl->max_chan);
		/*
 		 * Only process the event ring elements whose channel
		 * ID is within the maximum supported range.
		 */
 		if (chan < mhi_cntrl->max_chan) {
                      	mhi_chan = &mhi_cntrl->mhi_chan[chan];
                       	parse_xfer_event(mhi_cntrl, local_rp, mhi_chan);
                          	event_quota--;
 		}
 		break;
> 			/*
> 			 * Only process the event ring elements whose channel
> 			 * ID is within the maximum supported range.
> 			 */
> 			if (chan < mhi_cntrl->max_chan) {
>                          	mhi_chan = &mhi_cntrl->mhi_chan[chan];
>                          	parse_xfer_event(mhi_cntrl, local_rp, mhi_chan);
>                          	event_quota--;
> 			}
> 			break;
>
> This looks more clean.

> >   			parse_xfer_event(mhi_cntrl, local_rp, mhi_chan);
> >   			event_quota--;
> >   			break;
> > @@ -784,6 +787,7 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl,
> >   			break;
> >   		}
> >   
> > +next_event:
> >   		mhi_recycle_ev_ring_element(mhi_cntrl, ev_ring);
> >   		local_rp = ev_ring->rp;
> >   		dev_rp = mhi_to_virtual(ev_ring, er_ctxt->rp);
>
> So you want the count to get increased for skipped element also?
yeah idea is to have total count of events processed even if channel id 
is not correct for that event. This fix is a security fix considering 
that the MHI device is considered as non-secured and MHI host is trying
to continue function normally and just reporting it as warning.

> Thanks,
> Mani
>
> > @@ -820,6 +824,9 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl,
> >   		enum mhi_pkt_type type = MHI_TRE_GET_EV_TYPE(local_rp);
> >   
> >   		chan = MHI_TRE_GET_EV_CHID(local_rp);
> > +		if (WARN_ON(chan >= mhi_cntrl->max_chan))
> > +			goto next_event;
> > +
> >   		mhi_chan = &mhi_cntrl->mhi_chan[chan];
> >   
> >   		if (likely(type == MHI_PKT_TYPE_TX_EVENT)) {
> > @@ -830,6 +837,7 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl,
> >   			event_quota--;
> >   		}
> >   
> > +next_event:
> >   		mhi_recycle_ev_ring_element(mhi_cntrl, ev_ring);
> >   		local_rp = ev_ring->rp;
> >   		dev_rp = mhi_to_virtual(ev_ring, er_ctxt->rp);
Even this function has the same goto statement. For consistency i would 
do same thing here as well. Let me know what do you think about above 
suggestion for both functions.
> > --
> > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> > a Linux Foundation Collaborative Project

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project