[PATCH for-next] RDMA/core: Check invalid QP state for ib_modify_qp_is_ok()

[PATCH for-next] RDMA/core: Check invalid QP state for ib_modify_qp_is_ok()

[Weihang Li]

From: Xi Wang <wangxi11@huawei.com>

Out-of-bounds may occur in 'qp_state_table' when the caller passing wrong
QP state value.

Signed-off-by: Xi Wang <wangxi11@huawei.com>
Signed-off-by: Weihang Li <liweihang@huawei.com>
---
 drivers/infiniband/core/verbs.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index 28464c5..66ba4e6 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -1613,6 +1613,10 @@ bool ib_modify_qp_is_ok(enum ib_qp_state cur_state, enum ib_qp_state next_state,
 	    cur_state != IB_QPS_SQD && cur_state != IB_QPS_SQE)
 		return false;
 
+	if (cur_state >= ARRAY_SIZE(qp_state_table) ||
+	    next_state >= ARRAY_SIZE(qp_state_table[0]))
+		return false;
+
 	if (!qp_state_table[cur_state][next_state].valid)
 		return false;
 
-- 
2.8.1

Re: [PATCH for-next] RDMA/core: Check invalid QP state for ib_modify_qp_is_ok()

[Leon Romanovsky]

On Fri, Mar 19, 2021 at 05:02:25PM +0800, Weihang Li wrote:
> From: Xi Wang <wangxi11@huawei.com>
> 
> Out-of-bounds may occur in 'qp_state_table' when the caller passing wrong
> QP state value.

How is it possible? Do you have call stack to support it?

Thanks

> 
> Signed-off-by: Xi Wang <wangxi11@huawei.com>
> Signed-off-by: Weihang Li <liweihang@huawei.com>
> ---
>  drivers/infiniband/core/verbs.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
> index 28464c5..66ba4e6 100644
> --- a/drivers/infiniband/core/verbs.c
> +++ b/drivers/infiniband/core/verbs.c
> @@ -1613,6 +1613,10 @@ bool ib_modify_qp_is_ok(enum ib_qp_state cur_state, enum ib_qp_state next_state,
>  	    cur_state != IB_QPS_SQD && cur_state != IB_QPS_SQE)
>  		return false;
>  
> +	if (cur_state >= ARRAY_SIZE(qp_state_table) ||
> +	    next_state >= ARRAY_SIZE(qp_state_table[0]))
> +		return false;
> +
>  	if (!qp_state_table[cur_state][next_state].valid)
>  		return false;
>  
> -- 
> 2.8.1
> 

Re: [PATCH for-next] RDMA/core: Check invalid QP state for ib_modify_qp_is_ok()

[liweihang]

On 2021/3/20 17:34, Leon Romanovsky wrote:
> On Fri, Mar 19, 2021 at 05:02:25PM +0800, Weihang Li wrote:
>> From: Xi Wang <wangxi11@huawei.com>
>>
>> Out-of-bounds may occur in 'qp_state_table' when the caller passing wrong
>> QP state value.
> 
> How is it possible? Do you have call stack to support it?
> 
> Thanks
> 

ib_modify_qp_is_ok() is exported, I think any kernel modules can pass in
invalid QP state. Should we check it in such case?

Thanks
Weihang

>>
>> Signed-off-by: Xi Wang <wangxi11@huawei.com>
>> Signed-off-by: Weihang Li <liweihang@huawei.com>
>> ---
>>  drivers/infiniband/core/verbs.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
>> index 28464c5..66ba4e6 100644
>> --- a/drivers/infiniband/core/verbs.c
>> +++ b/drivers/infiniband/core/verbs.c
>> @@ -1613,6 +1613,10 @@ bool ib_modify_qp_is_ok(enum ib_qp_state cur_state, enum ib_qp_state next_state,
>>  	    cur_state != IB_QPS_SQD && cur_state != IB_QPS_SQE)
>>  		return false;
>>  
>> +	if (cur_state >= ARRAY_SIZE(qp_state_table) ||
>> +	    next_state >= ARRAY_SIZE(qp_state_table[0]))
>> +		return false;
>> +
>>  	if (!qp_state_table[cur_state][next_state].valid)
>>  		return false;
>>  
>> -- 
>> 2.8.1
>>

Re: [PATCH for-next] RDMA/core: Check invalid QP state for ib_modify_qp_is_ok()

[Leon Romanovsky]

