Re: Double free in trunk(4)

2015-06-12 Thread Dariusz Swiderski
 On 10 cze 2015, at 17:55, Martin Pieuchot m...@openbsd.org 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


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));
}
 }