ofp-actions: enforce valid range for table_id in goto_table instruction

Found a bug that OVS allows goto_table_id to be smaller than (or equal to)
the current table id where the flow resides. It potentially creates an
infinite loop when composing actions for a packet. To fix it, we just let
OVS returns an error message to prevent such flow to be programmed.

Signed-off-by: Jing Ai <jinga@google.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>

diff --git a/AUTHORS b/AUTHORS
index c887599..91d4dfd 100644 (file)
--- a/AUTHORS
+++ b/AUTHORS
@@ -46,6 +46,7 @@ Jarno Rajahalme         jarno.rajahalme@nsn.com
 Jean Tourrilhes         jt@hpl.hp.com
 Jeremy Stribling        strib@nicira.com
 Jesse Gross             jesse@nicira.com
+Jing Ai                 jinga@google.com
 Joe Perches             joe@perches.com
 Joe Stringer            joe@wand.net.nz
 Jun Nakajima            jun.nakajima@intel.com
@@ -154,7 +155,6 @@ Jari Sundell            sundell.software@gmail.com
 Jed Daniels             openvswitch@jeddaniels.com
 Jeff Merrick            jmerrick@vmware.com
 Jeongkeun Lee           jklee@hp.com
-Jing Ai                 ai_jing2000@hotmail.com
 Joan Cirer              joan@ev0.net
 John Galgay             john@galgay.net
 Kevin Mancuso           kevin.mancuso@rackspace.com

diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index c98e29a..58d0098 100644 (file)
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -1052,6 +1052,7 @@ ofpacts_pull_openflow11_actions(struct ofpbuf *openflow,
 enum ofperr
 ofpacts_pull_openflow11_instructions(struct ofpbuf *openflow,
                                      unsigned int instructions_len,
+                                     uint8_t table_id,
                                      struct ofpbuf *ofpacts)
 {
     static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
@@ -1119,6 +1120,10 @@ ofpacts_pull_openflow11_instructions(struct ofpbuf *openflow,
 
         oigt = instruction_get_OFPIT11_GOTO_TABLE(
             insts[OVSINST_OFPIT11_GOTO_TABLE]);
+        if (table_id >= oigt->table_id) {
+            error = OFPERR_OFPBRC_BAD_TABLE_ID;
+            goto exit;
+        }
         ogt = ofpact_put_GOTO_TABLE(ofpacts);
         ogt->table_id = oigt->table_id;
     }

diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index ffceb05..9a74bcc 100644 (file)
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -490,6 +490,7 @@ enum ofperr ofpacts_pull_openflow11_actions(struct ofpbuf *openflow,
                                             struct ofpbuf *ofpacts);
 enum ofperr ofpacts_pull_openflow11_instructions(struct ofpbuf *openflow,
                                                  unsigned int instructions_len,
+                                                 uint8_t table_id,
                                                  struct ofpbuf *ofpacts);
 enum ofperr ofpacts_check(const struct ofpact[], size_t ofpacts_len,
                           const struct flow *, int max_ports);

diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 3c8d5a2..cc2dc36 100644 (file)
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -1502,7 +1502,8 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
             return error;
         }
 
-        error = ofpacts_pull_openflow11_instructions(&b, b.size, ofpacts);
+        error = ofpacts_pull_openflow11_instructions(&b, b.size, ofm->table_id,
+                                                     ofpacts);
         if (error) {
             return error;
         }
@@ -2014,7 +2015,8 @@ ofputil_decode_flow_stats_reply(struct ofputil_flow_stats *fs,
         }
 
         if (ofpacts_pull_openflow11_instructions(msg, length - sizeof *ofs -
-                                                 padded_match_len, ofpacts)) {
+                                                 padded_match_len,
+                                                 ofs->table_id, ofpacts)) {
             VLOG_WARN_RL(&bad_ofmsg_rl, "OFPST_FLOW reply bad instructions");
             return EINVAL;
         }

diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at
index 2ecbdb5..b455bb9 100644 (file)
--- a/tests/ofp-actions.at
+++ b/tests/ofp-actions.at
@@ -356,6 +356,10 @@ dnl Goto-Table 1 instruction non-zero padding
 #  7: 01 -> 00
 0001 0008 01 000001
 
+dnl Goto-Table 1 instruction go back to the previous table.
+# bad OF1.1 instructions: OFPBRC_BAD_TABLE_ID
+2,0001 0008 01 000000
+
 dnl Goto-Table 1
 # actions=goto_table:1
 0001 0008 01 000000

diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 24b9434..48f0fbf 100644 (file)
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -2607,18 +2607,29 @@ ofctl_parse_ofp11_instructions(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
         enum ofperr error;
         size_t size;
         struct ds s;
+        const char *table_id;
+        char *instructions;
+
+        /* Parse table_id separated with the follow-up instructions by ",", if
+         * any. */
+        instructions = ds_cstr(&in);
+        table_id = NULL;
+        if (strstr(instructions, ",")) {
+            table_id = strsep(&instructions, ",");
+        }
 
         /* Parse hex bytes. */
         ofpbuf_init(&of11_in, 0);
-        if (ofpbuf_put_hex(&of11_in, ds_cstr(&in), NULL)[0] != '\0') {
+        if (ofpbuf_put_hex(&of11_in, instructions, NULL)[0] != '\0') {
             ovs_fatal(0, "Trailing garbage in hex data");
         }
 
         /* Convert to ofpacts. */
         ofpbuf_init(&ofpacts, 0);
         size = of11_in.size;
-        error = ofpacts_pull_openflow11_instructions(&of11_in, of11_in.size,
-                                                     &ofpacts);
+        error = ofpacts_pull_openflow11_instructions(
+            &of11_in, of11_in.size, table_id ? atoi(table_id) : 0,
+            &ofpacts);
         if (error) {
             printf("bad OF1.1 instructions: %s\n\n", ofperr_get_name(error));
             ofpbuf_uninit(&ofpacts);