On Mon, Mar 22, 2021 at 03:29:09AM +0000, liweihang wrote:
> On 2021/3/20 17:34, Leon Romanovsky wrote:
> > On Fri, Mar 19, 2021 at 05:02:25PM +0800, Weihang Li wrote:
> >> From: Xi Wang <wangxi11@huawei.com>
> >>
> >> Out-of-bounds may occur in 'qp_state_table' when the caller passing wrong
> >> QP state value.
> > 
> > How is it possible? Do you have call stack to support it?
> > 
> > Thanks
> > 
> 
> ib_modify_qp_is_ok() is exported, I think any kernel modules can pass in
> invalid QP state. Should we check it in such case?

No, it is caller responsibility to supply valid input.
In general case, for the kernel code, it can be seen as anti-pattern
if in-kernel API performs input sanity check.

You can add WARN_ON() if you want to catch programmers errors earlier.
However, I'm skeptical if it is really needed here. 

Thanks

> 
> Thanks
> Weihang
> 
> >>
> >> Signed-off-by: Xi Wang <wangxi11@huawei.com>
> >> Signed-off-by: Weihang Li <liweihang@huawei.com>
> >> ---
> >>  drivers/infiniband/core/verbs.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
> >> index 28464c5..66ba4e6 100644
> >> --- a/drivers/infiniband/core/verbs.c
> >> +++ b/drivers/infiniband/core/verbs.c
> >> @@ -1613,6 +1613,10 @@ bool ib_modify_qp_is_ok(enum ib_qp_state cur_state, enum ib_qp_state next_state,
> >>  	    cur_state != IB_QPS_SQD && cur_state != IB_QPS_SQE)
> >>  		return false;
> >>  
> >> +	if (cur_state >= ARRAY_SIZE(qp_state_table) ||
> >> +	    next_state >= ARRAY_SIZE(qp_state_table[0]))
> >> +		return false;
> >> +
> >>  	if (!qp_state_table[cur_state][next_state].valid)
> >>  		return false;
> >>  
> >> -- 
> >> 2.8.1
> >>
> 

Re: [PATCH for-next] RDMA/core: Check invalid QP state for ib_modify_qp_is_ok()

[liweihang]

On 2021/3/22 13:47, Leon Romanovsky wrote:
> On Mon, Mar 22, 2021 at 03:29:09AM +0000, liweihang wrote:
>> On 2021/3/20 17:34, Leon Romanovsky wrote:
>>> On Fri, Mar 19, 2021 at 05:02:25PM +0800, Weihang Li wrote:
>>>> From: Xi Wang <wangxi11@huawei.com>
>>>>
>>>> Out-of-bounds may occur in 'qp_state_table' when the caller passing wrong
>>>> QP state value.
>>> How is it possible? Do you have call stack to support it?
>>>
>>> Thanks
>>>
>> ib_modify_qp_is_ok() is exported, I think any kernel modules can pass in
>> invalid QP state. Should we check it in such case?
> No, it is caller responsibility to supply valid input.
> In general case, for the kernel code, it can be seen as anti-pattern
> if in-kernel API performs input sanity check.
> 
> You can add WARN_ON() if you want to catch programmers errors earlier.
> However, I'm skeptical if it is really needed here. 
> 
> Thanks
> 

I see, thank you for the explanation. In that case, we don't need this patch.

Weihang

Re: [PATCH for-next] RDMA/core: Check invalid QP state for ib_modify_qp_is_ok()

[liweihang]

On 2021/3/22 13:47, Leon Romanovsky wrote:
> On Mon, Mar 22, 2021 at 03:29:09AM +0000, liweihang wrote:
>> On 2021/3/20 17:34, Leon Romanovsky wrote:
>>> On Fri, Mar 19, 2021 at 05:02:25PM +0800, Weihang Li wrote:
>>>> From: Xi Wang <wangxi11@huawei.com>
>>>>
>>>> Out-of-bounds may occur in 'qp_state_table' when the caller passing wrong
>>>> QP state value.
>>> How is it possible? Do you have call stack to support it?
>>>
>>> Thanks
>>>
>> ib_modify_qp_is_ok() is exported, I think any kernel modules can pass in
>> invalid QP state. Should we check it in such case?
> No, it is caller responsibility to supply valid input.
> In general case, for the kernel code, it can be seen as anti-pattern
> if in-kernel API performs input sanity check.
> 
> You can add WARN_ON() if you want to catch programmers errors earlier.
> However, I'm skeptical if it is really needed here. 
> 
> Thanks
> 

