netdev - Re: [PATCH net] sctp: sctp should release assoc when sctp_make_abort_user return NULL in sctp

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 for Android: free password hash cracker in your pocket

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

Message-ID: <5673067B.6080001@gmail.com>
Date:	Thu, 17 Dec 2015 17:01:15 -0200
From:	Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:	Vlad Yasevich <vyasevich@...il.com>,
	Xin Long <lucien.xin@...il.com>,
	network dev <netdev@...r.kernel.org>,
	linux-sctp@...r.kernel.org
Cc:	vyasevic@...hat.com, davem@...emloft.net
Subject: Re: [PATCH net] sctp: sctp should release assoc when
 sctp_make_abort_user return NULL in sctp_close

Em 17-12-2015 16:29, Vlad Yasevich escreveu:
> On 12/17/2015 09:30 AM, Xin Long wrote:
>> In sctp_close, sctp_make_abort_user may return NULL because of memory
>> allocation failure. If this happens, it will bypass any state change
>> and never free the assoc. The assoc has no chance to be freed and it
>> will be kept in memory with the state it had even after the socket is
>> closed by sctp_close().
>>
>> So if sctp_make_abort_user fails to allocate memory, we should just
>> free the asoc, as there isn't much else that we can do.
>>
>> Signed-off-by: Xin Long <lucien.xin@...il.com>
>> Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
>> ---
>>   net/sctp/socket.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>> index 9b6cc6d..267b8f8 100644
>> --- a/net/sctp/socket.c
>> +++ b/net/sctp/socket.c
>> @@ -1513,8 +1513,12 @@ static void sctp_close(struct sock *sk, long timeout)
>>   			struct sctp_chunk *chunk;
>>
>>   			chunk = sctp_make_abort_user(asoc, NULL, 0);
>> -			if (chunk)
>> +			if (chunk) {
>>   				sctp_primitive_ABORT(net, asoc, chunk);
>> +			} else {
>> +				sctp_unhash_established(asoc);
>> +				sctp_association_free(asoc);
>> +			}
>
> I don't think you can do that for an association that has not been closed.
>
> I think a cleaner approach might be to update abort primitive handlers
> to handle a NULL chunk value and unconditionally call the primitive.
>
> This guarantees that any timers or waitqueues that might be active are
> stopped correctly.

sctp_association_free() is the one who does that job, even that way. All 
in between the primitive call and then the call to 
sctp_association_free() is just status changes and packet xmit, which 
doing this way we cut out when we are in memory pressure. pkt xmit or 
ULP events are likely going to fail too anyway.

sctp_sf_do_9_1_prm_abort() -> SCTP_CMD_ASSOC_FAILED ->
   sctp_cmd_assoc_failed -> ULP events, send abort, and 
SCTP_CMD_DELETE_TCB ->
     sctp_cmd_delete_tcb ->
       sctp_unhash_established(asoc);
       sctp_association_free(asoc);
and returns.

There is a check on sctp_cmd_delete_tcb() that avoids calling that on 
temp assocs on listening sockets, but that condition is false due to the 
check on sk_shutdown so it will call those two functions anyway.

   Marcelo

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