| Message ID | 1588718832-4891-4-git-send-email-bbhatt@codeaurora.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v6,1/8] bus: mhi: core: Refactor mhi queue APIs | expand |
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@codeaurora.org> > > > > > > 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@codeaurora.org> > > > Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org> > > > Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org> > > > --- > > > 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
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]; + if (WARN_ON(chan >= mhi_cntrl->max_chan)) + goto next_event; + 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); @@ -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);