diff mbox series

[ovs-dev] ofproto-dpif-xlate: Fix check_pkt_larger incomplete translation.

Message ID 20211029172648.3859172-1-numans@ovn.org
State Changes Requested
Headers show
Series [ovs-dev] ofproto-dpif-xlate: Fix check_pkt_larger incomplete translation. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test fail github build: failed

Commit Message

Numan Siddique Oct. 29, 2021, 5:26 p.m. UTC 
From: Numan Siddique <numans@ovn.org>

xlate_check_pkt_larger() sets ctx->exit to 'true' at the end
causing the translation to stop. This results in incomplete
datapath rules.

For example, for the below OF rules configured on a bridge,

table=0,in_port=1 actions=load:0x1->NXM_NX_REG1[[]],resubmit(,1),load:0x2->NXM_NX_REG1[[]],resubmit(,1),load:0x3->NXM_NX_REG1[[]],resubmit(,1)
table=1,in_port=1,reg1=0x1 actions=check_pkt_larger(200)->NXM_NX_REG0[[0]],resubmit(,4)
table=1,in_port=1,reg1=0x2 actions=output:2
table=1,in_port=1,reg1=0x3 actions=output:4
table=4,in_port=1 actions=output:3

the datapath flow should be

check_pkt_len(size=200,gt(3),le(3)),2,4

But right now it is:

check_pkt_len(size=200,gt(3),le(3))

Actions after the first resubmit(,1) in the first flow in table 0
are never applied.  This patch fixes this issue.

Fixes: 5b34f8fc3b38 ("Add a new OVS action check_pkt_larger")
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2018365
Reported-by: Ihar Hrachyshka <ihrachys@redhat.com>
Signed-off-by: Numan Siddique <numans@ovn.org>
---
 ofproto/ofproto-dpif-xlate.c |  9 ++++++++-
 tests/ofproto-dpif.at        | 17 +++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

Comments

