netdev - Re: [PATCH net-next 1/3] net: sctp: let sctp_destroy

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:	Sun, 09 Jun 2013 11:14:09 +0200
From:	Daniel Borkmann <dborkman@...hat.com>
To:	Neil Horman <nhorman@...driver.com>
CC:	davem@...emloft.net, netdev@...r.kernel.org,
	linux-sctp@...r.kernel.org
Subject: Re: [PATCH net-next 1/3] net: sctp: let sctp_destroy_sock destroy
 related members

On 06/09/2013 02:20 AM, Neil Horman wrote:
> On Fri, Jun 07, 2013 at 01:36:04PM +0200, Daniel Borkmann wrote:
>> On 06/07/2013 12:54 PM, Neil Horman wrote:
>>> I'm not sure this is safe.  Comment in sk_common_release indicates that the
>>> network can still find the socket in the receive path.  What if we receive a
>>> cookie chunk while the socket is being torn down?  We would wind up using the
>>> hmac to unpack it potentially after you just freed it.  I think you need to wait
>>> until you drop the last reference to the endpoint, not whenever you destroy the
>>> local socket.  Note that sctp_endpoint_free doesn't actually free anything, it
>>> just removes it from the hash list so it can't be found again, and drops a
>>> refcount.  If a parallel recieve op has already found it, hmac may still be
>>> used.
>>
>> Agreed, you're right, thanks for pointing this out Neil! Is it *always* guaranteed
>> that at the time the endpoint is destroyed in a deferred way (e.g. exactly in such
>> a scenario you describe), the socket structure is still alive and not yet freed?
>> Either the ep->base.sk test in sctp_endpoint_destroy() would then be unnecessary
>> or, if necessary, we should move crypto_free_hash() and sctp_put_port() within this
>> body since they deref. socket members (but then that memory would be leaked in case
>> ep->base.sk is NULL). Probably, it might be best to add sth like this to explicitly
>> decouple it from the endpoint, which is then called when all refs are released from
>> the socket; then we could call this from __sk_free() via sk->sk_destruct():
>>
> Thats a good question, I'm on vacation right now, so I'm not looking to closely
> at much (I've spent all day in a pool).  I think what you're proposing below
> probably makes sense.  Since the hmac crypo instance is allocated when the

Cool, sounds relaxing. :-) Have nice holidays then!

> socket transitions to the listening state in sctp_listen, it makes sense to
> destroy it in sctp_sock_destroy.  If we need to we can protect it as an rcu
> variable to protect it against parallel reads from cookie processing.  If it
> fails in that case, its irrelevant, as the local socket is shutting down anyway.

I'll evaluate this further and then send a v2 of the set, but I think it makes
sense this way.

Thanks,

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