Date: Mon, 31 Oct 2011 20:30:15 GMT From: Marko Zec <zec@freebsd.org> To: freebsd-net@FreeBSD.org Subject: Re: misc/162201: [patch] multicast forwarding cache hash always allocated with size 0, resulting in buffer overrun Message-ID: <201110312030.p9VKUFoR084208@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/162201; it has been noted by GNATS.
From: Marko Zec <zec@freebsd.org>
To: Stevan_Markovic@mcafee.com
Cc: glebius@freebsd.org,
freebsd-gnats-submit@freebsd.org,
bms@freebsd.org,
bz@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@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@FreeBSD.org]
> 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 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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201110312030.p9VKUFoR084208>