| Message ID | 1539124758-7478-2-git-send-email-pkusunyifeng@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,1/2] odp-util: Fix a bug that causes stack overflow | expand |
On Tue, Oct 09, 2018 at 03:39:18PM -0700, Yifeng Sun wrote: > When parse_odp_key_mask_attr runs into ufid, it returns length of ufid > without appending data into ofpbufs. This commit adds additional > checking for this case. > > Found this bug when debugging > https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=10850, > but not certain it is related. > > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> Thanks for the fix. It might be better to come up with a way to disallow a ufid here, since it doesn't make sense to put a ufid into a "set" action.
Thanks for the review. How about returning a -EINVAL in this case? On Thu, Oct 11, 2018 at 1:39 PM Ben Pfaff <blp@ovn.org> wrote: > On Tue, Oct 09, 2018 at 03:39:18PM -0700, Yifeng Sun wrote: > > When parse_odp_key_mask_attr runs into ufid, it returns length of ufid > > without appending data into ofpbufs. This commit adds additional > > checking for this case. > > > > Found this bug when debugging > > https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=10850, > > but not certain it is related. > > > > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> > > Thanks for the fix. > > It might be better to come up with a way to disallow a ufid here, since > it doesn't make sense to put a ufid into a "set" action. >
I think that there is only one caller of parse_odp_key_mask_attr() that needs to understand a ufid, which is odp_flow_from_string(). Maybe the code to parse a ufid could go there instead. On Thu, Oct 11, 2018 at 01:53:54PM -0700, Yifeng Sun wrote: > Thanks for the review. > > How about returning a -EINVAL in this case? > > On Thu, Oct 11, 2018 at 1:39 PM Ben Pfaff <blp@ovn.org> wrote: > > > On Tue, Oct 09, 2018 at 03:39:18PM -0700, Yifeng Sun wrote: > > > When parse_odp_key_mask_attr runs into ufid, it returns length of ufid > > > without appending data into ofpbufs. This commit adds additional > > > checking for this case. > > > > > > Found this bug when debugging > > > https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=10850, > > > but not certain it is related. > > > > > > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> > > > > Thanks for the fix. > > > > It might be better to come up with a way to disallow a ufid here, since > > it doesn't make sense to put a ufid into a "set" action. > >
Ok, I'll check it out. On Thu, Oct 11, 2018 at 2:04 PM Ben Pfaff <blp@ovn.org> wrote: > I think that there is only one caller of parse_odp_key_mask_attr() that > needs to understand a ufid, which is odp_flow_from_string(). Maybe the > code to parse a ufid could go there instead. > > On Thu, Oct 11, 2018 at 01:53:54PM -0700, Yifeng Sun wrote: > > Thanks for the review. > > > > How about returning a -EINVAL in this case? > > > > On Thu, Oct 11, 2018 at 1:39 PM Ben Pfaff <blp@ovn.org> wrote: > > > > > On Tue, Oct 09, 2018 at 03:39:18PM -0700, Yifeng Sun wrote: > > > > When parse_odp_key_mask_attr runs into ufid, it returns length of > ufid > > > > without appending data into ofpbufs. This commit adds additional > > > > checking for this case. > > > > > > > > Found this bug when debugging > > > > https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=10850, > > > > but not certain it is related. > > > > > > > > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> > > > > > > Thanks for the fix. > > > > > > It might be better to come up with a way to disallow a ufid here, since > > > it doesn't make sense to put a ufid into a "set" action. > > > >
diff --git a/lib/odp-util.c b/lib/odp-util.c index d482d5bcf968..f53530db40aa 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -2216,9 +2216,10 @@ parse_odp_action(const char *s, const struct simap *port_names, struct nlattr mask[1024 / sizeof(struct nlattr)]; struct ofpbuf maskbuf = OFPBUF_STUB_INITIALIZER(mask); struct nlattr *nested, *key; - size_t size; + size_t size, old_size; start_ofs = nl_msg_start_nested(actions, OVS_ACTION_ATTR_SET); + old_size = actions->size; retval = parse_odp_key_mask_attr(s + 4, port_names, actions, &maskbuf); if (retval < 0) { ofpbuf_uninit(&maskbuf); @@ -2233,7 +2234,7 @@ parse_odp_action(const char *s, const struct simap *port_names, key = nested + 1; size = nl_attr_get_size(mask); - if (size == nl_attr_get_size(key)) { + if (old_size != actions->size && size == nl_attr_get_size(key)) { /* Change to masked set action if not fully masked. */ if (!is_all_ones(mask + 1, size)) { /* Remove padding of eariler key payload */
When parse_odp_key_mask_attr runs into ufid, it returns length of ufid without appending data into ofpbufs. This commit adds additional checking for this case. Found this bug when debugging https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=10850, but not certain it is related. Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> --- lib/odp-util.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)