Ilya Maximets Nov. 3, 2021, 2:33 p.m. UTC | #1 
On 10/29/21 19:26, numans@ovn.org wrote:
> From: Numan Siddique <numans@ovn.org>
> 
> xlate_check_pkt_larger() sets ctx->exit to 'true' at the end
> causing the translation to stop. This results in incomplete
> datapath rules.
> 
> For example, for the below OF rules configured on a bridge,
> 
> table=0,in_port=1 actions=load:0x1->NXM_NX_REG1[[]],resubmit(,1),load:0x2->NXM_NX_REG1[[]],resubmit(,1),load:0x3->NXM_NX_REG1[[]],resubmit(,1)
> table=1,in_port=1,reg1=0x1 actions=check_pkt_larger(200)->NXM_NX_REG0[[0]],resubmit(,4)
> table=1,in_port=1,reg1=0x2 actions=output:2
> table=1,in_port=1,reg1=0x3 actions=output:4
> table=4,in_port=1 actions=output:3
> 
> the datapath flow should be
> 
> check_pkt_len(size=200,gt(3),le(3)),2,4
> 
> But right now it is:
> 
> check_pkt_len(size=200,gt(3),le(3))
> 
> Actions after the first resubmit(,1) in the first flow in table 0
> are never applied.  This patch fixes this issue.
> 
> Fixes: 5b34f8fc3b38 ("Add a new OVS action check_pkt_larger")
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2018365
> Reported-by: Ihar Hrachyshka <ihrachys@redhat.com>
> Signed-off-by: Numan Siddique <numans@ovn.org>
> ---
>  ofproto/ofproto-dpif-xlate.c |  9 ++++++++-
>  tests/ofproto-dpif.at        | 17 +++++++++++++++++
>  2 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 9d336bc6a6..e22a39a9f4 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -6371,6 +6371,7 @@ xlate_check_pkt_larger(struct xlate_ctx *ctx,
>       * then ctx->exit would be true. Reset to false so that we can
>       * do flow translation for 'IF_LESS_EQUAL' case. finish_freezing()
>       * would have taken care of Undoing the changes done for freeze. */
> +    bool old_exit = ctx->exit;
>      ctx->exit = false;
>  
>      offset_attr = nl_msg_start_nested(
> @@ -6395,7 +6396,7 @@ xlate_check_pkt_larger(struct xlate_ctx *ctx,
>      ctx->was_mpls = old_was_mpls;
>      ctx->conntracked = old_conntracked;
>      ctx->xin->flow = old_flow;
> -    ctx->exit = true;
> +    ctx->exit = old_exit;
>  }
>  
>  static void
> @@ -7192,6 +7193,12 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
>                                                               ofpacts_len);
>              xlate_check_pkt_larger(ctx, ofpact_get_CHECK_PKT_LARGER(a),
>                                     remaining_acts, remaining_acts_len);
> +            if (ctx->xbridge->support.check_pkt_len) {
> +                /* If datapath supports check_pkt_len, then
> +                 * xlate_check_pkt_larger() does the translation for the
> +                 * ofpacts following 'a'. */
> +                a = ofpact_end(ofpacts, ofpacts_len);

Hi, Numan.

Looks like this change triggers heap-buffer-overflow.
See the ASAN report in ovsrobot's run for details.

Best regards, Ilya Maximets.
Numan Siddique Nov. 3, 2021, 4:56 p.m. UTC | #2 
On Wed, Nov 3, 2021 at 10:34 AM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> On 10/29/21 19:26, numans@ovn.org wrote:
> > From: Numan Siddique <numans@ovn.org>
> >
> > xlate_check_pkt_larger() sets ctx->exit to 'true' at the end
> > causing the translation to stop. This results in incomplete
> > datapath rules.
> >
> > For example, for the below OF rules configured on a bridge,
> >
> > table=0,in_port=1 actions=load:0x1->NXM_NX_REG1[[]],resubmit(,1),load:0x2->NXM_NX_REG1[[]],resubmit(,1),load:0x3->NXM_NX_REG1[[]],resubmit(,1)
> > table=1,in_port=1,reg1=0x1 actions=check_pkt_larger(200)->NXM_NX_REG0[[0]],resubmit(,4)
> > table=1,in_port=1,reg1=0x2 actions=output:2
> > table=1,in_port=1,reg1=0x3 actions=output:4
> > table=4,in_port=1 actions=output:3
> >
> > the datapath flow should be
> >
> > check_pkt_len(size=200,gt(3),le(3)),2,4
> >
> > But right now it is:
> >
> > check_pkt_len(size=200,gt(3),le(3))
> >
> > Actions after the first resubmit(,1) in the first flow in table 0
> > are never applied.  This patch fixes this issue.
> >
> > Fixes: 5b34f8fc3b38 ("Add a new OVS action check_pkt_larger")
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2018365
> > Reported-by: Ihar Hrachyshka <ihrachys@redhat.com>
> > Signed-off-by: Numan Siddique <numans@ovn.org>
> > ---
> >  ofproto/ofproto-dpif-xlate.c |  9 ++++++++-
> >  tests/ofproto-dpif.at        | 17 +++++++++++++++++
> >  2 files changed, 25 insertions(+), 1 deletion(-)
> >
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > index 9d336bc6a6..e22a39a9f4 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -6371,6 +6371,7 @@ xlate_check_pkt_larger(struct xlate_ctx *ctx,
> >       * then ctx->exit would be true. Reset to false so that we can
> >       * do flow translation for 'IF_LESS_EQUAL' case. finish_freezing()
> >       * would have taken care of Undoing the changes done for freeze. */
> > +    bool old_exit = ctx->exit;
> >      ctx->exit = false;
> >
> >      offset_attr = nl_msg_start_nested(
> > @@ -6395,7 +6396,7 @@ xlate_check_pkt_larger(struct xlate_ctx *ctx,
> >      ctx->was_mpls = old_was_mpls;
> >      ctx->conntracked = old_conntracked;
> >      ctx->xin->flow = old_flow;
> > -    ctx->exit = true;
> > +    ctx->exit = old_exit;
> >  }
> >
> >  static void
> > @@ -7192,6 +7193,12 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
> >                                                               ofpacts_len);
> >              xlate_check_pkt_larger(ctx, ofpact_get_CHECK_PKT_LARGER(a),
> >                                     remaining_acts, remaining_acts_len);
> > +            if (ctx->xbridge->support.check_pkt_len) {
> > +                /* If datapath supports check_pkt_len, then
> > +                 * xlate_check_pkt_larger() does the translation for the
> > +                 * ofpacts following 'a'. */
> > +                a = ofpact_end(ofpacts, ofpacts_len);
>
> Hi, Numan.
>
> Looks like this change triggers heap-buffer-overflow.
> See the ASAN report in ovsrobot's run for details.

Oops.  Thanks for pointing this out.  I missed running tests with
libasan locally.

Thanks
Numan

>
> Best regards, Ilya Maximets.
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
diff mbox series

Patch

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 9d336bc6a6..e22a39a9f4 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -6371,6 +6371,7 @@  xlate_check_pkt_larger(struct xlate_ctx *ctx,
      * then ctx->exit would be true. Reset to false so that we can
      * do flow translation for 'IF_LESS_EQUAL' case. finish_freezing()
      * would have taken care of Undoing the changes done for freeze. */
+    bool old_exit = ctx->exit;
     ctx->exit = false;
 
     offset_attr = nl_msg_start_nested(
@@ -6395,7 +6396,7 @@  xlate_check_pkt_larger(struct xlate_ctx *ctx,
     ctx->was_mpls = old_was_mpls;
     ctx->conntracked = old_conntracked;
     ctx->xin->flow = old_flow;
-    ctx->exit = true;
+    ctx->exit = old_exit;
 }
 
 static void
@@ -7192,6 +7193,12 @@  do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
                                                              ofpacts_len);
             xlate_check_pkt_larger(ctx, ofpact_get_CHECK_PKT_LARGER(a),
                                    remaining_acts, remaining_acts_len);
+            if (ctx->xbridge->support.check_pkt_len) {
+                /* If datapath supports check_pkt_len, then
+                 * xlate_check_pkt_larger() does the translation for the
+                 * ofpacts following 'a'. */
+                a = ofpact_end(ofpacts, ofpacts_len);
+            }
             break;
         }
         }
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 31fb163a91..f7c8f98ce5 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -11452,6 +11452,23 @@  Megaflow: recirc_id=0x3,eth,ip,in_port=1,nw_frag=no
 Datapath actions: 4
 ])
 
+ovs-ofctl del-flows br0
+
+AT_DATA([flows.txt], [dnl
+table=0,in_port=1 actions=load:0x1->NXM_NX_REG1[[]],resubmit(,1),load:0x2->NXM_NX_REG1[[]],resubmit(,1),load:0x3->NXM_NX_REG1[[]],resubmit(,1)
+table=1,in_port=1,reg1=0x1 actions=check_pkt_larger(200)->NXM_NX_REG0[[0]],resubmit(,4)
+table=1,in_port=1,reg1=0x2 actions=output:2
+table=1,in_port=1,reg1=0x3 actions=output:4
+table=4,in_port=1 actions=output:3
+])
+
+AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt])
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout])
+AT_CHECK([cat stdout | grep Datapath -B1], [0], [dnl
+Megaflow: recirc_id=0,eth,ip,in_port=1,nw_frag=no
+Datapath actions: check_pkt_len(size=200,gt(3),le(3)),2,4
+])
+
 OVS_VSWITCHD_STOP
 AT_CLEANUP