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

[Gleb Smirnoff]

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

From: Gleb Smirnoff <glebius at FreeBSD.org>
To: Stevan Markovic <stevan_markovic at mcafee.com>
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
Date: Mon, 31 Oct 2011 21:10:22 +0300

   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.
 
 -- 
 Totus tuus, Glebius.