Re: [PATCH net v2] tcp: do not send a SYNACK to a broadcast or multicast address

[netdev-bot+sashiko]

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

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

- [High] The fix is incomplete.

--

tcp: do not send a SYNACK to a broadcast or multicast address

This patch makes inet_csk_route_req() reject SYNACK routes that have
RTCF_BROADCAST or RTCF_MULTICAST set. A SYN with a multicast source that
is looped back with its dst attached skips the martian source check. Its
SYNACK then goes through ip_mc_output(), and sk_mc_loop() reads past the
end of the request socket.

> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index 6a30f11384547..aa928015cd143 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c
> @@ -779,6 +779,9 @@ struct dst_entry *inet_csk_route_req(const struct sock *sk,
>  		goto no_route;
>  	if (opt && opt->opt.is_strictroute && rt->rt_uses_gateway)
>  		goto route_err;
> +	/* Never send a SYNACK to a broadcast or multicast destination. */
> +	if (rt->rt_flags & (RTCF_BROADCAST | RTCF_MULTICAST))
> +		goto route_err;

[Severity: High]
Does this cover the syncookie passive-open path? cookie_v4_check() in
net/ipv4/syncookies.c does its own route lookup to ireq->ir_rmt_addr.
The only check on the result is IS_ERR():

net/ipv4/syncookies.c:cookie_v4_check() {
    ...
	rt = ip_route_output_key(net, &fl4);
	if (IS_ERR(rt)) {
		SKB_DR_SET(reason, IP_OUTNOROUTES);
		goto out_free;
	}
    ...
	ret = tcp_get_cookie_sock(sk, skb, req, &rt->dst);
    ...
}

In the syncookie case, tcp_v4_syn_recv_sock() uses that dst as is.
Neither inet_csk_route_req() nor inet_csk_route_child_sock() runs.

So it looks like a full child socket can still be created with
inet_daddr set to a multicast address such as 224.0.0.1. Active opens
already refuse this in tcp_v4_connect():

	if (rt->rt_flags & (RTCF_MULTICAST | RTCF_BROADCAST)) {
		ip_rt_put(rt);
		return -ENETUNREACH;

The ACK that completes the handshake can come in the way the commit
message describes for the SYN: raw IP_HDRINCL, nft dup or TEE, with
saddr=224.0.0.1. tcp_v4_cookie_check() passes any non-SYN segment to
cookie_v4_check().

The cookie can pass in two ways:

- cookie_tcp_check(), after a recent overflow. A netns owner can set
  tcp_syncookies=2 and send one unicast SYN. After that, ack_seq/seq can
  be brute forced at about 2^-29 per packet.
- cookie_bpf_check(), when a BPF program is in use.

Once that child is closed and in TIME_WAIT, a segment from the peer
would go down this path:

tcp_v4_rcv()
  tcp_v4_timewait_ack()
    tcp_v4_send_ack()
      ip_send_unicast_reply(ctl_sk, tw, ...)
        ip_route_output_flow()   /* daddr 224.0.0.1, no rt_flags check */
        skb_set_owner_edemux(nskb, tw)
        ip_push_pending_frames()
          ip_send_skb()
            ip_local_out(net, skb->sk, ...)
              ip_mc_output()
                sk_mc_loop(tw)

sk_mc_loop() does not check sk_fullsock():

	case AF_INET:
		return inet_test_bit(MC_LOOP, sk);

Wouldn't that read inet_flags past the end of the tcp_timewait_sock
object? That is the same out-of-bounds read this patch fixes for request
sockets. This path comes from reading the code at this commit; it has
not been reproduced.

Could the same rt_flags rejection (ip_rt_put() and drop) go after
ip_route_output_key() in cookie_v4_check()? For symmetry it could also
go in inet_csk_route_child_sock(). In practice that one only runs after
inet_csk_route_req() has passed, so it is just an inconsistency.

Another option would be to change sk_mc_loop() and
ip_send_unicast_reply() so they do not treat non-full sockets as
inet_sock.

>  	rcu_read_unlock();
>  	return &rt->dst;
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008114942.1376889-1-theodorlarionov%40gmail.com