Re: [PATCH] tcp: fastopen: check rsk_drop_req() in tcp_fastopen_create_child()

[Jiayuan Chen <jiayuan.chen@linux.dev>]

on 9/2/26 8:12 PM, Yilin Zhang wrote:
> subflow_syn_recv_sock() destroys the freshly cloned child for an MP_JOIN
> SYN under the fatal fallback and hands it back with drop_req=true to
> tell the caller to drop both the request and the child. Of the three
> syn_recv_sock() callers, tcp_check_req() and the cookie path honor that
> contract; tcp_fastopen_create_child() only checks child != NULL.
>
> With an MPTCP listener and server-side Fast Open enabled, this can
> become a use-after-free:
>
>    1. fetch a TFO cookie,
>    2. complete a normal MP_CAPABLE handshake to learn the server's key
>       and compute the token offline,
>    3. send an MP_JOIN SYN carrying that token and the valid TFO cookie,
>    4. a subsequent accept() hands the freed socket to userspace, and
>       any fd operation triggers slab-use-after-free, leading to a
>       denial of service.
>
> Fixes: 90bf45134d55 ("mptcp: add new sock flag to deal with join subflows")
> Reported-by: Kimi Security Team <bug-report@moonshot.ai>
> Signed-off-by: Yilin Zhang <yilinzhang@moonshot.ai>
> ---
>   net/ipv4/tcp_fastopen.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
> index 6a031a1a6c9f..7a0b58b39a2c 100644
> --- a/net/ipv4/tcp_fastopen.c
> +++ b/net/ipv4/tcp_fastopen.c
> @@ -337,6 +337,11 @@ static struct sock *tcp_fastopen_create_child(struct sock *sk,
>   	if (!child)
>   		return NULL;
>   
> +	if (own_req && rsk_drop_req(req)) {
> +		sock_put(child);


The child is still locked by inet_csk_clone_lock.


We should do this instead:

diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
index 471c78be5513..22494ff746c7 100644
--- a/net/ipv4/tcp_fastopen.c
+++ b/net/ipv4/tcp_fastopen.c
@@ -337,6 +337,12 @@ static struct sock 
*tcp_fastopen_create_child(struct sock *sk,
         if (!child)
                 return NULL;

+       if (own_req && rsk_drop_req(req)) {
+               bh_unlock_sock(child);
+               sock_put(child);
+               return NULL;
+       }
+
         spin_lock(&queue->fastopenq.lock);
         queue->fastopenq.qlen++;
         spin_unlock(&queue->fastopenq.lock);


> +		return NULL;


tcp_conn_request will still send a SYN-ACK, but the RST was already sent


It's not clean. Maybe we should do something like this instead (untested)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 0f60a1dbf927..d371845d601a 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -7775,6 +7775,10 @@ int tcp_conn_request(struct request_sock_ops 
*rsk_ops,
                 READ_ONCE(sk->sk_data_ready)(sk);
                 bh_unlock_sock(fastopen_sk);
                 sock_put(fastopen_sk);
+       } else if (rsk_drop_req(req)) {
+               reqsk_free(req);
+               dst_release(dst);
+               return 0;
         } else {
                 tcp_rsk(req)->tfo_listener = false;
                 if (!want_cookie &&



BTW, since this is reported by LLM I think it is not difficult to have 
the LLM write a     reproducer and test this patch.