Message ID | 1588718832-4891-4-git-send-email-bbhatt@codeaurora.org (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | Bug fixes and improved logging in MHI | expand |
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? > + 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: /* * 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? 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); > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project >
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); /* * 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 >>
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);