[PATCH net 0/3] net: fixes for requests completing on a socket that no longer listens

[PATCH net 0/3] net: fixes for requests completing on a socket that no longer listens

[Hyunwoo Kim]

connect(AF_UNSPEC) followed by connect() turns a listener into an active
session, and IPV6_ADDRFORM on top of that turns it into an AF_INET socket.
The requests that pointed at that listener are still in the ehash, and
tcp_check_req() completes them without holding the listener lock.

All three patches fix a consequence of the same thing, a request
completing against a socket that is no longer a listener. The request is
released twice, or the child is allocated with the wrong size, or the
child inherits state the parent picked up in the meantime.

Hyunwoo Kim (3):
  ipv6: fix request socket use-after-free after IPV6_ADDRFORM
  net: fix out-of-bounds write in sk_clone() racing with IPV6_ADDRFORM
  tcp: do not inherit out_of_order_queue from parent

 net/core/sock.c          | 2 +-
 net/ipv4/tcp_minisocks.c | 1 +
 net/ipv6/ipv6_sockglue.c | 4 ++++
 3 files changed, 6 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH net 1/3] ipv6: fix request socket use-after-free after IPV6_ADDRFORM

[Hyunwoo Kim]

IPV6_ADDRFORM turns an AF_INET6 TCP socket into an AF_INET one. It requires
the socket to be established, and a listener can get there with
connect(AF_UNSPEC) followed by connect(). Request sockets queued while it
was listening are still there: inet_csk_listen_stop() leaves them in the
ehash, and their timers only drop them while the socket is not listening,
so making it listen again keeps them alive.

A request that arrived over IPv6 was hashed with inet6_ehashfn(). Its child
is cloned from the converted socket and hashed with inet_ehashfn(), so it
belongs in a different bucket.

inet_ehash_insert() locks the child's bucket, warns about the mismatching
hashes, and replaces the request with the child in the request's own bucket
anyway. reqsk_queue_unlink() locks the bucket the request is really in, so
there is no synchronization between the two. Both can see the request still
hashed and both can drop the reference the ehash holds.

The extra put takes the request's refcount to zero too early, so it is
freed while it is still on the listener's accept queue. The listener is
then closed, and inet_csk_listen_stop() reads the freed request and
writes to it in reqsk_put().

Refuse the conversion if inet_csk_reqsk_queue_len() is not zero. Nothing
clears that counter when a socket stops listening or listens again, so it
still accounts for the requests left in the ehash. A socket that never
listened is not affected.

Fixes: 079096f103fa ("tcp/dccp: install syn_recv requests into ehash table")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
---
 net/ipv6/ipv6_sockglue.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
index b4c977434c2e0a..64fc6127e75332 100644
--- a/net/ipv6/ipv6_sockglue.c
+++ b/net/ipv6/ipv6_sockglue.c
@@ -572,6 +572,10 @@ int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
 					retv = -EBUSY;
 					break;
 				}
+				if (inet_csk_reqsk_queue_len(sk)) {
+					retv = -EBUSY;
+					break;
+				}
 			} else {
 				break;
 			}
-- 
2.43.0

Re: [PATCH net 1/3] ipv6: fix request socket use-after-free after IPV6_ADDRFORM

[Jiayuan Chen]

On 8/17/26 5:03 PM, Hyunwoo Kim wrote:
> IPV6_ADDRFORM turns an AF_INET6 TCP socket into an AF_INET one. It requires
> the socket to be established, and a listener can get there with
> connect(AF_UNSPEC) followed by connect(). Request sockets queued while it
> was listening are still there: inet_csk_listen_stop() leaves them in the
> ehash, and their timers only drop them while the socket is not listening,
> so making it listen again keeps them alive.
>
> A request that arrived over IPv6 was hashed with inet6_ehashfn(). Its child
> is cloned from the converted socket and hashed with inet_ehashfn(), so it
> belongs in a different bucket.
>
> inet_ehash_insert() locks the child's bucket, warns about the mismatching
> hashes, and replaces the request with the child in the request's own bucket
> anyway. reqsk_queue_unlink() locks the bucket the request is really in, so
> there is no synchronization between the two. Both can see the request still
> hashed and both can drop the reference the ehash holds.
>
> The extra put takes the request's refcount to zero too early, so it is
> freed while it is still on the listener's accept queue. The listener is
> then closed, and inet_csk_listen_stop() reads the freed request and
> writes to it in reqsk_put().
>
> Refuse the conversion if inet_csk_reqsk_queue_len() is not zero. Nothing
> clears that counter when a socket stops listening or listens again, so it
> still accounts for the requests left in the ehash. A socket that never
> listened is not affected.
>
> Fixes: 079096f103fa ("tcp/dccp: install syn_recv requests into ehash table")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
> ---
>   net/ipv6/ipv6_sockglue.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
> index b4c977434c2e0a..64fc6127e75332 100644
> --- a/net/ipv6/ipv6_sockglue.c
> +++ b/net/ipv6/ipv6_sockglue.c
> @@ -572,6 +572,10 @@ int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
>   					retv = -EBUSY;
>   					break;
>   				}
> +				if (inet_csk_reqsk_queue_len(sk)) {
> +					retv = -EBUSY;
> +					break;
> +				}
>   			} else {
>   				break;
>   			}

Just thinking out loud.

Gating on inet_csk_reqsk_queue_len() reads a bit oddly, since we already 
require TCP_ESTABLISHED right above.

it's really just a proxy for "this socket used to listen and still has 
leftover requests.