Hi Leon,

By the way, we made this change because we noticed that ib_event_msg() and
ib_wc_status_msg() that tries to access an array performs input check in the
same file. Is there anything different between these kernel APIs? Or there is
some other reasons?

Thanks,
Weihang

Re: [PATCH for-next] RDMA/core: Check invalid QP state for ib_modify_qp_is_ok()

[Leon Romanovsky]

On Mon, Mar 22, 2021 at 07:11:47AM +0000, liweihang wrote:
> On 2021/3/22 13:47, Leon Romanovsky wrote:
> > On Mon, Mar 22, 2021 at 03:29:09AM +0000, liweihang wrote:
> >> On 2021/3/20 17:34, Leon Romanovsky wrote:
> >>> On Fri, Mar 19, 2021 at 05:02:25PM +0800, Weihang Li wrote:
> >>>> From: Xi Wang <wangxi11@huawei.com>
> >>>>
> >>>> Out-of-bounds may occur in 'qp_state_table' when the caller passing wrong
> >>>> QP state value.
> >>> How is it possible? Do you have call stack to support it?
> >>>
> >>> Thanks
> >>>
> >> ib_modify_qp_is_ok() is exported, I think any kernel modules can pass in
> >> invalid QP state. Should we check it in such case?
> > No, it is caller responsibility to supply valid input.
> > In general case, for the kernel code, it can be seen as anti-pattern
> > if in-kernel API performs input sanity check.
> > 
> > You can add WARN_ON() if you want to catch programmers errors earlier.
> > However, I'm skeptical if it is really needed here. 
> > 
> > Thanks
> > 
> 
> Hi Leon,
> 
> By the way, we made this change because we noticed that ib_event_msg() and
> ib_wc_status_msg() that tries to access an array performs input check in the
> same file. Is there anything different between these kernel APIs? Or there is
> some other reasons?

The main difference between them is the execution flow.
 * ib_modify_qp_is_ok() is called from the drivers, after verbs layer
   sanitized everything already and at this stage we are pretty safe.
 * ib_event_msg()/ib_wc_status_ms() are used by ULPs and maybe they can
   send invalid event. I personally don't know if it is possible or not.

Thanks

> 
> Thanks,
> Weihang
> 

Re: [PATCH for-next] RDMA/core: Check invalid QP state for ib_modify_qp_is_ok()

[liweihang]

On 2021/3/22 15:29, Leon Romanovsky wrote:
> On Mon, Mar 22, 2021 at 07:11:47AM +0000, liweihang wrote:
>> On 2021/3/22 13:47, Leon Romanovsky wrote:
>>> On Mon, Mar 22, 2021 at 03:29:09AM +0000, liweihang wrote:
>>>> On 2021/3/20 17:34, Leon Romanovsky wrote:
>>>>> On Fri, Mar 19, 2021 at 05:02:25PM +0800, Weihang Li wrote:
>>>>>> From: Xi Wang <wangxi11@huawei.com>
>>>>>>
>>>>>> Out-of-bounds may occur in 'qp_state_table' when the caller passing wrong
>>>>>> QP state value.
>>>>> How is it possible? Do you have call stack to support it?
>>>>>
>>>>> Thanks
>>>>>
>>>> ib_modify_qp_is_ok() is exported, I think any kernel modules can pass in
>>>> invalid QP state. Should we check it in such case?
>>> No, it is caller responsibility to supply valid input.
>>> In general case, for the kernel code, it can be seen as anti-pattern
>>> if in-kernel API performs input sanity check.
>>>
>>> You can add WARN_ON() if you want to catch programmers errors earlier.
>>> However, I'm skeptical if it is really needed here. 
>>>
>>> Thanks
>>>
>>
>> Hi Leon,
>>
>> By the way, we made this change because we noticed that ib_event_msg() and
>> ib_wc_status_msg() that tries to access an array performs input check in the
>> same file. Is there anything different between these kernel APIs? Or there is
>> some other reasons?
> 
> The main difference between them is the execution flow.
>  * ib_modify_qp_is_ok() is called from the drivers, after verbs layer
>    sanitized everything already and at this stage we are pretty safe.
>  * ib_event_msg()/ib_wc_status_ms() are used by ULPs and maybe they can
>    send invalid event. I personally don't know if it is possible or not.
> 
> Thanks
> 

Thank you, this is helpful.

Weihang