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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] net/ipv6: uninitialized fib_result in ip6_is_local_saddr()
- [Medium] sctp: missing flowi6_flags initialization in sctp_v6_get_dst()
--

commit a60f53e33acaf53f5beb0a9e9e20fd1e7c8f5aac
Author: Íñigo Huguet <ihuguet@riseup.net>

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

This patch prevents routing outbound IPv6 packets when the source address
is not configured on the host (with a fallback to FIB local routes for
loopback subnets), unless FLOWI_FLAG_ANYSRC is set in flowi6_flags.

[ ... ]

> 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;
> +	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;
> +	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;
> +}

[Severity: Critical]
Should fib_result be zero-initialized in ip6_is_local_saddr() before
calling fib6_lookup()?

In net/ipv6/ip6_output.c, ip6_is_local_saddr() declares fib_result on the
stack without initializing it, leaving fib_result.rt6 with uninitialized
stack data.

When custom IPv6 FIB rules are present (net->ipv6.fib6_has_custom_rules is
true), fib6_lookup() in net/ipv6/fib6_rules.c calls fib_rules_lookup() with
arg.lookup_ptr = fib6_table_lookup and arg.result = &fib_result:

net/ipv6/fib6_rules.c:fib6_lookup() {
	if (net->ipv6.fib6_has_custom_rules) {
		struct fib_lookup_arg arg = {
			.lookup_ptr = fib6_table_lookup,
			.lookup_data = &oif,
			.result = res,
			.flags = FIB_LOOKUP_NOREF,
		};
		...
		err = fib_rules_lookup(net->ipv6.fib6_rules_ops,
				       flowi6_to_flowi(fl6), flags, &arg);

During rule evaluation, fib_rules_lookup() in net/core/fib_rules.c invokes
fib6_rule_action() -> fib6_rule_action_alt() -> fib6_table_lookup() ->
rt6_select(). In net/ipv6/route.c, rt6_select() and __find_rr_leaf() only
populate res->f6i, res->nh, res->fib6_flags, and res->fib6_type, leaving
res->rt6 untouched.

When fib6_rule_action() returns 0, fib_rules_lookup() then calls
fib6_rule_suppress() in net/ipv6/fib6_rules.c:

net/ipv6/fib6_rules.c:fib6_rule_suppress() {
	struct fib6_result *res = arg->result;
	struct rt6_info *rt = res->rt6;
	struct net_device *dev = NULL;

	if (!rt)
		return false;

	if (rt->rt6i_idev)
		dev = rt->rt6i_idev->dev;

	if (rt->rt6i_dst.plen <= rule->suppress_prefixlen)
		goto suppress_route;

Because fib_result.rt6 is uninitialized, any non-zero stack value causes
fib6_rule_suppress() to dereference rt->rt6i_idev and rt->rt6i_dst.plen,
and potentially call ip6_rt_put_flags() on an arbitrary stack address.

> @@ -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;
> +	}
> +
>  #ifdef CONFIG_IPV6_OPTIMISTIC_DAD
>  	/*
>  	 * Here if the dst entry we've looked up

[Severity: Medium]
Does sctp_v6_get_dst() in net/sctp/ipv6.c also need to set
fl6->flowi6_flags = inet_sk_flowi_flags(sk) so that transparent IPv6 SCTP
sockets continue to work with this check?

IPv6 SCTP supports IPV6_TRANSPARENT and allows binding to non-local IPv6
addresses via sctp_inet6_bind_verify() -> sctp_v6_available():

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

However, in net/sctp/ipv6.c, sctp_v6_get_dst() zero-initializes _fl and
calls ip6_dst_lookup_flow(sock_net(sk), sk, fl6, final_p) without setting
fl6->flowi6_flags from inet_sk_flowi_flags(sk):

net/sctp/ipv6.c:sctp_v6_get_dst() {
	...
	memset(&_fl, 0, sizeof(_fl));
	...
	dst = ip6_dst_lookup_flow(sock_net(sk), sk, fl6, final_p);
	...
		fl6->saddr = laddr->a.v6.sin6_addr;
		fl6->fl6_sport = laddr->a.v6.sin6_port;
		final_p = fl6_update_dst(fl6, rcu_dereference(np->opt), &final);
		bdst = ip6_dst_lookup_flow(sock_net(sk), sk, fl6, final_p);

Because FLOWI_FLAG_ANYSRC is not set in fl6->flowi6_flags, outbound route
lookups for an IPv6 SCTP socket with IPV6_TRANSPARENT bound to a non-local
address will now fail in ip6_dst_lookup_tail() with -ENETUNREACH.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028191103.623156-1-ihuguet@riseup.net?part=2