Message-ID: <48E141F3.9000903@cosmosbay.com>
Date:	Mon, 29 Sep 2008 23:00:35 +0200
From:	Eric Dumazet <dada1@...mosbay.com>
To:	Neil Horman <nhorman@...driver.com>
Cc:	netdev@...r.kernel.org, kuznet@....inr.ac.ru, davem@...emloft.net,
	pekkas@...core.fi, jmorris@...ei.org, yoshfuji@...ux-ipv6.org,
	kaber@...sh.net
Subject: Re: [PATCH] net: implement emergency route cache rebulds when	gc_elasticity
 is exceeded

Neil Horman a écrit :
> On Mon, Sep 29, 2008 at 10:22:03PM +0200, Eric Dumazet wrote:
>> Neil Horman a écrit :
>>> Hey all-
>>> 	We currently have the ability to disable our route cache secret interval
>>> rebuild timer (by setting it to zero), but if we do that its possible for an
>>> attacker (if they guess our route cache hash secret, to fill our system with
>>> routes that all hash to the same bucket, destroying our performance.  This patch
>>> provides a backstop for that issues.  In the event that our rebuild interval is
>>> disabled (or very large), if any hash chain exceeds ip_rt_gc_elasticity, we do
>>> an emergency hash rebuild.  During the hash rebuild we:
>>> 1) warn the user of the emergency
>>> 2) disable the rebuild timer
>>> 3) invalidate the route caches
>>> 4) re-enable the rebuild timer with its old value
>>>
>>> Regards
>>> Neil
>> This sounds not good at all to me.
>>
>> 1) Dont set ip_rt_secret_interval to zero, this is plain silly, since
>>   you give attackers infinite time to break your machine.
>>
>> To quote Herbert (who allowed to set this interval to 0)
>>
>>    "Let me first state that disabling the route cache hash rebuild
>>     should not be done without extensive analysis on the risk profile
>>     and careful deliberation.
>>
>>     However, there are times when this can be done safely or for
>>     testing.  For example, when you have mechanisms for ensuring
>>     that offending parties do not exist in your network."
>>
> Thats really rather the motivation behind this.  The patch that Herbert
> submitted with that commit explicitly lets one disable their rebuild timer.  I
> agree its stupid to do that, but we added code to allow it.  This provides a
> patch to help people who are victimized because they've done exactly this
> (additionaly providing them a warning to stop doing it).

Strange... many kernel parameters can be set to hazardous values that make machine unusable...

ip_rt_gc_interval can also be set to a very large value : No more route cache gc

> 
> 
>> 2) Many machines have ip_rt_gc_elasticity set to 2,
>>   because they have a huge hash table, but low chain depths.
> Ok, that seem reasonable, and this isn't going to disallow that.  By the same
> resoning, people who have huge hash tables, and low chain depths won't
> want their low chain length being violated, would they?  This patch will warn
> them if their assumptions are being violated.

Warn only ? If I read your patch, you not only warn in this case.
(you invalidate cache for each struct net, potentially wraping rt_genid)

When you have 2^20 slots in route cache hash table, you dont care if few slots have 3 or 4 elements.
And chance is very high that more than one slot has 3 or even 4 elements, no need for an attacker.

Now if you change your code to something like

 if (unlikely(chain_length > some_quite_big_number &&
     ip_rt_secret_interval == 0)) {
 	do_something();
 }

some_quite_big_number could be >= 30 or something...

then it might be OK (at least it wont break common setups)



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists