netdev - Re: [PATCH] tcp: Fixed a TFO server bug that crashed kernel by raw sockets

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:	Wed, 19 Sep 2012 02:19:23 +0200
From:	Christoph Paasch <christoph.paasch@...ouvain.be>
To:	"H.K. Jerry Chu" <hkchu@...gle.com>
Cc:	davem@...emloft.net, netdev@...r.kernel.org, ncardwell@...gle.com,
	edumazet@...gle.com
Subject: Re: [PATCH] tcp: Fixed a TFO server bug that crashed kernel by raw sockets

On Tuesday 18 September 2012 16:35:51 H.K. Jerry Chu wrote:
> From: Jerry Chu <hkchu@...gle.com>
> 
> Crash dump msg looks like this:
> 
> <1>[34468.419809] BUG: unable to handle kernel paging request at
> ffffeb57000dc058 <1>[34468.426770] IP: [<ffffffff80383f9c>]
> kfree+0x4c/0x2d0
> ...
> <4>[34468.603362] Call Trace:
> <4>[34468.605802]  [<ffffffff807542a4>] inet_sock_destruct+0x174/0x1f0
> <4>[34468.611786]  [<ffffffff806ced53>] __sk_free+0x23/0x170
> <4>[34468.616907]  [<ffffffff806ceec5>] sk_free+0x25/0x30
> <4>[34468.621762]  [<ffffffff806d066a>] sk_common_release+0x7a/0x80
> <4>[34468.627481]  [<ffffffff80746782>] raw_close+0x22/0x30
> <4>[34468.632515]  [<ffffffff80753038>] inet_release+0x58/0x90
> <4>[34468.637802]  [<ffffffff806c9dd8>] sock_release+0x28/0x90
> <4>[34468.643087]  [<ffffffff806c9f07>] sock_close+0x17/0x30
> <4>[34468.648203]  [<ffffffff8039d60a>] fput+0xda/0x210
> <4>[34468.652894]  [<ffffffff80398e06>] filp_close+0x66/0x90
> <4>[34468.658016]  [<ffffffff802822cd>] put_files_struct+0x9d/0x120
> <4>[34468.663743]  [<ffffffff802823fa>] exit_files+0x4a/0x60
> <4>[34468.668857]  [<ffffffff802828e4>] do_exit+0x1a4/0x8c0
> <4>[34468.673886]  [<ffffffff803697db>] ? do_munmap+0x2ab/0x390
> <4>[34468.679256]  [<ffffffff80283384>] do_group_exit+0x44/0xa0
> <4>[34468.684632]  [<ffffffff8028341f>] sys_exit_group+0x3f/0x50
> <4>[34468.690094]  [<ffffffff807a5730>] sysenter_dispatch+0x7/0x1a
> 
> This bug was introduced as part of patch
> 7ab4551f3b391818e29263279031dca1e26417c6
> 
> It turns out the call "socket(PF_INET/PF_INET6, SOCK_RAW, IPPROTO_TCP)"
> will cause a raw socket to be created with protocol == IPPROTO_TCP
> so checking against the protocol field in sk alone is not sufficient
> to guarantee a TCP socket. One must also check type == SOCK_STREAM.
> 
> Signed-off-by: H.K. Jerry Chu <hkchu@...gle.com>
> Cc: Neal Cardwell <ncardwell@...gle.com>
> Cc: Eric Dumazet <edumazet@...gle.com>
> ---
>  net/ipv4/af_inet.c |   28 +++++++++++++++++-----------
>  1 files changed, 17 insertions(+), 11 deletions(-)

Why not moving the TCP-code out of inet_sock_destruct by modifying the sk_destruct
callback when TFO is in use? Like the below (only compile-tested) patch. That
way inet_sock_destruct stays TFO-free.


Cheers,
Christoph

---------

From: Christoph Paasch <christoph.paasch@...ouvain.be>
Date: Wed, 19 Sep 2012 02:06:53 +0200
Subject: [PATCH] Don't add TCP-code in inet_sock_destruct

Signed-off-by: Christoph Paasch <christoph.paasch@...ouvain.be>
---
 include/linux/tcp.h |    4 ++++
 net/ipv4/af_inet.c  |    2 --
 net/ipv4/tcp.c      |    7 +++++++
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index ae46df5..67c789a 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -574,6 +574,8 @@ static inline bool fastopen_cookie_present(struct tcp_fastopen_cookie *foc)
 	return foc->len != -1;
 }
 
+extern void tcp_sock_destruct(struct sock *sk);
+
 static inline int fastopen_init_queue(struct sock *sk, int backlog)
 {
 	struct request_sock_queue *queue =
@@ -585,6 +587,8 @@ static inline int fastopen_init_queue(struct sock *sk, int backlog)
 		    sk->sk_allocation);
 		if (queue->fastopenq == NULL)
 			return -ENOMEM;
+
+		sk->sk_destruct = tcp_sock_destruct;
 		spin_lock_init(&queue->fastopenq->lock);
 	}
 	queue->fastopenq->max_qlen = backlog;
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 845372b..766c596 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -149,8 +149,6 @@ void inet_sock_destruct(struct sock *sk)
 		pr_err("Attempt to release alive inet socket %p\n", sk);
 		return;
 	}
-	if (sk->sk_protocol == IPPROTO_TCP)
-		kfree(inet_csk(sk)->icsk_accept_queue.fastopenq);
 
 	WARN_ON(atomic_read(&sk->sk_rmem_alloc));
 	WARN_ON(atomic_read(&sk->sk_wmem_alloc));
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index df83d74..7b1e940 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2325,6 +2325,13 @@ int tcp_disconnect(struct sock *sk, int flags)
 }
 EXPORT_SYMBOL(tcp_disconnect);
 
+void tcp_sock_destruct(struct sock *sk)
+{
+	inet_sock_destruct(sk);
+
+	kfree(inet_csk(sk)->icsk_accept_queue.fastopenq);
+}
+
 static inline bool tcp_can_repair_sock(const struct sock *sk)
 {
 	return capable(CAP_NET_ADMIN) &&
-- 
1.7.9.5


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