On Tue, Nov 26, 2019 at 04:17:46PM -0800, Ayaka Koshibe 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?
OK claudio, one inline comment
>
> 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, u_int 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;
Instead of the goto this could be written as a do { } while(omlen > 0) loop.
> +}
> +
> +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:
>
--
:wq Claudio
