[PATCH net v2 6/8] tcp: fix use-after-free in the lockless listener path
Hyunwoo Kim <[email protected]>
| Newsgroups | org.kernel.vger.linux-kernel,org.kernel.vger.netdev,org.kernel.vger.stable |
|---|---|
| Message-ID | <[email protected]> |
tcp_v{4,6}_rcv() calls tcp_v{4,6}_do_rcv() without holding the socket
lock when sk->sk_state is TCP_LISTEN. Every other path into
tcp_v{4,6}_do_rcv() holds it.
tcp_v{4,6}_do_rcv() and tcp_rcv_state_process() below it read
sk->sk_state again. A listener can leave TCP_LISTEN through
connect(AF_UNSPEC), and if that happens in between, the second read
returns a different state.
tcp_rcv_established() or tcp_rcv_state_process() then runs without the
lock. If the second read returns TCP_SYN_SENT, the incoming SYN is
treated as a crossed SYN and reaches tcp_send_synack(). When the SYN skb
at the head of the retransmit queue is skb_cloned(), that function
replaces it with a copy and releases the original with
tcp_rtx_queue_unlink_and_free().
The original is the skb that a thread on another CPU is transmitting
right now in __tcp_transmit_skb(). skb_cloned() is true because the
clone made for that transmit is still alive. Once the transmit returns,
tcp_update_skb_after_send() calls list_move_tail() on the skb's
tcp_tsorted_anchor.
In short:
socket(AF_INET) -> bind() -> listen() // the socket that changes state
socket(AF_INET) -> bind() -> listen() // the peer
Several threads keep opening new sockets and connecting to the first
socket's address.
Another thread repeats this on the first socket:
connect(AF_UNSPEC) // TCP_LISTEN -> TCP_CLOSE
connect(peer address) // TCP_CLOSE -> TCP_SYN_SENT
// another CPU still sees a listener, handles
// one of those SYNs without the lock and
// releases the SYN skb that this connect()
// is transmitting
// -> use-after-free
connect(AF_UNSPEC)
listen() // TCP_LISTEN again
KASAN log:
BUG: KASAN: slab-use-after-free in __list_del_entry_valid_or_report+0x14/0x140
Read of size 8 at addr ffff88800a5d1460 by task poc/125
...
Call Trace:
__list_del_entry_valid_or_report+0x14/0x140
tcp_update_skb_after_send+0x62/0x170
__tcp_transmit_skb+0xe33/0x1e40
tcp_connect+0x1b67/0x2490
tcp_v4_connect+0x998/0xab0
__inet_stream_connect+0x22c/0x700
inet_stream_connect+0x48/0x70
__sys_connect+0x101/0x130
...
Allocated by task 125:
__alloc_skb+0xd1/0x370
tcp_stream_alloc_skb+0x2d/0x2b0
tcp_connect+0x72d/0x2490
tcp_v4_connect+0x998/0xab0
__inet_stream_connect+0x22c/0x700
inet_stream_connect+0x48/0x70
__sys_connect+0x101/0x130
...
The buggy address belongs to the object at ffff88800a5d1400
which belongs to the cache skbuff_fclone_cache of size 472
Instead of taking the lock, keep the lockless path from reading
sk->sk_state again to decide how to process the packet. Move the
TCP_LISTEN handling out of tcp_rcv_state_process() into
tcp_rcv_listen_state_process(), and let the TCP_LISTEN branch of
tcp_v{4,6}_rcv() call a new tcp_v{4,6}_rcv_listen(). Listener processing
does not change. The TCP_LISTEN arm of tcp_v{4,6}_do_rcv() is left
alone, because a socket can finish listen() after the state check and a
backlogged skb is then processed there.
Fixes: e994b2f0fb92 ("tcp: do not lock listener to process SYN packets")
Cc: [email protected]
Signed-off-by: Hyunwoo Kim <[email protected]>
---
include/net/tcp.h | 2 ++
net/ipv4/tcp_input.c | 64 +++++++++++++++++++++++++-------------------
net/ipv4/tcp_ipv4.c | 44 ++++++++++++++++++++++++++++--
net/ipv6/tcp_ipv6.c | 44 ++++++++++++++++++++++++++++--
4 files changed, 123 insertions(+), 31 deletions(-)
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 2c5b889530b556..add438d6561be5 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -392,6 +392,8 @@ void tcp_write_timer_handler(struct sock *sk);
void tcp_delack_timer_handler(struct sock *sk);
int tcp_ioctl(struct sock *sk, int cmd, int *karg);
enum skb_drop_reason tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb);
+enum skb_drop_reason tcp_rcv_listen_state_process(struct sock *sk,
+ struct sk_buff *skb);
void tcp_rcv_established(struct sock *sk, struct sk_buff *skb);
void tcp_rcvbuf_grow(struct sock *sk, u32 newval);
void tcp_rcv_space_adjust(struct sock *sk);
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 0f60a1dbf92746..77af18fba66b4d 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -7141,6 +7141,42 @@ static void tcp_rcv_synrecv_state_fastopen(struct sock *sk)
tcp_rearm_rto(sk);
}
+enum skb_drop_reason tcp_rcv_listen_state_process(struct sock *sk,
+ struct sk_buff *skb)
+{
+ const struct tcphdr *th = tcp_hdr(skb);
+ SKB_DR(reason);
+
+ if (th->ack)
+ return SKB_DROP_REASON_TCP_FLAGS;
+
+ if (th->rst) {
+ SKB_DR_SET(reason, TCP_RESET);
+ goto discard;
+ }
+ if (th->syn) {
+ if (th->fin) {
+ SKB_DR_SET(reason, TCP_FLAGS);
+ goto discard;
+ }
+ /* It is possible that we process SYN packets from backlog,
+ * so we need to make sure to disable BH and RCU right there.
+ */
+ rcu_read_lock();
+ local_bh_disable();
+ inet_csk(sk)->icsk_af_ops->conn_request(sk, skb);
+ local_bh_enable();
+ rcu_read_unlock();
+
+ consume_skb(skb);
+ return 0;
+ }
+ SKB_DR_SET(reason, TCP_FLAGS);
+discard:
+ tcp_drop_reason(sk, skb, reason);
+ return 0;
+}
+
/*
* This function implements the receiving procedure of RFC 793 for
* all states except ESTABLISHED and TIME_WAIT.
@@ -7152,7 +7188,6 @@ enum skb_drop_reason
tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb)
{
struct tcp_sock *tp = tcp_sk(sk);
- struct inet_connection_sock *icsk = inet_csk(sk);
const struct tcphdr *th = tcp_hdr(skb);
struct request_sock *req;
int queued = 0;
@@ -7164,32 +7199,7 @@ tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb)
goto discard;
case TCP_LISTEN:
- if (th->ack)
- return SKB_DROP_REASON_TCP_FLAGS;
-
- if (th->rst) {
- SKB_DR_SET(reason, TCP_RESET);
- goto discard;
- }
- if (th->syn) {
- if (th->fin) {
- SKB_DR_SET(reason, TCP_FLAGS);
- goto discard;
- }
- /* It is possible that we process SYN packets from backlog,
- * so we need to make sure to disable BH and RCU right there.
- */
- rcu_read_lock();
- local_bh_disable();
- icsk->icsk_af_ops->conn_request(sk, skb);
- local_bh_enable();
- rcu_read_unlock();
-
- consume_skb(skb);
- return 0;
- }
- SKB_DR_SET(reason, TCP_FLAGS);
- goto discard;
+ return tcp_rcv_listen_state_process(sk, skb);
case TCP_SYN_SENT:
tp->rx_opt.saw_tstamp = 0;
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 302afe8ebcbcc3..2fa8958380a5b9 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1828,7 +1828,7 @@ u16 tcp_v4_get_syncookie(struct sock *sk, struct iphdr *iph,
INDIRECT_CALLABLE_DECLARE(struct dst_entry *ipv4_dst_check(struct dst_entry *,
u32));
/* The socket must have it's spinlock held when we get
- * here, unless it is a TCP_LISTEN socket.
+ * here.
*
* We have a potential double-lock case here, so even when
* doing backlog processing we use the BH locking scheme.
@@ -1906,6 +1906,46 @@ int tcp_v4_do_rcv(struct sock *sk, struct sk_buff *skb)
goto discard;
}
+/* @sk is not locked here and can leave TCP_LISTEN; do not test sk_state. */
+static noinline int tcp_v4_rcv_listen(struct sock *sk, struct sk_buff *skb)
+{
+ enum skb_drop_reason reason;
+ struct sock *nsk;
+
+ reason = psp_sk_rx_policy_check(sk, skb);
+ if (reason)
+ goto err_discard;
+
+ if (tcp_checksum_complete(skb))
+ goto csum_err;
+
+ nsk = tcp_v4_cookie_check(sk, skb);
+ if (!nsk)
+ return 0;
+
+ if (nsk != sk) {
+ reason = tcp_child_process(sk, nsk, skb);
+ sock_put(nsk);
+ } else {
+ reason = tcp_rcv_listen_state_process(sk, skb);
+ }
+ if (!reason)
+ return 0;
+
+ tcp_v4_send_reset(sk, skb, sk_rst_convert_drop_reason(reason));
+discard:
+ sk_skb_reason_drop(sk, skb, reason);
+ return 0;
+
+csum_err:
+ reason = SKB_DROP_REASON_TCP_CSUM;
+ trace_tcp_bad_csum(skb);
+ TCP_INC_STATS(sock_net(sk), TCP_MIB_CSUMERRORS);
+err_discard:
+ TCP_INC_STATS(sock_net(sk), TCP_MIB_INERRS);
+ goto discard;
+}
+
enum skb_drop_reason tcp_add_backlog(struct sock *sk, struct sk_buff *skb)
{
u32 tail_gso_size, tail_gso_segs;
@@ -2243,7 +2283,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
skb->dev = NULL;
if (sk->sk_state == TCP_LISTEN) {
- ret = tcp_v4_do_rcv(sk, skb);
+ ret = tcp_v4_rcv_listen(sk, skb);
goto put_and_return;
}
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 9e9155b1b3aa75..a2deda9a4258bc 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1556,7 +1556,7 @@ static struct sock *tcp_v6_syn_recv_sock(const struct sock *sk, struct sk_buff *
INDIRECT_CALLABLE_DECLARE(struct dst_entry *ipv4_dst_check(struct dst_entry *,
u32));
/* The socket must have it's spinlock held when we get
- * here, unless it is a TCP_LISTEN socket.
+ * here.
*
* We have a potential double-lock case here, so even when
* doing backlog processing we use the BH locking scheme.
@@ -1704,6 +1704,46 @@ int tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb)
return 0;
}
+/* @sk is not locked here and can leave TCP_LISTEN; do not test sk_state. */
+static noinline int tcp_v6_rcv_listen(struct sock *sk, struct sk_buff *skb)
+{
+ enum skb_drop_reason reason;
+ struct sock *nsk;
+
+ reason = psp_sk_rx_policy_check(sk, skb);
+ if (reason)
+ goto err_discard;
+
+ if (tcp_checksum_complete(skb))
+ goto csum_err;
+
+ nsk = tcp_v6_cookie_check(sk, skb);
+ if (!nsk)
+ return 0;
+
+ if (nsk != sk) {
+ reason = tcp_child_process(sk, nsk, skb);
+ sock_put(nsk);
+ } else {
+ reason = tcp_rcv_listen_state_process(sk, skb);
+ }
+ if (!reason)
+ return 0;
+
+ tcp_v6_send_reset(sk, skb, sk_rst_convert_drop_reason(reason));
+discard:
+ sk_skb_reason_drop(sk, skb, reason);
+ return 0;
+
+csum_err:
+ reason = SKB_DROP_REASON_TCP_CSUM;
+ trace_tcp_bad_csum(skb);
+ TCP_INC_STATS(sock_net(sk), TCP_MIB_CSUMERRORS);
+err_discard:
+ TCP_INC_STATS(sock_net(sk), TCP_MIB_INERRS);
+ goto discard;
+}
+
static void tcp_v6_fill_cb(struct sk_buff *skb, const struct ipv6hdr *hdr,
const struct tcphdr *th)
{
@@ -1891,7 +1931,7 @@ INDIRECT_CALLABLE_SCOPE int tcp_v6_rcv(struct sk_buff *skb)
skb->dev = NULL;
if (sk->sk_state == TCP_LISTEN) {
- ret = tcp_v6_do_rcv(sk, skb);
+ ret = tcp_v6_rcv_listen(sk, skb);
goto put_and_return;
}
--
2.43.0