netdev - Re: [PATCH] sctp: Fix sk_ack_backlog wrap-around problem

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] [day] [month] [year] [list]

Date:	Thu, 12 Jun 2014 09:38:58 +0800
From:	Xufeng Zhang <xufeng.zhang@...driver.com>
To:	Vlad Yasevich <vyasevich@...il.com>
CC:	<nhorman@...driver.com>, <davem@...emloft.net>,
	<linux-sctp@...r.kernel.org>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] sctp: Fix sk_ack_backlog wrap-around problem

On 06/12/2014 12:55 AM, Vlad Yasevich wrote:
> On 06/11/2014 08:55 AM, Vlad Yasevich wrote:
>    
>> On 06/10/2014 10:37 PM, Xufeng Zhang wrote:
>>      
>>> Consider the scenario:
>>> For a TCP-style socket, while processing the COOKIE_ECHO chunk in
>>> sctp_sf_do_5_1D_ce(), after it has passed a series of sanity check,
>>> a new association would be created in sctp_unpack_cookie(), but afterwards,
>>> some processing maybe failed, and sctp_association_free() will be called to
>>> free the previously allocated association, in sctp_association_free(),
>>> sk_ack_backlog value is decremented for this socket, since the initial
>>> value for sk_ack_backlog is 0, after the decrement, it will be 65535,
>>> a wrap-around problem happens, and if we want to establish new associations
>>> afterward in the same socket, ABORT would be triggered since sctp deem the
>>> accept queue as full.
>>> Fix this issue by only decrementing sk_ack_backlog for associations in
>>> the endpoint's list.
>>>
>>> Fix-suggested-by: Neil Horman<nhorman@...driver.com>
>>> Signed-off-by: Xufeng Zhang<xufeng.zhang@...driver.com>
>>> ---
>>>   net/sctp/associola.c |    2 +-
>>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
>>> index 39579c3..60564f2 100644
>>> --- a/net/sctp/associola.c
>>> +++ b/net/sctp/associola.c
>>> @@ -330,7 +330,7 @@ void sctp_association_free(struct sctp_association *asoc)
>>>   	/* Only real associations count against the endpoint, so
>>>   	 * don't bother for if this is a temporary association.
>>>   	 */
>>> -	if (!asoc->temp) {
>>> +	if (!asoc->temp&&  !list_empty(&asoc->asocs)) {
>>>   		list_del(&asoc->asocs);
>>>
>>>   		/* Decrement the backlog value for a TCP-style listening
>>>
>>>        
>> I am not crazy about this patch.   It's been suggested before that may
>> be duplicate cookie processing should really be creating a temporary
>> association since that's is how that association is being used.
>>      
> I had another look at the description for triggering this issue and
> realized that I was thinking about something else when looking at
> this solution.
>
> There is however no need to test both the list and temp value.
> We can simply always test the that list is not empty before doing
> list_del().
>    

Thanks a lot for the comment!
I'll send V2 later.


Thanks,
Xufeng


> -vlad
>
>    
>>   It
>> might be nice at that approach.  It actually benefits us as the
>> association destruction would happen immediately instead of being delayed.
>>      
>
>    

--
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