segfault in age_routes() about pim6sd HOT 6 CLOSED

It's a use-after-free issue for a list traversal in age_routes():

• age_routes() loops over this:
  

  pim6sd/src/timer.c

  Lines 671 to 674 in 26c54d4

  	for (rp_grp_entry_ptr = cand_rp_ptr->rp_grp_next;
  	rp_grp_entry_ptr != (rp_grp_entry_t *) NULL;
  	rp_grp_entry_ptr = rp_grp_entry_ptr->rp_grp_next)
  	{

• And inside this loop, this is called:
  

  pim6sd/src/timer.c

  Lines 1190 to 1194 in 26c54d4

  	if (IF_ISEMPTY(&mrtentry_srcs->leaves))
  	{
  	delete_mrtentry(mrtentry_srcs);
  	continue;
  	}

• delete_mrtentry() then calls delete_grpentry() (1st call) which then calls delete_rp_grp_entry()

• delete_rp_grp_entry() then free()'s the rp_grp_entry_ptr we are currently using in the loop:
  

  pim6sd/src/rp.c

  Line 768 in 26c54d4

  free((char *) rp_grp_entry_delete);

• Before the delete_mrtentry() call rp_grp_entry_ptr->rp_grp_next was NULL. However after the free() it contains other, random values (when things crash).

from pim6sd.

Ouch, good catch! The code path in age_routes() is hariy, to say the least! Interestingly enough pimd doesn't contain any embedded-RP code, so this is something that must have been added by the pim6sd team.

There is an age_misc() as well, which also calls delete_rp_grp_entry(), so it's possible the code in delete_grpentry() is not needed if the rp_grp entry is cleaned up in a second pass, which is safer anyway.

from pim6sd.

Btw. valgrind also shows the use-after-free quite nicely, including the details where it was malloc'd, free'd and accessed:

$ valgrind --track-origins=yes /home/linus/dev-priv/pim6sd/src/pim6sd -d pim_bootstrap,pim_join_prune -n -f /tmp/pimtest/router2.conf
[...]
04:20:42.949 warning - setsockopt MRT6_DEL_MFC: No such file or directory
==2722== Invalid read of size 8
==2722==    at 0x1271A9: age_routes (timer.c:673)
==2722==    by 0x11492D: timer (main.c:577)
==2722==    by 0x10AF09: age_callout_queue (callout.c:130)
==2722==    by 0x10AA55: main (main.c:523)
==2722==  Address 0x4a95240 is 0 bytes inside a block of size 64 free'd
==2722==    at 0x48369AB: free (vg_replace_malloc.c:530)
==2722==    by 0x119F9B: delete_grpentry (mrt.c:611)
==2722==    by 0x11A26C: delete_mrtentry (mrt.c:701)
==2722==    by 0x127184: age_routes (timer.c:1215)
==2722==    by 0x11492D: timer (main.c:577)
==2722==    by 0x10AF09: age_callout_queue (callout.c:130)
==2722==    by 0x10AA55: main (main.c:523)
==2722==  Block was alloc'd at
==2722==    at 0x483577F: malloc (vg_replace_malloc.c:299)
==2722==    by 0x12649A: add_rp_grp_entry (rp.c:658)
==2722==    by 0x1258A5: rp_grp_match (rp.c:1274)
==2722==    by 0x11AE27: find_route (mrt.c:294)
==2722==    by 0x124981: process_cache_miss (route.c:1007)
==2722==    by 0x124981: process_kernel_call (route.c:924)
==2722==    by 0x10AB0F: main (main.c:535)
==2722==
==2722== Invalid read of size 8
==2722==    at 0x1271BF: age_routes (timer.c:424)
==2722==    by 0x11492D: timer (main.c:577)
==2722==    by 0x10AF09: age_callout_queue (callout.c:130)
==2722==    by 0x10AA55: main (main.c:523)
==2722==  Address 0x4a95140 is 0 bytes inside a block of size 32 free'd
==2722==    at 0x48369AB: free (vg_replace_malloc.c:530)
==2722==    by 0x125F6A: delete_rp_grp_entry (rp.c:756)
==2722==    by 0x119F9B: delete_grpentry (mrt.c:611)
==2722==    by 0x11A26C: delete_mrtentry (mrt.c:701)
==2722==    by 0x127184: age_routes (timer.c:1215)
==2722==    by 0x11492D: timer (main.c:577)
==2722==    by 0x10AF09: age_callout_queue (callout.c:130)
==2722==    by 0x10AA55: main (main.c:523)
==2722==  Block was alloc'd at
==2722==    at 0x483577F: malloc (vg_replace_malloc.c:299)
==2722==    by 0x12645A: add_cand_rp (rp.c:409)
==2722==    by 0x12645A: add_rp_grp_entry (rp.c:586)
==2722==    by 0x1258A5: rp_grp_match (rp.c:1274)
==2722==    by 0x11AE27: find_route (mrt.c:294)
==2722==    by 0x124981: process_cache_miss (route.c:1007)
==2722==    by 0x124981: process_kernel_call (route.c:924)
==2722==    by 0x10AB0F: main (main.c:535)
[...]

It also has nothing to do with PIM Join/Prune. I get the same with no multicast listener for this group. However it's the multicast sender side triggering it. As long as I start no ping6 -i 0.2 -t 16 -q ff7e:0240:fd5c:725:2841:4203::123 everything is fine. Also if I start ping6 later, nothing happens. Only if I start ping6 to the embedded-RP address at an early point only then valgrind reports an issue for this.

If I set the group_prefix to ff3e:... (so no embedded-RP address) then valgrind reports no issue for this either. Conceptually I do not quite understand yet how this would make a difference on the RP itself. Why would the RP itself need to care if it is an embedded-RP address or not?

Shouldn't it only make a difference for either a multicast router receiving an according MLD report on its interface or a multicast router receiving a PIM Join/Prune message?

(Hm, and yes, saw the comment in main.c, that age_routes() is supposed to clean the mrtentry objects and age_misc() is supposed to clean cand-RP list and other things. But I'm wondering whether something shouldn't be added or was added wrongly in the first place)

from pim6sd.

Hm, if I just delete this section in delete_grpentry():

Then I have an issue of an RP-Set entry which was added on an intemediate node via a PIM Join/Prune never timing out.

On such an intermediate node between the listener and RP, the entry is always shown with a "Hold" and "Age" of 65535:

---------------------------RP-Set----------------------------
Current BSR address: :: Prio: 0 Timeout: 160
RP-address(Upstream)/Group prefix             Prio Hold Age
fd5c:725:2841:4203::2(myself)
     ff7e:240:fd5c:725:2841:4203::/96         1    65535 65535

So I guess the IF_TIMEOUT(rp_grp_entry_ptr->holdtime) in age_misc() therefore never applies.

I tried the easy approach of just storing the next pointer at the start of the two for loops. And both valgrind seems happy with that and entries are still deleted fine on intermediate nodes.

Going to make a PR in a second.

from pim6sd.

Btw., there were also a few more reports by valgrind with a Syscall param [...] points to uninitialised byte(s) and on exit it reported 34 missing calls to free().

I love valgrind :-).

I guess it would make sense to try to fix these first, before any refactoring? With a clean valgrind report it'd be easier to spot and bisect memory related regressions introduced by any larger changes in the future.

from pim6sd.

Well done! Valgrind is one of the best gifts to humanity and engineers :-)

from pim6sd.