Bug 162201 - [ip] [patch] multicast forwarding cache hash always allocated with size 0, resulting in buffer overrun
Summary: [ip] [patch] multicast forwarding cache hash always allocated with size 0, re...
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: Unspecified
Hardware: Any Any
: Normal Affects Only Me
Assignee: freebsd-bugs (Nobody)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-31 16:30 UTC by Stevan Markovic
Modified: 2019-01-19 20:10 UTC (History)
1 user (show)

See Also:

Attachments
file.diff (1.21 KB, patch)
2011-10-31 16:30 UTC, Stevan Markovic 		no flags Details | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Stevan Markovic 2011-10-31 16:30:10 UTC 
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. 

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.

Fix simply moves variable initialization before its usage in vnet_mroute_init.

This bug is discovered and fixed in McAfee Inc development.

Fix: Fix simply moves mfchashsize variable initialization before its usage in vnet_mroute_init.

Patch attached with submission follows:
How-To-Repeat: Hard to reproduce since system behavior after memory overwrite is unpredictable.  Multicast forwarding cashe hash overrun always happens after:
a) configuring xorp to use PIM/SM
b) starting xorp_rtrmgr
c) stopping xorp_rtrmgr.
Comment 1 Mark Linimon freebsd_committer freebsd_triage 2011-10-31 16:46:16 UTC 
Responsible Changed
From-To: freebsd-bugs->freebsd-net

Over to maintainer(s).
Comment 2 Gleb Smirnoff freebsd_committer freebsd_triage 2011-10-31 18:10:22 UTC 
  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> 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.
Comment 3 Stevan Markovic 2011-10-31 18:26:43 UTC 
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@FreeBSD.org]=20
Sent: Monday, October 31, 2011 2:10 PM
To: Markovic, Stevan
Cc: freebsd-gnats-submit@FreeBSD.org; zec@FreeBSD.org; bms@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> 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.
Comment 4 Marko Zec freebsd_committer freebsd_triage 2011-10-31 20:13:27 UTC 
On Monday 31 October 2011 19:26:43 Stevan_Markovic@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
Comment 5 Mark Linimon freebsd_committer freebsd_triage 2011-12-04 00:22:32 UTC 
Responsible Changed
From-To: freebsd-net->zec

Marko expressed an interest during the audit-trail.
Comment 6 dfilter service freebsd_committer freebsd_triage 2012-03-04 18:59:48 UTC 
Author: zec
Date: Sun Mar  4 18:59:38 2012
New Revision: 232517
URL: http://svn.freebsd.org/changeset/base/232517

Log:
  Change SYSINIT priorities so that ip_mroute_modevent() is executed
  before vnet_mroute_init(), since vnet_mroute_init() depends on mfchashsize
  tunable to be set, and that is done in in ip_mroute_modevent().
  Apparently I broke that ordering with r208744 almost 2 years ago...
  
  PR:		kern/162201
  Submitted by:	Stevan Markovic (mcafee.com)
  MFC after:	3 days

Modified:
  head/sys/netinet/ip_mroute.c

Modified: head/sys/netinet/ip_mroute.c
==============================================================================
--- head/sys/netinet/ip_mroute.c	Sun Mar  4 18:55:33 2012	(r232516)
+++ head/sys/netinet/ip_mroute.c	Sun Mar  4 18:59:38 2012	(r232517)
@@ -2822,7 +2822,7 @@ vnet_mroute_init(const void *unused __un
 	callout_init(&V_bw_meter_ch, CALLOUT_MPSAFE);
 }
 
