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