Re: switchd(8): add flow_mod validation
On Mon, Oct 24, 2016 at 07:05:08PM +0200, Rafael Zalamena wrote:
> On Wed, Oct 12, 2016 at 05:39:17PM +0200, Rafael Zalamena wrote:
> > This diff teaches switchd(8) how to validate flow_mod messages, more
> > specifically the flow instructions and actions. The oxm validations
> > were already implemented so we get them for free here.
>
> I've updated the flow_mod diff to also include the following changes:
> - Better loop detection like I did for tcpdump(8);
> - A small fix in packet-in OXM parsing (see note below);
> - Reuse actions validation for packet-out and implement missing
>payload truncation check;
> - Moved the new code away from packet-out to make diff looks less
>confusing;
>
> Note:
> In packet-in we shouldn't use omlen with header size included, it only
> works because the padding is zeroed out. To avoid one more loop and
> errornous zero header reading we should remove the header size.
I broke the last diff into more pieces, one of them was the header and it
was commited last week. So here is the new diff for the flow mod validation.
ok?
Index: ofp13.c
===
RCS file: /home/obsdcvs/src/usr.sbin/switchd/ofp13.c,v
retrieving revision 1.21
diff -u -p -r1.21 ofp13.c
--- ofp13.c 13 Oct 2016 08:29:14 - 1.21
+++ ofp13.c 31 Oct 2016 16:00:10 -
@@ -59,6 +59,12 @@ int ofp13_features_reply(struct switchd
int ofp13_validate_error(struct switchd *,
struct sockaddr_storage *, struct sockaddr_storage *,
struct ofp_header *, struct ibuf *);
+int ofp13_validate_action(struct switchd *, struct ofp_header *,
+ struct ibuf *, off_t *, struct ofp_action_header *);
+int ofp13_validate_instruction(struct switchd *, struct ofp_header *,
+ struct ibuf *, off_t *, struct ofp_instruction *);
+int ofp13_validate_flow_mod(struct switchd *, struct sockaddr_storage *,
+ struct sockaddr_storage *, struct ofp_header *, struct ibuf *);
int ofp13_validate_oxm_basic(struct ibuf *, off_t, int, uint8_t);
int ofp13_validate_oxm(struct switchd *, struct ofp_ox_match *,
struct ofp_header *, struct ibuf *, off_t);
@@ -129,7 +135,7 @@ struct ofp_callback ofp13_callbacks[] =
{ OFP_T_FLOW_REMOVED, ofp13_flow_removed, NULL },
{ OFP_T_PORT_STATUS,NULL, NULL },
{ OFP_T_PACKET_OUT, NULL, ofp13_validate_packet_out },
- { OFP_T_FLOW_MOD, NULL, NULL },
+ { OFP_T_FLOW_MOD, NULL, ofp13_validate_flow_mod },
{ OFP_T_GROUP_MOD, NULL, NULL },
{ OFP_T_PORT_MOD, NULL, NULL },
{ OFP_T_TABLE_MOD, NULL, NULL },
@@ -646,6 +652,298 @@ ofp13_features_reply(struct switchd *sc,
#endif
ofp13_setconfig(sc, con, OFP_CONFIG_FRAG_NORMAL,
OFP_CONTROLLER_MAXLEN_NO_BUFFER);
+
+ return (0);
+}
+
+int
+ofp13_validate_action(struct switchd *sc, struct ofp_header *oh,
+struct ibuf *ibuf, off_t *off, struct ofp_action_header *ah)
+{
+ struct ofp_action_output*ao;
+ struct ofp_action_mpls_ttl *amt;
+ struct ofp_action_push *ap;
+ struct ofp_action_pop_mpls *apm;
+ struct ofp_action_group *ag;
+ struct ofp_action_nw_ttl*ant;
+ struct ofp_action_set_field *asf;
+ struct ofp_action_set_queue *asq;
+ struct ofp_ox_match *oxm;
+ size_t len;
+ int type;
+ off_tmoff;
+
+ type = ntohs(ah->ah_type);
+ len = ntohs(ah->ah_len);
+
+ switch (type) {
+ case OFP_ACTION_OUTPUT:
+ if (len != sizeof(*ao))
+ return (-1);
+ if ((ao = ibuf_seek(ibuf, *off, sizeof(*ao))) == NULL)
+ return (-1);
+
+ *off += len;
+ log_debug("\t\taction %s len %lu port %s max_len %d",
+ print_map(type, ofp_action_map), len,
+ print_map(ntohl(ao->ao_port), ofp_port_map),
+ ntohs(ao->ao_max_len));
+ break;
+ case OFP_ACTION_SET_MPLS_TTL:
+ if (len != sizeof(*amt))
+ return (-1);
+ if ((amt = ibuf_seek(ibuf, *off, sizeof(*amt))) == NULL)
+ return (-1);
+
+ *off += len;
+ log_debug("\t\taction %s len %lu ttl %d",
+ print_map(type, ofp_action_map), len, amt->amt_ttl);
+ break;
+ case OFP_ACTION_PUSH_VLAN:
+ case OFP_ACTION_PUSH_MPLS:
+ case OFP_ACTION_PUSH_PBB:
+ if (len != sizeof(*ap))
+ return (-1);
+ if ((ap = ibuf_seek(ibuf, *off, sizeof(*ap))) == NULL)
+ return (-1);
+
+ *off += len;
+
Re: switchd(8): add flow_mod validation
On Wed, Oct 12, 2016 at 05:39:17PM +0200, Rafael Zalamena wrote:
> This diff teaches switchd(8) how to validate flow_mod messages, more
> specifically the flow instructions and actions. The oxm validations
> were already implemented so we get them for free here.
I've updated the flow_mod diff to also include the following changes:
- Better loop detection like I did for tcpdump(8);
- A small fix in packet-in OXM parsing (see note below);
- Reuse actions validation for packet-out and implement missing
payload truncation check;
- Moved the new code away from packet-out to make diff looks less
confusing;
Note:
In packet-in we shouldn't use omlen with header size included, it only
works because the padding is zeroed out. To avoid one more loop and
errornous zero header reading we should remove the header size.
ok?
Index: sys/net/ofp.h
===
RCS file: /home/obsdcvs/src/sys/net/ofp.h,v
retrieving revision 1.2
diff -u -p -r1.2 ofp.h
--- sys/net/ofp.h 30 Sep 2016 12:40:00 - 1.2
+++ sys/net/ofp.h 12 Oct 2016 15:22:59 -
@@ -315,14 +315,14 @@ struct ofp_action_push {
uint16_tap_type;
uint16_tap_len;
uint16_tap_ethertype;
- uint8_t pad[2];
+ uint8_t ap_pad[2];
} __packed;
struct ofp_action_pop_mpls {
uint16_tapm_type;
uint16_tapm_len;
uint16_tapm_ethertype;
- uint8_t pad[2];
+ uint8_t apm_pad[2];
} __packed;
struct ofp_action_group {
@@ -342,6 +342,12 @@ struct ofp_action_set_field {
uint16_tasf_type;
uint16_tasf_len;
uint8_t asf_field[4];
+} __packed;
+
+struct ofp_action_set_queue {
+ uint16_tasq_type;
+ uint16_tasq_len;
+ uint32_tasq_queue_id;
} __packed;
/* Packet-Out Message */
Index: usr.sbin/switchd/ofp13.c
===
RCS file: /home/obsdcvs/src/usr.sbin/switchd/ofp13.c,v
retrieving revision 1.21
diff -u -p -r1.21 ofp13.c
--- usr.sbin/switchd/ofp13.c13 Oct 2016 08:29:14 - 1.21
+++ usr.sbin/switchd/ofp13.c24 Oct 2016 16:49:46 -
@@ -59,6 +59,12 @@ int ofp13_features_reply(struct switchd
int ofp13_validate_error(struct switchd *,
struct sockaddr_storage *, struct sockaddr_storage *,
struct ofp_header *, struct ibuf *);
+int ofp13_validate_action(struct switchd *, struct ofp_header *,
+ struct ibuf *, off_t *, struct ofp_action_header *);
+int ofp13_validate_instruction(struct switchd *, struct ofp_header *,
+ struct ibuf *, off_t *, struct ofp_instruction *);
+int ofp13_validate_flow_mod(struct switchd *, struct sockaddr_storage *,
+ struct sockaddr_storage *, struct ofp_header *, struct ibuf *);
int ofp13_validate_oxm_basic(struct ibuf *, off_t, int, uint8_t);
int ofp13_validate_oxm(struct switchd *, struct ofp_ox_match *,
struct ofp_header *, struct ibuf *, off_t);
@@ -129,7 +135,7 @@ struct ofp_callback ofp13_callbacks[] =
{ OFP_T_FLOW_REMOVED, ofp13_flow_removed, NULL },
{ OFP_T_PORT_STATUS,NULL, NULL },
{ OFP_T_PACKET_OUT, NULL, ofp13_validate_packet_out },
- { OFP_T_FLOW_MOD, NULL, NULL },
+ { OFP_T_FLOW_MOD, NULL, ofp13_validate_flow_mod },
{ OFP_T_GROUP_MOD, NULL, NULL },
{ OFP_T_PORT_MOD, NULL, NULL },
{ OFP_T_TABLE_MOD, NULL, NULL },
@@ -421,6 +427,7 @@ ofp13_validate_packet_in(struct switchd
log_debug("\tmatch type %s length %zu (padded to %zu)",
print_map(ntohs(om->om_type), ofp_match_map),
mlen, OFP_ALIGN(mlen) + ETHER_ALIGN);
+ mlen -= sizeof(*om);
/* current match offset, aligned offset after all matches */
moff = off + sizeof(*om);
@@ -468,10 +475,9 @@ ofp13_validate_packet_out(struct switchd
struct ofp_header *oh, struct ibuf *ibuf)
{
struct ofp_packet_out *pout;
- size_t len;
- off_toff;
+ size_t len, plen, diff;
+ off_toff, noff;
struct ofp_action_header*ah;
- struct ofp_action_output*ao;
off = 0;
if ((pout = ibuf_seek(ibuf, off, sizeof(*pout))) == NULL) {
@@ -480,36 +486,43 @@ ofp13_validate_packet_out(struct switchd
return (-1);
}
- log_debug("\tbuffer %d port %s "
- "actions length %u",
+ off += sizeof(*pout);
+ len = ntohs(pout->pout_actions_len);
+ log_debug("\tbuffer %d in_port %s actions_len %lu",
ntohl(pout->pout_buffer_id),
- print_map(ntohl(pout->pout_in_port),
switchd(8): add flow_mod validation
This diff teaches switchd(8) how to validate flow_mod messages, more
specifically the flow instructions and actions. The oxm validations
were already implemented so we get them for free here.
ok?
Index: sys/net/ofp.h
===
RCS file: /cvs/src/sys/net/ofp.h,v
retrieving revision 1.2
diff -u -p -r1.2 ofp.h
--- sys/net/ofp.h 30 Sep 2016 12:40:00 - 1.2
+++ sys/net/ofp.h 12 Oct 2016 15:36:30 -
@@ -315,14 +315,14 @@ struct ofp_action_push {
uint16_tap_type;
uint16_tap_len;
uint16_tap_ethertype;
- uint8_t pad[2];
+ uint8_t ap_pad[2];
} __packed;
struct ofp_action_pop_mpls {
uint16_tapm_type;
uint16_tapm_len;
uint16_tapm_ethertype;
- uint8_t pad[2];
+ uint8_t apm_pad[2];
} __packed;
struct ofp_action_group {
@@ -342,6 +342,12 @@ struct ofp_action_set_field {
uint16_tasf_type;
uint16_tasf_len;
uint8_t asf_field[4];
+} __packed;
+
+struct ofp_action_set_queue {
+ uint16_tasq_type;
+ uint16_tasq_len;
+ uint32_tasq_queue_id;
} __packed;
/* Packet-Out Message */
Index: usr.sbin/switchd/ofp13.c
===
RCS file: /cvs/src/usr.sbin/switchd/ofp13.c,v
retrieving revision 1.20
diff -u -p -r1.20 ofp13.c
--- usr.sbin/switchd/ofp13.c12 Oct 2016 15:18:56 - 1.20
+++ usr.sbin/switchd/ofp13.c12 Oct 2016 15:36:30 -
@@ -54,6 +54,12 @@ int ofp13_echo_request(struct switchd *
int ofp13_validate_error(struct switchd *,
struct sockaddr_storage *, struct sockaddr_storage *,
struct ofp_header *, struct ibuf *);
+int ofp13_validate_action(struct switchd *, struct ofp_header *,
+ struct ibuf *, off_t *, struct ofp_action_header *);
+int ofp13_validate_instruction(struct switchd *, struct ofp_header *,
+ struct ibuf *, off_t *, struct ofp_instruction *);
+int ofp13_validate_flow_mod(struct switchd *, struct sockaddr_storage *,
+ struct sockaddr_storage *, struct ofp_header *, struct ibuf *);
int ofp13_validate_oxm_basic(struct ibuf *, off_t, int, uint8_t);
int ofp13_validate_oxm(struct switchd *, struct ofp_ox_match *,
struct ofp_header *, struct ibuf *, off_t);
@@ -121,7 +127,7 @@ struct ofp_callback ofp13_callbacks[] =
{ OFP_T_FLOW_REMOVED, ofp13_flow_removed, NULL },
{ OFP_T_PORT_STATUS,NULL, NULL },
{ OFP_T_PACKET_OUT, NULL, ofp13_validate_packet_out },
- { OFP_T_FLOW_MOD, NULL, NULL },
+ { OFP_T_FLOW_MOD, NULL, ofp13_validate_flow_mod },
{ OFP_T_GROUP_MOD, NULL, NULL },
{ OFP_T_PORT_MOD, NULL, NULL },
{ OFP_T_TABLE_MOD, NULL, NULL },
@@ -501,6 +507,274 @@ ofp13_validate_packet_out(struct switchd
if (pout->pout_buffer_id == (uint32_t)-1)
break;
off += ntohs(ah->ah_len);
+ }
+
+ return (0);
+}
+
+int
+ofp13_validate_action(struct switchd *sc, struct ofp_header *oh,
+struct ibuf *ibuf, off_t *off, struct ofp_action_header *ah)
+{
+ struct ofp_action_output*ao;
+ struct ofp_action_mpls_ttl *amt;
+ struct ofp_action_push *ap;
+ struct ofp_action_pop_mpls *apm;
+ struct ofp_action_group *ag;
+ struct ofp_action_nw_ttl*ant;
+ struct ofp_action_set_field *asf;
+ struct ofp_action_set_queue *asq;
+ struct ofp_ox_match *oxm;
+ int len, type;
+ off_tmoff;
+
+ type = ntohs(ah->ah_type);
+ len = ntohs(ah->ah_len);
+ switch (type) {
+ case OFP_ACTION_OUTPUT:
+ if ((ao = ibuf_seek(ibuf, *off, sizeof(*ao))) == NULL)
+ return (-1);
+
+ *off += len;
+ log_debug("\t\taction %s len %d port %u max_len %d",
+ print_map(type, ofp_action_map), len, ntohl(ao->ao_port),
+ ntohs(ao->ao_max_len));
+ break;
+ case OFP_ACTION_SET_MPLS_TTL:
+ if ((amt = ibuf_seek(ibuf, *off, sizeof(*amt))) == NULL)
+ return (-1);
+
+ *off += len;
+ log_debug("\t\taction %s len %d ttl %d",
+ print_map(type, ofp_action_map), len, amt->amt_ttl);
+ break;
+ case OFP_ACTION_PUSH_VLAN:
+ case OFP_ACTION_PUSH_MPLS:
+ case OFP_ACTION_PUSH_PBB:
+ if ((ap = ibuf_seek(ibuf, *off, sizeof(*ap))) == NULL)
+ return (-1);
+
+ *off += len;
+ log_debug("\t\taction
