misc/162201: [patch] multicast forwarding cache hash always allocated with size 0, resulting in buffer overrun

[Marko Zec]

The following reply was made to PR kern/162201; it has been noted by GNATS.

From: Marko Zec <zec at freebsd.org>
To: Stevan_Markovic at mcafee.com
Cc: glebius at freebsd.org,
 freebsd-gnats-submit at freebsd.org,
 bms at freebsd.org,
 bz at freebsd.org
Subject: Re: misc/162201: [patch] multicast forwarding cache hash always allocated with size 0, resulting in buffer overrun
Date: Mon, 31 Oct 2011 21:13:27 +0100

 On Monday 31 October 2011 19:26:43 Stevan_Markovic at mcafee.com wrote:
 > Hi,
 >
 > Gleb, no, I have not tried to eliminate VNET_SYSINITS and I do not think it
 > can be done. My understanding is that VNET_SYSINIT initializes virtual
 > network stack instance specific data. Eliminating it would prevent using
 > multicast in multiple virtual network stacks.
 
 vnet_mroute_init() should be triggered after ip_mroute_modevent() is done, not 
 before, I think that's the whole wisdom here.  I'll throw a look at this...
 
 Marko
 
 
 
 > Stevan
 >
 > -----Original Message-----
 > From: Gleb Smirnoff [mailto:glebius at FreeBSD.org]
 > Sent: Monday, October 31, 2011 2:10 PM
 > To: Markovic, Stevan
 > Cc: freebsd-gnats-submit at FreeBSD.org; zec at FreeBSD.org; bms at FreeBSD.org;
 > bz at FreeBSD.org Subject: Re: misc/162201: [patch] multicast forwarding cache
 > hash always allocated with size 0, resulting in buffer overrun
 >
 >   Hi,
 >
 > On Mon, Oct 31, 2011 at 04:22:00PM +0000, Stevan Markovic wrote:
 > S> >Description:
 > S> Bug is observed as kernel panic shortly after stopping XORP
 > (www.xorp.org) configured for PIM/SM routing. Debugging discovered that at
 > the time of MALLOC for V_nexpire in ip_mroute.c::vnet_mroute_init() size
 > variable mfchashsize always has value 0. S>
 > S> Why: variable mfchashsize is initialized in module event handler which
 > is executed with SI_ORDER_ANY  ordering tag which happens _after_ variable
 > usage in MALLOC in VNET_SYSINIT with SI_ORDER_MIDDLE. S>
 > S> Fix simply moves variable initialization before its usage in
 > vnet_mroute_init. S>
 > S> This bug is discovered and fixed in McAfee Inc development.
 > S> >How-To-Repeat:
 > S> Hard to reproduce since system behavior after memory overwrite is
 > unpredictable.  Multicast forwarding cashe hash overrun always happens
 > after: S> a) configuring xorp to use PIM/SM
 > S> b) starting xorp_rtrmgr
 > S> c) stopping xorp_rtrmgr.
 > S>
 > S> >Fix:
 > S> Fix simply moves mfchashsize variable initialization before its usage in
 > vnet_mroute_init. S>
 > S> Patch attached with submission follows:
 > S>
 > S> Index: ip_mroute.c
 > S> ===================================================================
 > S> RCS file: /projects/freebsd/src_cvsup/src/sys/netinet/ip_mroute.c,v
 > S> retrieving revision 1.161
 > S> diff -u -r1.161 ip_mroute.c
 > S> --- ip_mroute.c	22 Nov 2010 19:32:54 -0000	1.161
 > S> +++ ip_mroute.c	31 Oct 2011 15:54:53 -0000
 > S> @@ -2814,7 +2814,13 @@
 > S>  static void
 > S>  vnet_mroute_init(const void *unused __unused)
 > S>  {
 > S> -
 > S> +	mfchashsize = MFCHASHSIZE;
 > S> +	if (TUNABLE_ULONG_FETCH("net.inet.ip.mfchashsize", &mfchashsize) &&
 > S> +	    !powerof2(mfchashsize)) {
 > S> +		printf("WARNING: %s not a power of 2; using default\n",
 > S> +		    "net.inet.ip.mfchashsize");
 > S> +		mfchashsize = MFCHASHSIZE;
 > S> +	}
 > S>  	MALLOC(V_nexpire, u_char *, mfchashsize, M_MRTABLE, M_WAITOK|M_ZERO);
 > S>  	bzero(V_bw_meter_timers, sizeof(V_bw_meter_timers));
 > S>  	callout_init(&V_expire_upcalls_ch, CALLOUT_MPSAFE);
 > S> @@ -2855,13 +2861,6 @@
 > S>  	MFC_LOCK_INIT();
 > S>  	VIF_LOCK_INIT();
 > S>
 > S> -	mfchashsize = MFCHASHSIZE;
 > S> -	if (TUNABLE_ULONG_FETCH("net.inet.ip.mfchashsize", &mfchashsize) &&
 > S> -	    !powerof2(mfchashsize)) {
 > S> -		printf("WARNING: %s not a power of 2; using default\n",
 > S> -		    "net.inet.ip.mfchashsize");
 > S> -		mfchashsize = MFCHASHSIZE;
 > S> -	}
 > S>
 > S>  	pim_squelch_wholepkt = 0;
 > S>  	TUNABLE_ULONG_FETCH("net.inet.pim.squelch_wholepkt",
 >
 > Have you tried to remove these VNET_SYSINITs at all and do all the
 > initialization in the ip_mroute_modevent() itself? From first glance
 > I see no reason for separate malloc() + callout_init()s.
 >
 > I am putting guys, who made and reviewed the commit, into Cc.