Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/2] net: dccp: fix structure use-after-free
@ 2020-10-13 17:18 Kleber Sacilotto de Souza
  2020-10-13 17:18 ` [PATCH 1/2] dccp: ccid: move timers to struct dccp_sock Kleber Sacilotto de Souza
  2020-10-13 17:18 ` [PATCH 2/2] Revert "dccp: don't free ccid2_hc_tx_sock struct in dccp_disconnect()" Kleber Sacilotto de Souza
  0 siblings, 2 replies; 10+ messages in thread
From: Kleber Sacilotto de Souza @ 2020-10-13 17:18 UTC (permalink / raw)
  To: netdev
  Cc: Gerrit Renker, David S. Miller, Jakub Kicinski,
	Thadeu Lima de Souza Cascardo, Gustavo A. R. Silva,
	Alexander A. Klimov, Kees Cook, Eric Dumazet, Alexey Kodanev,
	dccp, linux-kernel

This patchset addresses the following CVE:

CVE-2020-16119 - DCCP CCID structure use-after-free

Hadar Manor reported that by reusing a socket with an attached
dccps_hc_tx_ccid as a listener, it will be used after being released,
leading to DoS and potentially code execution.

The first patch moves the ccid timers to struct dccp_sock to avoid its
use-after-free, the second patch reverts 2677d2067731 "dccp: don't free
ccid2_hc_tx_sock struct in dccp_disconnect()" that's not needed anymore
and would cause another use-after-free.

Thadeu Lima de Souza Cascardo (2):
  dccp: ccid: move timers to struct dccp_sock
  Revert "dccp: don't free ccid2_hc_tx_sock struct in dccp_disconnect()"

 include/linux/dccp.h   |  2 ++
 net/dccp/ccids/ccid2.c | 32 +++++++++++++++++++-------------
 net/dccp/ccids/ccid3.c | 30 ++++++++++++++++++++----------
 net/dccp/proto.c       |  2 ++
 4 files changed, 43 insertions(+), 23 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/2] dccp: ccid: move timers to struct dccp_sock
  2020-10-13 17:18 [PATCH 0/2] net: dccp: fix structure use-after-free Kleber Sacilotto de Souza
@ 2020-10-13 17:18 ` Kleber Sacilotto de Souza
  2020-10-13 18:58   ` Richard Sailer
                     ` (2 more replies)
  2020-10-13 17:18 ` [PATCH 2/2] Revert "dccp: don't free ccid2_hc_tx_sock struct in dccp_disconnect()" Kleber Sacilotto de Souza
  1 sibling, 3 replies; 10+ messages in thread
From: Kleber Sacilotto de Souza @ 2020-10-13 17:18 UTC (permalink / raw)
  To: netdev
  Cc: Gerrit Renker, David S. Miller, Jakub Kicinski,
	Thadeu Lima de Souza Cascardo, Gustavo A. R. Silva,
	Alexander A. Klimov, Kees Cook, Eric Dumazet, Alexey Kodanev,
	dccp, linux-kernel

From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>

When dccps_hc_tx_ccid is freed, ccid timers may still trigger. The reason
del_timer_sync can't be used is because this relies on keeping a reference
to struct sock. But as we keep a pointer to dccps_hc_tx_ccid and free that
during disconnect, the timer should really belong to struct dccp_sock.

This addresses CVE-2020-16119.

Fixes: 839a6094140a (net: dccp: Convert timers to use timer_setup())
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>
---
 include/linux/dccp.h   |  2 ++
 net/dccp/ccids/ccid2.c | 32 +++++++++++++++++++-------------
 net/dccp/ccids/ccid3.c | 30 ++++++++++++++++++++----------
 3 files changed, 41 insertions(+), 23 deletions(-)

diff --git a/include/linux/dccp.h b/include/linux/dccp.h
index 07e547c02fd8..504afa1a4be6 100644
--- a/include/linux/dccp.h
+++ b/include/linux/dccp.h
@@ -259,6 +259,7 @@ struct dccp_ackvec;
  * @dccps_sync_scheduled - flag which signals "send out-of-band message soon"
  * @dccps_xmitlet - tasklet scheduled by the TX CCID to dequeue data packets
  * @dccps_xmit_timer - used by the TX CCID to delay sending (rate-based pacing)
