[ovs-dev] [PATCH v3 2/3] ofp-parse: Harden checking with group selection_method.

[Jarno Rajahalme]

Only allow fields when "selection_method=hash".  Only allow
selection_method_param when a non-nil selection_method is given.

Signed-off-by: Jarno Rajahalme <jarno at ovn.org>
---
v3: New patch for v3.

 lib/ofp-parse.c          | 12 ++++++++++++
 tests/ofproto-dpif.at    | 10 ++++++++++
 utilities/ovs-ofctl.8.in | 16 ++++++++--------
 3 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 92c4693..2980a1d 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -1668,6 +1668,18 @@ parse_ofp_group_mod_str__(struct ofputil_group_mod *gm, int command,
         goto out;
     }
 
+    /* Exclude fields for non "hash" selection method. */
+    if (strcmp(gm->props.selection_method, "hash") &&
+        gm->props.fields.values_size) {
+        error = xstrdup("fields may only be specified with \"selection_method=hash\"");
+        goto out;
+    }
+    /* Exclude selection_method_param if no selection_method is given. */
+    if (gm->props.selection_method[0] == 0
+        && gm->props.selection_method_param != 0) {
+        error = xstrdup("selection_method_param is only allowed with \"selection_method\"");
+        goto out;
+    }
     if (fields & F_COMMAND_BUCKET_ID) {
         if (!(fields & F_COMMAND_BUCKET_ID_ALL || had_command_bucket_id)) {
             error = xstrdup("must specify a command bucket id");
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 2978cc5..cc38858 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -449,6 +449,16 @@ AT_CHECK([sort results | uniq | sed 's/1[[01]]/1?/'], [0],
   [Datapath actions: 1?
 ])
 
+# Check that fields are rejected without "selection_method=hash".
+AT_CHECK([ovs-ofctl -O OpenFlow15 add-group br0 'group_id=1235,type=select,fields(eth_dst,ip_dst,tcp_dst),bucket=output:10,bucket=output:11'], 1, [], [dnl
+ovs-ofctl: fields may only be specified with "selection_method=hash"
+])
+
+# Check that selection_method_param without selection_method is rejected.
+AT_CHECK([ovs-ofctl -O OpenFlow15 add-group br0 'group_id=1235,type=select,selection_method_param=1,bucket=output:10,bucket=output:11'], 1, [], [dnl
+ovs-ofctl: selection_method_param is only allowed with "selection_method"
+])
+
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
index b648ab8..675c308 100644
--- a/utilities/ovs-ofctl.8.in
+++ b/utilities/ovs-ofctl.8.in
@@ -2887,14 +2887,14 @@ when using Open vSwitch 2.4 and later with OpenFlow 1.5 and later.
 .IP \fBfields\fR=\fIfield\fR
 .IQ \fBfields(\fIfield\fR[\fB=\fImask\fR]\fR...\fB)\fR
 The field parameters to selection method selected by the
-\fBselection_method\fR field.  The syntax is described in \fBFlow Syntax\fR
-with the additional restrictions that if a value is provided it is
-treated as a wildcard mask and wildcard masks following a slash are
-prohibited. The pre-requisites of fields must be provided by any flows that
-output to the group. The use of the fields is defined by the lower-layer
-that implements the \fBselection_method\fR.  They are optional if the
-\fBselection_method\fR field is specified as a non-empty string.
-Prohibited otherwise. The default is no fields.
+\fBselection_method\fR field.  The syntax is described in \fBFlow
+Syntax\fR with the additional restrictions that if a value is provided
+it is treated as a wildcard mask and wildcard masks following a slash
+are prohibited. The pre-requisites of fields must be provided by any
+flows that output to the group.  The use of the fields is defined by
+the lower-layer that implements the \fBselection_method\fR.  They are
+optional if the \fBselection_method\fR field is specified as ``hash',
+prohibited otherwise.  The default is no fields.
 .IP
 This option will use a Netronome OpenFlow extension which is only supported
 when using Open vSwitch 2.4 and later with OpenFlow 1.5 and later.
-- 
2.1.4