| Message ID | 1616144545-18159-1-git-send-email-liweihang@huawei.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Leon Romanovsky |
| Headers | show |
| Series | [for-next] RDMA/core: Check invalid QP state for ib_modify_qp_is_ok() | expand |
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 >
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 >>
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 > >> >
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
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
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 >
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
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;