From bff7eeb648a03598c57ac9a8288132379640cb2c Mon Sep 17 00:00:00 2001 From: Jing Ai Date: Wed, 5 Jun 2013 13:18:09 -0700 Subject: [PATCH] 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 Signed-off-by: Ben Pfaff --- AUTHORS | 2 +- lib/ofp-actions.c | 5 +++++ lib/ofp-actions.h | 1 + lib/ofp-util.c | 6 ++++-- tests/ofp-actions.at | 4 ++++ utilities/ovs-ofctl.c | 17 ++++++++++++++--- 6 files changed, 29 insertions(+), 6 deletions(-) diff --git a/AUTHORS b/AUTHORS index c88759987..91d4dfd13 100644 --- 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 c98e29a63..58d0098ee 100644 --- 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 ffceb05b7..9a74bcc60 100644 --- 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 3c8d5a254..cc2dc3632 100644 --- 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 2ecbdb517..b455bb933 100644 --- 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 24b943421..48f0fbf4a 100644 --- 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); -- 2.47.0