OpenFlow 1.3 defines packet header patterns of interest using TLVs (OXMs)

that represent various header fields. One place where OXMs are used is in
the sef_field action, which contains one OXM representing the header field
to set, followed by padding to align the action in the OpenFlow message to
64 bits. Currently, we assume that a set_field action can contain multiple
OXMs and that they do not need to be padded.

This matches the way we handle OpenFlow messages that contain set_field
actions so that we follow the specs.

OK ori claudio

3 files changed, 76 insertions, 112 deletions

diff --git a/sys/net/switchofp.c b/sys/net/switchofp.c
index f2a34be7add..587a9c99d38 100644
--- a/sys/net/switchofp.c
+++ b/sys/net/switchofp.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: switchofp.c,v 1.75 2019/11/21 17:24:15 akoshibe Exp $	*/
+/*	$OpenBSD: switchofp.c,v 1.76 2019/11/27 17:37:32 akoshibe Exp $	*/
 
 /*
  * Copyright (c) 2016 Kazuya GODA <goda@openbsd.org>
@@ -2174,7 +2174,8 @@ swofp_validate_action(struct switch_softc *sc, struct ofp_action_header *ah,
 		}
 		break;
 	case OFP_ACTION_SET_FIELD:
-		if (ahlen < sizeof(struct ofp_action_set_field)) {
+		if (ahlen < sizeof(struct ofp_action_set_field) ||
+		    ahlen != OFP_ALIGN(ahlen)) {
 			*err = OFP_ERRACTION_LEN;
 			return (-1);
 		}
@@ -2189,22 +2190,37 @@ swofp_validate_action(struct switch_softc *sc, struct ofp_action_header *ah,
 		dptr = (uint8_t *)ah;
 		dptr += sizeof(struct ofp_action_set_field) -
 		    offsetof(struct ofp_action_set_field, asf_field);
-		while (oxmlen > 0) {
-			oxm = (struct ofp_ox_match *)dptr;
-			if (swofp_validate_oxm(oxm, err)) {
-				if (*err == OFP_ERRMATCH_BAD_LEN)
-					*err = OFP_ERRACTION_SET_LEN;
-				else
-					*err = OFP_ERRACTION_SET_TYPE;
+		oxm = (struct ofp_ox_match *)dptr;
+		oxmlen -= sizeof(struct ofp_ox_match);
+		if (oxmlen < oxm->oxm_length) {
+			*err = OFP_ERRACTION_SET_LEN;
+			return (-1);
+		}
+		/* Remainder is padding. */
+		oxmlen -= oxm->oxm_length;
+		if (oxmlen >= OFP_ALIGNMENT) {
+			*err = OFP_ERRACTION_SET_LEN;
+			return (-1);
+		}
 
+		if (swofp_validate_oxm(oxm, err)) {
+			if (*err == OFP_ERRMATCH_BAD_LEN)
+				*err = OFP_ERRACTION_SET_LEN;
+			else
+				*err = OFP_ERRACTION_SET_TYPE;
+			return (-1);
+		}
+
+	 	dptr += sizeof(struct ofp_ox_match) + oxm->oxm_length;
+		while (oxmlen > 0) {
+			if (*dptr != 0) {
+				*err = OFP_ERRACTION_SET_ARGUMENT;
 				return (-1);
 			}
-
-			dptr += sizeof(*oxm) + oxm->oxm_length;
-			oxmlen -= sizeof(*oxm) + oxm->oxm_length;
+			oxmlen--;
+			dptr++;
 		}
 		break;
-
 	default:
 		/* Unknown/unsupported action. */
 		*err = OFP_ERRACTION_TYPE;
