tcp: fix NULL pointer dereference when restarting a Fast Open listener

When a valid RST is received for a Fast Open child accepted before the three-way handshake completes, reqsk_fastopen_remove() adds the request_sock to the listener's RST list if the listener is in TCP_LISTEN.

An application can call shutdown(SHUT_RD) and then listen() on the same listener socket. inet_csk_listen_stop() checks rskq_rst_head without holding fastopenq.lock. If rskq_rst_head is NULL, it does not acquire the lock and therefore does not synchronize with an insertion by reqsk_fastopen_remove() after that function has passed the TCP_LISTEN check.

The subsequent reqsk_queue_alloc() also initializes rskq_rst_head, rskq_rst_tail, and qlen in that order without holding the lock. If a request_sock is added between the initialization of rskq_rst_head and rskq_rst_tail, rskq_rst_head points to the request_sock while rskq_rst_tail is NULL.

When a valid RST is later received for another accepted Fast Open child, rskq_rst_head is not NULL, so reqsk_fastopen_remove() attempts to write the request_sock pointer to rskq_rst_tail->dl_next while rskq_rst_tail is NULL. KASAN reports:

  BUG: KASAN: null-ptr-deref in reqsk_fastopen_remove+0x147/0x210
  Write of size 8 at addr 0000000000000088 by task swapper/3/0
  ...
  Call Trace:
   <IRQ>
   ...
   reqsk_fastopen_remove+0x147/0x210
   tcp_check_req+0xa70/0x1060
   ...
   tcp_rcv_state_process+0x1cb/0x2ae0
   ...
   tcp_v4_do_rcv+0x1fe/0x740
   tcp_v4_rcv+0x1744/0x2070
   ...
   ip_protocol_deliver_rcu+0x5c/0x280
   ip_local_deliver_finish+0x171/0x330
   ip_local_deliver+0x107/0x370
   ...
   </IRQ>

To widen the race window, the issue was reproduced after adding mdelay(3000) immediately after reqsk_fastopen_remove() checks that the listener is in TCP_LISTEN and immediately after reqsk_queue_alloc() sets rskq_rst_head to NULL.

Make inet_csk_listen_stop() acquire fastopenq.lock regardless of the value of rskq_rst_head and detach the list while holding the lock. If reqsk_fastopen_remove() has already passed the TCP_LISTEN check, inet_csk_listen_stop() waits until the list insertion completes.

Also initialize rskq_rst_head, rskq_rst_tail, and qlen under the same lock in reqsk_queue_alloc(). Drop the references to the detached request_sock objects after releasing the lock.

Fixes 0536fcc039a8 ("tcp: prepare fastopen code for upcoming listener changes")

Cc [email protected]

Assisted-by LLM

Signed-off-by Hyunwoo Kim <[email protected]>

---
 net/ipv4/inet_connection_sock.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

net/ipv4/inet_connection_sock.c +0 -0

index e56beec438773..5590fc7efb88d 100644

Quoted 70 lines · Hyunwoo Kim

When a valid RST is received for a Fast Open child accepted before the three-way handshake completes, reqsk_fastopen_remove() adds the request_sock to the listener's RST list if the listener is in TCP_LISTEN.

An application can call shutdown(SHUT_RD) and then listen() on the same listener socket. inet_csk_listen_stop() checks rskq_rst_head without holding fastopenq.lock. If rskq_rst_head is NULL, it does not acquire the lock and therefore does not synchronize with an insertion by reqsk_fastopen_remove() after that function has passed the TCP_LISTEN check.

The subsequent reqsk_queue_alloc() also initializes rskq_rst_head, rskq_rst_tail, and qlen in that order without holding the lock. If a request_sock is added between the initialization of rskq_rst_head and rskq_rst_tail, rskq_rst_head points to the request_sock while rskq_rst_tail is NULL.

When a valid RST is later received for another accepted Fast Open child, rskq_rst_head is not NULL, so reqsk_fastopen_remove() attempts to write the request_sock pointer to rskq_rst_tail->dl_next while rskq_rst_tail is NULL. KASAN reports:

  BUG: KASAN: null-ptr-deref in reqsk_fastopen_remove+0x147/0x210
  Write of size 8 at addr 0000000000000088 by task swapper/3/0
  ...
  Call Trace:
   <IRQ>
   ...
   reqsk_fastopen_remove+0x147/0x210
   tcp_check_req+0xa70/0x1060
   ...
   tcp_rcv_state_process+0x1cb/0x2ae0
   ...
   tcp_v4_do_rcv+0x1fe/0x740
   tcp_v4_rcv+0x1744/0x2070
   ...
   ip_protocol_deliver_rcu+0x5c/0x280
   ip_local_deliver_finish+0x171/0x330
   ip_local_deliver+0x107/0x370
   ...
   </IRQ>

To widen the race window, the issue was reproduced after adding mdelay(3000) immediately after reqsk_fastopen_remove() checks that the listener is in TCP_LISTEN and immediately after reqsk_queue_alloc() sets rskq_rst_head to NULL.

Make inet_csk_listen_stop() acquire fastopenq.lock regardless of the value of rskq_rst_head and detach the list while holding the lock. If reqsk_fastopen_remove() has already passed the TCP_LISTEN check, inet_csk_listen_stop() waits until the list insertion completes.

Also initialize rskq_rst_head, rskq_rst_tail, and qlen under the same lock in reqsk_queue_alloc(). Drop the references to the detached request_sock objects after releasing the lock.

Fixes 0536fcc039a8 ("tcp: prepare fastopen code for upcoming listener changes")

Cc [email protected]

Assisted-by LLM

Signed-off-by Hyunwoo Kim <[email protected]>

---
 net/ipv4/inet_connection_sock.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

net/ipv4/inet_connection_sock.c +0 -0

index e56beec438773..5590fc7efb88d 100644

net/ipv4/inet_connection_sock.c static int inet_ulp_can_listen(const struct sock *sk) · line 1340

Quoted 8 lines

net/ipv4/inet_connection_sock.c +2 -0

@@ -1333,9 +1333,11 @@ static int inet_ulp_can_listen(const struct sock *sk)
 
 static void reqsk_queue_alloc(struct request_sock_queue *queue)
 {
+       spin_lock_bh(&queue->fastopenq.lock);
        queue->fastopenq.rskq_rst_head = NULL;
        queue->fastopenq.rskq_rst_tail = NULL;
        queue->fastopenq.qlen = 0;
+       spin_unlock_bh(&queue->fastopenq.lock);

I do not think this spin_lock_bh() is _needed_ here, this hunk is misleading/distracting.

Once inet_csk_listen_stop() takes fastopenq.lock unconditionally, reqsk_fastopen_remove() either inserted its req before we grabbed the lock (and we free it), or it will see sk_state != TCP_LISTEN and will not insert anything. Nothing can be added to rskq_rst_head until inet_csk_listen_start() sets TCP_LISTEN again, after reqsk_queue_alloc().

The double mdelay() needed to reproduce the NULL deref shows the window:
shutdown() + listen() would have to complete while reqsk_fastopen_remove() is between its TCP_LISTEN check and the list insertion.

Please let's focus on the real issue, which is solely in inet_csk_listen_stop().

Also note the more likely outcome of this race is a leak: a req stranded in rskq_rst_head holds a reference on the listener, and a subsequent close() will not call inet_csk_listen_stop() again. This is worth mentioning in the changelog.

Please send a v2 with only the inet_csk_listen_stop() part.