From aaef26cef6a8c36d0022c140fcfeb4f4789a5fa0 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 31 Jan 2021 08:52:44 -0500 Subject: [PATCH 1/2] ospf6d: prevent use after free Valgrind reports: 2437395-==2437395== Invalid read of size 8 2437395:==2437395== at 0x40B610: ospf6_asbr_update_route_ecmp_path (ospf6_asbr.c:327) 2437395-==2437395== by 0x40BC7C: ospf6_asbr_lsa_add (ospf6_asbr.c:544) 2437395-==2437395== by 0x40C5DF: ospf6_asbr_lsentry_add (ospf6_asbr.c:829) 2437395-==2437395== by 0x42D88D: ospf6_top_brouter_hook_add (ospf6_top.c:185) 2437395-==2437395== by 0x4188E3: ospf6_intra_brouter_calculation (ospf6_intra.c:2320) 2437395-==2437395== by 0x42C624: ospf6_spf_calculation_thread (ospf6_spf.c:638) 2437395-==2437395== by 0x4904B7E: thread_call (thread.c:1681) 2437395-==2437395== by 0x48CAA27: frr_run (libfrr.c:1126) 2437395-==2437395== by 0x40AF43: main (ospf6_main.c:232) 2437395-==2437395== Address 0x5c668a8 is 24 bytes inside a block of size 256 free'd 2437395:==2437395== at 0x48399AB: free (vg_replace_malloc.c:538) 2437395-==2437395== by 0x429027: ospf6_route_delete (ospf6_route.c:419) 2437395-==2437395== by 0x429027: ospf6_route_unlock (ospf6_route.c:460) 2437395-==2437395== by 0x429027: ospf6_route_remove (ospf6_route.c:887) 2437395-==2437395== by 0x40B343: ospf6_asbr_update_route_ecmp_path (ospf6_asbr.c:318) 2437395-==2437395== by 0x40BC7C: ospf6_asbr_lsa_add (ospf6_asbr.c:544) 2437395-==2437395== by 0x40C5DF: ospf6_asbr_lsentry_add (ospf6_asbr.c:829) 2437395-==2437395== by 0x42D88D: ospf6_top_brouter_hook_add (ospf6_top.c:185) 2437395-==2437395== by 0x4188E3: ospf6_intra_brouter_calculation (ospf6_intra.c:2320) 2437395-==2437395== by 0x42C624: ospf6_spf_calculation_thread (ospf6_spf.c:638) 2437395-==2437395== by 0x4904B7E: thread_call (thread.c:1681) 2437395-==2437395== by 0x48CAA27: frr_run (libfrr.c:1126) 2437395-==2437395== by 0x40AF43: main (ospf6_main.c:232) 2437395-==2437395== Block was alloc'd at 2437395:==2437395== at 0x483AB65: calloc (vg_replace_malloc.c:760) 2437395-==2437395== by 0x48D2A32: qcalloc (memory.c:115) 2437395-==2437395== by 0x427CE4: ospf6_route_create (ospf6_route.c:402) 2437395-==2437395== by 0x40BA8A: ospf6_asbr_lsa_add (ospf6_asbr.c:490) 2437395-==2437395== by 0x40C5DF: ospf6_asbr_lsentry_add (ospf6_asbr.c:829) 2437395-==2437395== by 0x42D88D: ospf6_top_brouter_hook_add (ospf6_top.c:185) 2437395-==2437395== by 0x4188E3: ospf6_intra_brouter_calculation (ospf6_intra.c:2320) 2437395-==2437395== by 0x42C624: ospf6_spf_calculation_thread (ospf6_spf.c:638) 2437395-==2437395== by 0x4904B7E: thread_call (thread.c:1681) 2437395-==2437395== by 0x48CAA27: frr_run (libfrr.c:1126) 2437395-==2437395== by 0x40AF43: main (ospf6_main.c:232) ospfv3 loops through the ecmp routes to decide what to clean up. In some situations the code free's up an existing route at the head of the list. Cleaning the pointers in the list but never touching the original pointer. In that case notice and update the old pointer. Signed-off-by: Donald Sharp --- ospf6d/ospf6_asbr.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/ospf6d/ospf6_asbr.c b/ospf6d/ospf6_asbr.c index fdfd53276e..3449f48267 100644 --- a/ospf6d/ospf6_asbr.c +++ b/ospf6d/ospf6_asbr.c @@ -210,7 +210,7 @@ void ospf6_asbr_update_route_ecmp_path(struct ospf6_route *old, struct ospf6_route *route, struct ospf6 *ospf6) { - struct ospf6_route *old_route; + struct ospf6_route *old_route, *next_route; struct ospf6_path *ecmp_path, *o_path = NULL; struct listnode *anode, *anext; struct listnode *nnode, *rnode, *rnext; @@ -220,9 +220,11 @@ void ospf6_asbr_update_route_ecmp_path(struct ospf6_route *old, /* check for old entry match with new route origin, * delete old entry. */ - for (old_route = old; old_route; old_route = old_route->next) { + for (old_route = old; old_route; old_route = next_route) { bool route_updated = false; + next_route = old_route->next; + if (!ospf6_route_is_same(old_route, route) || (old_route->path.type != route->path.type)) continue; @@ -315,6 +317,8 @@ void ospf6_asbr_update_route_ecmp_path(struct ospf6_route *old, old_route->path.cost, route->path.cost); } + if (old == old_route) + old = next_route; ospf6_route_remove(old_route, ospf6->route_table); } From a013777abc919c6287b2c467aca11ae6e97e9efe Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 31 Jan 2021 08:56:00 -0500 Subject: [PATCH 2/2] zebra: Prevent sending of unininted data valgrind is reporting: 2448137-==2448137== Thread 5 zebra_apic: 2448137-==2448137== Syscall param writev(vector[...]) points to uninitialised byte(s) 2448137:==2448137== at 0x4D6FDDD: __writev (writev.c:26) 2448137-==2448137== by 0x4D6FDDD: writev (writev.c:24) 2448137-==2448137== by 0x48A35F5: buffer_flush_available (buffer.c:431) 2448137-==2448137== by 0x48A3504: buffer_flush_all (buffer.c:237) 2448137-==2448137== by 0x495948: zserv_write (zserv.c:263) 2448137-==2448137== by 0x4904B7E: thread_call (thread.c:1681) 2448137-==2448137== by 0x48BD3E5: fpt_run (frr_pthread.c:308) 2448137-==2448137== by 0x4C61EA6: start_thread (pthread_create.c:477) 2448137-==2448137== by 0x4D78DEE: clone (clone.S:95) 2448137-==2448137== Address 0x720c3ce is 62 bytes inside a block of size 4,120 alloc'd 2448137:==2448137== at 0x483877F: malloc (vg_replace_malloc.c:307) 2448137-==2448137== by 0x48D2977: qmalloc (memory.c:110) 2448137-==2448137== by 0x48A30E3: buffer_add (buffer.c:135) 2448137-==2448137== by 0x48A30E3: buffer_put (buffer.c:161) 2448137-==2448137== by 0x49591B: zserv_write (zserv.c:256) 2448137-==2448137== by 0x4904B7E: thread_call (thread.c:1681) 2448137-==2448137== by 0x48BD3E5: fpt_run (frr_pthread.c:308) 2448137-==2448137== by 0x4C61EA6: start_thread (pthread_create.c:477) 2448137-==2448137== by 0x4D78DEE: clone (clone.S:95) 2448137-==2448137== Uninitialised value was created by a stack allocation 2448137:==2448137== at 0x43E490: zserv_encode_vrf (zapi_msg.c:103) Effectively we are sending `struct vrf_data` without ensuring data has been properly initialized. Signed-off-by: Donald Sharp --- zebra/zapi_msg.c | 1 + 1 file changed, 1 insertion(+) diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index 8e68984823..21bff96b7d 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -104,6 +104,7 @@ static void zserv_encode_vrf(struct stream *s, struct zebra_vrf *zvrf) struct vrf_data data; const char *netns_name = zvrf_ns_name(zvrf); + memset(&data, 0, sizeof(data)); data.l.table_id = zvrf->table_id; if (netns_name)