[ovs-dev,2/2] odp-util: Handle returning when parse_odp_key_mask_attr handles ufid

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
Related show

Commit Message

Yifeng Sun Oct. 9, 2018, 10:39 p.m.
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(-)

Comments

Ben Pfaff Oct. 11, 2018, 8:39 p.m. | #1
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.
Yifeng Sun Oct. 11, 2018, 8:53 p.m. | #2
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.
>
Ben Pfaff Oct. 11, 2018, 9:04 p.m. | #3
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.
> >
Yifeng Sun Oct. 11, 2018, 9:34 p.m. | #4
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.
> > >
>

Patch

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  */