Re: [PATCH net v3 2/3] net/ipv6: don't route packets with unknown source address

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 5 · Low: 1

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] ip6_is_local_saddr() in net/ipv6/ip6_output.c declares `struct
  fib6_result fib_result;` without initializing it and passes it to…
- [Medium] The new source-address check lives only in
  ip6_dst_lookup_tail() and is not part of the core IPv6 output route…
- [Medium] The ip6_is_local_saddr() fallback that is supposed to keep
  AnyIP working calls fib6_lookup(net, 0, &fl6, ...) on a freshly zeroed…
- [Medium] net/sctp/ipv6.c:sctp_v6_get_dst() builds its own flowi6
  (memset to zero) and calls ip6_dst_lookup_flow(), but never sets…
- [Medium] The new source check runs only in ip6_dst_lookup_tail(), and
  ip6_sk_dst_lookup_flow() returns the socket's cached dst before…
- [Medium] The new source check runs before the socket takes its route-
  cache cookie, and nothing orders the two against address deletion.
- [Low] The new non-local source check in ip6_dst_lookup_tail() runs
  before the existing IPv4-mapped source check (ip6_output.c:1275-1279,…

--

net/ipv6: don't route packets with unknown source address

This patch adds a source address check to ip6_dst_lookup_tail(). Flows
whose saddr is not configured on the host are rejected with -ENETUNREACH
unless FLOWI_FLAG_ANYSRC is set, and ANYSRC is now set in the IPv6
places where IPv4 sets it. A FIB lookup fallback in ip6_is_local_saddr()
is meant to keep loopback subnets (AnyIP) working.

> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index 25fe0ba98b1a6..8715cf43f00ac 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -1134,6 +1134,33 @@ static struct dst_entry *ip6_sk_dst_check(struct sock *sk,
>  	return dst;
>  }
>  
> +/* Check if the address belongs to the host */
> +static bool ip6_is_local_saddr(struct net *net, struct in6_addr *saddr)
> +{
> +	struct fib6_result fib_result;

[Severity: High]
Should fib_result be zero-initialized here? The other fib6_lookup()
callers use "struct fib6_result res = {};".

If net->ipv6.fib6_has_custom_rules is set (any ip -6 rule, VRF,
wg-quick and so on), fib6_lookup() goes through fib_rules_lookup(). That
calls ops->suppress after each successful rule action:

fib_rules_lookup()
  fib6_rule_action()
    fib6_rule_action_alt()    /* returns 0, never writes res->rt6 */
  fib6_rule_suppress()

net/ipv6/fib6_rules.c:fib6_rule_suppress() {
	struct fib6_result *res = arg->result;
	struct rt6_info *rt = res->rt6;
	...
	if (!rt)
		return false;

	if (rt->rt6i_idev)
		dev = rt->rt6i_idev->dev;
	...
	if (rt->rt6i_dst.plen <= rule->suppress_prefixlen)
		goto suppress_route;
	...
suppress_route:
	ip6_rt_put_flags(rt, flags);
}

Only __fib6_rule_action() writes res->rt6. Here, whatever was left on
the stack would be dereferenced as an rt6_info pointer. If a rule has
suppress_prefixlen set, that pointer could also be released as a dst.

Any flow without ANYSRC and with a non-configured saddr reaches this
path, for example:

- a socket using IPV6_FREEBIND
- an established TCP socket whose address's valid_lft has expired
- IPV6_PKTINFO with nonlocal bind

CONFIG_INIT_STACK_ALL_ZERO builds would hide the problem.

> +	struct flowi6 fl6;
> +	bool result;
> +
> +	if (ipv6_chk_addr_namespace(net, saddr) ||
> +	    ipv6_chk_acast_addr_src(net, NULL, saddr))
> +		return true;
> +
> +	/* Fallback to FIB local routes so that loopback subnets work. */
> +	memset(&fl6, 0, sizeof(fl6));
> +	fl6.daddr = *saddr;

[Severity: Medium]
Can this fallback find AnyIP local routes that live in a VRF table or in
a policy-routed table?

Only daddr is set here. The caller's flowi6_l3mdev, oif, iif and mark
are all dropped, even though ip6_dst_lookup_tail() has the original fl6
available and passes only &fl6->saddr.

With the VRF l3mdev rule installed, fib6_lookup()->l3mdev_update_flow()
finds no l3mdev because oif and iif are 0. The rule then never matches:

net/l3mdev/l3mdev.c:l3mdev_fib_rule_match() {
	...
	if (!fl->flowi_l3mdev)
		return 0;
	...
}

Tables selected by fwmark, iif or oif rules are skipped the same way.

For example, with "ip -6 route add local 2001:db8::/64 dev lo table
<vrf-table>", a socket in that VRF that replies from 2001:db8::5 would
now get -ENETUNREACH. The commit message presents AnyIP as handled, but
this seems to work only for local routes in the local or main tables.

> +	result = false;
> +
> +	rcu_read_lock();
> +	if (fib6_lookup(net, 0, &fl6, &fib_result, 0) == 0 &&
> +	    fib_result.fib6_type == RTN_LOCAL &&
> +	    fib_result.nh->nh_common.nhc_dev) {
> +		result = true;
> +	}
> +	rcu_read_unlock();
> +
> +	return result;
> +}
> +
>  static int ip6_dst_lookup_tail(struct net *net, const struct sock *sk,
>  			       struct dst_entry **dst, struct flowi6 *fl6)
>  {
> @@ -1191,6 +1218,17 @@ static int ip6_dst_lookup_tail(struct net *net, const struct sock *sk,
>  	if (err)
>  		goto out_err_release;
>  
> +	/* From RFC 4862, "an invalid address MUST NOT be used as the source
> +	 * address of outbound packets". Exceptions to this must set the ANYSRC
> +	 * flag (i.e. transparent sockets).
> +	 */
> +	if (!ipv6_addr_any(&fl6->saddr) &&
> +	    !(fl6->flowi6_flags & FLOWI_FLAG_ANYSRC) &&
> +	    !ip6_is_local_saddr(net, &fl6->saddr)) {
> +		err = -ENETUNREACH;
> +		goto out_err_release;
> +	}
> +

[Severity: Low]
Does this new check shadow the existing v4-mapped check further down in
ip6_dst_lookup_tail(), which came from commits ec5e3b0a1d41 and
00ea1ceebe0d?

	if (ipv6_addr_v4mapped(&fl6->saddr) &&
	    !(ipv6_addr_v4mapped(&fl6->daddr) || ipv6_addr_any(&fl6->daddr))) {
		err = -EAFNOSUPPORT;
		goto out_err_release;
	}

ip6_is_local_saddr() always returns false for ::ffff:a.b.c.d. So any
flow without ANYSRC that has a v4-mapped saddr now gets -ENETUNREACH,
and IPSTATS_MIB_OUTNOROUTES is incremented even though a route was
found.

For example, a dual-stack UDP socket bound to ::ffff:127.0.0.1 that
calls sendto() to a native IPv6 peer used to get EAFNOSUPPORT and now
gets ENETUNREACH. A v4-mapped saddr with a v4-mapped or :: daddr, which
used to be allowed, would now be rejected too.

[Severity: Medium]
Is SCTP missing an inet_sk_flowi_flags() update like the other
protocols got in this patch?

sctp_v6_get_dst() in net/sctp/ipv6.c builds its own flow and never sets
flowi6_flags:

net/sctp/ipv6.c:sctp_v6_get_dst() {
	...
	memset(&_fl, 0, sizeof(_fl));
	fl6->daddr = daddr->v6.sin6_addr;
	...
	dst = ip6_dst_lookup_flow(sock_net(sk), sk, fl6, final_p);
	...
		bdst = ip6_dst_lookup_flow(sock_net(sk), sk, fl6, final_p);
	...
}

SCTP does allow binding to non-local addresses:

net/sctp/ipv6.c:sctp_v6_available() {
	...
	res = ipv6_can_nonlocal_bind(net, &sp->inet) ||
	      ipv6_chk_addr(net, in6, dev, 0);
	...
}

Take an SCTPv6 socket with IPV6_TRANSPARENT that is bound to a
non-local address. Both the first lookup and each bind-address lookup in
the loop would now fail with -ENETUNREACH, leaving t->dst NULL.

IPv4 SCTP (sctp_v4_get_dst()) does not set ANYSRC either, so this
matches IPv4. It is still new breakage for IPv6 SCTP TPROXY users.

[Severity: Medium]
Can the socket dst cache bypass this check?

ip6_sk_dst_lookup_flow() returns the cached dst before it reaches
ip6_dst_lookup_tail():

net/ipv6/ip6_output.c:ip6_sk_dst_lookup_flow() {
	struct dst_entry *dst = sk_dst_check(sk, inet6_sk(sk)->dst_cookie);

	dst = ip6_sk_dst_check(sk, dst, fl6);
	if (dst)
		return dst;
	...
}

ip6_sk_dst_check() compares fl6->saddr only under
CONFIG_IPV6_SUBTREES. Otherwise it checks just daddr and oif.

Consider a connected UDPv6 socket with IPV6_FREEBIND, or a host with
ip_nonlocal_bind=1. ip6_datagram_send_ctl() copies any IPV6_PKTINFO
ipi6_addr into fl6->saddr, because ipv6_can_nonlocal_bind() is true.

udpv6_sendmsg() clears connected when cmsgs are present, but that only
stops the dst from being stored. The dst cached by connect() is still
returned, and the packet goes out with an arbitrary non-local source.
The same send without a cached dst would be rejected with -ENETUNREACH.

IPv4 udp_sendmsg() clears connected before sk_dst_check() when cmsgs
are present, so its core check always runs.

[Severity: Medium]
Is there a window where an address deletion is missed and the stale dst
then stays cached?

The check here runs before the socket samples its route cookie in
ip6_dst_store(), and nothing orders the two against ipv6_del_addr():

CPU0 (socket lock, e.g. inet6_csk_route_socket())
ip6_dst_lookup_tail()
  ip6_is_local_saddr()            /* address still found */

CPU1 (RTNL)
ipv6_del_addr()
  hlist_del_init_rcu(&ifp->addr_lst)
  __ipv6_ifa_notify(RTM_DELADDR)
    rt_genid_bump_ipv6(net)       /* flushes fn_sernum */

CPU0
ip6_dst_store()
  rt6_get_cookie()
    fib6_get_cookie_safe()        /* reads the post-flush sernum */

For routes without a nexthop object (rt->sernum == 0), the cached dst
now carries a current cookie. Later calls through __sk_dst_check() ->
ip6_dst_check() accept it, and ip6_dst_lookup_tail() is not run again
for that socket. Packets would keep going out with the removed source
until some unrelated routing change flushes the trees again. Routes that
use nexthop objects are not affected.

>  #ifdef CONFIG_IPV6_OPTIMISTIC_DAD
>  	/*
>  	 * Here if the dst entry we've looked up

[ ... ]

> diff --git a/net/ipv6/netfilter.c b/net/ipv6/netfilter.c
> index a7025ec870359..716fc6e246de9 100644
> --- a/net/ipv6/netfilter.c
> +++ b/net/ipv6/netfilter.c

[ ... ]

> @@ -41,6 +42,12 @@ int ip6_route_me_harder(struct net *net, struct sock *sk_partial, struct sk_buff
>  	};
>  	int err;
>  
> +	fl6.flowi6_flags = sk ? inet_sk_flowi_flags(sk) : 0;
> +	if (saddr_type & (IPV6_ADDR_UNICAST | IPV6_ADDR_LINKLOCAL))
> +		fl6.flowi6_flags |= FLOWI_FLAG_ANYSRC;
> +	else
> +		fl6.saddr = in6addr_any;
> +

[Severity: Medium]
Does this ANYSRC assignment have any effect? This flow goes straight to
ip6_route_output(), and in net/ipv6 the flag is read only by the new
check in ip6_dst_lookup_tail(). xfrm_decode_session() also rebuilds fl6
afterwards.

This points to a wider gap: the new check is not in the core IPv6
output lookup, so direct ip6_route_output() callers skip it. In IPv4 the
check sits in ip_route_output_key_hash_rcu() and covers every output
lookup, including __xfrm4_dst_lookup().

On the IPv6 side, xfrm6_dst_lookup() copies the SA's saddr and routes it
without any source check:

net/ipv6/xfrm6_policy.c:xfrm6_dst_lookup() {
	...
	if (params->saddr)
		memcpy(&fl6.saddr, params->saddr, sizeof(fl6.saddr));
	...
	dst = ip6_route_output(params->net, NULL, &fl6);
	...
}

So IPsec tunnel-mode outer headers would still go out with a removed or
expired local address. Other direct ip6_route_output() users
(ip6_route_me_harder(), collect_md tunnels, seg6/rpl) are also
unchecked.

This hunk also clears fl6.saddr for non-unicast source types (::,
v4-mapped, multicast) before the reroute lookup. The commit message does
not mention that change.

Given this, is the statement in the commit message accurate?

  This aligns the behaviour with the IPv4 stack.

>  	if (sk && sk->sk_bound_dev_if)
>  		fl6.flowi6_oif = sk->sk_bound_dev_if;
>  	else if (strict)

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261028191103.623156-1-ihuguet%40riseup.net