| Message ID | 1630477719-2804-1-git-send-email-wangyunjian@huawei.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] odp-util: Fix a null pointer dereference in odp_flow_format() | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
Good catch!!! Can you also check for mask_len before calling nl_attr_find__() as is done in other places in the same function? *Vasu Dasari* On Wed, Sep 1, 2021 at 2:29 AM Yunjian Wang <wangyunjian@huawei.com> wrote: > This patch fixes (dereference after null check) coverity issue. > For this reason, we should add null check of 'mask' before calling > nl_attr_find__() because the 'mask' maybe null. > > Addresses-Coverity: ("Dereference after null check") > Fixes: e6cc0babc25d ("ovs-dpctl: Add mega flow support") > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> > --- > v2: > * update code styles suggested by Aaron Conole > --- > lib/odp-util.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/odp-util.c b/lib/odp-util.c > index 7729a9060..bf427f027 100644 > --- a/lib/odp-util.c > +++ b/lib/odp-util.c > @@ -4618,7 +4618,7 @@ odp_flow_format(const struct nlattr *key, size_t > key_len, > } > ds_put_char(ds, ')'); > } > - if (!has_ethtype_key) { > + if (!has_ethtype_key && mask) { > const struct nlattr *ma = nl_attr_find__(mask, mask_len, > > OVS_KEY_ATTR_ETHERTYPE); > if (ma) { > -- > 2.18.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
Yunjian Wang <wangyunjian@huawei.com> writes: > This patch fixes (dereference after null check) coverity issue. > For this reason, we should add null check of 'mask' before calling > nl_attr_find__() because the 'mask' maybe null. > > Addresses-Coverity: ("Dereference after null check") > Fixes: e6cc0babc25d ("ovs-dpctl: Add mega flow support") > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> > --- Thanks! Acked-by: Aaron Conole <aconole@redhat.com>
Hi Vasu, Vasu Dasari <vdasari@gmail.com> writes: > Good catch!!! > Can you also check for mask_len before calling nl_attr_find__() as is done > in other places in the same function? I asked to remove the mask_len check. nl_attr_find__() already does such check. I scanned the code to find such examples, but only one exists (which is in odp-util.c). Other places either don't validate at all, or were pulled during an nl_attr_get_size() call. I don't see why such check is needed. Maybe I missed something? > *Vasu Dasari* > > > On Wed, Sep 1, 2021 at 2:29 AM Yunjian Wang <wangyunjian@huawei.com> wrote: > >> This patch fixes (dereference after null check) coverity issue. >> For this reason, we should add null check of 'mask' before calling >> nl_attr_find__() because the 'mask' maybe null. >> >> Addresses-Coverity: ("Dereference after null check") >> Fixes: e6cc0babc25d ("ovs-dpctl: Add mega flow support") >> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> >> --- >> v2: >> * update code styles suggested by Aaron Conole >> --- >> lib/odp-util.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/lib/odp-util.c b/lib/odp-util.c >> index 7729a9060..bf427f027 100644 >> --- a/lib/odp-util.c >> +++ b/lib/odp-util.c >> @@ -4618,7 +4618,7 @@ odp_flow_format(const struct nlattr *key, size_t >> key_len, >> } >> ds_put_char(ds, ')'); >> } >> - if (!has_ethtype_key) { >> + if (!has_ethtype_key && mask) { >> const struct nlattr *ma = nl_attr_find__(mask, mask_len, >> >> OVS_KEY_ATTR_ETHERTYPE); >> if (ma) { >> -- >> 2.18.1 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Hi Aaron, I did not look at your comment before. You are right that, mask_len parameter is validated via nl_attr_is_valid(). It is ok not to check for mask_len. Just that in the function odp_flow_format(), mask_len is used in 3 places. Two of the places have the check for it and this is the third one that I suggested to have the check. I am ok with not checking. Acked-by: Vasu Dasari <vdasari@gmail.com> Best, -Vasu *Vasu Dasari* On Wed, Sep 1, 2021 at 9:09 AM Aaron Conole <aconole@redhat.com> wrote: > > Hi Vasu, > > Vasu Dasari <vdasari@gmail.com> writes: > > > Good catch!!! > > Can you also check for mask_len before calling nl_attr_find__() as is > done > > in other places in the same function? > > I asked to remove the mask_len check. nl_attr_find__() already does > such check. I scanned the code to find such examples, but only one > exists (which is in odp-util.c). Other places either don't validate at > all, or were pulled during an nl_attr_get_size() call. > > I don't see why such check is needed. Maybe I missed something? > > > *Vasu Dasari* > > > > > > On Wed, Sep 1, 2021 at 2:29 AM Yunjian Wang <wangyunjian@huawei.com> > wrote: > > > >> This patch fixes (dereference after null check) coverity issue. > >> For this reason, we should add null check of 'mask' before calling > >> nl_attr_find__() because the 'mask' maybe null. > >> > >> Addresses-Coverity: ("Dereference after null check") > >> Fixes: e6cc0babc25d ("ovs-dpctl: Add mega flow support") > >> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> > >> --- > >> v2: > >> * update code styles suggested by Aaron Conole > >> --- > >> lib/odp-util.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/lib/odp-util.c b/lib/odp-util.c > >> index 7729a9060..bf427f027 100644 > >> --- a/lib/odp-util.c > >> +++ b/lib/odp-util.c > >> @@ -4618,7 +4618,7 @@ odp_flow_format(const struct nlattr *key, size_t > >> key_len, > >> } > >> ds_put_char(ds, ')'); > >> } > >> - if (!has_ethtype_key) { > >> + if (!has_ethtype_key && mask) { > >> const struct nlattr *ma = nl_attr_find__(mask, mask_len, > >> > >> OVS_KEY_ATTR_ETHERTYPE); > >> if (ma) { > >> -- > >> 2.18.1 > >> > >> _______________________________________________ > >> dev mailing list > >> dev@openvswitch.org > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >> > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On 9/1/21 16:07, Vasu Dasari wrote: > Hi Aaron, > > I did not look at your comment before. You are right that, mask_len > parameter is validated via nl_attr_is_valid(). It is ok not to check for > mask_len. > > Just that in the function odp_flow_format(), mask_len is used in 3 places. > Two of the places have the check for it and this is the third one that I > suggested to have the check. I am ok with not checking. > > Acked-by: Vasu Dasari <vdasari@gmail.com> Thanks! Applied and backported down to 2.12. Best regards, Ilya Maximets.
diff --git a/lib/odp-util.c b/lib/odp-util.c index 7729a9060..bf427f027 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -4618,7 +4618,7 @@ odp_flow_format(const struct nlattr *key, size_t key_len, } ds_put_char(ds, ')'); } - if (!has_ethtype_key) { + if (!has_ethtype_key && mask) { const struct nlattr *ma = nl_attr_find__(mask, mask_len, OVS_KEY_ATTR_ETHERTYPE); if (ma) {
This patch fixes (dereference after null check) coverity issue. For this reason, we should add null check of 'mask' before calling nl_attr_find__() because the 'mask' maybe null. Addresses-Coverity: ("Dereference after null check") Fixes: e6cc0babc25d ("ovs-dpctl: Add mega flow support") Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> --- v2: * update code styles suggested by Aaron Conole --- lib/odp-util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)