On Thu, May 4, 2023 at 12:14 PM Lorenzo Bianconi < [email protected]> wrote:
> According to RFC8415 [0], section 8 "Client/Server Message Formats": > "Options are stored serially in the "options" field, with no padding > between the options. Options are byte-aligned but are not aligned > in any other way (such as on 2-byte or 4-byte boundaries)." > > Fix possible unaligned accesses in DHCPv6 code reading/writing > dhcpv6_opt_header structure that trigger undefined behaviour issues > reported by ubsan sanitizer. > > [0] https://www.rfc-editor.org/rfc/rfc8415.html > > Signed-off-by: Lorenzo Bianconi <[email protected]> > --- > controller/pinctrl.c | 11 +++++++---- > lib/actions.c | 23 ++++++++++++----------- > 2 files changed, 19 insertions(+), 15 deletions(-) > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index 97a5e392f..bf285c9e5 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -2444,19 +2444,22 @@ compose_out_dhcpv6_opts(struct ofpbuf *userdata, > struct ofpbuf *out_dhcpv6_opts, ovs_be32 iaid) > { > while (userdata->size) { > - struct dhcp_opt6_header *userdata_opt = ofpbuf_try_pull( > - userdata, sizeof *userdata_opt); > + ovs_be16 opt_size, opt_code; > + uint8_t *userdata_opt = ofpbuf_try_pull( > + userdata, sizeof (struct dhcp_opt6_header)); > if (!userdata_opt) { > return false; > } > > - size_t size = ntohs(userdata_opt->size); > + memcpy(&opt_size, userdata_opt + sizeof opt_code, sizeof > opt_size); > + size_t size = ntohs(opt_size); > uint8_t *userdata_opt_data = ofpbuf_try_pull(userdata, size); > if (!userdata_opt_data) { > return false; > } > > - switch (ntohs(userdata_opt->opt_code)) { > + memcpy(&opt_code, userdata_opt, sizeof opt_code); > + switch (ntohs(opt_code)) { > case DHCPV6_OPT_SERVER_ID_CODE: > { > /* The Server Identifier option carries a DUID > diff --git a/lib/actions.c b/lib/actions.c > index 781549d75..35c0c2308 100644 > --- a/lib/actions.c > +++ b/lib/actions.c > @@ -2882,27 +2882,28 @@ static void > encode_put_dhcpv6_option(const struct ovnact_gen_option *o, > struct ofpbuf *ofpacts) > { > - struct dhcp_opt6_header *opt = ofpbuf_put_uninit(ofpacts, sizeof > *opt); > + ovs_be16 opt_size, opt_code = htons(o->option->code); > const union expr_constant *c = o->value.values; > size_t n_values = o->value.n_values; > - size_t size; > + uint8_t *opt; > > - opt->opt_code = htons(o->option->code); > + opt = ofpbuf_put_uninit(ofpacts, sizeof (struct dhcp_opt6_header)); > + memcpy(opt, &opt_code, sizeof opt_code); > > if (!strcmp(o->option->type, "ipv6")) { > - size = n_values * sizeof(struct in6_addr); > - opt->size = htons(size); > + opt_size = htons(n_values * sizeof(struct in6_addr)); > + memcpy(opt + sizeof opt_code, &opt_size, sizeof opt_size); > for (size_t i = 0; i < n_values; i++) { > ofpbuf_put(ofpacts, &c[i].value.ipv6, sizeof(struct > in6_addr)); > } > } else if (!strcmp(o->option->type, "mac")) { > - size = sizeof(struct eth_addr); > - opt->size = htons(size); > - ofpbuf_put(ofpacts, &c->value.mac, size); > + opt_size = htons(sizeof(struct eth_addr)); > + memcpy(opt + sizeof opt_code, &opt_size, sizeof opt_size); > + ofpbuf_put(ofpacts, &c->value.mac, sizeof(struct eth_addr)); > } else if (!strcmp(o->option->type, "str")) { > - size = strlen(c->string); > - opt->size = htons(size); > - ofpbuf_put(ofpacts, c->string, size); > + opt_size = htons(strlen(c->string)); > + memcpy(opt + sizeof opt_code, &opt_size, sizeof opt_size); > + ofpbuf_put(ofpacts, c->string, strlen(c->string)); > } > } > > -- > 2.40.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Hi Lorenzo, it seems that the change could be reduced to something like down below. diff --git a/controller/pinctrl.c b/controller/pinctrl.c index 97a5e392f..761783562 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -2444,19 +2444,19 @@ compose_out_dhcpv6_opts(struct ofpbuf *userdata, struct ofpbuf *out_dhcpv6_opts, ovs_be32 iaid) { while (userdata->size) { - struct dhcp_opt6_header *userdata_opt = ofpbuf_try_pull( + struct dhcpv6_opt_header *userdata_opt = ofpbuf_try_pull( userdata, sizeof *userdata_opt); if (!userdata_opt) { return false; } - size_t size = ntohs(userdata_opt->size); + size_t size = ntohs(userdata_opt->len); uint8_t *userdata_opt_data = ofpbuf_try_pull(userdata, size); if (!userdata_opt_data) { return false; } - switch (ntohs(userdata_opt->opt_code)) { + switch (ntohs(userdata_opt->code)) { case DHCPV6_OPT_SERVER_ID_CODE: { /* The Server Identifier option carries a DUID diff --git a/lib/actions.c b/lib/actions.c index 781549d75..2b566c85e 100644 --- a/lib/actions.c +++ b/lib/actions.c @@ -2882,26 +2882,26 @@ static void encode_put_dhcpv6_option(const struct ovnact_gen_option *o, struct ofpbuf *ofpacts) { - struct dhcp_opt6_header *opt = ofpbuf_put_uninit(ofpacts, sizeof *opt); + struct dhcpv6_opt_header *opt = ofpbuf_put_uninit(ofpacts, sizeof *opt); const union expr_constant *c = o->value.values; size_t n_values = o->value.n_values; size_t size; - opt->opt_code = htons(o->option->code); + opt->code = htons(o->option->code); if (!strcmp(o->option->type, "ipv6")) { size = n_values * sizeof(struct in6_addr); - opt->size = htons(size); + opt->len = htons(size); for (size_t i = 0; i < n_values; i++) { ofpbuf_put(ofpacts, &c[i].value.ipv6, sizeof(struct in6_addr)); } } else if (!strcmp(o->option->type, "mac")) { size = sizeof(struct eth_addr); - opt->size = htons(size); + opt->len = htons(size); ofpbuf_put(ofpacts, &c->value.mac, size); } else if (!strcmp(o->option->type, "str")) { size = strlen(c->string); - opt->size = htons(size); + opt->len = htons(size); ofpbuf_put(ofpacts, c->string, size); } } diff --git a/lib/ovn-l7.h b/lib/ovn-l7.h index 2b20bc380..d718ed39a 100644 --- a/lib/ovn-l7.h +++ b/lib/ovn-l7.h @@ -240,12 +240,6 @@ struct dhcp_opt_header { #define DHCP_OPT_PAYLOAD(hdr) \ (void *)((char *)hdr + sizeof(struct dhcp_opt_header)) -/* Used in the OpenFlow PACKET_IN userdata */ -struct dhcp_opt6_header { - ovs_be16 opt_code; - ovs_be16 size; -}; - /* These are not defined in ovs/lib/dhcp.h, hence defining here. */ #define OVN_DHCP_MSG_DECLINE 4 #define OVN_DHCP_MSG_RELEASE 7 The main problem seems to be that we have used "struct dhcp_opt6_header" that is not packed as opposed to "struct dhcpv6_opt_header", so we should be fine by replacing it and removing the former header. Do you have a test case that could reproduce the unaligned access, to see if the replacement is really a proper fix? Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
