From: Hemant Kumar <[email protected]>
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 <[email protected]>
---
drivers/bus/mhi/core/main.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index 23154f1..ba8afa7 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -827,6 +827,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))
+ continue;
+
mhi_chan = &mhi_cntrl->mhi_chan[chan];
if (likely(type == MHI_PKT_TYPE_TX_EVENT)) {
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On 4/27/2020 12:32 PM, Bhaumik Bhatt wrote:
> From: Hemant Kumar <[email protected]>
>
> 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 <[email protected]>
I believe your SOB is needed here after Hemant's per section 11 of
Documentation/process/submitting-patches.rst
> ---
> drivers/bus/mhi/core/main.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index 23154f1..ba8afa7 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -827,6 +827,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))
> + continue;
> +
> mhi_chan = &mhi_cntrl->mhi_chan[chan];
>
> if (likely(type == MHI_PKT_TYPE_TX_EVENT)) {
>
How does this work?
I understand the need for the range check, however I looks like this
change doesn't do proper handling.
Since all you do is "continue", we'll remain stuck in the while loop
this exists in, continuously "processing" the same event, failing the
same check, and spamming the kernel log with the WARN_ON output until
the end of time.
What am I missing?
What is the behavior you want? Do you want to just drop/ignore the
invalid event, or issue a reset of the device because it is clearly
misbehaving?
--
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
On 4/27/20 11:50 AM, Jeffrey Hugo wrote:
> On 4/27/2020 12:32 PM, Bhaumik Bhatt wrote:
>> From: Hemant Kumar <[email protected]>
>>
>> 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 <[email protected]>
>
> I believe your SOB is needed here after Hemant's per section 11 of
> Documentation/process/submitting-patches.rst
>
>> ---
>> drivers/bus/mhi/core/main.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
>> index 23154f1..ba8afa7 100644
>> --- a/drivers/bus/mhi/core/main.c
>> +++ b/drivers/bus/mhi/core/main.c
>> @@ -827,6 +827,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))
>> + continue;
>> +
>> mhi_chan = &mhi_cntrl->mhi_chan[chan];
>> if (likely(type == MHI_PKT_TYPE_TX_EVENT)) {
>>
>
> How does this work?
>
> I understand the need for the range check, however I looks like this
> change doesn't do proper handling.
>
> Since all you do is "continue", we'll remain stuck in the while loop
> this exists in, continuously "processing" the same event, failing the
> same check, and spamming the kernel log with the WARN_ON output until
> the end of time.
>
> What am I missing?
Idea was to drop the event and continue, but continue statement is
wrong here, we need to recycle the event and continue after that. Will
fix in V2.
>
> What is the behavior you want? Do you want to just drop/ignore the
> invalid event, or issue a reset of the device because it is clearly
> misbehaving?
>
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project