Double free in trunk(4)

2015-06-10 Thread Martin Pieuchot
During clone/destroy stress tests on pseudo-interfaces I found a double
free easily reproducible with dhclient(8) running on top of a trunk(4).

The problem comes from trunk_ether_delmulti() which is almost identical
to carp's version except that it always free "mc".

So when you do "# ifconfig trunk0 destroy" the kernel first brings the
interface down and generates a routing message.  dhclient(8) receives
this message and removes the address it's configured on trunk0 and with
it the "default multicast group".  If dhclient(8) lose the race, it will
fail to remove the multicast address but it will free "mc"!

Then ifconfig's thread which was asleep in trunk_ether_purgemulti() -> 
trunk_ioctl_allports() wakes up and tries to free "mc" a second time:

 uvm_fault(0xd5647bd0, 0x0, 0, 1) -> e
 kernel: page fault trap, code=0
 Stopped at  trunk_ether_purgemulti+0xc2:movl0x104(%edx),%eax
 ddb> tr
 trunk_ether_purgemulti(d131b800,d131b800,0,d041d091,0) at trunk_ether_purgemult
 i+0xc2
 trunk_clone_destroy(d131b800,0,f3766dec,d1064e94,f3766ef4) at trunk_clone_destr
 oy+0x16
 ifioctl(d54d10f0,80206979,f3766e84,d54a42dc,2) at ifioctl+0x232
 sys_ioctl(d54a42dc,f3766f60,f3766f80,d0566f17,d54a42dc) at sys_ioctl+0x257
 syscall() at syscall+0x247

Here's a simple fix that also reduces the differences with carp's
version.  ok?

Index: net/if_trunk.c
===
RCS file: /cvs/src/sys/net/if_trunk.c,v
retrieving revision 1.101
diff -u -p -r1.101 if_trunk.c
--- net/if_trunk.c  9 Jun 2015 14:50:14 -   1.101
+++ net/if_trunk.c  10 Jun 2015 15:30:10 -
@@ -857,18 +857,20 @@ trunk_ether_delmulti(struct trunk_softc 
if ((error = ether_delmulti(ifr, &tr->tr_ac)) != ENETRESET)
return (error);
 
-   if ((error = trunk_ioctl_allports(tr, SIOCDELMULTI,
-   (caddr_t)ifr)) != 0) {
+   /* We no longer use this multicast address.  Tell parent so. */
+   error = trunk_ioctl_allports(tr, SIOCDELMULTI, (caddr_t)ifr);
+   if (error == 0) {
+   SLIST_REMOVE(&tr->tr_mc_head, mc, trunk_mc, mc_entries);
+   free(mc, M_DEVBUF, sizeof(*mc));
+   } else {
/* XXX At least one port failed to remove the address */
if (tr->tr_ifflags & IFF_DEBUG) {
printf("%s: failed to remove multicast address "
-   "on all ports\n", tr->tr_ifname);
+   "on all ports (%d)\n", tr->tr_ifname, error);
}
+   (void)ether_addmulti(ifr, &tr->tr_ac);
}
 
-   SLIST_REMOVE(&tr->tr_mc_head, mc, trunk_mc, mc_entries);
-   free(mc, M_DEVBUF, 0);
-
return (0);
 }
 
@@ -886,7 +888,7 @@ trunk_ether_purgemulti(struct trunk_soft
trunk_ioctl_allports(tr, SIOCDELMULTI, (caddr_t)ifr);
 
SLIST_REMOVE(&tr->tr_mc_head, mc, trunk_mc, mc_entries);
-   free(mc, M_DEVBUF, 0);
+   free(mc, M_DEVBUF, sizeof(*mc));
}
 }
 



Re: Double free in trunk(4)

2015-06-12 Thread Dariusz Swiderski
> On 10 cze 2015, at 17:55, Martin Pieuchot  wrote:
> 
> During clone/destroy stress tests on pseudo-interfaces I found a double
> free easily reproducible with dhclient(8) running on top of a trunk(4).
> 
> The problem comes from trunk_ether_delmulti() which is almost identical
> to carp's version except that it always free "mc".
> 
> So when you do "# ifconfig trunk0 destroy" the kernel first brings the
> interface down and generates a routing message.  dhclient(8) receives
> this message and removes the address it's configured on trunk0 and with
> it the "default multicast group".  If dhclient(8) lose the race, it will
> fail to remove the multicast address but it will free "mc"!
> 
> Then ifconfig's thread which was asleep in trunk_ether_purgemulti() ->
> trunk_ioctl_allports() wakes up and tries to free "mc" a second time:
> 
> uvm_fault(0xd5647bd0, 0x0, 0, 1) -> e
> kernel: page fault trap, code=0
> Stopped at  trunk_ether_purgemulti+0xc2:movl0x104(%edx),%eax
> ddb> tr
> trunk_ether_purgemulti(d131b800,d131b800,0,d041d091,0) at 
> trunk_ether_purgemult
> i+0xc2
> trunk_clone_destroy(d131b800,0,f3766dec,d1064e94,f3766ef4) at 
> trunk_clone_destr
> oy+0x16
> ifioctl(d54d10f0,80206979,f3766e84,d54a42dc,2) at ifioctl+0x232
> sys_ioctl(d54a42dc,f3766f60,f3766f80,d0566f17,d54a42dc) at sys_ioctl+0x257
> syscall() at syscall+0x247
> 
> Here's a simple fix that also reduces the differences with carp's
> version.  ok?

Hi,

No regression on my machine and code looks sane, so ok dms@

btw.: since you introduced such nice debug logging, would be nice
to see it in both vlan(4) and carp(4) :)

greets
--
Maciej 'sfires' Swiderski
---
SysAdm | SecOff  | DS14145-RIPE| DS11-6BONE
193.178.161.0/24 | 3ffe:8010:7:2a::/64 | AS16288
---
A mouse is a device used to point at the xterm you want to type in.




signature.asc
Description: Message signed with OpenPGP using GPGMail