Hi Hemant,
On Fri, May 08, 2020 at 10:34:13AM -0700, Hemant Kumar wrote:
> 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);
Okay.
> /*
> * 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.
>
Okay.
> >
> > 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.
Above looks good to me. So please go ahead.
Thanks,
Mani
> > > --
> > > 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
