On Tue, 26 Nov 2019 16:17:46 -0800, Ayaka Koshibe <akosh...@openbsd.org> wrote:
> Currently, we assume that OpenFlow 1.3 set_field actions can contain > multiple header match fields (OXMs). According to specification, a > set_field action contains exactly one OXM, followed by padding to align > the action field in a OpenFlow message to 64 bits. > > This diff changes how we handle OpenFlow messages that contain sef_field > actions to match the specs. > > Feedback? OK? After a bunch of explanation from akoshibe@ on how the protocol is structured, ok ori. Get someone else who pokes more at network stuff to take a second look, though. > > Ayaka > > Index: usr.sbin/switchd/ofp13.c > =================================================================== > RCS file: /cvs/src/usr.sbin/switchd/ofp13.c,v > retrieving revision 1.46 > diff -u -p -u -r1.46 ofp13.c > --- usr.sbin/switchd/ofp13.c 21 Nov 2019 06:22:57 -0000 1.46 > +++ usr.sbin/switchd/ofp13.c 26 Nov 2019 23:01:44 -0000 > @@ -780,7 +780,7 @@ ofp13_validate_action(struct switchd *sc > 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 > 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: > Index: sys/net/switchofp.c > =================================================================== > RCS file: /cvs/src/sys/net/switchofp.c,v > retrieving revision 1.75 > diff -u -p -u -r1.75 switchofp.c > --- sys/net/switchofp.c 21 Nov 2019 17:24:15 -0000 1.75 > +++ sys/net/switchofp.c 26 Nov 2019 23:07:51 -0000 > @@ -2174,7 +2174,8 @@ swofp_validate_action(struct switch_soft > } > 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_soft > 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; > Index: usr.sbin/tcpdump/print-ofp.c > =================================================================== > RCS file: /cvs/src/usr.sbin/tcpdump/print-ofp.c,v > retrieving revision 1.11 > diff -u -p -u -r1.11 print-ofp.c > --- usr.sbin/tcpdump/print-ofp.c 28 Nov 2016 17:47:15 -0000 1.11 > +++ usr.sbin/tcpdump/print-ofp.c 27 Nov 2019 00:15:12 -0000 > @@ -232,10 +232,40 @@ ofp_print_setconfig(const u_char *bp, u_ > } > > void > +ofp_print_oxm_field(const u_char *bp, size_t length, int omlen, int once) > +{ > + struct ofp_ox_match *oxm; > + > + 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; > + > + if (once) > + return; > + > + omlen -= min(sizeof(*oxm) + oxm->oxm_length, omlen); > + if (omlen) > + goto parse_next_oxm; > +} > + > +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 +306,7 @@ ofp_print_packetin(const u_char *bp, u_i > 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 +332,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 +364,7 @@ ofp_print_flowremoved(const u_char *bp, > 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 +442,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 +486,10 @@ ofp_print_flowmod(const u_char *bp, u_in > 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_field(bp, length, omlen, 0); > > - 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; > + bp += omlen; > + length -= omlen; > > /* Skip padding if any. */ > if (padsize) { > @@ -1017,7 +988,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 +1000,7 @@ action_print_setfield(const u_char *bp, > 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 +1044,7 @@ ofp_print_action(struct ofp_action_heade > break; > > case OFP_ACTION_SET_FIELD: > + action_print_setfield(bp, length); > break; > > case OFP_ACTION_COPY_TTL_OUT: > -- Ori Bernstein