Skip to content

Commit 6490a48

Browse files
committed
address: fix use after free
When adding an address to an interface, we use gr_vec_add() which may call realloc() internally and may free the old memory area. This can cause use after free bugs as detected by libasan in the CI: + grcli interface add port gm2dgn1 devargs net_tap1,iface=gm2dgn1 mac f0:0d:ac:dc:00:01 ... DEBUG: GROUT: iface_event: iface event [0xacdc0001] POST_ADD triggered for iface gm2dgn1. INFO: GROUT: mcast6_addr_add: gm2dgn1: joining multicast group ff02::1:ffdc:1 INFO: GROUT: mcast6_addr_add: gm2dgn1: joining multicast group ff01::1 INFO: GROUT: mcast6_addr_add: gm2dgn1: joining multicast group ff02::1 INFO: GROUT: mcast6_addr_add: gm2dgn1: joining multicast group ff01::2 NOTICE: GROUT: mcast6_addr_add: gm2dgn1: joining multicast group ff02::2 GROUT: trace_log_packet: [rx gm2dgn1] f0:0d:ac:dc:00:01 > 33:33:00:00:00:16 / IPv6 :: > ff02::16 ttl=1 proto=HOPOPT(0) / ICMPv6 type=143 code=0, (pkt_len=90) ================================================================= ==90746==ERROR: AddressSanitizer: heap-use-after-free on address 0x504000006390 at pc 0x560cd400d1b7 bp 0x7f6cbdfd7760 sp 0x7f6cbdfd7750 READ of size 8 at 0x504000006390 thread T4 #0 0x560cd400d1b6 in __gr_vec_hdr ../main/gr_vec.h:34 #1 0x560cd400d284 in gr_vec_len ../main/gr_vec.h:50 DPDK#2 0x560cd400dc98 in mcast6_get_member ../modules/ip6/control/address.c:92 DPDK#3 0x560cd404d30e in ip6_input_process ../modules/ip6/datapath/ip6_input.c:96 DPDK#4 0x560cd3f8ef05 in __rte_node_process ../subprojects/dpdk/lib/graph/rte_graph_worker_common.h:207 DPDK#5 0x560cd3f8ef05 in rte_graph_walk_rtc ../subprojects/dpdk/lib/graph/rte_graph_model_rtc.h:42 DPDK#6 0x560cd3f8f78f in rte_graph_walk ../subprojects/dpdk/lib/graph/rte_graph_worker.h:38 DPDK#7 0x560cd3f91c1c in gr_datapath_loop ../modules/infra/datapath/main_loop.c:252 ... 0x504000006390 is located 0 bytes inside of 48-byte region [0x504000006390,0x5040000063c0) freed by thread T0 here: #0 0x7f6d498fc778 in realloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:85 #1 0x560cd400d364 in __gr_vec_grow ../main/gr_vec.h:69 DPDK#2 0x560cd400e68c in mcast6_addr_add ../modules/ip6/control/address.c:141 DPDK#3 0x560cd4010bc6 in ip6_iface_event_handler ../modules/ip6/control/address.c:373 DPDK#4 0x560cd3f0209e in gr_event_push ../main/event.c:24 DPDK#5 0x560cd3f2d9b0 in iface_create ../modules/infra/control/iface.c:134 DPDK#6 0x560cd3f08c1a in iface_add ../modules/infra/api/iface.c:33 ... Prevent this from happening by cloning the address vector before adding anything to it. Add the new address to the clone and make it visible to datapath threads. Use the RCU to ensure all threads have seen the new vector and *then* free the old vector. Link: https://github.com/DPDK/grout/actions/runs/18442009960/job/52543656749#step:9:4363 Signed-off-by: Robin Jarry <rjarry@redhat.com>
1 parent 6a7e9a4 commit 6490a48

2 files changed

Lines changed: 37 additions & 3 deletions

File tree

modules/ip/control/address.c

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
#include <gr_module.h>
1111
#include <gr_net_types.h>
1212
#include <gr_queue.h>
13+
#include <gr_rcu.h>
1314
#include <gr_vec.h>
1415

1516
#include <event2/event.h>
@@ -108,7 +109,18 @@ static struct api_out addr_add(const void *request, struct api_ctx *) {
108109
if (ret < 0)
109110
return api_out(-ret, 0, NULL);
110111

111-
gr_vec_add(ifaddrs->nh, nh);
112+
// gr_vec_add may realloc() and free the old vector
113+
// Clone it and append to the clone.
114+
gr_vec struct nexthop **nhs_copy = gr_vec_clone(ifaddrs->nh);
115+
gr_vec struct nexthop **nhs_old = ifaddrs->nh;
116+
gr_vec_add(nhs_copy, nh);
117+
ifaddrs->nh = nhs_copy;
118+
if (nhs_old != NULL) {
119+
// Once all datapath workers have seen the new clone, free the old one.
120+
rte_rcu_qsbr_synchronize(gr_datapath_rcu(), RTE_QSBR_THRID_INVALID);
121+
gr_vec_free(nhs_old);
122+
}
123+
112124
gr_event_push(GR_EVENT_IP_ADDR_ADD, &req->addr);
113125

114126
return api_out(0, 0, NULL);

modules/ip6/control/address.c

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
#include <gr_module.h>
1111
#include <gr_net_types.h>
1212
#include <gr_queue.h>
13+
#include <gr_rcu.h>
1314
#include <gr_vec.h>
1415

1516
#include <event2/event.h>
@@ -138,7 +139,17 @@ static int mcast6_addr_add(const struct iface *iface, const struct rte_ipv6_addr
138139
}
139140

140141
nexthop_incref(nh);
141-
gr_vec_add(maddrs->nh, nh);
142+
// gr_vec_add may realloc() and free the old vector
143+
// Clone it and append to the clone.
144+
gr_vec struct nexthop **nhs_copy = gr_vec_clone(maddrs->nh);
145+
gr_vec struct nexthop **nhs_old = maddrs->nh;
146+
gr_vec_add(nhs_copy, nh);
147+
maddrs->nh = nhs_copy;
148+
if (nhs_old != NULL) {
149+
// Once all datapath workers have seen the new clone, free the old one.
150+
rte_rcu_qsbr_synchronize(gr_datapath_rcu(), RTE_QSBR_THRID_INVALID);
151+
gr_vec_free(nhs_old);
152+
}
142153

143154
// add ethernet filter
144155
return iface_add_eth_addr(iface->id, &mac);
@@ -228,7 +239,18 @@ iface6_addr_add(const struct iface *iface, const struct rte_ipv6_addr *ip, uint8
228239
if (ret < 0)
229240
return errno_set(-ret);
230241

231-
gr_vec_add(addrs->nh, nh);
242+
// gr_vec_add may realloc() and free the old vector
243+
// Clone it and append to the clone.
244+
gr_vec struct nexthop **nhs_copy = gr_vec_clone(addrs->nh);
245+
gr_vec struct nexthop **nhs_old = addrs->nh;
246+
gr_vec_add(nhs_copy, nh);
247+
addrs->nh = nhs_copy;
248+
if (nhs_old != NULL) {
249+
// Once all datapath workers have seen the new clone, free the old one.
250+
rte_rcu_qsbr_synchronize(gr_datapath_rcu(), RTE_QSBR_THRID_INVALID);
251+
gr_vec_free(nhs_old);
252+
}
253+
232254
gr_event_push(
233255
GR_EVENT_IP6_ADDR_ADD,
234256
&(struct gr_ip6_ifaddr) {

0 commit comments

Comments
 (0)