Skip to content

Commit

Permalink
ofproto: Fix OVS crash when reverting old flows in bundle commit
Browse files Browse the repository at this point in the history
During bundle commit flows which are added in bundle are applied
to ofproto in-order. In case if a flow cannot be added (e.g. flow
action is go-to group id which does not exist), OVS tries to
revert back all previous flows which were successfully applied
from the same bundle. This is possible since OVS maintains list
of old flows which were replaced by flows from the bundle.

While reinserting old flows ovs asserts due to check on rule
state != RULE_INITIALIZED.  This will work only for new flows, but
for old flow the rule state will be RULE_REMOVED. This is causing
an assert and OVS crash.

The ovs assert check should be modified to != RULE_INSERTED to prevent
any existing rule being re-inserted and allow new rules and old rules
(in case of revert) to get inserted.

Here is an example to trigger the assert:

$ ovs-vsctl add-br br-test -- set Bridge br-test datapath_type=netdev

$ cat flows.txt
flow add table=1,priority=0,in_port=2,actions=NORMAL
flow add table=1,priority=0,in_port=3,actions=NORMAL

$ ovs-ofctl dump-flows -OOpenflow13 br-test
 cookie=0x0, duration=2.465s, table=1, n_packets=0, n_bytes=0, priority=0,in_port=2 actions=NORMAL
 cookie=0x0, duration=2.465s, table=1, n_packets=0, n_bytes=0, priority=0,in_port=3 actions=NORMAL

$ cat flow-modify.txt
flow modify table=1,priority=0,in_port=2,actions=drop
flow modify table=1,priority=0,in_port=3,actions=group:10

$ ovs-ofctl bundle br-test flow-modify.txt -OOpenflow13

First flow rule will be modified since it is a valid rule. However second
rule is invalid since no group with id 10 exists. Bundle commit tries to
revert (insert) the first rule to old flow which results in ovs_assert at
ofproto_rule_insert__() since old rule->state = RULE_REMOVED.

Signed-off-by: Vishal Deep Ajmera <vishal.deep.ajmera@ericsson.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
  • Loading branch information
vishaldeepajmera authored and blp committed Jun 18, 2018
1 parent f9df636 commit 0befd1f
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion ofproto/ofproto.c
Original file line number Diff line number Diff line change
Expand Up @@ -8465,7 +8465,7 @@ ofproto_rule_insert__(struct ofproto *ofproto, struct rule *rule)
const struct rule_actions *actions = rule_get_actions(rule);

/* A rule may not be reinserted. */
ovs_assert(rule->state == RULE_INITIALIZED);
ovs_assert(rule->state != RULE_INSERTED);

if (rule->hard_timeout || rule->idle_timeout) {
ovs_list_insert(&ofproto->expirable, &rule->expirable);
Expand Down

1 comment on commit 0befd1f

@abergmann
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've contacted MITRE and they have assigned CVE-2018-17205 to this issue.

Please # to comment.