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

[Stevan_Markovic at McAfee.com]

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

From: <Stevan_Markovic at McAfee.com>
To: <glebius at FreeBSD.org>
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 13:26:43 -0500

 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 net=
 work stack instance specific data. Eliminating it would prevent using multi=
 cast in multiple virtual network stacks.=20
 
 Stevan=20
 
 -----Original Message-----
 From: Gleb Smirnoff [mailto:glebius at FreeBSD.org]=20
 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@=
 FreeBSD.org
 Subject: Re: misc/162201: [patch] multicast forwarding cache hash always al=
 located 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.or=
 g) 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 mfcha=
 shsize always has value 0.=20
 S>=20
 S> Why: variable mfchashsize is initialized in module event handler which i=
 s executed with SI_ORDER_ANY  ordering tag which happens _after_ variable u=
 sage in MALLOC in VNET_SYSINIT with SI_ORDER_MIDDLE.
 S>=20
 S> Fix simply moves variable initialization before its usage in vnet_mroute=
 _init.
 S>=20
 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 unpred=
 ictable.  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>=20
 S> >Fix:
 S> Fix simply moves mfchashsize variable initialization before its usage in=
  vnet_mroute_init.
 S>=20
 S> Patch attached with submission follows:
 S>=20
 S> Index: ip_mroute.c
 S> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
 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 =3D 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 =3D 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> =20
 S> -	mfchashsize =3D 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 =3D MFCHASHSIZE;
 S> -	}
 S> =20
 S>  	pim_squelch_wholepkt =3D 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.
 
 --=20
 Totus tuus, Glebius.