[PATCH net v2 7/8] net: clear sk_tsq_flags in sk_clone()

[Hyunwoo Kim <imv4bel@gmail.com>]

A socket returned by accept() can be freed while its fd is still open. A
setsockopt() on that fd then hits a use-after-free.

TCP_TSQ_DEFERRED owns a socket reference. tcp_tsq_handler() sets the bit
and calls sock_hold() when the socket is owned by user, and
tcp_release_cb() clears the bit and calls __sock_put(). sock_copy() gives
the child the bit but not the reference, so the __sock_put() that runs when
accept() locks and unlocks the child has nothing to pair with. The socket
is freed by the next put, which in the log below came from a timer.

A listener can be holding this bit. An established socket becomes a
listener again through connect(AF_UNSPEC) and listen(), and
tcp_clear_xmit_timers() only calls hrtimer_try_to_cancel(), so a pacing
callback that is already running survives. That callback sets the bit while
accept() holds the socket lock, and in the same window the child is created
in the TCP_NEW_SYN_RECV branch of tcp_v4_rcv(), which does not take the
listener lock.

All seven bits in sk_tsq_flags describe work pending on the parent, and
four of them own a reference. They are set in four different places, so
clear the whole word in sk_clone().

In short:

  socket(AF_INET) -> setsockopt(SO_MAX_PACING_RATE, 100000)
                  -> setsockopt(TCP_MAXSEG, 1200) -> bind -> connect(peer)
  send(64KB)                          // arms the pacing timer, sock_hold()
  connect(AF_UNSPEC)                  // stop being connected
  listen()                            // become a listener again
  setsockopt(TCP_DEFER_ACCEPT)
      // another socket connects, and with TCP_DEFER_ACCEPT there is no
      // child yet
  accept()                            // the child is created when one byte
                                      // arrives. while accept() holds the
                                      // lock the pacing callback sets the
                                      // bit, and the child is cloned by
                                      // the receive path, which does not
                                      // take the listener lock
      // a timer on the child does the last put and the socket is freed
  setsockopt(accepted, TCP_NODELAY)   // use-after-free

KASAN log:

  BUG: KASAN: slab-use-after-free in sock_common_setsockopt+0x44/0x80
  Read of size 8 at addr ffff88800e2ab668 by task repro/94
  ...
  Call Trace:
   sock_common_setsockopt+0x44/0x80
   do_sock_setsockopt+0x15e/0x2b0
   __sys_setsockopt+0x9e/0xe0
   __x64_sys_setsockopt+0x64/0x80
  ...
  Allocated by task 95:
   sk_prot_alloc+0x45/0x170
   sk_clone+0x49/0x960
   inet_csk_clone_lock+0x29/0x2c0
   tcp_create_openreq_child+0x2a/0xf20
   tcp_v4_syn_recv_sock+0xd3/0x7e0
   tcp_check_req+0x374/0xff0
   tcp_v4_rcv+0xc2d/0x2040
  ...
  Freed by task 0:
   slab_free_after_rcu_debug+0xc5/0x200
   rcu_core+0x4de/0xd30
  ...
  Last potentially related work creation:
   kmem_cache_free+0x11d/0x5f0
   __sk_destruct+0x29a/0x3d0
   call_timer_fn+0x12f/0x3f0
   __run_timers+0x4a4/0x5e0
  ...
  The buggy address belongs to the object at ffff88800e2ab640
   which belongs to the cache TCP of size 3200

Fixes: 73a6bab5aa2a ("tcp: switch pacing timer to softirq based hrtimer")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
---
 net/core/sock.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/core/sock.c b/net/core/sock.c
index 098e58b40f304b..06fbb19824e267 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2533,6 +2533,7 @@ struct sock *sk_clone(const struct sock *sk, const gfp_t priority,
 	newsk->sk_reserved_mem  = 0;
 	DEBUG_NET_WARN_ON_ONCE(newsk->sk_drop_counters);
 	sk_drops_reset(newsk);
+	newsk->sk_tsq_flags	= 0;
 	newsk->sk_send_head	= NULL;
 	newsk->sk_userlocks	= sk->sk_userlocks & ~SOCK_BINDPORT_LOCK;
 	atomic_set(&newsk->sk_zckey, 0);
-- 
2.43.0