netdev - Re: [PATCH] sctp: Do not trigger BUG_ON when deleting assoc without primary path

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, 17 Oct 2013 14:35:56 -0400
From:	Vlad Yasevich <vyasevich@...il.com>
To:	Daniel Borkmann <dborkman@...hat.com>
CC:	netdev@...r.kernel.org, linux-sctp@...r.kernel.org,
	Mark Thomas <Mark.Thomas@...aswitch.com>,
	Neil Horman <nhorman@...driver.com>
Subject: Re: [PATCH] sctp: Do not trigger BUG_ON when deleting assoc without
 primary path

On 10/17/2013 02:25 PM, Daniel Borkmann wrote:
> On 10/17/2013 08:01 PM, Daniel Borkmann wrote:
>> On 10/17/2013 07:30 PM, Vlad Yasevich wrote:
>>> It is possible to enter sctp_cmd_delete_tcb() without having a
>>> primary path.  The situations this most often happens in is
>>> when duplication cookie processing is triggered.  In this
>>> case, we are deleting a temporarily created association that
>>> is not fully populated.   Additially, at the time we
>>> are deleting the offending association, it is really too
>>> late to issue a BUG!
>>>
>>> This was introduced by:
>>> commit f9e42b853523cda0732022c2e0473c183f7aec65
>>>     net: sctp: sideeffect: throw BUG if primary_path is NULL
>>
>> Sure, lets remove it, but then we could still get a WARN() [sure,
>> better than BUG], if the user at the very same time checks procfs
>> through sctp_seq_dump_local_addrs(), see discussion we had here [1]:
>>
>>   It may trigger the crash later if the user performs some action on the
>>   association that touches the primary. That's the reason why I was
>>   proposing the checks below.
>>
>>   With the checks in command interpreter, we are only left with the
>>   possibility that primary_path changes to NULL during the association
>>   lifetime, which code audit doesn't support right now.  If that ever
>>   changes we would at least have a bit more information to go on.
>>
>>   [1] http://patchwork.ozlabs.org/patch/251099/
>
> Meaning, all I'm saying is that with f9e42b853 we wanted to find exactly
> such a case we have right now, that is, that an assoc could enter the
> hashtable w/o primary path, no?

But it didn't enter a hash table in this case.  SCTP_CMD_NEW_ASOC
was never issued.  The sequence was:
	SCTP_CMD_SET_ASOC
	SCTP_CMD_DELETE_TCB

Such association would never be found through /proc since it was never
hashed.  Such association would never be found the user since it
is only really alive while the packet is processed.  By all rights
it should be marked as 'temp', but it isn't due to cookie processing.

May be we should update cookie processing function to allow it
to create temp associations if so desired.

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