The real issue is we should drain req when disconnect the listen socket, 
at least we should avoid replaces the request with the child even when 
sk->sk_hash != osk->sk_hash.

But that's the hot path, so hardening it for such a rare corner case 
isn't worth the cost.

[PATCH net 2/3] net: fix out-of-bounds write in sk_clone() racing with IPV6_ADDRFORM

[Hyunwoo Kim]

sk_clone() allocates the child from sk->sk_prot, and IPV6_ADDRFORM can
change sk_prot under it. The conversion requires the socket to be
established, and a listener gets there with connect(AF_UNSPEC) followed
by connect().

tcp_check_req() completes a request without the listener lock, so it can
run while the conversion is in progress. IPV6_ADDRFORM stores sk_prot
before icsk_af_ops, so tcp_check_req() can still call
tcp_v6_syn_recv_sock() once sk_prot is tcp_prot. The child then comes
from tcp_prot's slab while the AF_INET6 code treats it as a tcp6_sock.

tcp_inet6_sk() is a fixed offset into tcp6_sock, and in a child sized by
tcp_prot that offset is the end of the object. The ipv6_pinfo copy is
therefore a slab out-of-bounds write of sizeof(struct ipv6_pinfo) bytes
past the child.

The out-of-bounds address is also stored in the child's pinet6, so
everything that reaches the socket through inet6_sk() keeps writing
there. A request that arrived over IPv4 takes the same copy in
tcp_v6_mapped_child_init().

Checking sk_prot before the clone does not help. It can change between
that check and the read inside sk_clone(). Use sk_prot_creator instead.
It is set once in sk_alloc() and never changes, and the socket is
already freed back through it. No caller that replaces sk_prot installs
a proto with a larger obj_size than the creator, so the child gets the
size the parent object actually has.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
---
 net/core/sock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/sock.c b/net/core/sock.c
index 1ad41904db25b4..098e58b40f304b 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2479,7 +2479,7 @@ static void sk_init_common(struct sock *sk)
 struct sock *sk_clone(const struct sock *sk, const gfp_t priority,
 		      bool lock)
 {
-	struct proto *prot = READ_ONCE(sk->sk_prot);
+	struct proto *prot = sk->sk_prot_creator;
 	struct sk_filter *filter;
 	bool is_charged = true;
 	struct sock *newsk;
-- 
2.43.0

[PATCH net 3/3] tcp: do not inherit out_of_order_queue from parent

[Hyunwoo Kim]

A child gets a copy of the parent's out_of_order_queue, which can be non
empty when/if parent morphs from listener to active session. Parent and
child then point at the same rbtree.

The parent is no longer a listener, so inet_csk_reqsk_queue_add() forgets
the child immediately, and tcp_disconnect() frees the skbs the parent
still owns. The parent's own root and ooo_last_skb are left alone, so it
keeps using those skbs. That is a use-after-free, and the parent frees
them a second time when it closes.

We need to make sure this can not happen, by initializing the queue after
socket cloning.

Very similar to commit 8b485ce69876 ("tcp: do not inherit fastopen_req
from parent")

Fixes: 9f5afeae5152 ("tcp: use an RB tree for ooo receive queue")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
---
 net/ipv4/tcp_minisocks.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index 6ab3e3a0b43173..d13813d50947dd 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -591,6 +591,7 @@ struct sock *tcp_create_openreq_child(const struct sock *sk,
 	newtp->total_retrans = req->num_retrans;
 
 	tcp_init_xmit_timers(newsk);
+	newtp->out_of_order_queue = RB_ROOT;
 	WRITE_ONCE(newtp->write_seq, newtp->pushed_seq = treq->snt_isn + 1);
 
 	if (sock_flag(newsk, SOCK_KEEPOPEN))
-- 
2.43.0

Re: [PATCH net 3/3] tcp: do not inherit out_of_order_queue from parent

[Jiayuan Chen]

On 8/17/26 5:03 PM, Hyunwoo Kim wrote:
> A child gets a copy of the parent's out_of_order_queue, which can be non
> empty when/if parent morphs from listener to active session. Parent and
> child then point at the same rbtree.
>
> The parent is no longer a listener, so inet_csk_reqsk_queue_add() forgets
> the child immediately, and tcp_disconnect() frees the skbs the parent
> still owns. The parent's own root and ooo_last_skb are left alone, so it
> keeps using those skbs. That is a use-after-free, and the parent frees
> them a second time when it closes.
>
> We need to make sure this can not happen, by initializing the queue after
> socket cloning.
>
> Very similar to commit 8b485ce69876 ("tcp: do not inherit fastopen_req
> from parent")
>
> Fixes: 9f5afeae5152 ("tcp: use an RB tree for ooo receive queue")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
> ---
>   net/ipv4/tcp_minisocks.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
> index 6ab3e3a0b43173..d13813d50947dd 100644
> --- a/net/ipv4/tcp_minisocks.c
> +++ b/net/ipv4/tcp_minisocks.c
> @@ -591,6 +591,7 @@ struct sock *tcp_create_openreq_child(const struct sock *sk,
>   	newtp->total_retrans = req->num_retrans;
>   
>   	tcp_init_xmit_timers(newsk);
> +	newtp->out_of_order_queue = RB_ROOT;


Does tcp_rtx_queue suffer from the same issue?


>   	WRITE_ONCE(newtp->write_seq, newtp->pushed_seq = treq->snt_isn + 1);
>   
>   	if (sock_flag(newsk, SOCK_KEEPOPEN))