> On 28.10.2016, at 19:20, Rafael Zalamena <[email protected]> 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;
>
Yes, adding the validation step is the right approach!
And this approach is what we also do in switchd (and iked's ikev2 parser).
> Other small fixes:
Small fixes should go in separately:
> - Simplify OFP_FLOW_MOD_MSG_INSTRUCTION_OFFSET() macro;
OK
> - Change swofp_flow_table_add() malloc behaviour to be like all the rest
> of the code (don't wait for memory);
OK
> - swofp_flow_entry_put_instructions() doesn't need a pointer to an mbuf
> pointer, we only read it;
OK
> - Change the validation function parameters: we can't check for non-errors
> using the value 0, because the spec actually uses it for some errors.
> Instead use int pointer to get error value and use the return value only
> to find out if the validation succeeded or not;
OK
> - Return more accurate error messages: some error messages were being sent
> with the wrong type/code combination;
OK
> - Removed swofp_validate_flow_action_set_field() as it was incorporated
> in action validation;
>
This is part of the validation diff.
Could you commit the small fixes first and send an updated validation diff?
> TODO for next diffs:
> - We still need to code the validation for group messages, but since I
> haven't started using it yet, I didn't touch it;
> - We still need to implement validation for some specific OXM errors:
> duplicated OXM, invalid wildcard, invalid value or missing prereq;
> - swofp_validate_buckets() could use validate_action() with some effort;
>
Sure
> Even though switchd(8) does packet validation (both on input and output),
> we should commit this to enable other vendors/software to use the OpenBSD
> switch(4) directly, otherwise we will need more effort to implement this
> for switchd(8) relaying and force people to use switchd(8) needlessly.
>
Full ack,
a) it is a very important design decision that switch(4) does NOT depend
on switchd(8). This is not 100% true at the moment (as switch(4) doesn't fully
behave like a normal socket, eg. it only supports full single packet read/write
I/O),
but it should be possible to connect any user space controller to /dev/switch0,
eg. by using a tool like socat to connect it to ryu instead of switchd.
b) The kernel should never "trust" user land to do the right thing. Crashing it
from
user input via /dev/switch* would be a bad thing. And we cannot even guarantee
that switchd(8) is absolutely safe.
> ok?
>
Small fixes OK first, see above.
Reyk
> Index: net/ofp.h
> ===================================================================
> RCS file: /home/obsdcvs/src/sys/net/ofp.h,v
> retrieving revision 1.2
> diff -u -p -r1.2 ofp.h
> --- net/ofp.h 30 Sep 2016 12:40:00 -0000 1.2
> +++ net/ofp.h 25 Oct 2016 08:50:14 -0000
> @@ -95,7 +95,7 @@ struct ofp_hello_element_versionbitmap {
>
> /* Ports */
> #define OFP_PORT_MAX 0xffffff00 /* Maximum number of physical
> ports */
> -#define OFP_PORT_INPUT 0xfffffff8 /* Send back to input
> port */
> +#define OFP_PORT_INPUT 0xfffffff8 /* Send back to input
> port */
> #define OFP_PORT_FLOWTABLE 0xfffffff9 /* Perform actions in flow
> table */
> #define OFP_PORT_NORMAL 0xfffffffa /* Let switch decide */
> #define OFP_PORT_FLOOD 0xfffffffb /* All non-block ports
> except input */
> @@ -179,9 +179,9 @@ struct ofp_switch_features {
>
> /* Switch capabilities */
> #define OFP_SWCAP_FLOW_STATS 0x1 /* Flow statistics */
> -#define OFP_SWCAP_TABLE_STATS 0x2 /* Table statistics */
> -#define OFP_SWCAP_PORT_STATS 0x4 /* Port statistics */
> -#define OFP_SWCAP_GROUP_STATS 0x8 /* Group statistics */
> +#define OFP_SWCAP_TABLE_STATS 0x2 /* Table statistics */
> +#define OFP_SWCAP_PORT_STATS 0x4 /* Port statistics */
> +#define OFP_SWCAP_GROUP_STATS 0x8 /* Group statistics */
> #define OFP_SWCAP_IP_REASM 0x20 /* Can reassemble IP frags */
> #define OFP_SWCAP_QUEUE_STATS 0x40 /* Queue statistics */
> #define OFP_SWCAP_ARP_MATCH_IP 0x80 /* Match IP addresses
> in ARP pkts */
> @@ -314,15 +314,15 @@ struct ofp_action_mpls_ttl {
> struct ofp_action_push {
> uint16_t ap_type;
> uint16_t ap_len;
> - uint16_t ap_ethertype;
> - uint8_t pad[2];
> + uint16_t ap_ethertype;
> + uint8_t ap_pad[2];
> } __packed;
>
> struct ofp_action_pop_mpls {
> uint16_t apm_type;
> uint16_t apm_len;
> uint16_t apm_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_t asf_type;
> uint16_t asf_len;
> uint8_t asf_field[4];
> +} __packed;
> +
> +struct ofp_action_set_queue {
> + uint16_t asq_type;
> + uint16_t asq_len;
> + uint32_t asq_queue_id;
> } __packed;
>
> /* Packet-Out Message */
> Index: net/switchofp.c
> ===================================================================
> RCS file: /home/obsdcvs/src/sys/net/switchofp.c,v
> retrieving revision 1.18
> diff -u -p -r1.18 switchofp.c
> --- net/switchofp.c 28 Oct 2016 09:01:49 -0000 1.18
> +++ net/switchofp.c 28 Oct 2016 14:48:49 -0000
> @@ -209,8 +209,13 @@ 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_flow_match(struct ofp_match *);
> -int swofp_validate_flow_instruction(struct ofp_instruction *);
> +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 *, size_t,
> + int *);
> +int swofp_validate_action(struct ofp_action_header *, size_t, int *);
> +int swofp_flow_entry_put_instructions(struct mbuf *,
> + struct swofp_flow_entry *, int *);
>
> /*
> * OpenFlow protocol compare oxm
> @@ -388,9 +393,9 @@ for ((curr) = (head); (caddr_t)curr < ((
> /*
> * Get instruction offset in ofp_flow_mod
> */
> -#define OFP_FLOW_MOD_MSG_INSTRUCTION_OFFSET(ofm)
> \
> -(offsetof(struct ofp_flow_mod, fm_match) + ntohs((ofm)->fm_match.om_length)
> + \
> - (((0x8 - (ntohs((ofm)->fm_match.om_length) % 0x8)) & 0x7)))
> +#define OFP_FLOW_MOD_MSG_INSTRUCTION_OFFSET(ofm) \
> + (offsetof(struct ofp_flow_mod, fm_match) + \
> + OFP_ALIGN(ntohs((ofm)->fm_match.om_length)))
>
> /*
> * Instructions "FOREACH" in ofp_flow_mod. You can get header using
> @@ -1238,9 +1243,10 @@ swofp_flow_table_add(struct switch_softc
> if ((swft = swofp_flow_table_lookup(sc, table_id)) != NULL)
> return (swft);
>
> - new = malloc(sizeof(*new), M_DEVBUF, M_WAITOK|M_ZERO);
> - new->swft_table_id = table_id;
> + if ((new = malloc(sizeof(*new), M_DEVBUF, M_NOWAIT|M_ZERO)) == NULL)
> + return (NULL);
>
> + new->swft_table_id = table_id;
> TAILQ_FOREACH(swft, &ofs->swofs_table_list, swft_table_next) {
> if (table_id < swft->swft_table_id)
> break;
> @@ -1804,75 +1810,240 @@ swofp_ox_cmp_ether_addr(struct ofp_ox_ma
> }
> }
>
> -
> -/* TODO: validation for match */
> int
> -swofp_validate_flow_match(struct ofp_match *om)
> +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)
> - return (OFP_ERRMATCH_BAD_FIELD);
> - if (handler->oxm_match == NULL)
> - return (OFP_ERRMATCH_BAD_FIELD);
> + hasmask = OFP_OXM_GET_HASMASK(oxm);
> + length = oxm->oxm_length;
> +
> + handler = swofp_lookup_oxm_handler(oxm);
> + if (handler == NULL || handler->oxm_match == NULL) {
> + *err = OFP_ERRMATCH_BAD_FIELD;
> + return (-1);
> + }
> +
> + 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)
> +swofp_validate_flow_instruction(struct ofp_instruction *oi, size_t total,
> + int *err)
> {
> struct ofp_action_header *oah;
> struct ofp_instruction_actions *oia;
> - int error = 0;
> + 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)
> - break;
> +
> + /* 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:
> + /* TODO missing struct in ofp.h. */
> + 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 = EINVAL;
> + /* Unknown/unsupported action. */
> + *err = OFP_ERRACTION_TYPE;
> + return (-1);
> }
>
> - return (error);
> + 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);
> }
>
> int
> @@ -4565,15 +4736,15 @@ swofp_send_flow_removed(struct switch_so
> * OpenFlow protocol FLOW MOD message handlers
> */
> int
> -swofp_flow_entry_put_instructions(struct mbuf **m,
> - struct swofp_flow_entry *swfe)
> +swofp_flow_entry_put_instructions(struct mbuf *m,
> + struct swofp_flow_entry *swfe, int *err)
> {
> struct ofp_flow_mod *ofm;
> struct ofp_instruction *oi;
> caddr_t inst;
> - int start, len, off, error;
> + int start, len, off;
>
> - ofm = mtod((*m), struct ofp_flow_mod *);
> + ofm = mtod(m, struct ofp_flow_mod *);
>
> /*
> * Clear instructions from flow entry. It's necessary to only modify
> @@ -4585,43 +4756,72 @@ swofp_flow_entry_put_instructions(struct
> start = OFP_FLOW_MOD_MSG_INSTRUCTION_OFFSET(ofm);
> len = ntohs(ofm->fm_oh.oh_length) - start;
> for (off = start; off < start + len; off += ntohs(oi->i_len)) {
> - oi = (struct ofp_instruction *)(mtod(*m, caddr_t) + off);
> -
> - if ((error = swofp_validate_flow_instruction(oi)))
> - goto failed;
> + oi = (struct ofp_instruction *)(mtod(m, caddr_t) + off);
> + if (swofp_validate_flow_instruction(oi,
> + len - (off - start), err))
> + return (-1);
>
> if ((inst = malloc(ntohs(oi->i_len), M_DEVBUF,
> - M_DONTWAIT|M_ZERO)) == NULL) {
> - error = OFP_ERRFLOWMOD_UNKNOWN;
> - goto failed;
> + M_DONTWAIT|M_ZERO)) == NULL) {
> + *err = OFP_ERRINST_EPERM;
> + return (-1);
> }
> memcpy(inst, oi, ntohs(oi->i_len));
>
> switch (ntohs(oi->i_type)) {
> case OFP_INSTRUCTION_T_GOTO_TABLE:
> + if (swfe->swfe_goto_table)
> + free(swfe->swfe_goto_table, M_DEVBUF,
> + ntohs(oi->i_len));
> +
> swfe->swfe_goto_table =
> (struct ofp_instruction_goto_table *)inst;
> break;
> case OFP_INSTRUCTION_T_WRITE_META:
> + if (swfe->swfe_write_metadata)
> + free(swfe->swfe_write_metadata, M_DEVBUF,
> + ntohs(oi->i_len));
> +
> swfe->swfe_write_metadata =
> (struct ofp_instruction_write_metadata *)inst;
> break;
> case OFP_INSTRUCTION_T_WRITE_ACTIONS:
> + if (swfe->swfe_write_actions)
> + free(swfe->swfe_write_actions, M_DEVBUF,
> + ntohs(oi->i_len));
> +
> swfe->swfe_write_actions =
> (struct ofp_instruction_actions *)inst;
> break;
> case OFP_INSTRUCTION_T_APPLY_ACTIONS:
> + if (swfe->swfe_apply_actions)
> + free(swfe->swfe_apply_actions, M_DEVBUF,
> + ntohs(oi->i_len));
> +
> swfe->swfe_apply_actions =
> (struct ofp_instruction_actions *)inst;
> break;
> case OFP_INSTRUCTION_T_CLEAR_ACTIONS:
> + if (swfe->swfe_clear_actions)
> + free(swfe->swfe_clear_actions, M_DEVBUF,
> + ntohs(oi->i_len));
> +
> swfe->swfe_clear_actions =
> (struct ofp_instruction_actions *)inst;
> break;
> case OFP_INSTRUCTION_T_METER:
> - swfe->swfe_meter = (struct ofp_instruction_meter *)inst;
> + if (swfe->swfe_meter)
> + free(swfe->swfe_meter, M_DEVBUF,
> + ntohs(oi->i_len));
> +
> + swfe->swfe_meter =
> + (struct ofp_instruction_meter *)inst;
> break;
> case OFP_INSTRUCTION_T_EXPERIMENTER:
> + if (swfe->swfe_experimenter)
> + free(swfe->swfe_experimenter, M_DEVBUF,
> + ntohs(oi->i_len));
> +
> swfe->swfe_experimenter =
> (struct ofp_instruction_experimenter *)inst;
> break;
> @@ -4632,9 +4832,6 @@ swofp_flow_entry_put_instructions(struct
> }
>
> return (0);
> -
> - failed:
> - return (error);
> }
>
> int
> @@ -4645,7 +4842,8 @@ 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 *);
> ofm = mtod(m, struct ofp_flow_mod *);
> @@ -4662,8 +4860,30 @@ swofp_flow_mod_cmd_add(struct switch_sof
> goto ofp_error;
> }
>
> - if ((swft = swofp_flow_table_lookup(sc, ofm->fm_table_id)) == NULL)
> - swft = swofp_flow_table_add(sc, ofm->fm_table_id);
> + 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;
> + }
> +
> + swft = swofp_flow_table_add(sc, ofm->fm_table_id);
> + if (swft == NULL) {
> + error = OFP_ERRFLOWMOD_TABLE_ID;
> + goto ofp_error;
> + }
>
> if ((old_swfe = swofp_flow_search_by_table(swft, om,
> ntohs(ofm->fm_priority)))) {
> @@ -4687,26 +4907,25 @@ swofp_flow_mod_cmd_add(struct switch_sof
> nanouptime(&swfe->swfe_installed_time);
> nanouptime(&swfe->swfe_idle_time);
>
> - if ((error = swofp_validate_flow_match(om)))
> - 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)))
> + if (swofp_flow_entry_put_instructions(m, swfe, &error)) {
> + etype = OFP_ERRTYPE_BAD_INSTRUCTION;
> goto ofp_error_free_flow;
> + }
>
> if (old_swfe) {
> if (!(ntohs(ofm->fm_flags) & OFP_FLOWFLAG_RESET_COUNTS)) {
> @@ -4730,7 +4949,7 @@ swofp_flow_mod_cmd_add(struct switch_sof
> ofp_error_free_flow:
> swofp_flow_entry_free(&swfe);
> ofp_error:
> - swofp_send_error(sc, m, OFP_ERRTYPE_FLOW_MOD_FAILED, error);
> + swofp_send_error(sc, m, etype, error);
> return (0);
> }
>
> @@ -4743,7 +4962,8 @@ 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 *);
> ofm = mtod(m, struct ofp_flow_mod *);
> @@ -4760,6 +4980,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;
> @@ -4777,7 +5016,7 @@ swofp_flow_mod_cmd_common_modify(struct
> ntohl(ofm->fm_out_group)))
> continue;
>
> - if ((error = swofp_flow_entry_put_instructions(&m, swfe))) {
> + if (swofp_flow_entry_put_instructions(m, swfe, &error)) {
> /*
> * If error occurs in swofp_flow_entry_put_instructions,
> * the flow entry might be half-way modified. So the
> @@ -4785,6 +5024,7 @@ swofp_flow_mod_cmd_common_modify(struct
> */
> swofp_flow_entry_delete(sc, swft, swfe,
> OFP_FLOWREM_REASON_DELETE);
> + etype = OFP_ERRTYPE_BAD_INSTRUCTION;
> goto ofp_error;
> }
>
> @@ -4798,7 +5038,7 @@ swofp_flow_mod_cmd_common_modify(struct
> return (0);
>
> ofp_error:
> - swofp_send_error(sc, m, OFP_ERRTYPE_FLOW_MOD_FAILED, error);
> + swofp_send_error(sc, m, etype, error);
> return (0);
> }
>
> @@ -4823,10 +5063,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))
> @@ -4842,6 +5102,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
> @@ -5067,7 +5331,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 };
>
> @@ -5084,6 +5348,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
>
