Re: [PATCH net] tcp: ipv6: keep request pktopts alive until request destruction

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 1 · High: 0 (1) · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Critical] This patch adds a remote-triggerable use-after-free on the
  IPv6 syncookie path.

Pre-existing issues:
- [High] The patch fixes the request-migration ownership race for
  ireq->pktopts but leaves the same race open for the other request…

--

tcp: ipv6: keep request pktopts alive until request destruction

This patch removes the code in tcp_v6_syn_recv_sock() that dropped the
request's ireq->pktopts reference when the child was created. It also
makes inet_reqsk_clone() take its own skb reference for migrated requests,
so every request now releases pktopts from tcp_v6_reqsk_destructor(). The
aim is to close a use-after-free where a concurrent SYNACK retransmit
reads ireq->pktopts after it has been freed.

> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index 93b306d91b6dc..66f8ce25aa383 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c
> @@ -936,6 +936,11 @@ static struct request_sock *inet_reqsk_clone(struct request_sock *req,
>  		      req->rsk_ops->obj_size - offsetof(struct sock, sk_dontcopy_end),
>  		      /* alloc is larger than struct, see above */);
>  
> +#if IS_ENABLED(CONFIG_IPV6)
> +	if (inet_rsk(nreq)->pktopts)
> +		skb_get(inet_rsk(nreq)->pktopts);
> +#endif
> +
>  	sk_node_init(&nreq_sk->sk_node);
>  	nreq_sk->sk_tx_queue_mapping = req_sk->sk_tx_queue_mapping;
>  #ifdef CONFIG_SOCK_RX_QUEUE_MAPPING
> @@ -982,7 +987,6 @@ static void reqsk_migrate_reset(struct request_sock *req)
>  	req->saved_syn = NULL;
>  #if IS_ENABLED(CONFIG_IPV6)
>  	inet_rsk(req)->ipv6_opt = NULL;
> -	inet_rsk(req)->pktopts = NULL;
>  #else
>  	inet_rsk(req)->ireq_opt = NULL;
>  #endif

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. The same
migration race still seems to be open for ireq->ipv6_opt, which is the
other request field that tcp_v6_send_synack() reads.

inet_reqsk_clone() copies the ipv6_opt pointer into nreq with memcpy. It
takes no reference and makes no copy. reqsk_migrate_reset(oreq) then
clears the pointer on the old request, which leaves nreq as the only
owner. Both reqsk_timer_handler() and inet_csk_complete_hashdance() go
through this path.

Another CPU may still hold oreq. It found oreq through ehash, migrated it
via reuseport_migrate_sock(), and then got a retransmitted SYN:

tcp_check_req()
  inet_rtx_syn_ack()
    tcp_v6_send_synack()

That CPU can be in:

net/ipv6/tcp_ipv6.c:tcp_v6_send_synack() {
    ...
		rcu_read_lock();
		opt = ireq->ipv6_opt;
		if (!opt)
			opt = rcu_dereference(np->opt);
		err = ip6_xmit(sk, skb, fl6, skb->mark ? : READ_ONCE(sk->sk_mark),
			       opt, tclass, READ_ONCE(sk->sk_priority));
		rcu_read_unlock();
    ...
}

Meanwhile nreq is released (after accept(), on listener close, or on
expiry) and runs:

static void tcp_v6_reqsk_destructor(struct request_sock *req)
{
	kfree(inet_rsk(req)->ipv6_opt);
	consume_skb(inet_rsk(req)->pktopts);
}

This kfree() is not deferred through RCU, so the reader's rcu_read_lock()
does not protect opt. Can ip6_xmit() then read a freed struct
ipv6_txoptions while it pushes the extension headers?

ipv6_opt is only non-NULL with CALIPSO labelling (calipso_req_setattr()).
The race also needs SO_REUSEPORT, net.ipv4.tcp_migrate_req=1 and a
listener that is closing.

This patch already reworks pktopts ownership in inet_reqsk_clone() and
reqsk_migrate_reset(). Should ipv6_opt be handled the same way there?

> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index e006a30360eae..0b51cb4ad9604 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -1522,11 +1522,12 @@ static struct sock *tcp_v6_syn_recv_sock(const struct sock *sk, struct sk_buff *
>  	if (*own_req) {
>  		tcp_move_syn(newtp, req);
>  
> -		/* Clone pktoptions received with SYN, if we own the req */
> +		/* Clone pktoptions received with SYN. Keep the request's
> +		 * reference until the request is destroyed, as a concurrent
> +		 * SYN retransmit may still read it.
> +		 */
>  		if (ireq->pktopts) {
>  			newnp->pktoptions = skb_clone_and_charge_r(ireq->pktopts, newsk);
> -			consume_skb(ireq->pktopts);
> -			ireq->pktopts = NULL;
>  			if (newnp->pktoptions)
>  				tcp_v6_restore_cb(newnp->pktoptions);
>  		}

[Severity: Critical]
Does removing this consume_skb() create a use-after-free on the IPv6
syncookie path?

On that path ireq->pktopts does not hold the SYN. cookie_v6_check()
stores the final ACK skb, which is still being processed, by bumping the
refcount directly:

net/ipv6/syncookies.c:cookie_v6_check() {
    ...
	if (ipv6_opt_accepted(sk, skb, &TCP_SKB_CB(skb)->header.h6) ||
	    np->rxopt.bits.rxinfo || np->rxopt.bits.rxoinfo ||
	    np->rxopt.bits.rxhlim || np->rxopt.bits.rxohlim) {
		refcount_inc(&skb->users);
		ireq->pktopts = skb;
	}
    ...
}

Before this patch, the consume_skb() above dropped skb->users back to 1
before the child processed the skb.

Now tcp_get_cookie_sock() calls inet_csk_reqsk_queue_add(sk, req, child).
The request on the accept queue still points at the in-flight ACK, and
skb->users is 2.

The same skb is then passed to the child:

tcp_v6_do_rcv()
  tcp_child_process(sk, nsk, skb)
    tcp_rcv_state_process()
      tcp_data_queue()

For a pure ACK, tcp_data_queue() frees the skb without checking
skb->users:

	if (TCP_SKB_CB(skb)->seq == TCP_SKB_CB(skb)->end_seq) {
		__kfree_skb(skb);
		return;
	}

The consume label in tcp_rcv_state_process() and tcp_collapse_one() also
use __kfree_skb() in the same way.

When the request is released later, either by reqsk_put() from accept()
or by inet_csk_listen_stop(), the destructor runs:

tcp_v6_reqsk_destructor()
    consume_skb(inet_rsk(req)->pktopts);

Wouldn't this decrement the refcount of, and possibly free again, an skb
that has already been freed?

The trigger needs a remote peer that gets syncookies sent (SYN queue
overflow with the default tcp_syncookies=1, or tcp_syncookies=2) and then
completes the handshake with a pure ACK. The only local requirement is a
common IPv6 rx option on the listener, such as IPV6_RECVPKTINFO or
IPV6_RECVHOPLIMIT.

Syncookie requests are never hashed and never retransmit SYNACKs, so the
race this patch fixes cannot happen for them. Could the cookie path keep
dropping the request's pktopts reference here, or could cookie_v6_check()
store a separate clone instead of the in-flight skb?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/82c31e4d5043f3dbd11b44177cfba779d335aae3.1793599929.git.x-professor%40outlook.com