-VNET_SYSINIT(vnet_mroute_init, SI_SUB_PSEUDO, SI_ORDER_MIDDLE, vnet_mroute_init,
+VNET_SYSINIT(vnet_mroute_init, SI_SUB_PSEUDO, SI_ORDER_ANY, vnet_mroute_init,
 	NULL);
 
 static void
@@ -2945,4 +2945,4 @@ static moduledata_t ip_mroutemod = {
     0
 };
 
-DECLARE_MODULE(ip_mroute, ip_mroutemod, SI_SUB_PSEUDO, SI_ORDER_ANY);
+DECLARE_MODULE(ip_mroute, ip_mroutemod, SI_SUB_PSEUDO, SI_ORDER_MIDDLE);
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 7 dfilter service freebsd_committer freebsd_triage 2012-03-28 13:45:54 UTC 
Author: zec
Date: Wed Mar 28 12:45:35 2012
New Revision: 233604
URL: http://svn.freebsd.org/changeset/base/233604

Log:
  MFC r232517:
    Change SYSINIT priorities so that ip_mroute_modevent() is executed
    before vnet_mroute_init(), since vnet_mroute_init() depends on mfchashsize
    tunable to be set, and that is done in in ip_mroute_modevent().
    Apparently I broke that ordering with r208744 almost 2 years ago...
  
    PR:           kern/162201
    Submitted by: Stevan Markovic (mcafee.com)
    MFC after:    3 days

Modified:
  stable/9/sys/netinet/ip_mroute.c
Directory Properties:
  stable/9/sys/   (props changed)
  stable/9/sys/amd64/include/xen/   (props changed)
  stable/9/sys/boot/   (props changed)
  stable/9/sys/boot/i386/efi/   (props changed)
  stable/9/sys/boot/ia64/efi/   (props changed)
  stable/9/sys/boot/ia64/ski/   (props changed)
  stable/9/sys/boot/powerpc/boot1.chrp/   (props changed)
  stable/9/sys/boot/powerpc/ofw/   (props changed)
  stable/9/sys/cddl/contrib/opensolaris/   (props changed)
  stable/9/sys/conf/   (props changed)
  stable/9/sys/contrib/dev/acpica/   (props changed)
  stable/9/sys/contrib/octeon-sdk/   (props changed)
  stable/9/sys/contrib/pf/   (props changed)
  stable/9/sys/contrib/x86emu/   (props changed)
  stable/9/sys/fs/   (props changed)
  stable/9/sys/fs/ntfs/   (props changed)
  stable/9/sys/i386/conf/XENHVM   (props changed)

Modified: stable/9/sys/netinet/ip_mroute.c
==============================================================================
--- stable/9/sys/netinet/ip_mroute.c	Wed Mar 28 12:41:17 2012	(r233603)
+++ stable/9/sys/netinet/ip_mroute.c	Wed Mar 28 12:45:35 2012	(r233604)
@@ -2822,7 +2822,7 @@ vnet_mroute_init(const void *unused __un
 	callout_init(&V_bw_meter_ch, CALLOUT_MPSAFE);
 }
 
-VNET_SYSINIT(vnet_mroute_init, SI_SUB_PSEUDO, SI_ORDER_MIDDLE, vnet_mroute_init,
+VNET_SYSINIT(vnet_mroute_init, SI_SUB_PSEUDO, SI_ORDER_ANY, vnet_mroute_init,
 	NULL);
 
 static void
@@ -2945,4 +2945,4 @@ static moduledata_t ip_mroutemod = {
     0
 };
 
-DECLARE_MODULE(ip_mroute, ip_mroutemod, SI_SUB_PSEUDO, SI_ORDER_ANY);
+DECLARE_MODULE(ip_mroute, ip_mroutemod, SI_SUB_PSEUDO, SI_ORDER_MIDDLE);
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 8 dfilter service freebsd_committer freebsd_triage 2012-03-28 13:46:20 UTC 
Author: zec
Date: Wed Mar 28 12:46:12 2012
New Revision: 233605
URL: http://svn.freebsd.org/changeset/base/233605

Log:
  MFC r232517:
    Change SYSINIT priorities so that ip_mroute_modevent() is executed
    before vnet_mroute_init(), since vnet_mroute_init() depends on mfchashsize
    tunable to be set, and that is done in in ip_mroute_modevent().
    Apparently I broke that ordering with r208744 almost 2 years ago...
  
    PR:           kern/162201
    Submitted by: Stevan Markovic (mcafee.com)
    MFC after:    3 days

Modified:
  stable/8/sys/netinet/ip_mroute.c
Directory Properties:
  stable/8/sys/   (props changed)
  stable/8/sys/amd64/include/xen/   (props changed)
  stable/8/sys/boot/   (props changed)
  stable/8/sys/cddl/contrib/opensolaris/   (props changed)
  stable/8/sys/contrib/dev/acpica/   (props changed)
  stable/8/sys/contrib/pf/   (props changed)
  stable/8/sys/dev/e1000/   (props changed)
  stable/8/sys/i386/conf/XENHVM   (props changed)

Modified: stable/8/sys/netinet/ip_mroute.c
==============================================================================
--- stable/8/sys/netinet/ip_mroute.c	Wed Mar 28 12:45:35 2012	(r233604)
+++ stable/8/sys/netinet/ip_mroute.c	Wed Mar 28 12:46:12 2012	(r233605)
@@ -2822,7 +2822,7 @@ vnet_mroute_init(const void *unused __un
 	callout_init(&V_bw_meter_ch, CALLOUT_MPSAFE);
 }
 
-VNET_SYSINIT(vnet_mroute_init, SI_SUB_PSEUDO, SI_ORDER_MIDDLE, vnet_mroute_init,
+VNET_SYSINIT(vnet_mroute_init, SI_SUB_PSEUDO, SI_ORDER_ANY, vnet_mroute_init,
 	NULL);
 
 static void
@@ -2945,4 +2945,4 @@ static moduledata_t ip_mroutemod = {
     0
 };
 
-DECLARE_MODULE(ip_mroute, ip_mroutemod, SI_SUB_PSEUDO, SI_ORDER_ANY);
+DECLARE_MODULE(ip_mroute, ip_mroutemod, SI_SUB_PSEUDO, SI_ORDER_MIDDLE);
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 9 Eitan Adler freebsd_committer freebsd_triage 2017-12-31 08:00:51 UTC 
For bugs matching the following criteria:

Status: In Progress Changed: (is less than) 2014-06-01

Reset to default assignee and clear in-progress tags.

Mail being skipped
Comment 10 Oleksandr Tymoshenko freebsd_committer freebsd_triage 2019-01-19 20:10:38 UTC 
There was a commit referencing this bug, but it's still not closed and has been inactive for some time. Closing as "fixed". Please re-open it if the issue hasn't been completely resolved.