+ * @dccps_ccid_timer - used by the CCIDs
  * @dccps_syn_rtt - RTT sample from Request/Response exchange (in usecs)
  */
 struct dccp_sock {
@@ -303,6 +304,7 @@ struct dccp_sock {
 	__u8				dccps_sync_scheduled:1;
 	struct tasklet_struct		dccps_xmitlet;
 	struct timer_list		dccps_xmit_timer;
+	struct timer_list		dccps_ccid_timer;
 };
 
 static inline struct dccp_sock *dccp_sk(const struct sock *sk)
diff --git a/net/dccp/ccids/ccid2.c b/net/dccp/ccids/ccid2.c
index 3da1f77bd039..dbca1f1e2449 100644
--- a/net/dccp/ccids/ccid2.c
+++ b/net/dccp/ccids/ccid2.c
@@ -126,21 +126,26 @@ static void dccp_tasklet_schedule(struct sock *sk)
 
 static void ccid2_hc_tx_rto_expire(struct timer_list *t)
 {
-	struct ccid2_hc_tx_sock *hc = from_timer(hc, t, tx_rtotimer);
-	struct sock *sk = hc->sk;
-	const bool sender_was_blocked = ccid2_cwnd_network_limited(hc);
+	struct dccp_sock *dp = from_timer(dp, t, dccps_ccid_timer);
+	struct sock *sk = (struct sock *)dp;
+	struct ccid2_hc_tx_sock *hc;
+	bool sender_was_blocked;
 
 	bh_lock_sock(sk);
+
+	if (inet_sk_state_load(sk) == DCCP_CLOSED)
+		goto out;
+
+	hc = ccid_priv(dp->dccps_hc_tx_ccid);
+	sender_was_blocked = ccid2_cwnd_network_limited(hc);
+
 	if (sock_owned_by_user(sk)) {
-		sk_reset_timer(sk, &hc->tx_rtotimer, jiffies + HZ / 5);
+		sk_reset_timer(sk, &dp->dccps_ccid_timer, jiffies + HZ / 5);
 		goto out;
 	}
 
 	ccid2_pr_debug("RTO_EXPIRE\n");
 
-	if (sk->sk_state == DCCP_CLOSED)
-		goto out;
-
 	/* back-off timer */
 	hc->tx_rto <<= 1;
 	if (hc->tx_rto > DCCP_RTO_MAX)
@@ -166,7 +171,7 @@ static void ccid2_hc_tx_rto_expire(struct timer_list *t)
 	if (sender_was_blocked)
 		dccp_tasklet_schedule(sk);
 	/* restart backed-off timer */
-	sk_reset_timer(sk, &hc->tx_rtotimer, jiffies + hc->tx_rto);
+	sk_reset_timer(sk, &dp->dccps_ccid_timer, jiffies + hc->tx_rto);
 out:
 	bh_unlock_sock(sk);
 	sock_put(sk);
@@ -330,7 +335,7 @@ static void ccid2_hc_tx_packet_sent(struct sock *sk, unsigned int len)
 	}
 #endif
 
-	sk_reset_timer(sk, &hc->tx_rtotimer, jiffies + hc->tx_rto);
+	sk_reset_timer(sk, &dp->dccps_ccid_timer, jiffies + hc->tx_rto);
 
 #ifdef CONFIG_IP_DCCP_CCID2_DEBUG
 	do {
@@ -700,9 +705,9 @@ static void ccid2_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb)
 
 	/* restart RTO timer if not all outstanding data has been acked */
 	if (hc->tx_pipe == 0)
-		sk_stop_timer(sk, &hc->tx_rtotimer);
+		sk_stop_timer(sk, &dp->dccps_ccid_timer);
 	else
-		sk_reset_timer(sk, &hc->tx_rtotimer, jiffies + hc->tx_rto);
+		sk_reset_timer(sk, &dp->dccps_ccid_timer, jiffies + hc->tx_rto);
 done:
 	/* check if incoming Acks allow pending packets to be sent */
 	if (sender_was_blocked && !ccid2_cwnd_network_limited(hc))
@@ -737,17 +742,18 @@ static int ccid2_hc_tx_init(struct ccid *ccid, struct sock *sk)
 	hc->tx_last_cong = hc->tx_lsndtime = hc->tx_cwnd_stamp = ccid2_jiffies32;
 	hc->tx_cwnd_used = 0;
 	hc->sk		 = sk;
-	timer_setup(&hc->tx_rtotimer, ccid2_hc_tx_rto_expire, 0);
+	timer_setup(&dp->dccps_ccid_timer, ccid2_hc_tx_rto_expire, 0);
 	INIT_LIST_HEAD(&hc->tx_av_chunks);
 	return 0;
 }
 
 static void ccid2_hc_tx_exit(struct sock *sk)
 {
+	struct dccp_sock *dp = dccp_sk(sk);
 	struct ccid2_hc_tx_sock *hc = ccid2_hc_tx_sk(sk);
 	int i;
 
-	sk_stop_timer(sk, &hc->tx_rtotimer);
+	sk_stop_timer(sk, &dp->dccps_ccid_timer);
 
 	for (i = 0; i < hc->tx_seqbufc; i++)
 		kfree(hc->tx_seqbuf[i]);
diff --git a/net/dccp/ccids/ccid3.c b/net/dccp/ccids/ccid3.c
index b9ee1a4a8955..685f4d046c0d 100644
--- a/net/dccp/ccids/ccid3.c
+++ b/net/dccp/ccids/ccid3.c
@@ -184,17 +184,24 @@ static inline void ccid3_hc_tx_update_win_count(struct ccid3_hc_tx_sock *hc,
 
 static void ccid3_hc_tx_no_feedback_timer(struct timer_list *t)
 {
-	struct ccid3_hc_tx_sock *hc = from_timer(hc, t, tx_no_feedback_timer);
-	struct sock *sk = hc->sk;
+	struct dccp_sock *dp = from_timer(dp, t, dccps_ccid_timer);
+	struct ccid3_hc_tx_sock *hc;
+	struct sock *sk = (struct sock *)dp;
 	unsigned long t_nfb = USEC_PER_SEC / 5;
 
 	bh_lock_sock(sk);
+
+	if (inet_sk_state_load(sk) == DCCP_CLOSED)
+		goto out;
+
 	if (sock_owned_by_user(sk)) {
 		/* Try again later. */
 		/* XXX: set some sensible MIB */
 		goto restart_timer;
 	}
 
+	hc = ccid_priv(dp->dccps_hc_tx_ccid);
+
 	ccid3_pr_debug("%s(%p, state=%s) - entry\n", dccp_role(sk), sk,
 		       ccid3_tx_state_name(hc->tx_state));
 
@@ -250,8 +257,8 @@ static void ccid3_hc_tx_no_feedback_timer(struct timer_list *t)
 		t_nfb = max(hc->tx_t_rto, 2 * hc->tx_t_ipi);
 
 restart_timer:
-	sk_reset_timer(sk, &hc->tx_no_feedback_timer,
-			   jiffies + usecs_to_jiffies(t_nfb));
+	sk_reset_timer(sk, &dp->dccps_ccid_timer,
+		       jiffies + usecs_to_jiffies(t_nfb));
 out:
 	bh_unlock_sock(sk);
 	sock_put(sk);
@@ -280,7 +287,7 @@ static int ccid3_hc_tx_send_packet(struct sock *sk, struct sk_buff *skb)
 		return -EBADMSG;
 
 	if (hc->tx_state == TFRC_SSTATE_NO_SENT) {
-		sk_reset_timer(sk, &hc->tx_no_feedback_timer, (jiffies +
+		sk_reset_timer(sk, &dp->dccps_ccid_timer, (jiffies +
 			       usecs_to_jiffies(TFRC_INITIAL_TIMEOUT)));
 		hc->tx_last_win_count	= 0;
 		hc->tx_t_last_win_count = now;
@@ -354,6 +361,7 @@ static void ccid3_hc_tx_packet_sent(struct sock *sk, unsigned int len)
 static void ccid3_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb)
 {
 	struct ccid3_hc_tx_sock *hc = ccid3_hc_tx_sk(sk);
+	struct dccp_sock *dp = dccp_sk(sk);
 	struct tfrc_tx_hist_entry *acked;
 	ktime_t now;
 	unsigned long t_nfb;
@@ -420,7 +428,7 @@ static void ccid3_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb)
 			       (unsigned int)(hc->tx_x >> 6));
 
 	/* unschedule no feedback timer */
-	sk_stop_timer(sk, &hc->tx_no_feedback_timer);
+	sk_stop_timer(sk, &dp->dccps_ccid_timer);
 
 	/*
 	 * As we have calculated new ipi, delta, t_nom it is possible
@@ -445,8 +453,8 @@ static void ccid3_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb)
 		       "expire in %lu jiffies (%luus)\n",
 		       dccp_role(sk), sk, usecs_to_jiffies(t_nfb), t_nfb);
 
-	sk_reset_timer(sk, &hc->tx_no_feedback_timer,
-			   jiffies + usecs_to_jiffies(t_nfb));
+	sk_reset_timer(sk, &dp->dccps_ccid_timer,
+		       jiffies + usecs_to_jiffies(t_nfb));
 }
 
 static int ccid3_hc_tx_parse_options(struct sock *sk, u8 packet_type,
@@ -488,21 +496,23 @@ static int ccid3_hc_tx_parse_options(struct sock *sk, u8 packet_type,
 
 static int ccid3_hc_tx_init(struct ccid *ccid, struct sock *sk)
 {
+	struct dccp_sock *dp = dccp_sk(sk);
 	struct ccid3_hc_tx_sock *hc = ccid_priv(ccid);
 
 	hc->tx_state = TFRC_SSTATE_NO_SENT;
 	hc->tx_hist  = NULL;
 	hc->sk	     = sk;
-	timer_setup(&hc->tx_no_feedback_timer,
+	timer_setup(&dp->dccps_ccid_timer,
 		    ccid3_hc_tx_no_feedback_timer, 0);
 	return 0;
 }
 
 static void ccid3_hc_tx_exit(struct sock *sk)
 {
+	struct dccp_sock *dp = dccp_sk(sk);
 	struct ccid3_hc_tx_sock *hc = ccid3_hc_tx_sk(sk);
 
-	sk_stop_timer(sk, &hc->tx_no_feedback_timer);
+	sk_stop_timer(sk, &dp->dccps_ccid_timer);
 	tfrc_tx_hist_purge(&hc->tx_hist);
 }
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/2] Revert "dccp: don't free ccid2_hc_tx_sock struct in dccp_disconnect()"
  2020-10-13 17:18 [PATCH 0/2] net: dccp: fix structure use-after-free Kleber Sacilotto de Souza
  2020-10-13 17:18 ` [PATCH 1/2] dccp: ccid: move timers to struct dccp_sock Kleber Sacilotto de Souza
@ 2020-10-13 17:18 ` Kleber Sacilotto de Souza
  2020-10-13 18:59   ` Richard Sailer
  2020-10-15  3:42   ` Jakub Kicinski
  1 sibling, 2 replies; 10+ messages in thread
From: Kleber Sacilotto de Souza @ 2020-10-13 17:18 UTC (permalink / raw)
  To: netdev
  Cc: Gerrit Renker, David S. Miller, Jakub Kicinski,
	Thadeu Lima de Souza Cascardo, Gustavo A. R. Silva,
	Alexander A. Klimov, Kees Cook, Eric Dumazet, Alexey Kodanev,
	dccp, linux-kernel

From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>

This reverts commit 2677d20677314101293e6da0094ede7b5526d2b1.

This fixes an issue that after disconnect, dccps_hc_tx_ccid will still be
kept, allowing the socket to be reused as a listener socket, and the cloned
socket will free its dccps_hc_tx_ccid, leading to a later use after free,
when the listener socket is closed.

This addresses CVE-2020-16119.

Fixes: 2677d2067731 (dccp: don't free ccid2_hc_tx_sock struct in dccp_disconnect())
Reported-by: Hadar Manor
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>
---
 net/dccp/proto.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/dccp/proto.c b/net/dccp/proto.c
index 6d705d90c614..359e848dba6c 100644
--- a/net/dccp/proto.c
+++ b/net/dccp/proto.c
@@ -279,7 +279,9 @@ int dccp_disconnect(struct sock *sk, int flags)
 
 	dccp_clear_xmit_timers(sk);
 	ccid_hc_rx_delete(dp->dccps_hc_rx_ccid, sk);
+	ccid_hc_tx_delete(dp->dccps_hc_tx_ccid, sk);
 	dp->dccps_hc_rx_ccid = NULL;
+	dp->dccps_hc_tx_ccid = NULL;
 
 	__skb_queue_purge(&sk->sk_receive_queue);
 	__skb_queue_purge(&sk->sk_write_queue);
-- 
2.25.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] dccp: ccid: move timers to struct dccp_sock
  2020-10-13 17:18 ` [PATCH 1/2] dccp: ccid: move timers to struct dccp_sock Kleber Sacilotto de Souza
@ 2020-10-13 18:58   ` Richard Sailer
  2020-10-15  3:43   ` Jakub Kicinski
  2020-10-16 22:30   ` Jakub Kicinski
  2 siblings, 0 replies; 10+ messages in thread
From: Richard Sailer @ 2020-10-13 18:58 UTC (permalink / raw)
  To: Kleber Sacilotto de Souza, netdev
  Cc: Gerrit Renker, David S. Miller, Jakub Kicinski,
	Thadeu Lima de Souza Cascardo, Gustavo A. R. Silva,
	Alexander A. Klimov, Kees Cook, Eric Dumazet, Alexey Kodanev,
	dccp, linux-kernel

[-- Attachment #1.1: Type: text/plain, Size: 800 bytes --]

On 13/10/2020 19:18, Kleber Sacilotto de Souza wrote:
> From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> 
> When dccps_hc_tx_ccid is freed, ccid timers may still trigger. The reason
> del_timer_sync can't be used is because this relies on keeping a reference
> to struct sock. But as we keep a pointer to dccps_hc_tx_ccid and free that
> during disconnect, the timer should really belong to struct dccp_sock.
> 
> This addresses CVE-2020-16119.
> 
> Fixes: 839a6094140a (net: dccp: Convert timers to use timer_setup())
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>

Acked-bd: Richard Sailer <richard_siegfried@systemli.org>

Implementation and concept looks fine to me


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] Revert "dccp: don't free ccid2_hc_tx_sock struct in dccp_disconnect()"
  2020-10-13 17:18 ` [PATCH 2/2] Revert "dccp: don't free ccid2_hc_tx_sock struct in dccp_disconnect()" Kleber Sacilotto de Souza
@ 2020-10-13 18:59   ` Richard Sailer
  2020-10-15  3:42   ` Jakub Kicinski
  1 sibling, 0 replies; 10+ messages in thread
From: Richard Sailer @ 2020-10-13 18:59 UTC (permalink / raw)
  To: Kleber Sacilotto de Souza, netdev
  Cc: Gerrit Renker, David S. Miller, Jakub Kicinski,
	Thadeu Lima de Souza Cascardo, Gustavo A. R. Silva,
	Alexander A. Klimov, Kees Cook, Eric Dumazet, Alexey Kodanev,
	dccp, linux-kernel

[-- Attachment #1.1: Type: text/plain, Size: 834 bytes --]

On 13/10/2020 19:18, Kleber Sacilotto de Souza wrote:
> rom: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> 
> This reverts commit 2677d20677314101293e6da0094ede7b5526d2b1.
> 
> This fixes an issue that after disconnect, dccps_hc_tx_ccid will still be
> kept, allowing the socket to be reused as a listener socket, and the cloned
> socket will free its dccps_hc_tx_ccid, leading to a later use after free,
> when the listener socket is closed.
> 
> This addresses CVE-2020-16119.
> 
> Fixes: 2677d2067731 (dccp: don't free ccid2_hc_tx_sock struct in dccp_disconnect())
> Reported-by: Hadar Manor
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>
> ---
Acked-by: Richard Sailer <richard_siegfried@systemli.org>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] Revert "dccp: don't free ccid2_hc_tx_sock struct in dccp_disconnect()"
  2020-10-13 17:18 ` [PATCH 2/2] Revert "dccp: don't free ccid2_hc_tx_sock struct in dccp_disconnect()" Kleber Sacilotto de Souza
  2020-10-13 18:59   ` Richard Sailer
@ 2020-10-15  3:42   ` Jakub Kicinski
  2020-10-15  9:23     ` Kleber Souza
  1 sibling, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2020-10-15  3:42 UTC (permalink / raw)
  To: Kleber Sacilotto de Souza
  Cc: netdev, Gerrit Renker, David S. Miller,
	Thadeu Lima de Souza Cascardo, Gustavo A. R. Silva,
	Alexander A. Klimov, Kees Cook, Eric Dumazet, Alexey Kodanev,
	dccp, linux-kernel

On Tue, 13 Oct 2020 19:18:49 +0200 Kleber Sacilotto de Souza wrote:
> From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> 
> This reverts commit 2677d20677314101293e6da0094ede7b5526d2b1.
> 
> This fixes an issue that after disconnect, dccps_hc_tx_ccid will still be
> kept, allowing the socket to be reused as a listener socket, and the cloned
> socket will free its dccps_hc_tx_ccid, leading to a later use after free,
> when the listener socket is closed.
> 
> This addresses CVE-2020-16119.
> 
> Fixes: 2677d2067731 (dccp: don't free ccid2_hc_tx_sock struct in dccp_disconnect())
> Reported-by: Hadar Manor

Does this person has an email address?

> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] dccp: ccid: move timers to struct dccp_sock
  2020-10-13 17:18 ` [PATCH 1/2] dccp: ccid: move timers to struct dccp_sock Kleber Sacilotto de Souza
  2020-10-13 18:58   ` Richard Sailer
@ 2020-10-15  3:43   ` Jakub Kicinski
  2020-10-15 10:53     ` Thadeu Lima de Souza Cascardo
  2020-10-16 22:30   ` Jakub Kicinski
  2 siblings, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2020-10-15  3:43 UTC (permalink / raw)
  To: Kleber Sacilotto de Souza
  Cc: netdev, Gerrit Renker, David S. Miller,
	Thadeu Lima de Souza Cascardo, Gustavo A. R. Silva,
	Alexander A. Klimov, Kees Cook, Eric Dumazet, Alexey Kodanev,
	dccp, linux-kernel

On Tue, 13 Oct 2020 19:18:48 +0200 Kleber Sacilotto de Souza wrote:
> From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> 
> When dccps_hc_tx_ccid is freed, ccid timers may still trigger. The reason
> del_timer_sync can't be used is because this relies on keeping a reference
> to struct sock. But as we keep a pointer to dccps_hc_tx_ccid and free that
> during disconnect, the timer should really belong to struct dccp_sock.
> 
> This addresses CVE-2020-16119.
> 
> Fixes: 839a6094140a (net: dccp: Convert timers to use timer_setup())

Presumably you chose this commit because the fix won't apply beyond it?
But it really fixes 2677d2067731 (dccp: don't free.. right?

> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] Revert "dccp: don't free ccid2_hc_tx_sock struct in dccp_disconnect()"
  2020-10-15  3:42   ` Jakub Kicinski
@ 2020-10-15  9:23     ` Kleber Souza
  0 siblings, 0 replies; 10+ messages in thread
From: Kleber Souza @ 2020-10-15  9:23 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: netdev, Gerrit Renker, David S. Miller,
	Thadeu Lima de Souza Cascardo, Gustavo A. R. Silva,
	Alexander A. Klimov, Kees Cook, Eric Dumazet, Alexey Kodanev,
	dccp, linux-kernel

On 15.10.20 05:42, Jakub Kicinski wrote:
> On Tue, 13 Oct 2020 19:18:49 +0200 Kleber Sacilotto de Souza wrote:
>> From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
>>
>> This reverts commit 2677d20677314101293e6da0094ede7b5526d2b1.
>>
>> This fixes an issue that after disconnect, dccps_hc_tx_ccid will still be
>> kept, allowing the socket to be reused as a listener socket, and the cloned
>> socket will free its dccps_hc_tx_ccid, leading to a later use after free,
>> when the listener socket is closed.
>>
>> This addresses CVE-2020-16119.
>>
>> Fixes: 2677d2067731 (dccp: don't free ccid2_hc_tx_sock struct in dccp_disconnect())
>> Reported-by: Hadar Manor
> 
> Does this person has an email address?

We have received this report via a private Launchpad bug and the submitter
didn't provide any public email address, so we have only their name.

> 
>> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
>> Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] dccp: ccid: move timers to struct dccp_sock
  2020-10-15  3:43   ` Jakub Kicinski
@ 2020-10-15 10:53     ` Thadeu Lima de Souza Cascardo
  0 siblings, 0 replies; 10+ messages in thread
From: Thadeu Lima de Souza Cascardo @ 2020-10-15 10:53 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Kleber Sacilotto de Souza, netdev, Gerrit Renker,
	David S. Miller, Gustavo A. R. Silva, Alexander A. Klimov,
	Kees Cook, Eric Dumazet, Alexey Kodanev, dccp, linux-kernel

On Wed, Oct 14, 2020 at 08:43:22PM -0700, Jakub Kicinski wrote:
> On Tue, 13 Oct 2020 19:18:48 +0200 Kleber Sacilotto de Souza wrote:
> > From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> > 
> > When dccps_hc_tx_ccid is freed, ccid timers may still trigger. The reason
> > del_timer_sync can't be used is because this relies on keeping a reference
> > to struct sock. But as we keep a pointer to dccps_hc_tx_ccid and free that
> > during disconnect, the timer should really belong to struct dccp_sock.
> > 
> > This addresses CVE-2020-16119.
> > 
> > Fixes: 839a6094140a (net: dccp: Convert timers to use timer_setup())
> 
> Presumably you chose this commit because the fix won't apply beyond it?
> But it really fixes 2677d2067731 (dccp: don't free.. right?

Well, it should also fix cases where dccps_hc_tx_ccid{,_private} has been freed
right after the timer is stopped.

So, we could add:
Fixes: 2a91aa396739 ([DCCP] CCID2: Initial CCID2 (TCP-Like) implementation)
Fixes: 7c657876b63c ([DCCP]: Initial implementation)

But I wouldn't say that this fixes 2677d2067731, unless there is argument to
say that it fixes it because it claimed to fix what is being fixed here. But
even the code that it removed was supposed to be stopping the timer, so how
could it ever fix what it was claiming to fix?

Thanks.
Cascardo.

> 
> > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> > Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] dccp: ccid: move timers to struct dccp_sock
  2020-10-13 17:18 ` [PATCH 1/2] dccp: ccid: move timers to struct dccp_sock Kleber Sacilotto de Souza
  2020-10-13 18:58   ` Richard Sailer
  2020-10-15  3:43   ` Jakub Kicinski
@ 2020-10-16 22:30   ` Jakub Kicinski
  2 siblings, 0 replies; 10+ messages in thread
From: Jakub Kicinski @ 2020-10-16 22:30 UTC (permalink / raw)
  To: Kleber Sacilotto de Souza, Eric Dumazet
  Cc: netdev, Gerrit Renker, David S. Miller,
	Thadeu Lima de Souza Cascardo, Gustavo A. R. Silva,
	Alexander A. Klimov, Kees Cook, Alexey Kodanev, dccp,
	linux-kernel

On Tue, 13 Oct 2020 19:18:48 +0200 Kleber Sacilotto de Souza wrote:
> From: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> 
> When dccps_hc_tx_ccid is freed, ccid timers may still trigger. The reason
> del_timer_sync can't be used is because this relies on keeping a reference
> to struct sock. But as we keep a pointer to dccps_hc_tx_ccid and free that
> during disconnect, the timer should really belong to struct dccp_sock.
> 
> This addresses CVE-2020-16119.
> 
> Fixes: 839a6094140a (net: dccp: Convert timers to use timer_setup())
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>

I've been mulling over this fix.

The layering violation really doesn't sit well.

We're reusing the timer object. What if we are really unlucky, the
fires and gets blocked by a cosmic ray just as it's about to try to
lock the socket, then user manages to reconnect, and timer starts
again. Potentially with a different CCID algo altogether?

Is disconnect ever called under the BH lock?  Maybe plumb a bool
argument through to ccid*_hc_tx_exit() and do a sk_stop_timer_sync()
when called from disconnect()?

Or do refcounting on ccid_priv so that the timer holds both the socket
and the priv?

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-13 17:18 [PATCH 0/2] net: dccp: fix structure use-after-free Kleber Sacilotto de Souza
2020-10-13 17:18 ` [PATCH 1/2] dccp: ccid: move timers to struct dccp_sock Kleber Sacilotto de Souza
2020-10-13 18:58   ` Richard Sailer
2020-10-15  3:43   ` Jakub Kicinski
2020-10-15 10:53     ` Thadeu Lima de Souza Cascardo
2020-10-16 22:30   ` Jakub Kicinski
2020-10-13 17:18 ` [PATCH 2/2] Revert "dccp: don't free ccid2_hc_tx_sock struct in dccp_disconnect()" Kleber Sacilotto de Souza
2020-10-13 18:59   ` Richard Sailer
2020-10-15  3:42   ` Jakub Kicinski
2020-10-15  9:23     ` Kleber Souza

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git