diff mbox

sctp: deny peeloff operation on asocs with threads sleeping on it

Message ID 20170420123257.GA11798@lorien.valinor.li
State Not Applicable, archived
Delegated to: David Miller
Headers show

Commit Message

Salvatore Bonaccorso April 20, 2017, 12:32 p.m. UTC 
Hi 

According to the documentation I should have sent this to
netdev@vger.kernel.org rather than stable@vger.kernel.org.

Rationale: Whilst 00eff2ebbd229758e90659907724c14dd5a18339 went to
stable, the dfcb9f4f99f1e9a49e43398a7bfbf56927544af1 commit is
missing.

Full quoting, my original misleaded mail:

On Thu, Apr 20, 2017 at 01:44:05PM +0200, Salvatore Bonaccorso wrote:
> Hi
> 
> Apparently the following commit
> 
> dfcb9f4f99f1e9a49e43398a7bfbf56927544af1 (sctp: deny peeloff operation
> on asocs with threads sleeping on it)
> 
> (was not CC'ed stable@vger.kernel.org, but was already applied in
> 3.2.87 and 3.16.42) is missing from 4.9:
> 
> 2dcab598484185dea7ec22219c76dcdd59e3cb90 was included in 4.9.11 via
> 00eff2ebbd229758e90659907724c14dd5a18339 . But
> dfcb9f4f99f1e9a49e43398a7bfbf56927544af1 is missing from 4.9.
> 
> This was assigned CVE-2017-6353.
> 
> Reference:
> https://marc.info/?l=linux-netdev&m=148785309416337&w=2
> http://www.openwall.com/lists/oss-security/2017/02/27/2
> 
> Regards,
> Salvatore

Regards,
Salvatore

Comments

Marcelo Ricardo Leitner April 20, 2017, 1:54 p.m. UTC | #1 
Em 20-04-2017 09:32, Salvatore Bonaccorso escreveu:
> Hi
> 
> According to the documentation I should have sent this to
> netdev@vger.kernel.org rather than stable@vger.kernel.org.
> 
> Rationale: Whilst 00eff2ebbd229758e90659907724c14dd5a18339 went to
> stable, the dfcb9f4f99f1e9a49e43398a7bfbf56927544af1 commit is
> missing.

Cc'ing Greg too.

This is probably because I noticed this commit was missing when Greg 
posted the commits being considered for backports, then I raised the 
concern and Greg decided to pull it too before David replied.

Not sure if this caused some out-of-the-process stuff.

> 
> Full quoting, my original misleaded mail:
> 
> On Thu, Apr 20, 2017 at 01:44:05PM +0200, Salvatore Bonaccorso wrote:
>> Hi
>>
>> Apparently the following commit
>>
>> dfcb9f4f99f1e9a49e43398a7bfbf56927544af1 (sctp: deny peeloff operation
>> on asocs with threads sleeping on it)
>>
>> (was not CC'ed stable@vger.kernel.org, but was already applied in
>> 3.2.87 and 3.16.42) is missing from 4.9:
>>
>> 2dcab598484185dea7ec22219c76dcdd59e3cb90 was included in 4.9.11 via
>> 00eff2ebbd229758e90659907724c14dd5a18339 . But
>> dfcb9f4f99f1e9a49e43398a7bfbf56927544af1 is missing from 4.9.
>>
>> This was assigned CVE-2017-6353.
>>
>> Reference:
>> https://marc.info/?l=linux-netdev&m=148785309416337&w=2
>> http://www.openwall.com/lists/oss-security/2017/02/27/2
>>
>> Regards,
>> Salvatore
> 
> Regards,
> Salvatore
>
diff mbox

Patch

From dfcb9f4f99f1e9a49e43398a7bfbf56927544af1 Mon Sep 17 00:00:00 2001
From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: Thu, 23 Feb 2017 09:31:18 -0300
Subject: [PATCH] sctp: deny peeloff operation on asocs with threads sleeping
 on it

commit 2dcab5984841 ("sctp: avoid BUG_ON on sctp_wait_for_sndbuf")
attempted to avoid a BUG_ON call when the association being used for a
sendmsg() is blocked waiting for more sndbuf and another thread did a
peeloff operation on such asoc, moving it to another socket.

As Ben Hutchings noticed, then in such case it would return without
locking back the socket and would cause two unlocks in a row.

Further analysis also revealed that it could allow a double free if the
application managed to peeloff the asoc that is created during the
sendmsg call, because then sctp_sendmsg() would try to free the asoc
that was created only for that call.

This patch takes another approach. It will deny the peeloff operation
if there is a thread sleeping on the asoc, so this situation doesn't
exist anymore. This avoids the issues described above and also honors
the syscalls that are already being handled (it can be multiple sendmsg
calls).

Joint work with Xin Long.

Fixes: 2dcab5984841 ("sctp: avoid BUG_ON on sctp_wait_for_sndbuf")
Cc: Alexander Popov <alex.popov@linux.com>
Cc: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/sctp/socket.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index b532148..465a9c8 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -4862,6 +4862,12 @@  int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
 	if (!asoc)
 		return -EINVAL;
 
+	/* If there is a thread waiting on more sndbuf space for
+	 * sending on this asoc, it cannot be peeled.
+	 */
+	if (waitqueue_active(&asoc->wait))
+		return -EBUSY;
+
 	/* An association cannot be branched off from an already peeled-off
 	 * socket, nor is this supported for tcp style sockets.
 	 */
@@ -7599,8 +7605,6 @@  static int sctp_wait_for_sndbuf(struct sctp_association *asoc, long *timeo_p,
 		 */
 		release_sock(sk);
 		current_timeo = schedule_timeout(current_timeo);
-		if (sk != asoc->base.sk)
-			goto do_error;
 		lock_sock(sk);
 
 		*timeo_p = current_timeo;
-- 
2.1.4