Re: [PATCH net] tcp: reject v4mapped source addresses in IPv6 SYN cookie ACKs

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] The patch closes only the v4mapped trigger.

--

tcp: reject v4mapped source addresses in IPv6 SYN cookie ACKs

This patch makes cookie_v6_check() reject ACKs whose IPv6 source address
is v4mapped, before any child socket is created. It matches the check
already done in tcp_v6_conn_request(). Before this, a SYN cookie child
with a v4mapped peer could be found by tcp_v4_err() and end up caching an
IPv4 rtable, which ip6_xmit() then used as an rt6_info.

> diff --git a/net/ipv6/syncookies.c b/net/ipv6/syncookies.c
> index b581cb1ee2e8a..6c5bb4bbd349c 100644
> --- a/net/ipv6/syncookies.c
> +++ b/net/ipv6/syncookies.c
> @@ -189,6 +189,12 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
>  	    !th->ack || th->rst)
>  		goto out;
>  
> +	if (ipv6_addr_v4mapped(&ipv6_hdr(skb)->saddr)) {
> +		__IP6_INC_STATS(net, NULL, IPSTATS_MIB_INHDRERRORS);
> +		SKB_DR_SET(reason, IP_INHDR);
> +		goto out_drop;
> +	}
> +
>  	if (cookie_bpf_ok(skb)) {
>  		req = cookie_bpf_check(sk, skb);
>  	} else {

[Severity: Medium]
This closes the v4mapped case. The commit message also names the missing
IPv4 address initialization as part of the chain:

  "cookie_v6_check() initializes the IPv6 address fields of the request
  socket, but not its IPv4 address fields."

Is that still a problem for native IPv6 SYN cookie connections after this
patch? Further down in cookie_v6_check(), only the IPv6 fields are
written:

	ireq->ir_v6_rmt_addr = ipv6_hdr(skb)->saddr;
	ireq->ir_v6_loc_addr = ipv6_hdr(skb)->daddr;

The normal SYN path sets the IPv4 fields in tcp_v6_init_req():

	ireq->ir_rmt_addr = LOOPBACK4_IPV6;
	ireq->ir_loc_addr = LOOPBACK4_IPV6;

On the cookie path, the request socket comes from a slab allocation that
is not zeroed:

cookie_tcp_check()
  cookie_tcp_reqsk_alloc()
    inet_reqsk_alloc()
      reqsk_alloc_noprof()
        kmem_cache_alloc_noprof(ops->slab, GFP_ATOMIC | __GFP_NOWARN)

Neither inet_reqsk_alloc() nor cookie_tcp_reqsk_init() sets skc_daddr or
skc_rcv_saddr. inet_csk_clone_lock() then copies those fields into the
child:

	newsk->sk_daddr = ireq->ir_rmt_addr;
	newsk->sk_rcv_saddr = ireq->ir_loc_addr;
	newinet->inet_saddr = ireq->ir_loc_addr;

Since commit a3a128f611a965 ("inet: consolidate inet_csk_clone_lock()"),
tcp_v6_syn_recv_sock() no longer writes LOOPBACK4_IPV6 into the child.
So it looks like leftover slab bytes stay in inet_daddr, inet_rcv_saddr
and inet_saddr of every AF_INET6 child created from a SYN cookie.

Both ipv6_update_options() and do_ip_setsockopt(IP_OPTIONS) branch on
that value:

	inet_sk(sk)->inet_daddr != LOOPBACK4_IPV6

Could this make icsk_ext_hdr_len and the MSS different for a SYN cookie
child than for a normally accepted IPv6 child? Could it also trigger KMSAN
uninit-value reports? The leftover bytes would also show up in the
inet_sock_set_state tracepoint and the BPF ip4 context fields.

Would it make sense to also set ir_rmt_addr and ir_loc_addr to
LOOPBACK4_IPV6 in cookie_v6_check(), as tcp_v6_init_req() does? That
would be in addition to the v4mapped rejection, not a replacement for it.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/avFvW7nn7GIZmZ-a%40v4bel