diff mbox series

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

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

Commit Message

Bhaumik Bhatt May 5, 2020, 10:47 p.m. UTC 
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(+)

Comments

Manivannan Sadhasivam May 8, 2020, 5:45 a.m. UTC | #1 
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
>
Hemant Kumar May 8, 2020, 5:34 p.m. UTC | #2 
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
>>
Manivannan Sadhasivam May 8, 2020, 6:06 p.m. UTC | #3 
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 mbox series

Patch

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);