[PATCH net v2] tcp: free saved_syn after an RCU grace period

[Yingjie Wang]

tcp_reqsk_record_syn() allocates saved_syn separately from its request
socket. bpf_sock_ops_get_syn() can load that pointer while processing a
retransmitted SYN under RCU.

There are two ownership transfers which can let another CPU reclaim the
allocation while such a reader is still using it.

First, tcp_move_syn() transfers req->saved_syn to a newly created child
socket. The child can be closed and tcp_saved_syn_free() can reclaim the
allocation before a reader of the old request finishes.

Second, inet_reqsk_clone() shallow-copies request_sock during request
migration. The migrated clone becomes the owner after reqsk_migrate_reset()
clears the old request's pointer, but a reader which already loaded the
pointer from the old request can still be using it when the clone is
released.

Mark the request and full-socket saved_syn pointers as RCU protected. Use
rcu_dereference() in the BPF readers, rcu_assign_pointer() when publishing
the saved SYN to the child socket, and RCU_INIT_POINTER() at initialization
and clearing sites. Embed an rcu_head in struct saved_syn and defer
reclamation in both release paths. Clear a live full-socket pointer before
scheduling reclamation, so a new RCU reader cannot acquire it after the
grace period starts.

This follows commit e3f33b0a1d89 ("ipv4: free inet_opt and ireq_opt
after an RCU grace period"), which fixed the same ownership-transfer
pattern for IPv4 options.

The issue was confirmed with KASAN on v7.3-16007-g36aad21d50d1 using
temporary kernel instrumentation to widen the reader and request-migration
window. KASAN reported a slab-use-after-free read in
bpf_sock_ops_get_syn(), with the allocation freed by __reqsk_free() from
reqsk_timer_handler(). The same explicit migration test completed without
a report with deferred reclamation. A patched kernel without the delay
instrumentation also completed a successful migration and seven saved-SYN
reads without a KASAN report.

Fixes: 0813a841566f ("bpf: tcp: Allow bpf prog to write and parse TCP header option")
Fixes: c905dee62232 ("tcp: Migrate TCP_NEW_SYN_RECV requests at retransmitting SYN+ACKs.")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Yingjie Wang <x-professor@xxxxxxxxxxx>
---
 include/linux/tcp.h             | 16 +++++++++++-----
 include/net/request_sock.h      |  4 +++-
 net/core/filter.c               | 16 +++++++++++-----
 net/ipv4/inet_connection_sock.c |  6 +++---
 net/ipv4/tcp.c                  | 14 +++++++++-----
 net/ipv4/tcp_input.c            |  2 +-
 6 files changed, 38 insertions(+), 20 deletions(-)

diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index d18f7bc1e5d0..5ba4d81e431a 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -536,7 +536,7 @@ struct tcp_sock {
 	 * socket. Used to retransmit SYNACKs etc.
 	 */
 	struct request_sock __rcu *fastopen_rsk;
-	struct saved_syn *saved_syn;
+	struct saved_syn __rcu *saved_syn;
 };
 
 enum tsq_enum {
@@ -618,14 +618,20 @@ static inline void fastopen_queue_tune(struct sock *sk, int backlog)
 static inline void tcp_move_syn(struct tcp_sock *tp,
 				struct request_sock *req)
 {
-	tp->saved_syn = req->saved_syn;
-	req->saved_syn = NULL;
+	struct saved_syn *saved_syn;
+
+	saved_syn = rcu_dereference(req->saved_syn);
+	rcu_assign_pointer(tp->saved_syn, saved_syn);
+	RCU_INIT_POINTER(req->saved_syn, NULL);
 }
 
 static inline void tcp_saved_syn_free(struct tcp_sock *tp)
 {
-	kfree(tp->saved_syn);
-	tp->saved_syn = NULL;
+	struct saved_syn *saved_syn;
+
+	saved_syn = rcu_dereference_protected(tp->saved_syn, 1);
+	RCU_INIT_POINTER(tp->saved_syn, NULL);
+	kfree_rcu(saved_syn, rcu);
 }
 
 static inline u32 tcp_saved_syn_len(const struct saved_syn *saved_syn)
diff --git a/include/net/request_sock.h b/include/net/request_sock.h
index 5a9c826a7092..ddc134f1d15d 100644
--- a/include/net/request_sock.h
+++ b/include/net/request_sock.h
@@ -16,6 +16,7 @@
 #include <linux/types.h>
 #include <linux/bug.h>
 #include <linux/refcount.h>
+#include <linux/rcupdate.h>
 
 #include <net/sock.h>
 #include <net/rstreason.h>
@@ -39,6 +40,7 @@ struct request_sock_ops {
 };
 
 struct saved_syn {
+	struct rcu_head rcu;
 	u32 mac_hdrlen;
 	u32 network_hdrlen;
 	u32 tcp_hdrlen;
@@ -68,7 +70,7 @@ struct request_sock {
 	struct timer_list		rsk_timer;
 	const struct request_sock_ops	*rsk_ops;
 	struct sock			*sk;
-	struct saved_syn		*saved_syn;
+	struct saved_syn __rcu		*saved_syn;
 	u32				secid;
 	u32				peer_secid;
 	u32				timeout;
diff --git a/net/core/filter.c b/net/core/filter.c
index 09c60a58c82a..38a4b48e1fb0 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5714,12 +5714,18 @@ static int sol_tcp_sockopt(struct sock *sk, int optname,
 
 	if (getopt) {
 		if (optname == TCP_SAVED_SYN) {
+			struct saved_syn *saved_syn;
 			struct tcp_sock *tp = tcp_sk(sk);
 
-			if (!tp->saved_syn ||
-			    *optlen > tcp_saved_syn_len(tp->saved_syn))
+			rcu_read_lock();
+			saved_syn = rcu_dereference(tp->saved_syn);
+			if (!saved_syn ||
+			    *optlen > tcp_saved_syn_len(saved_syn)) {
+				rcu_read_unlock();
 				return -EINVAL;
-			memcpy(optval, tp->saved_syn->data, *optlen);
+			}
+			memcpy(optval, saved_syn->data, *optlen);
+			rcu_read_unlock();
 			/* It cannot free tp->saved_syn here because it
 			 * does not know if the user space still needs it.
 			 */
@@ -6125,9 +6131,9 @@ static int bpf_sock_ops_get_syn(struct bpf_sock_ops_kern *bpf_sock,
 			 * not be available.  It has to resort to
 			 * saved_syn (if it is saved).
 			 */
-			saved_syn = inet_reqsk(sk)->saved_syn;
+			saved_syn = rcu_dereference(inet_reqsk(sk)->saved_syn);
 		else
-			saved_syn = tcp_sk(sk)->saved_syn;
+			saved_syn = rcu_dereference(tcp_sk(sk)->saved_syn);
 
 		if (!saved_syn)
 			return -ENOENT;
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index 93b306d91b6d..1d54057a86ef 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -869,7 +869,7 @@ reqsk_alloc_noprof(const struct request_sock_ops *ops, struct sock *sk_listener,
 	req_to_sk(req)->sk_prot = sk_listener->sk_prot;
 	sk_node_init(&req_to_sk(req)->sk_node);
 	sk_tx_queue_clear(req_to_sk(req));
-	req->saved_syn = NULL;
+	RCU_INIT_POINTER(req->saved_syn, NULL);
 	req->syncookie = 0;
 	req->num_timeout = 0;
 	req->num_retrans = 0;
@@ -908,7 +908,7 @@ void __reqsk_free(struct request_sock *req)
 	req->rsk_ops->destructor(req);
 	if (req->rsk_listener)
 		sock_put(req->rsk_listener);
-	kfree(req->saved_syn);
+	kfree_rcu(rcu_dereference_protected(req->saved_syn, 1), rcu);
 	kmem_cache_free(req->rsk_ops->slab, req);
 }
 
@@ -979,7 +979,7 @@ static void reqsk_queue_migrated(struct request_sock_queue *queue,
 
 static void reqsk_migrate_reset(struct request_sock *req)
 {
-	req->saved_syn = NULL;
+	RCU_INIT_POINTER(req->saved_syn, NULL);
 #if IS_ENABLED(CONFIG_IPV6)
 	inet_rsk(req)->ipv6_opt = NULL;
 	inet_rsk(req)->pktopts = NULL;
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index b9de7c302ba8..7e8f7c9d5f53 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -4722,13 +4722,17 @@ int do_tcp_getsockopt(struct sock *sk, int level,
 		val = tp->save_syn;
 		break;
 	case TCP_SAVED_SYN: {
+		struct saved_syn *saved_syn;
+
 		if (copy_from_sockptr(&len, optlen, sizeof(int)))
 			return -EFAULT;
 
 		sockopt_lock_sock(sk);
-		if (tp->saved_syn) {
-			if (len < tcp_saved_syn_len(tp->saved_syn)) {
-				len = tcp_saved_syn_len(tp->saved_syn);
+		saved_syn = rcu_dereference_protected(tp->saved_syn,
+						      lockdep_sock_is_held(sk));
+		if (saved_syn) {
+			if (len < tcp_saved_syn_len(saved_syn)) {
+				len = tcp_saved_syn_len(saved_syn);
 				if (copy_to_sockptr(optlen, &len, sizeof(int))) {
 					sockopt_release_sock(sk);
 					return -EFAULT;
@@ -4736,12 +4740,12 @@ int do_tcp_getsockopt(struct sock *sk, int level,
 				sockopt_release_sock(sk);
 				return -EINVAL;
 			}
-			len = tcp_saved_syn_len(tp->saved_syn);
+			len = tcp_saved_syn_len(saved_syn);
 			if (copy_to_sockptr(optlen, &len, sizeof(int))) {
 				sockopt_release_sock(sk);
 				return -EFAULT;
 			}
-			if (copy_to_sockptr(optval, tp->saved_syn->data, len)) {
+			if (copy_to_sockptr(optval, saved_syn->data, len)) {
 				sockopt_release_sock(sk);
 				return -EFAULT;
 			}
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index ebd2c12e42d5..5151bb16a8b9 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -7638,7 +7638,7 @@ static void tcp_reqsk_record_syn(const struct sock *sk,
 			saved_syn->network_hdrlen = skb_network_header_len(skb);
 			saved_syn->tcp_hdrlen = tcp_hdrlen(skb);
 			memcpy(saved_syn->data, base, len);
-			req->saved_syn = saved_syn;
+			RCU_INIT_POINTER(req->saved_syn, saved_syn);
 		}
 	}
 }

base-commit: c3e67bd82b9b9b1a5118f09a429ccefe2aab0741
-- 
2.43.0