On Fri, Oct 28, 2016 at 07:56:12PM +0400, Reyk Floeter wrote:
> > On 28.10.2016, at 19:20, Rafael Zalamena <rzalam...@gmail.com> wrote:
> > This diff teaches switch(4) how to do more validations on dynamic input
> > field types, like: ofp_match (has N oxms), ofp_action_header (might be
> > followed by N actions) and ofp_instruction (might have N actions inside).
> >
> > This is important because the internal switch structures reuse the ofp_match
> > and friends from the packet and blindly trusts it to be correct, so to be
> > able to do that we must ensure that we are receiving them correctly.
> > We need to pay special attention to the fields that these macros use:
> > - OFP_OXM_FOREACH;
> > - OFP_ACTION_FOREACH;
> > - OFP_FLOW_MOD_MSG_INSTRUCTION_OFFSET;
> > - OFP_FLOW_MOD_INSTRUCTON_FOREACH;
> > - OFP_I_ACTIONS_FOREACH;
> > - OFP_BUCKETS_FOREACH;
> ---snip---
I've commited most of the small diffs, but one of them turned out to be
different so I sent another email ('switch(4): input validation:
swofp_flow_entry_put_instructions').
Here is the diff after all those commits, with just one small change:
I have implemented set_queue action validation that was missing.
ok?
Index: switchofp.c
===================================================================
RCS file: /cvs/src/sys/net/switchofp.c,v
retrieving revision 1.23
diff -u -p -r1.23 switchofp.c
--- switchofp.c 31 Oct 2016 08:06:27 -0000 1.23
+++ switchofp.c 31 Oct 2016 14:52:13 -0000
@@ -211,8 +211,11 @@ int swofp_flow_cmp_strict(struct swofp_
int swofp_flow_filter(struct swofp_flow_entry *, uint64_t, uint64_t,
uint32_t, uint32_t);
void swofp_flow_timeout(struct switch_softc *);
+int swofp_validate_oxm(struct ofp_ox_match *, int *);
int swofp_validate_flow_match(struct ofp_match *, int *);
-int swofp_validate_flow_instruction(struct ofp_instruction *, int *);
+int swofp_validate_flow_instruction(struct ofp_instruction *, size_t,
+ int *);
+int swofp_validate_action(struct ofp_action_header *, size_t, int *);
/*
* OpenFlow protocol compare oxm
@@ -1807,75 +1810,242 @@ swofp_ox_cmp_ether_addr(struct ofp_ox_ma
}
}
-
-/* TODO: validation for match */
int
-swofp_validate_flow_match(struct ofp_match *om, int *err)
+swofp_validate_oxm(struct ofp_ox_match *oxm, int *err)
{
- struct ofp_oxm_class *handler;
- struct ofp_ox_match *oxm;
+ struct ofp_oxm_class *handler;
+ int length, hasmask;
+ int neededlen;
- OFP_OXM_FOREACH(om, ntohs(om->om_length), oxm) {
- handler = swofp_lookup_oxm_handler(oxm);
- if (handler == NULL ||
- handler->oxm_match == NULL) {
- *err = OFP_ERRMATCH_BAD_FIELD;
- return (-1);
- }
+ handler = swofp_lookup_oxm_handler(oxm);
+ if (handler == NULL || handler->oxm_match == NULL) {
+ *err = OFP_ERRMATCH_BAD_FIELD;
+ return (-1);
+ }
+
+ hasmask = OFP_OXM_GET_HASMASK(oxm);
+ length = oxm->oxm_length;
+
+ neededlen = (hasmask) ?
+ (handler->oxm_len * 2) : (handler->oxm_len);
+ if (oxm->oxm_length != neededlen) {
+ *err = OFP_ERRMATCH_BAD_LEN;
+ return (-1);
}
return (0);
}
int
-swofp_validate_flow_action_set_field(struct ofp_action_set_field *oasf)
+swofp_validate_flow_match(struct ofp_match *om, int *err)
{
- struct ofp_ox_match *oxm;
- struct ofp_oxm_class *handler;
-
- oxm = (struct ofp_ox_match *)oasf->asf_field;
+ struct ofp_ox_match *oxm;
- handler = swofp_lookup_oxm_handler(oxm);
- if (handler == NULL)
- return (OFP_ERRACTION_SET_TYPE);
- if (handler->oxm_set == NULL)
- return (OFP_ERRACTION_SET_TYPE);
+ /*
+ * TODO this function is missing checks for:
+ * - OFP_ERRMATCH_BAD_TAG;
+ * - OFP_ERRMATCH_BAD_VALUE;
+ * - OFP_ERRMATCH_BAD_MASK;
+ * - OFP_ERRMATCH_BAD_PREREQ;
+ * - OFP_ERRMATCH_DUP_FIELD;
+ */
+ OFP_OXM_FOREACH(om, ntohs(om->om_length), oxm) {
+ if (swofp_validate_oxm(oxm, err))
+ return (*err);
+ }
return (0);
}
-/* TODO: validation for instruction */
int
-swofp_validate_flow_instruction(struct ofp_instruction *oi, int *error)
+swofp_validate_flow_instruction(struct ofp_instruction *oi, size_t total,
+ int *err)
{
struct ofp_action_header *oah;
struct ofp_instruction_actions *oia;
+ int ilen;
+
+ ilen = ntohs(oi->i_len);
+ /* Check for bigger than packet or smaller than header. */
+ if (ilen > total || ilen < sizeof(*oi)) {
+ *err = OFP_ERRINST_BAD_LEN;
+ return (-1);
+ }
switch (ntohs(oi->i_type)) {
case OFP_INSTRUCTION_T_GOTO_TABLE:
+ if (ilen != sizeof(struct ofp_instruction_goto_table)) {
+ *err = OFP_ERRINST_BAD_LEN;
+ return (-1);
+ }
+ break;
case OFP_INSTRUCTION_T_WRITE_META:
+ if (ilen != sizeof(struct ofp_instruction_write_metadata)) {
+ *err = OFP_ERRINST_BAD_LEN;
+ return (-1);
+ }
+ break;
case OFP_INSTRUCTION_T_METER:
- case OFP_INSTRUCTION_T_EXPERIMENTER:
+ if (ilen != sizeof(struct ofp_instruction_meter)) {
+ *err = OFP_ERRINST_BAD_LEN;
+ return (-1);
+ }
break;
+
case OFP_INSTRUCTION_T_WRITE_ACTIONS:
case OFP_INSTRUCTION_T_CLEAR_ACTIONS:
- break;
case OFP_INSTRUCTION_T_APPLY_ACTIONS:
+ if (ilen < sizeof(*oia)) {
+ *err = OFP_ERRINST_BAD_LEN;
+ return (-1);
+ }
+
oia = (struct ofp_instruction_actions *)oi;
- OFP_I_ACTIONS_FOREACH(oia, oah) {
- if (ntohs(oah->ah_type) == OFP_ACTION_SET_FIELD) {
- *error = swofp_validate_flow_action_set_field(
- (struct ofp_action_set_field *)oah);
- if (*error)
- return (-1);
+
+ /* Validate actions before iterating over them. */
+ oah = (struct ofp_action_header *)
+ ((uint8_t *)oia + sizeof(*oia));
+ if (swofp_validate_action(oah, ilen - sizeof(*oia), err))
+ return (-1);
+ break;
+
+ case OFP_INSTRUCTION_T_EXPERIMENTER:
+ /* FALLTHROUGH */
+ default:
+ *err = OFP_ERRINST_UNKNOWN_INST;
+ return (-1);
+ }
+
+ return (0);
+}
+
+int
+swofp_validate_action(struct ofp_action_header *ah, size_t ahtotal, int *err)
+{
+ struct ofp_action_handler *oah;
+ struct ofp_ox_match *oxm;
+ uint8_t *dptr;
+ int ahtype, ahlen, oxmlen;
+
+ /* No actions. */
+ if (ahtotal == 0)
+ return (0);
+
+ /* Check if we have at least the first header. */
+ if (ahtotal < sizeof(*ah)) {
+ *err = OFP_ERRACTION_LEN;
+ return (-1);
+ }
+
+ parse_next_action:
+ ahtype = ntohs(ah->ah_type);
+ ahlen = ntohs(ah->ah_len);
+ if (ahlen < sizeof(*ah) || ahlen > ahtotal) {
+ *err = OFP_ERRACTION_LEN;
+ return (-1);
+ }
+
+ switch (ahtype) {
+ case OFP_ACTION_OUTPUT:
+ if (ahlen != sizeof(struct ofp_action_output)) {
+ *err = OFP_ERRACTION_LEN;
+ return (-1);
+ }
+ break;
+ case OFP_ACTION_GROUP:
+ if (ahlen != sizeof(struct ofp_action_group)) {
+ *err = OFP_ERRACTION_LEN;
+ return (-1);
+ }
+ break;
+ case OFP_ACTION_SET_QUEUE:
+ if (ahlen != sizeof(struct ofp_action_set_queue)) {
+ *err = OFP_ERRACTION_LEN;
+ return (-1);
+ }
+ break;
+ case OFP_ACTION_SET_MPLS_TTL:
+ if (ahlen != sizeof(struct ofp_action_mpls_ttl)) {
+ *err = OFP_ERRACTION_LEN;
+ return (-1);
+ }
+ break;
+ case OFP_ACTION_SET_NW_TTL:
+ if (ahlen != sizeof(struct ofp_action_nw_ttl)) {
+ *err = OFP_ERRACTION_LEN;
+ return (-1);
+ }
+ break;
+ case OFP_ACTION_COPY_TTL_OUT:
+ case OFP_ACTION_COPY_TTL_IN:
+ case OFP_ACTION_DEC_MPLS_TTL:
+ case OFP_ACTION_POP_VLAN:
+ if (ahlen != sizeof(struct ofp_action_header)) {
+ *err = OFP_ERRACTION_LEN;
+ return (-1);
+ }
+ break;
+ case OFP_ACTION_PUSH_VLAN:
+ case OFP_ACTION_PUSH_MPLS:
+ case OFP_ACTION_PUSH_PBB:
+ if (ahlen != sizeof(struct ofp_action_push)) {
+ *err = OFP_ERRACTION_LEN;
+ return (-1);
+ }
+ break;
+ case OFP_ACTION_POP_MPLS:
+ if (ahlen != sizeof(struct ofp_action_pop_mpls)) {
+ *err = OFP_ERRACTION_LEN;
+ return (-1);
+ }
+ break;
+ case OFP_ACTION_SET_FIELD:
+ if (ahlen < sizeof(struct ofp_action_set_field)) {
+ *err = OFP_ERRACTION_LEN;
+ return (-1);
+ }
+
+ oxmlen = ahlen - sizeof(struct ofp_action_set_field);
+ if (oxmlen < sizeof(*oxm)) {
+ *err = OFP_ERRACTION_LEN;
+ return (-1);
+ }
+
+ dptr = (uint8_t *)ah;
+ dptr += sizeof(struct ofp_action_set_field);
+ while (oxmlen) {
+ 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;
+
+ return (-1);
}
+
+ dptr += sizeof(*oxm) + oxm->oxm_length;
+ oxmlen -= sizeof(*oxm) + oxm->oxm_length;
}
break;
+
default:
- *error = OFP_ERRINST_UNKNOWN_INST;
+ /* Unknown/unsupported action. */
+ *err = OFP_ERRACTION_TYPE;
+ return (-1);
+ }
+
+ oah = swofp_lookup_action_handler(ahtype);
+ /* Unknown/unsupported action. */
+ if (oah == NULL) {
+ *err = OFP_ERRACTION_TYPE;
return (-1);
}
+ ahtotal -= min(ahlen, ahtotal);
+ if (ahtotal)
+ goto parse_next_action;
+
return (0);
}
@@ -4591,7 +4761,8 @@ swofp_flow_entry_put_instructions(struct
for (off = start; off < start + len; off += ntohs(oi->i_len)) {
oi = (struct ofp_instruction *)(mtod(m, caddr_t) + off);
- if (swofp_validate_flow_instruction(oi, &error))
+ if (swofp_validate_flow_instruction(oi,
+ len - (off - start), &error))
goto failed;
if ((inst = malloc(ntohs(oi->i_len), M_DEVBUF,
@@ -4649,7 +4820,7 @@ swofp_flow_mod_cmd_add(struct switch_sof
struct ofp_match *om;
struct swofp_flow_entry *swfe, *old_swfe;
struct swofp_flow_table *swft;
- int error;
+ int error, omlen;
uint16_t etype = OFP_ERRTYPE_FLOW_MOD_FAILED;
oh = mtod(m, struct ofp_header *);
@@ -4667,6 +4838,25 @@ swofp_flow_mod_cmd_add(struct switch_sof
goto ofp_error;
}
+ omlen = ntohs(om->om_length);
+ /*
+ * 1) ofp_match header must have at least its own size,
+ * otherwise memcpy() will fail later;
+ * 2) OXM filters can't be bigger than the packet.
+ */
+ if (omlen < sizeof(*om) &&
+ omlen >= (m->m_len - sizeof(*ofm))) {
+ etype = OFP_ERRTYPE_BAD_REQUEST;
+ error = OFP_ERRREQ_LEN;
+ goto ofp_error;
+ }
+
+ /* Validate that the OXM are in-place and correct. */
+ if (swofp_validate_flow_match(om, &error)) {
+ etype = OFP_ERRTYPE_BAD_MATCH;
+ goto ofp_error;
+ }
+
if ((swft = swofp_flow_table_add(sc, ofm->fm_table_id)) == NULL) {
error = OFP_ERRFLOWMOD_TABLE_ID;
goto ofp_error;
@@ -4694,22 +4884,19 @@ swofp_flow_mod_cmd_add(struct switch_sof
nanouptime(&swfe->swfe_installed_time);
nanouptime(&swfe->swfe_idle_time);
- if (swofp_validate_flow_match(om, &error))
- goto ofp_error;
-
- if ((swfe->swfe_match = malloc(ntohs(om->om_length), M_DEVBUF,
+ if ((swfe->swfe_match = malloc(omlen, M_DEVBUF,
M_DONTWAIT|M_ZERO)) == NULL) {
error = OFP_ERRFLOWMOD_UNKNOWN;
goto ofp_error_free_flow;
}
- memcpy(swfe->swfe_match, om, ntohs(om->om_length));
+ memcpy(swfe->swfe_match, om, omlen);
/*
* If the ofp_match structure is empty and priority is zero, then
* this is a special flow type called table-miss which is the last
* flow to match.
*/
- if (ntohs(om->om_length) == sizeof(*om) && swfe->swfe_priority == 0)
+ if (omlen == sizeof(*om) && swfe->swfe_priority == 0)
swfe->swfe_tablemiss = 1;
if ((error = swofp_flow_entry_put_instructions(m, swfe))) {
@@ -4752,7 +4939,7 @@ swofp_flow_mod_cmd_common_modify(struct
struct ofp_match *om;
struct swofp_flow_entry *swfe;
struct swofp_flow_table *swft;
- int error;
+ int error, omlen;
uint16_t etype = OFP_ERRTYPE_FLOW_MOD_FAILED;
oh = mtod(m, struct ofp_header *);
@@ -4770,6 +4957,25 @@ swofp_flow_mod_cmd_common_modify(struct
goto ofp_error;
}
+ omlen = ntohs(om->om_length);
+ /*
+ * 1) ofp_match header must have at least its own size,
+ * otherwise memcpy() will fail later;
+ * 2) OXM filters can't be bigger than the packet.
+ */
+ if (omlen < sizeof(*om) &&
+ omlen >= (m->m_len - sizeof(*ofm))) {
+ etype = OFP_ERRTYPE_BAD_MATCH;
+ error = OFP_ERRMATCH_BAD_LEN;
+ goto ofp_error;
+ }
+
+ /* Validate that the OXM are in-place and correct. */
+ if (swofp_validate_flow_match(om, &error)) {
+ etype = OFP_ERRTYPE_BAD_MATCH;
+ goto ofp_error;
+ }
+
if ((swft = swofp_flow_table_lookup(sc, ofm->fm_table_id)) == NULL) {
error = OFP_ERRFLOWMOD_TABLE_ID;
goto ofp_error;
@@ -4834,10 +5040,30 @@ swofp_flow_mod_cmd_common_delete(struct
struct ofp_flow_mod *ofm;
struct ofp_match *om;
struct swofp_flow_table *swft;
+ int error, omlen;
+ uint16_t etype = OFP_ERRTYPE_FLOW_MOD_FAILED;
ofm = (struct ofp_flow_mod *)(mtod(m, caddr_t));
om = &ofm->fm_match;
+ omlen = ntohs(om->om_length);
+ /*
+ * 1) ofp_match header must have at least its own size,
+ * otherwise memcpy() will fail later;
+ * 2) OXM filters can't be bigger than the packet.
+ */
+ if (omlen < sizeof(*om) &&
+ omlen >= (m->m_len - sizeof(*ofm))) {
+ error = OFP_ERRFLOWMOD_UNKNOWN;
+ goto ofp_error;
+ }
+
+ /* Validate that the OXM are in-place and correct. */
+ if (swofp_validate_flow_match(om, &error)) {
+ etype = OFP_ERRTYPE_BAD_MATCH;
+ goto ofp_error;
+ }
+
TAILQ_FOREACH(swft, &ofs->swofs_table_list, swft_table_next) {
if ((ofm->fm_table_id != OFP_TABLE_ID_ALL) &&
(ofm->fm_table_id != swft->swft_table_id))
@@ -4853,6 +5079,10 @@ swofp_flow_mod_cmd_common_delete(struct
m_freem(m);
return (0);
+
+ ofp_error:
+ swofp_send_error(sc, m, etype, error);
+ return (-1);
}
int
@@ -5078,7 +5308,7 @@ swofp_recv_packet_out(struct switch_soft
struct ofp_packet_out *pout;
struct ofp_action_header *ah;
struct mbuf *mc = NULL, *mcn;
- int al_start, al_len, off;
+ int al_start, al_len, off, error;
struct switch_flow_classify swfcl = {};
struct swofp_pipline_desc swpld = { .swpld_swfcl = &swfcl };
@@ -5095,6 +5325,15 @@ swofp_recv_packet_out(struct switch_soft
pout = mtod(m, struct ofp_packet_out *);
al_start = offsetof(struct ofp_packet_out, pout_actions);
+
+ /* Validate actions before anything else. */
+ ah = (struct ofp_action_header *)
+ ((uint8_t *)pout + sizeof(*pout));
+ if (swofp_validate_action(ah, al_len, &error)) {
+ swofp_send_error(sc, m, OFP_ERRTYPE_BAD_REQUEST, error);
+ return (EINVAL);
+ }
+
if (pout->pout_buffer_id == OFP_PKTOUT_NO_BUFFER) {
/*
* It's not necessary to deep copy at here because it's done
