| Message ID | 20230714123907.417977-1-dceara@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] pinctrl: Cap the max size of a prefix delegation DUID value. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On 7/14/23 14:39, Dumitru Ceara wrote: > It's specified in RFC 8415. This also avoids having to free/realloc the > pfd->uuid.data memory. That part was not correct anyway and was flagged > by ASAN as a memleak: > > Direct leak of 42 byte(s) in 3 object(s) allocated from: > #0 0x55e5b6354c9e in malloc (/workspace/ovn-tmp/controller/ovn-controller+0x2edc9e) (BuildId: f963f8c756bd5a2207a9b3c922d4362e46bb3162) > #1 0x55e5b671878d in xmalloc__ /workspace/ovn-tmp/ovs/lib/util.c:140:15 > #2 0x55e5b671878d in xmalloc /workspace/ovn-tmp/ovs/lib/util.c:175:12 > #3 0x55e5b642cebc in pinctrl_parse_dhcpv6_reply /workspace/ovn-tmp/controller/pinctrl.c:997:20 > #4 0x55e5b642cebc in pinctrl_handle_dhcp6_server /workspace/ovn-tmp/controller/pinctrl.c:1040:9 > #5 0x55e5b642cebc in process_packet_in /workspace/ovn-tmp/controller/pinctrl.c:3210:9 > #6 0x55e5b642cebc in pinctrl_recv /workspace/ovn-tmp/controller/pinctrl.c:3290:9 > #7 0x55e5b642cebc in pinctrl_handler /workspace/ovn-tmp/controller/pinctrl.c:3385:17 > #8 0x55e5b66ef664 in ovsthread_wrapper /workspace/ovn-tmp/ovs/lib/ovs-thread.c:423:12 > #9 0x7faa30194b42 (/lib/x86_64-linux-gnu/libc.so.6+0x94b42) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d) > > Fixes: faa44a0c60a3 ("controller: IPv6 Prefix-Delegation: introduce RENEW/REBIND msg support") I forgot to add Ales as co-author, I apologize. Co-authored-by: Ales Musil <amusil@redhat.com> Signed-off-by: Ales Musil <amusil@redhat.com> > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > controller/pinctrl.c | 33 ++++++++++++++++----------------- > 1 file changed, 16 insertions(+), 17 deletions(-) > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index 6027ba0afb..bed90fe0b7 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -674,6 +674,14 @@ enum { > PREFIX_REBIND, > }; > > +/* According to RFC 8415, section 11: > + * A DUID consists of a 2-octet type code represented in network byte > + * order, followed by a variable number of octets that make up the > + * actual identifier. The length of the DUID (not including the type > + * code) is at least 1 octet and at most 128 octets. > +*/ > +#define DHCPV6_MAX_DUID_LEN 130 > + > struct ipv6_prefixd_state { > long long int next_announce; > long long int last_complete; > @@ -683,7 +691,7 @@ struct ipv6_prefixd_state { > struct eth_addr sa; > /* server_id_info */ > struct { > - uint8_t *data; > + uint8_t data[DHCPV6_MAX_DUID_LEN]; > uint8_t len; > } uuid; > struct in6_addr ipv6_addr; > @@ -899,7 +907,7 @@ pinctrl_prefixd_state_handler(const struct flow *ip_flow, > struct eth_addr sa, struct in6_addr server_addr, > char prefix_len, unsigned t1, unsigned t2, > unsigned plife_time, unsigned vlife_time, > - uint8_t *uuid, uint8_t uuid_len) > + const uint8_t *uuid, uint8_t uuid_len) > { > struct ipv6_prefixd_state *pfd; > > @@ -908,7 +916,7 @@ pinctrl_prefixd_state_handler(const struct flow *ip_flow, > pfd->state = PREFIX_PENDING; > pfd->server_addr = server_addr; > pfd->sa = sa; > - pfd->uuid.data = uuid; > + memcpy(pfd->uuid.data, uuid, uuid_len); > pfd->uuid.len = uuid_len; > pfd->plife_time = plife_time * 1000; > pfd->vlife_time = vlife_time * 1000; > @@ -933,8 +941,9 @@ pinctrl_parse_dhcpv6_reply(struct dp_packet *pkt_in, > unsigned char *in_dhcpv6_data = (unsigned char *)(udp_in + 1); > size_t dlen = MIN(ntohs(udp_in->udp_len), dp_packet_l4_size(pkt_in)); > unsigned t1 = 0, t2 = 0, vlife_time = 0, plife_time = 0; > - uint8_t *end = (uint8_t *)udp_in + dlen, *uuid = NULL; > + uint8_t *end = (uint8_t *) udp_in + dlen; > uint8_t prefix_len = 0, uuid_len = 0; > + uint8_t uuid[DHCPV6_MAX_DUID_LEN]; > struct in6_addr ipv6 = in6addr_any; > bool status = false; > unsigned aid = 0; > @@ -993,8 +1002,7 @@ pinctrl_parse_dhcpv6_reply(struct dp_packet *pkt_in, > break; > } > case DHCPV6_OPT_SERVER_ID_CODE: > - uuid_len = ntohs(in_opt->len); > - uuid = xmalloc(uuid_len); > + uuid_len = MIN(ntohs(in_opt->len), DHCPV6_MAX_DUID_LEN); > memcpy(uuid, in_opt + 1, uuid_len); > break; > default: > @@ -1014,8 +1022,6 @@ pinctrl_parse_dhcpv6_reply(struct dp_packet *pkt_in, > pinctrl_prefixd_state_handler(ip_flow, ipv6, aid, eth->eth_src, > ip6_src, prefix_len, t1, t2, > plife_time, vlife_time, uuid, uuid_len); > - } else if (uuid) { > - free(uuid); > } > } > > @@ -1212,10 +1218,7 @@ static bool ipv6_prefixd_should_inject(void) > if (pfd->state == PREFIX_RENEW && > cur_time > pfd->last_complete + pfd->t2) { > pfd->state = PREFIX_REBIND; > - if (pfd->uuid.len) { > - free(pfd->uuid.data); > - pfd->uuid.len = 0; > - } > + pfd->uuid.len = 0; > return true; > } > if (pfd->state == PREFIX_REBIND && > @@ -1409,12 +1412,8 @@ prepare_ipv6_prefixd(struct ovsdb_idl_txn *ovnsb_idl_txn, > SHASH_FOR_EACH_SAFE (iter, &ipv6_prefixd) { > struct ipv6_prefixd_state *pfd = iter->data; > if (pfd->last_used + IPV6_PREFIXD_STALE_TIMEOUT < time_msec()) { > - if (pfd->uuid.len) { > - free(pfd->uuid.data); > - pfd->uuid.len = 0; > - } > - free(pfd); > shash_delete(&ipv6_prefixd, iter); > + free(pfd); > } > } >
On Fri, Jul 14, 2023 at 2:44 PM Dumitru Ceara <dceara@redhat.com> wrote: > On 7/14/23 14:39, Dumitru Ceara wrote: > > It's specified in RFC 8415. This also avoids having to free/realloc the > > pfd->uuid.data memory. That part was not correct anyway and was flagged > > by ASAN as a memleak: > > > > Direct leak of 42 byte(s) in 3 object(s) allocated from: > > #0 0x55e5b6354c9e in malloc > (/workspace/ovn-tmp/controller/ovn-controller+0x2edc9e) (BuildId: > f963f8c756bd5a2207a9b3c922d4362e46bb3162) > > #1 0x55e5b671878d in xmalloc__ > /workspace/ovn-tmp/ovs/lib/util.c:140:15 > > #2 0x55e5b671878d in xmalloc > /workspace/ovn-tmp/ovs/lib/util.c:175:12 > > #3 0x55e5b642cebc in pinctrl_parse_dhcpv6_reply > /workspace/ovn-tmp/controller/pinctrl.c:997:20 > > #4 0x55e5b642cebc in pinctrl_handle_dhcp6_server > /workspace/ovn-tmp/controller/pinctrl.c:1040:9 > > #5 0x55e5b642cebc in process_packet_in > /workspace/ovn-tmp/controller/pinctrl.c:3210:9 > > #6 0x55e5b642cebc in pinctrl_recv > /workspace/ovn-tmp/controller/pinctrl.c:3290:9 > > #7 0x55e5b642cebc in pinctrl_handler > /workspace/ovn-tmp/controller/pinctrl.c:3385:17 > > #8 0x55e5b66ef664 in ovsthread_wrapper > /workspace/ovn-tmp/ovs/lib/ovs-thread.c:423:12 > > #9 0x7faa30194b42 (/lib/x86_64-linux-gnu/libc.so.6+0x94b42) > (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d) > > > > Fixes: faa44a0c60a3 ("controller: IPv6 Prefix-Delegation: introduce > RENEW/REBIND msg support") > > I forgot to add Ales as co-author, I apologize. > > Co-authored-by: Ales Musil <amusil@redhat.com> > Signed-off-by: Ales Musil <amusil@redhat.com> > > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > --- > > controller/pinctrl.c | 33 ++++++++++++++++----------------- > > 1 file changed, 16 insertions(+), 17 deletions(-) > > > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > > index 6027ba0afb..bed90fe0b7 100644 > > --- a/controller/pinctrl.c > > +++ b/controller/pinctrl.c > > @@ -674,6 +674,14 @@ enum { > > PREFIX_REBIND, > > }; > > > > +/* According to RFC 8415, section 11: > > + * A DUID consists of a 2-octet type code represented in network byte > > + * order, followed by a variable number of octets that make up the > > + * actual identifier. The length of the DUID (not including the type > > + * code) is at least 1 octet and at most 128 octets. > > +*/ > > +#define DHCPV6_MAX_DUID_LEN 130 > > + > > struct ipv6_prefixd_state { > > long long int next_announce; > > long long int last_complete; > > @@ -683,7 +691,7 @@ struct ipv6_prefixd_state { > > struct eth_addr sa; > > /* server_id_info */ > > struct { > > - uint8_t *data; > > + uint8_t data[DHCPV6_MAX_DUID_LEN]; > > uint8_t len; > > } uuid; > > struct in6_addr ipv6_addr; > > @@ -899,7 +907,7 @@ pinctrl_prefixd_state_handler(const struct flow > *ip_flow, > > struct eth_addr sa, struct in6_addr > server_addr, > > char prefix_len, unsigned t1, unsigned t2, > > unsigned plife_time, unsigned vlife_time, > > - uint8_t *uuid, uint8_t uuid_len) > > + const uint8_t *uuid, uint8_t uuid_len) > > { > > struct ipv6_prefixd_state *pfd; > > > > @@ -908,7 +916,7 @@ pinctrl_prefixd_state_handler(const struct flow > *ip_flow, > > pfd->state = PREFIX_PENDING; > > pfd->server_addr = server_addr; > > pfd->sa = sa; > > - pfd->uuid.data = uuid; > > + memcpy(pfd->uuid.data, uuid, uuid_len); > > pfd->uuid.len = uuid_len; > > pfd->plife_time = plife_time * 1000; > > pfd->vlife_time = vlife_time * 1000; > > @@ -933,8 +941,9 @@ pinctrl_parse_dhcpv6_reply(struct dp_packet *pkt_in, > > unsigned char *in_dhcpv6_data = (unsigned char *)(udp_in + 1); > > size_t dlen = MIN(ntohs(udp_in->udp_len), > dp_packet_l4_size(pkt_in)); > > unsigned t1 = 0, t2 = 0, vlife_time = 0, plife_time = 0; > > - uint8_t *end = (uint8_t *)udp_in + dlen, *uuid = NULL; > > + uint8_t *end = (uint8_t *) udp_in + dlen; > > uint8_t prefix_len = 0, uuid_len = 0; > > + uint8_t uuid[DHCPV6_MAX_DUID_LEN]; > > struct in6_addr ipv6 = in6addr_any; > > bool status = false; > > unsigned aid = 0; > > @@ -993,8 +1002,7 @@ pinctrl_parse_dhcpv6_reply(struct dp_packet *pkt_in, > > break; > > } > > case DHCPV6_OPT_SERVER_ID_CODE: > > - uuid_len = ntohs(in_opt->len); > > - uuid = xmalloc(uuid_len); > > + uuid_len = MIN(ntohs(in_opt->len), DHCPV6_MAX_DUID_LEN); > > memcpy(uuid, in_opt + 1, uuid_len); > > break; > > default: > > @@ -1014,8 +1022,6 @@ pinctrl_parse_dhcpv6_reply(struct dp_packet > *pkt_in, > > pinctrl_prefixd_state_handler(ip_flow, ipv6, aid, eth->eth_src, > > ip6_src, prefix_len, t1, t2, > > plife_time, vlife_time, uuid, > uuid_len); > > - } else if (uuid) { > > - free(uuid); > > } > > } > > > > @@ -1212,10 +1218,7 @@ static bool ipv6_prefixd_should_inject(void) > > if (pfd->state == PREFIX_RENEW && > > cur_time > pfd->last_complete + pfd->t2) { > > pfd->state = PREFIX_REBIND; > > - if (pfd->uuid.len) { > > - free(pfd->uuid.data); > > - pfd->uuid.len = 0; > > - } > > + pfd->uuid.len = 0; > > return true; > > } > > if (pfd->state == PREFIX_REBIND && > > @@ -1409,12 +1412,8 @@ prepare_ipv6_prefixd(struct ovsdb_idl_txn > *ovnsb_idl_txn, > > SHASH_FOR_EACH_SAFE (iter, &ipv6_prefixd) { > > struct ipv6_prefixd_state *pfd = iter->data; > > if (pfd->last_used + IPV6_PREFIXD_STALE_TIMEOUT < time_msec()) { > > - if (pfd->uuid.len) { > > - free(pfd->uuid.data); > > - pfd->uuid.len = 0; > > - } > > - free(pfd); > > shash_delete(&ipv6_prefixd, iter); > > + free(pfd); > > } > > } > > > > Looks good to me, thanks. Acked-by: Ales Musil <amusil@redhat.com>
Bleep bloop. Greetings Dumitru Ceara, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Ales Musil <amusil@redhat.com> Lines checked: 135, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
> It's specified in RFC 8415. This also avoids having to free/realloc the > pfd->uuid.data memory. That part was not correct anyway and was flagged > by ASAN as a memleak: > > Direct leak of 42 byte(s) in 3 object(s) allocated from: > #0 0x55e5b6354c9e in malloc (/workspace/ovn-tmp/controller/ovn-controller+0x2edc9e) (BuildId: f963f8c756bd5a2207a9b3c922d4362e46bb3162) > #1 0x55e5b671878d in xmalloc__ /workspace/ovn-tmp/ovs/lib/util.c:140:15 > #2 0x55e5b671878d in xmalloc /workspace/ovn-tmp/ovs/lib/util.c:175:12 > #3 0x55e5b642cebc in pinctrl_parse_dhcpv6_reply /workspace/ovn-tmp/controller/pinctrl.c:997:20 > #4 0x55e5b642cebc in pinctrl_handle_dhcp6_server /workspace/ovn-tmp/controller/pinctrl.c:1040:9 > #5 0x55e5b642cebc in process_packet_in /workspace/ovn-tmp/controller/pinctrl.c:3210:9 > #6 0x55e5b642cebc in pinctrl_recv /workspace/ovn-tmp/controller/pinctrl.c:3290:9 > #7 0x55e5b642cebc in pinctrl_handler /workspace/ovn-tmp/controller/pinctrl.c:3385:17 > #8 0x55e5b66ef664 in ovsthread_wrapper /workspace/ovn-tmp/ovs/lib/ovs-thread.c:423:12 > #9 0x7faa30194b42 (/lib/x86_64-linux-gnu/libc.so.6+0x94b42) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d) > > Fixes: faa44a0c60a3 ("controller: IPv6 Prefix-Delegation: introduce RENEW/REBIND msg support") > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > controller/pinctrl.c | 33 ++++++++++++++++----------------- > 1 file changed, 16 insertions(+), 17 deletions(-) thx for fixing it. Acked-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index 6027ba0afb..bed90fe0b7 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -674,6 +674,14 @@ enum { > PREFIX_REBIND, > }; > > +/* According to RFC 8415, section 11: > + * A DUID consists of a 2-octet type code represented in network byte > + * order, followed by a variable number of octets that make up the > + * actual identifier. The length of the DUID (not including the type > + * code) is at least 1 octet and at most 128 octets. > +*/ > +#define DHCPV6_MAX_DUID_LEN 130 > + > struct ipv6_prefixd_state { > long long int next_announce; > long long int last_complete; > @@ -683,7 +691,7 @@ struct ipv6_prefixd_state { > struct eth_addr sa; > /* server_id_info */ > struct { > - uint8_t *data; > + uint8_t data[DHCPV6_MAX_DUID_LEN]; > uint8_t len; > } uuid; > struct in6_addr ipv6_addr; > @@ -899,7 +907,7 @@ pinctrl_prefixd_state_handler(const struct flow *ip_flow, > struct eth_addr sa, struct in6_addr server_addr, > char prefix_len, unsigned t1, unsigned t2, > unsigned plife_time, unsigned vlife_time, > - uint8_t *uuid, uint8_t uuid_len) > + const uint8_t *uuid, uint8_t uuid_len) > { > struct ipv6_prefixd_state *pfd; > > @@ -908,7 +916,7 @@ pinctrl_prefixd_state_handler(const struct flow *ip_flow, > pfd->state = PREFIX_PENDING; > pfd->server_addr = server_addr; > pfd->sa = sa; > - pfd->uuid.data = uuid; > + memcpy(pfd->uuid.data, uuid, uuid_len); > pfd->uuid.len = uuid_len; > pfd->plife_time = plife_time * 1000; > pfd->vlife_time = vlife_time * 1000; > @@ -933,8 +941,9 @@ pinctrl_parse_dhcpv6_reply(struct dp_packet *pkt_in, > unsigned char *in_dhcpv6_data = (unsigned char *)(udp_in + 1); > size_t dlen = MIN(ntohs(udp_in->udp_len), dp_packet_l4_size(pkt_in)); > unsigned t1 = 0, t2 = 0, vlife_time = 0, plife_time = 0; > - uint8_t *end = (uint8_t *)udp_in + dlen, *uuid = NULL; > + uint8_t *end = (uint8_t *) udp_in + dlen; > uint8_t prefix_len = 0, uuid_len = 0; > + uint8_t uuid[DHCPV6_MAX_DUID_LEN]; > struct in6_addr ipv6 = in6addr_any; > bool status = false; > unsigned aid = 0; > @@ -993,8 +1002,7 @@ pinctrl_parse_dhcpv6_reply(struct dp_packet *pkt_in, > break; > } > case DHCPV6_OPT_SERVER_ID_CODE: > - uuid_len = ntohs(in_opt->len); > - uuid = xmalloc(uuid_len); > + uuid_len = MIN(ntohs(in_opt->len), DHCPV6_MAX_DUID_LEN); > memcpy(uuid, in_opt + 1, uuid_len); > break; > default: > @@ -1014,8 +1022,6 @@ pinctrl_parse_dhcpv6_reply(struct dp_packet *pkt_in, > pinctrl_prefixd_state_handler(ip_flow, ipv6, aid, eth->eth_src, > ip6_src, prefix_len, t1, t2, > plife_time, vlife_time, uuid, uuid_len); > - } else if (uuid) { > - free(uuid); > } > } > > @@ -1212,10 +1218,7 @@ static bool ipv6_prefixd_should_inject(void) > if (pfd->state == PREFIX_RENEW && > cur_time > pfd->last_complete + pfd->t2) { > pfd->state = PREFIX_REBIND; > - if (pfd->uuid.len) { > - free(pfd->uuid.data); > - pfd->uuid.len = 0; > - } > + pfd->uuid.len = 0; > return true; > } > if (pfd->state == PREFIX_REBIND && > @@ -1409,12 +1412,8 @@ prepare_ipv6_prefixd(struct ovsdb_idl_txn *ovnsb_idl_txn, > SHASH_FOR_EACH_SAFE (iter, &ipv6_prefixd) { > struct ipv6_prefixd_state *pfd = iter->data; > if (pfd->last_used + IPV6_PREFIXD_STALE_TIMEOUT < time_msec()) { > - if (pfd->uuid.len) { > - free(pfd->uuid.data); > - pfd->uuid.len = 0; > - } > - free(pfd); > shash_delete(&ipv6_prefixd, iter); > + free(pfd); > } > } > > -- > 2.31.1 >
On 7/14/23 17:17, Lorenzo Bianconi wrote: >> It's specified in RFC 8415. This also avoids having to free/realloc the >> pfd->uuid.data memory. That part was not correct anyway and was flagged >> by ASAN as a memleak: >> >> Direct leak of 42 byte(s) in 3 object(s) allocated from: >> #0 0x55e5b6354c9e in malloc (/workspace/ovn-tmp/controller/ovn-controller+0x2edc9e) (BuildId: f963f8c756bd5a2207a9b3c922d4362e46bb3162) >> #1 0x55e5b671878d in xmalloc__ /workspace/ovn-tmp/ovs/lib/util.c:140:15 >> #2 0x55e5b671878d in xmalloc /workspace/ovn-tmp/ovs/lib/util.c:175:12 >> #3 0x55e5b642cebc in pinctrl_parse_dhcpv6_reply /workspace/ovn-tmp/controller/pinctrl.c:997:20 >> #4 0x55e5b642cebc in pinctrl_handle_dhcp6_server /workspace/ovn-tmp/controller/pinctrl.c:1040:9 >> #5 0x55e5b642cebc in process_packet_in /workspace/ovn-tmp/controller/pinctrl.c:3210:9 >> #6 0x55e5b642cebc in pinctrl_recv /workspace/ovn-tmp/controller/pinctrl.c:3290:9 >> #7 0x55e5b642cebc in pinctrl_handler /workspace/ovn-tmp/controller/pinctrl.c:3385:17 >> #8 0x55e5b66ef664 in ovsthread_wrapper /workspace/ovn-tmp/ovs/lib/ovs-thread.c:423:12 >> #9 0x7faa30194b42 (/lib/x86_64-linux-gnu/libc.so.6+0x94b42) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d) >> >> Fixes: faa44a0c60a3 ("controller: IPv6 Prefix-Delegation: introduce RENEW/REBIND msg support") >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> --- >> controller/pinctrl.c | 33 ++++++++++++++++----------------- >> 1 file changed, 16 insertions(+), 17 deletions(-) > > thx for fixing it. > > Acked-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > Thanks, Lorenzo and Ales! I applied this to main and to all branches down to 22.03. Regards, Dumitru
diff --git a/controller/pinctrl.c b/controller/pinctrl.c index 6027ba0afb..bed90fe0b7 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -674,6 +674,14 @@ enum { PREFIX_REBIND, }; +/* According to RFC 8415, section 11: + * A DUID consists of a 2-octet type code represented in network byte + * order, followed by a variable number of octets that make up the + * actual identifier. The length of the DUID (not including the type + * code) is at least 1 octet and at most 128 octets. +*/ +#define DHCPV6_MAX_DUID_LEN 130 + struct ipv6_prefixd_state { long long int next_announce; long long int last_complete; @@ -683,7 +691,7 @@ struct ipv6_prefixd_state { struct eth_addr sa; /* server_id_info */ struct { - uint8_t *data; + uint8_t data[DHCPV6_MAX_DUID_LEN]; uint8_t len; } uuid; struct in6_addr ipv6_addr; @@ -899,7 +907,7 @@ pinctrl_prefixd_state_handler(const struct flow *ip_flow, struct eth_addr sa, struct in6_addr server_addr, char prefix_len, unsigned t1, unsigned t2, unsigned plife_time, unsigned vlife_time, - uint8_t *uuid, uint8_t uuid_len) + const uint8_t *uuid, uint8_t uuid_len) { struct ipv6_prefixd_state *pfd; @@ -908,7 +916,7 @@ pinctrl_prefixd_state_handler(const struct flow *ip_flow, pfd->state = PREFIX_PENDING; pfd->server_addr = server_addr; pfd->sa = sa; - pfd->uuid.data = uuid; + memcpy(pfd->uuid.data, uuid, uuid_len); pfd->uuid.len = uuid_len; pfd->plife_time = plife_time * 1000; pfd->vlife_time = vlife_time * 1000; @@ -933,8 +941,9 @@ pinctrl_parse_dhcpv6_reply(struct dp_packet *pkt_in, unsigned char *in_dhcpv6_data = (unsigned char *)(udp_in + 1); size_t dlen = MIN(ntohs(udp_in->udp_len), dp_packet_l4_size(pkt_in)); unsigned t1 = 0, t2 = 0, vlife_time = 0, plife_time = 0; - uint8_t *end = (uint8_t *)udp_in + dlen, *uuid = NULL; + uint8_t *end = (uint8_t *) udp_in + dlen; uint8_t prefix_len = 0, uuid_len = 0; + uint8_t uuid[DHCPV6_MAX_DUID_LEN]; struct in6_addr ipv6 = in6addr_any; bool status = false; unsigned aid = 0; @@ -993,8 +1002,7 @@ pinctrl_parse_dhcpv6_reply(struct dp_packet *pkt_in, break; } case DHCPV6_OPT_SERVER_ID_CODE: - uuid_len = ntohs(in_opt->len); - uuid = xmalloc(uuid_len); + uuid_len = MIN(ntohs(in_opt->len), DHCPV6_MAX_DUID_LEN); memcpy(uuid, in_opt + 1, uuid_len); break; default: @@ -1014,8 +1022,6 @@ pinctrl_parse_dhcpv6_reply(struct dp_packet *pkt_in, pinctrl_prefixd_state_handler(ip_flow, ipv6, aid, eth->eth_src, ip6_src, prefix_len, t1, t2, plife_time, vlife_time, uuid, uuid_len); - } else if (uuid) { - free(uuid); } } @@ -1212,10 +1218,7 @@ static bool ipv6_prefixd_should_inject(void) if (pfd->state == PREFIX_RENEW && cur_time > pfd->last_complete + pfd->t2) { pfd->state = PREFIX_REBIND; - if (pfd->uuid.len) { - free(pfd->uuid.data); - pfd->uuid.len = 0; - } + pfd->uuid.len = 0; return true; } if (pfd->state == PREFIX_REBIND && @@ -1409,12 +1412,8 @@ prepare_ipv6_prefixd(struct ovsdb_idl_txn *ovnsb_idl_txn, SHASH_FOR_EACH_SAFE (iter, &ipv6_prefixd) { struct ipv6_prefixd_state *pfd = iter->data; if (pfd->last_used + IPV6_PREFIXD_STALE_TIMEOUT < time_msec()) { - if (pfd->uuid.len) { - free(pfd->uuid.data); - pfd->uuid.len = 0; - } - free(pfd); shash_delete(&ipv6_prefixd, iter); + free(pfd); } }
It's specified in RFC 8415. This also avoids having to free/realloc the pfd->uuid.data memory. That part was not correct anyway and was flagged by ASAN as a memleak: Direct leak of 42 byte(s) in 3 object(s) allocated from: #0 0x55e5b6354c9e in malloc (/workspace/ovn-tmp/controller/ovn-controller+0x2edc9e) (BuildId: f963f8c756bd5a2207a9b3c922d4362e46bb3162) #1 0x55e5b671878d in xmalloc__ /workspace/ovn-tmp/ovs/lib/util.c:140:15 #2 0x55e5b671878d in xmalloc /workspace/ovn-tmp/ovs/lib/util.c:175:12 #3 0x55e5b642cebc in pinctrl_parse_dhcpv6_reply /workspace/ovn-tmp/controller/pinctrl.c:997:20 #4 0x55e5b642cebc in pinctrl_handle_dhcp6_server /workspace/ovn-tmp/controller/pinctrl.c:1040:9 #5 0x55e5b642cebc in process_packet_in /workspace/ovn-tmp/controller/pinctrl.c:3210:9 #6 0x55e5b642cebc in pinctrl_recv /workspace/ovn-tmp/controller/pinctrl.c:3290:9 #7 0x55e5b642cebc in pinctrl_handler /workspace/ovn-tmp/controller/pinctrl.c:3385:17 #8 0x55e5b66ef664 in ovsthread_wrapper /workspace/ovn-tmp/ovs/lib/ovs-thread.c:423:12 #9 0x7faa30194b42 (/lib/x86_64-linux-gnu/libc.so.6+0x94b42) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d) Fixes: faa44a0c60a3 ("controller: IPv6 Prefix-Delegation: introduce RENEW/REBIND msg support") Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- controller/pinctrl.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-)