diff --git a/usr.sbin/switchd/ofp13.c b/usr.sbin/switchd/ofp13.c
index f02ad1f8840..721302451a7 100644
--- a/usr.sbin/switchd/ofp13.c
+++ b/usr.sbin/switchd/ofp13.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ofp13.c,v 1.46 2019/11/21 06:22:57 akoshibe Exp $	*/
+/*	$OpenBSD: ofp13.c,v 1.47 2019/11/27 17:37:32 akoshibe Exp $	*/
 
 /*
  * Copyright (c) 2013-2016 Reyk Floeter <reyk@openbsd.org>
@@ -780,7 +780,7 @@ ofp13_validate_action(struct switchd *sc, struct ofp_header *oh,
 		    print_map(type, ofp_action_map), len, ant->ant_ttl);
 		break;
 	case OFP_ACTION_SET_FIELD:
-		if (len < sizeof(*asf))
+		if (len < sizeof(*asf) || len != OFP_ALIGN(len))
 			return (-1);
 		if ((asf = ibuf_seek(ibuf, *off, sizeof(*asf))) == NULL)
 			return (-1);
@@ -791,16 +791,14 @@ ofp13_validate_action(struct switchd *sc, struct ofp_header *oh,
 		    print_map(type, ofp_action_map), len);
 
 		len -= sizeof(*asf) - sizeof(asf->asf_field);
-		while (len > 0) {
-			if ((oxm = ibuf_seek(ibuf, moff, sizeof(*oxm)))
-			    == NULL)
-				return (-1);
-			if (ofp13_validate_oxm(sc, oxm, oh, ibuf, moff) == -1)
-				return (-1);
+		if ((oxm = ibuf_seek(ibuf, moff, sizeof(*oxm))) == NULL)
+			return (-1);
+		if (ofp13_validate_oxm(sc, oxm, oh, ibuf, moff) == -1)
+			return (-1);
 
-			len -= sizeof(*oxm) + oxm->oxm_length;
-			moff += sizeof(*oxm) + oxm->oxm_length;
-		}
+		len -= sizeof(*oxm) + oxm->oxm_length;
+		if (len >= OFP_ALIGNMENT)
+			return (-1);
 		break;
 
 	default:
diff --git a/usr.sbin/tcpdump/print-ofp.c b/usr.sbin/tcpdump/print-ofp.c
index 8dd70d4830c..c9037183512 100644
--- a/usr.sbin/tcpdump/print-ofp.c
+++ b/usr.sbin/tcpdump/print-ofp.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: print-ofp.c,v 1.11 2016/11/28 17:47:15 jca Exp $	*/
+/*	$OpenBSD: print-ofp.c,v 1.12 2019/11/27 17:37:32 akoshibe Exp $	*/
 
 /*
  * Copyright (c) 2016 Rafael Zalamena <rzalamena@openbsd.org>
@@ -232,10 +232,39 @@ ofp_print_setconfig(const u_char *bp, u_int length)
 }
 
 void
+ofp_print_oxm_field(const u_char *bp, u_int length, int omlen, int once)
+{
+	struct ofp_ox_match		*oxm;
+
+	do {
+		if (length < sizeof(*oxm)) {
+			printf(" [|OpenFlow]");
+			return;
+		}
+
+		oxm = (struct ofp_ox_match *)bp;
+		bp += sizeof(*oxm);
+		length -= sizeof(*oxm);
+		if (length < oxm->oxm_length) {
+			printf(" [|OpenFlow]");
+			return;
+		}
+
+		ofp_print_oxm(oxm, bp, length);
+		bp += oxm->oxm_length;
+		length -= oxm->oxm_length;
+
+		if (once)
+			return;
+
+		omlen -= min(sizeof(*oxm) + oxm->oxm_length, omlen);
+	} while (omlen > 0);
+}
+
+void
 ofp_print_packetin(const u_char *bp, u_int length)
 {
 	struct ofp_packet_in		*pin;
-	struct ofp_ox_match		*oxm;
 	int				 omtype, omlen;
 	int				 haspacket = 0;
 	const u_char			*pktptr;
@@ -276,27 +305,7 @@ ofp_print_packetin(const u_char *bp, u_int length)
 	if (omlen == 0)
 		goto print_packet;
 
- parse_next_oxm:
-	if (length < sizeof(*oxm)) {
-		printf(" [|OpenFlow]");
-		return;
-	}
-
-	oxm = (struct ofp_ox_match *)bp;
-	bp += sizeof(*oxm);
-	length -= sizeof(*oxm);
-	if (length < oxm->oxm_length) {
-		printf(" [|OpenFlow]");
-		return;
-	}
-
-	ofp_print_oxm(oxm, bp, length);
-
-	bp += oxm->oxm_length;
-	length -= oxm->oxm_length;
-	omlen -= min(sizeof(*oxm) + oxm->oxm_length, omlen);
-	if (omlen)
-		goto parse_next_oxm;
+	ofp_print_oxm_field(bp, length, omlen, 0);
 
  print_packet:
 	if (haspacket == 0)
@@ -322,7 +331,6 @@ void
 ofp_print_flowremoved(const u_char *bp, u_int length)
 {
 	struct ofp_flow_removed			*fr;
-	struct ofp_ox_match			*oxm;
 	int					 omtype, omlen;
 
 	if (length < sizeof(*fr)) {
@@ -355,27 +363,7 @@ ofp_print_flowremoved(const u_char *bp, u_int length)
 	bp += sizeof(*fr);
 	length -= sizeof(*fr);
 
- parse_next_oxm:
-	if (length < sizeof(*oxm)) {
-		printf(" [|OpenFlow]");
-		return;
-	}
-
-	oxm = (struct ofp_ox_match *)bp;
-	bp += sizeof(*oxm);
-	length -= sizeof(*oxm);
-	if (length < oxm->oxm_length) {
-		printf(" [|OpenFlow]");
-		return;
-	}
-
-	ofp_print_oxm(oxm, bp, length);
-
-	bp += oxm->oxm_length;
-	length -= oxm->oxm_length;
-	omlen -= min(sizeof(*oxm) + oxm->oxm_length, omlen);
-	if (omlen)
-		goto parse_next_oxm;
+	ofp_print_oxm_field(bp, length, omlen, 0);
 }
 
 void
@@ -453,7 +441,6 @@ void
 ofp_print_flowmod(const u_char *bp, u_int length)
 {
 	struct ofp_flow_mod			*fm;
-	struct ofp_ox_match			*oxm;
 	struct ofp_instruction			*i;
 	int					 omtype, omlen, ilen;
 	int					 instructionslen, padsize;
@@ -498,27 +485,10 @@ ofp_print_flowmod(const u_char *bp, u_int length)
 		goto parse_next_instruction;
 	}
 
- parse_next_oxm:
-	if (length < sizeof(*oxm)) {
-		printf(" [|OpenFlow]");
-		return;
-	}
-
-	oxm = (struct ofp_ox_match *)bp;
-	bp += sizeof(*oxm);
-	length -= sizeof(*oxm);
-	if (length < oxm->oxm_length) {
-		printf(" [|OpenFlow]");
-		return;
-	}
-
-	ofp_print_oxm(oxm, bp, length);
+	ofp_print_oxm_field(bp, length, omlen, 0);
 
-	bp += oxm->oxm_length;
-	length -= oxm->oxm_length;
-	omlen -= min(sizeof(*oxm) + oxm->oxm_length, omlen);
-	if (omlen)
-		goto parse_next_oxm;
+	bp += omlen;
+	length -= omlen;
 
 	/* Skip padding if any. */
 	if (padsize) {
@@ -1017,7 +987,6 @@ void
 action_print_setfield(const u_char *bp, u_int length)
 {
 	struct ofp_action_set_field	*asf;
-	struct ofp_ox_match		*oxm;
 	int				 omlen;
 
 	if (length < (sizeof(*asf) - AH_UNPADDED)) {
@@ -1030,27 +999,7 @@ action_print_setfield(const u_char *bp, u_int length)
 	if (omlen == 0)
 		return;
 
- parse_next_oxm:
-	if (length < sizeof(*oxm)) {
-		printf(" [|OpenFlow]");
-		return;
-	}
-
-	oxm = (struct ofp_ox_match *)bp;
-	bp += sizeof(*oxm);
-	length -= sizeof(*oxm);
-	if (length < oxm->oxm_length) {
-		printf(" [|OpenFlow]");
-		return;
-	}
-
-	ofp_print_oxm(oxm, bp, length);
-
-	bp += oxm->oxm_length;
-	length -= oxm->oxm_length;
-	omlen -= min(sizeof(*oxm) + oxm->oxm_length, omlen);
-	if (omlen)
-		goto parse_next_oxm;
+	ofp_print_oxm_field(bp, length, omlen, 1);
 }
 
 void
@@ -1094,6 +1043,7 @@ ofp_print_action(struct ofp_action_header *ah, const u_char *bp, u_int length)
 		break;
 
 	case OFP_ACTION_SET_FIELD:
+		action_print_setfield(bp, length);
 		break;
 
 	case OFP_ACTION_COPY_TTL_OUT: