[ovs-dev] odp-util: Fix parsing corner case for encap_nsh() actions.

Message ID 20171128182032.22981-1-blp@ovn.org
State New
Headers show
Series
  • [ovs-dev] odp-util: Fix parsing corner case for encap_nsh() actions.
Related show

Commit Message

Ben Pfaff Nov. 28, 2017, 6:20 p.m.
When nothing matched, the code would loop forever.

Found with libfuzzer.

Reported-by: Bhargava Shastry <bshastry@sec.t-labs.tu-berlin.de>
Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 lib/odp-util.c | 2 ++
 tests/odp.at   | 8 ++++++++
 2 files changed, 10 insertions(+)

Comments

Jan Scheurich Dec. 1, 2017, 2:25 p.m. | #1
Thanks for spotting this. 

The proposed correction is correct, but the other error cases in the function use the pattern
    ret = -EINVAL;
    goto out;

Can you please align or use
    ret = -EINVAL;
    break;

BR, Jan

Acked-by: Jan Scheurich <jan.scheurich@ericsson.com>

> -----Original Message-----
> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Ben Pfaff
> Sent: Tuesday, 28 November, 2017 19:21
> To: dev@openvswitch.org
> Cc: Ben Pfaff <blp@ovn.org>; Bhargava Shastry <bshastry@sec.t-labs.tu-berlin.de>
> Subject: [ovs-dev] [PATCH] odp-util: Fix parsing corner case for encap_nsh() actions.
> 
> When nothing matched, the code would loop forever.
> 
> Found with libfuzzer.
> 
> Reported-by: Bhargava Shastry <bshastry@sec.t-labs.tu-berlin.de>
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> ---
>  lib/odp-util.c | 2 ++
>  tests/odp.at   | 8 ++++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index 45a890c46aa0..3e30b9ae7719 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -1870,6 +1870,8 @@ parse_odp_encap_nsh_action(const char *s, struct ofpbuf *actions)
>              }
>              continue;
>          }
> +
> +        return -EINVAL;
>      }
>  out:
>      if (ret < 0) {
> diff --git a/tests/odp.at b/tests/odp.at
> index cd01b32d72ef..1a80322890eb 100644
> --- a/tests/odp.at
> +++ b/tests/odp.at
> @@ -362,3 +362,11 @@ AT_CHECK_UNQUOTED([ovstest test-odp parse-actions < actions.txt], [0],
>    [`cat actions.txt`
>  ])
>  AT_CLEANUP
> +
> +AT_SETUP([OVS datapath actions parsing and formatting - invalid forms])
> +dnl This caused a hang in older versions.
> +AT_CHECK([echo 'encap_nsh@:{@' | ovstest test-odp parse-actions
> +], [0], [dnl
> +odp_actions_from_string: error
> +])
> +AT_CLEANUP
> --
> 2.10.2
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ben Pfaff Dec. 1, 2017, 4:45 p.m. | #2
Thanks for the review.

Sure.  I made that change and applied this to master and branch-2.8.

On Fri, Dec 01, 2017 at 02:25:40PM +0000, Jan Scheurich wrote:
> Thanks for spotting this. 
> 
> The proposed correction is correct, but the other error cases in the function use the pattern
>     ret = -EINVAL;
>     goto out;
> 
> Can you please align or use
>     ret = -EINVAL;
>     break;
> 
> BR, Jan
> 
> Acked-by: Jan Scheurich <jan.scheurich@ericsson.com>
> 
> > -----Original Message-----
> > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Ben Pfaff
> > Sent: Tuesday, 28 November, 2017 19:21
> > To: dev@openvswitch.org
> > Cc: Ben Pfaff <blp@ovn.org>; Bhargava Shastry <bshastry@sec.t-labs.tu-berlin.de>
> > Subject: [ovs-dev] [PATCH] odp-util: Fix parsing corner case for encap_nsh() actions.
> > 
> > When nothing matched, the code would loop forever.
> > 
> > Found with libfuzzer.
> > 
> > Reported-by: Bhargava Shastry <bshastry@sec.t-labs.tu-berlin.de>
> > Signed-off-by: Ben Pfaff <blp@ovn.org>
> > ---
> >  lib/odp-util.c | 2 ++
> >  tests/odp.at   | 8 ++++++++
> >  2 files changed, 10 insertions(+)
> > 
> > diff --git a/lib/odp-util.c b/lib/odp-util.c
> > index 45a890c46aa0..3e30b9ae7719 100644
> > --- a/lib/odp-util.c
> > +++ b/lib/odp-util.c
> > @@ -1870,6 +1870,8 @@ parse_odp_encap_nsh_action(const char *s, struct ofpbuf *actions)
> >              }
> >              continue;
> >          }
> > +
> > +        return -EINVAL;
> >      }
> >  out:
> >      if (ret < 0) {
> > diff --git a/tests/odp.at b/tests/odp.at
> > index cd01b32d72ef..1a80322890eb 100644
> > --- a/tests/odp.at
> > +++ b/tests/odp.at
> > @@ -362,3 +362,11 @@ AT_CHECK_UNQUOTED([ovstest test-odp parse-actions < actions.txt], [0],
> >    [`cat actions.txt`
> >  ])
> >  AT_CLEANUP
> > +
> > +AT_SETUP([OVS datapath actions parsing and formatting - invalid forms])
> > +dnl This caused a hang in older versions.
> > +AT_CHECK([echo 'encap_nsh@:{@' | ovstest test-odp parse-actions
> > +], [0], [dnl
> > +odp_actions_from_string: error
> > +])
> > +AT_CLEANUP
> > --
> > 2.10.2
> > 
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Patch

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 45a890c46aa0..3e30b9ae7719 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -1870,6 +1870,8 @@  parse_odp_encap_nsh_action(const char *s, struct ofpbuf *actions)
             }
             continue;
         }
+
+        return -EINVAL;
     }
 out:
     if (ret < 0) {
diff --git a/tests/odp.at b/tests/odp.at
index cd01b32d72ef..1a80322890eb 100644
--- a/tests/odp.at
+++ b/tests/odp.at
@@ -362,3 +362,11 @@  AT_CHECK_UNQUOTED([ovstest test-odp parse-actions < actions.txt], [0],
   [`cat actions.txt`
 ])
 AT_CLEANUP
+
+AT_SETUP([OVS datapath actions parsing and formatting - invalid forms])
+dnl This caused a hang in older versions.
+AT_CHECK([echo 'encap_nsh@:{@' | ovstest test-odp parse-actions
+], [0], [dnl
+odp_actions_from_string: error
+])
+AT_CLEANUP