netdev - Re: [PATCH] Add IPv6 support to TCP SYN cookies

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 07 Feb 2008 10:40:19 +0100
From:	Eric Dumazet <dada1@...mosbay.com>
To:	Evgeniy Polyakov <johnpol@....mipt.ru>
Cc:	Glenn Griffin <ggriffin.kernel@...il.com>,
	Alan Cox <alan@...rguk.ukuu.org.uk>,
	Andi Kleen <andi@...stfloor.org>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Add IPv6 support to TCP SYN cookies

Evgeniy Polyakov a écrit :
> On Wed, Feb 06, 2008 at 10:30:24AM -0800, Glenn Griffin (ggriffin.kernel@...il.com) wrote:
>   
>>>> +static u32 cookie_hash(struct in6_addr *saddr, struct in6_addr *daddr,
>>>> +		       __be16 sport, __be16 dport, u32 count, int c)
>>>> +{
>>>> +	__u32 tmp[16 + 5 + SHA_WORKSPACE_WORDS];
>>>>         
>>> This huge buffer should not be allocated on stack.
>>>       
>> I can replace it will a kmalloc, but for my benefit what's the practical
>> size we try and limit the stack to?  It seemed at first glance to me
>> that 404 bytes plus the arguments, etc. was not such a large buffer for
>> a non-recursive function.  Plus the alternative with a kmalloc requires
>>     
>
> Well, maybe for connection establishment path it is not, but it is
> absolutely the case in the sending and sometimes receiving pathes for 4k
> stacks. The main problem is that bugs which happen because of stack
> overflow are so much obscure, that it is virtually impossible to detect
> where overflow happend. 'Debug stack overflow' somehow does not help to
> detect it.
>
> Usually there is about 1-1.5 kb of free stack for each process, so this
> change will cut one third of the free stack, getting into account that
> something can store ipv6 addresses on stack too, this can end up badly.
>
>   
>> propogating the possible error status back up to tcp_ipv6.c in the event
>> we are unable to allocate enough memory, so it can simply drop the
>> connection.  Not an impossible task by any means but it does
>> significantly complicate things and I would like to know it's worth the
>> effort.  Also would it be worth it to provide a supplemental patch for
>> the ipv4 implementation as it allocates the same buffer?
>>     
>
> One can reorganize syncookie support to work with request hash tables
> too, so that we could allocate per hash-bucket space and use it as a
> scratchpad for cookies.
>
>   
Or maybe use percpu storage for that...

I am not sure if cookie_hash() is always called with preemption disabled.
(If not, we have to use get_cpu_var()/put_cpu_var())

[NET] IPV4: lower stack usage in cookie_hash() function

400 bytes allocated on stack might be a litle bit too much. Using a 
per_cpu var is more friendly.

Signed-off-by: Eric Dumazet <dada1@...mosbay.com>



View attachment "cookie_scratch.patch" of type "text/plain" (682 bytes)