[PATCH net v2 1/8] tcp: fix use-after-free of the listener's ipv6_pinfo after IPV6_ADDRFORM

IPV6_ADDRFORM switches an established AF_INET6 TCP socket to tcp_prot and
ipv4_specific. The socket is still a tcp6_sock, so ->pinet6 keeps pointing
at the ipv6_pinfo inside it, and sk_destruct is left alone because
commit d38afeec26ed ("tcp/udp: Call inet6_destroy_sock() in IPv6
sk->sk_destruct().") uses it to clean up the IPv6 resources.

After connect(AF_UNSPEC) the socket can listen() again. Its children are
then created by tcp_v4_syn_recv_sock() with opt_child_init NULL, and
sk_clone() allocates them from tcp_prot, so each one is a plain tcp_sock
that inherits ->pinet6 and sk_destruct from the listener.
tcp_v6_mapped_child_init(), added by commit 858d2a4f67ff ("tcp: fix
potential race in tcp_v6_syn_recv_sock()"), would overwrite ->pinet6, but
tcp_v6_syn_recv_sock() is the only caller that passes it and it is not
involved here.

A child can outlive the listener. INET_ECN_xmit() and INET_ECN_dontxmit()
test inet6_sk(sk) and not sk_family, so tcp_ecn_send() updates np->tclass
through the stale pointer, and the child's destructor runs
inet6_cleanup_sock() on the freed listener.

In short:

  socket(AF_INET6) -> bind -> listen
      // a client connects over IPv4
  accept()                              // the child is v4-mapped
  setsockopt(IPV6_ADDRFORM, PF_INET)    // it becomes an AF_INET socket
  connect(AF_UNSPEC) -> bind -> listen  // reuse it as an IPv4 server
      // a client connects again
  accept()
  close(the listener)
  close(the accepted socket)            // use-after-free

KASAN log:

  BUG: KASAN: slab-use-after-free in __tcp_transmit_skb+0x1070/0x2020
  Read of size 1 at addr ffff888016671af3 by task poc/111
  ...
  Call Trace:
   __tcp_transmit_skb+0x1070/0x2020
   tcp_write_xmit+0xace/0x3380
   __tcp_push_pending_frames+0x58/0x180
   __tcp_close+0x4b8/0x7d0
   tcp_close+0x23/0x90
   inet_release+0x93/0x100
   __sock_release+0x66/0x130
   sock_close+0x18/0x20
   __fput+0x1f0/0x4c0
   __x64_sys_close+0x55/0x90
  ...
  BUG: KASAN: slab-use-after-free in inet6_cleanup_sock+0x61/0x140
  Write of size 8 at addr ffff888016671b20 by task poc/111
  ...
  Call Trace:
   inet6_cleanup_sock+0x61/0x140
   inet6_sock_destruct+0x12/0x20
   __sk_destruct+0x4f/0x420
   inet_release+0x93/0x100
   __sock_release+0x66/0x130
   sock_close+0x18/0x20
   __fput+0x1f0/0x4c0
   __x64_sys_close+0x55/0x90
  ...
  Allocated by task 111:
   sk_prot_alloc+0x45/0x170
   sk_clone+0x49/0x970
   inet_csk_clone_lock+0x29/0x2c0
   tcp_create_openreq_child+0x2a/0x10a0
   tcp_v4_syn_recv_sock+0xd3/0x850
   tcp_v6_syn_recv_sock+0xc12/0xd80
   tcp_check_req+0x390/0x1080
   tcp_v4_rcv+0xc15/0x21c0
  ...
  Freed by task 14:
   slab_free_after_rcu_debug+0xd5/0x220
   rcu_core+0x4fe/0xe20
  ...
  The buggy address belongs to the object at ffff888016670e40
   which belongs to the cache TCPv6 of size 3328

Fix this by clearing ->pinet6 and ->ipv6_fl_list on the child, and by
returning early from inet6_cleanup_sock() when there is no ipv6_pinfo.
tcp_v6_mapped_child_init() sets both fields, so the v4-mapped path is not
affected. The converted listener keeps its own ipv6_pinfo, so there is
nothing to clear on the IPV6_ADDRFORM side.

Clearing ->pinet6 in tcp_disconnect() instead would keep this out of the
fast path, but it leaves the pointer NULL on a socket that still has a
file descriptor, and of the 120 inet6_sk() call sites only the two in
inet_ecn.h check it for NULL, so _all_ the others have to be found and
guarded first. At the point patched here the child is not in the ehash yet
and has no sk_socket. Once the two fields are cleared it is no different
from any other AF_INET child.

Fixes: d38afeec26ed ("tcp/udp: Call inet6_destroy_sock() in IPv6 sk->sk_destruct().")
Cc: [email protected]
Signed-off-by: Hyunwoo Kim <[email protected]>
---
Changes in v2:
- Clear ->pinet6 and ->ipv6_fl_list right after the inet fields are set
instead of in an else arm of the opt_child_init test, so the child is
already consistent on the put_and_exit path and the existing test is left
untouched.
- Explain why this is not done in tcp_disconnect().
- Add the reproducer and the KASAN reports.
- v1: https://lore.kernel.org/all/antr7RCJAO578ZFW@v4bel/
---
 net/ipv4/tcp_ipv4.c | 8 ++++++++
 net/ipv6/af_inet6.c | 4 ++++
 2 files changed, 12 insertions(+)

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 190c7af4cf923a..302afe8ebcbcc3 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1714,6 +1714,14 @@ struct sock *tcp_v4_syn_recv_sock(const struct sock *sk, struct sk_buff *skb,
 		inet_csk(newsk)->icsk_ext_hdr_len = inet_opt->opt.optlen;
 	atomic_set(&newinet->inet_id, get_random_u16());
 
+#if IS_ENABLED(CONFIG_IPV6)
+	/* Never inherit the listener's ipv6_pinfo; IPV6_ADDRFORM leaves it set
+	 * on an AF_INET socket.  tcp_v6_mapped_child_init() installs our own.
+	 */
+	newinet->pinet6 = NULL;
+	newinet->ipv6_fl_list = NULL;
+#endif
+
 	/* Set ToS of the new socket based upon the value of incoming SYN.
 	 * ECT bits are set later in tcp_init_transfer().
 	 */
diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
index 282912a1199992..68b330f6941d04 100644
--- a/net/ipv6/af_inet6.c
+++ b/net/ipv6/af_inet6.c
@@ -479,6 +479,10 @@ void inet6_cleanup_sock(struct sock *sk)
 	struct sk_buff *skb;
 	struct ipv6_txoptions *opt;
 
+	/* AF_INET child of an IPV6_ADDRFORM'ed listener: nothing of its own. */
+	if (!np)
+		return;
+
 	/* Release rx options */
 
 	skb = xchg(&np->pktoptions, NULL);
-- 
2.43.0