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

[PATCH net] 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.

Both request and full TCP sockets currently free saved_syn with plain
kfree().  Embed an rcu_head in struct saved_syn and use kfree_rcu() for
both teardown paths.  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 this change.  A patched kernel without the delay
instrumentation also completed a successful migration and seven saved-SYN
reads without a KASAN report.

Full allmodconfig and allyesconfig builds completed with W=1.  Existing
warnings outside the modified files were made non-fatal with
KCFLAGS=-Wno-error; the modified files emitted none.

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@vger.kernel.org
Signed-off-by: Yingjie Wang <x-professor@outlook.com>
---
 include/linux/tcp.h             | 2 +-
 include/net/request_sock.h      | 2 ++
 net/ipv4/inet_connection_sock.c | 2 +-
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index d18f7bc1e5d0..dd3b097daac9 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -624,7 +624,7 @@ static inline void tcp_move_syn(struct tcp_sock *tp,
 
 static inline void tcp_saved_syn_free(struct tcp_sock *tp)
 {
-	kfree(tp->saved_syn);
+	kfree_rcu(tp->saved_syn, rcu);
 	tp->saved_syn = NULL;
 }
 
diff --git a/include/net/request_sock.h b/include/net/request_sock.h
index 5a9c826a7092..b6ccfc31147d 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;
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index 93b306d91b6d..a2622eccaff1 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -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(req->saved_syn, rcu);
 	kmem_cache_free(req->rsk_ops->slab, req);
 }
 

base-commit: 902407bc588575af0ceae5de4f635fe1e024f258
-- 
2.43.0

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.