[PATCH net-next v2 0/3] ipv6: tie the dst cookie to the lookup that returned the dst

[PATCH net-next v2 0/3] ipv6: tie the dst cookie to the lookup that returned the dst

[Ralf Lici]

Hi,

This series implements a first part of the model proposed in [1] to
address the late cookie sampling race: a dst can be stored together with
a cookie read after a FIB change that its lookup did not see, and then
passes dst_check until the next change.

The case that made me post it as a series is xfrm. Adding a policy
restamps every fib6 node, so a socket whose lookup ran just before the
insertion keeps its plain dst and leaks plaintext although the policy
matches. I reproduced it for TCP connect and connected UDP, and the same
window lets a more specific blackhole route be bypassed for routes using
nexthop objects. The ovpn patch [2] hit the same pattern in
dst_cache_set_ip6.

The conversion is initially limited to tcp_v6_connect,
ip6_datagram_dst_update and ip6_sk_dst_lookup_flow. Other TCP TX cache
paths, including established-socket route refills, header rebuilds and
accepted-socket setup, still use late cookie sampling. This gives the
lookup-cookie API a small set of users and concrete fixes to review. If
the approach is accepted, I plan to follow up with a broader conversion
across the networking core and drivers.

Compared to [1], the fallback cookie is 0 instead of the pre-lookup
generation, so fib6_flush_trees is left alone, and nexthop-object dsts
are checked in ip6_dst_check instead of passing the generation down to
ip6_rt_pcpu_alloc. The other cache users (dst_cache, sctp, ipvs,
flowtable, sk_rx_dst, the xfrm bundle cookies) are not converted and
behave as before.

I tested it in a VM with lockdep, PROVE_RCU and DEBUG_ATOMIC_SLEEP,
using a debug-only delay after the lookup to hit the window: no
warnings, and the plaintext and blackhole bypass are gone in these test
cases.

[1] https://lore.kernel.org/netdev/20260901122501.482920-1-ralf@mandelbit.com/
[2] https://lore.kernel.org/netdev/cover.1789639346.git.ralf@mandelbit.com/

Thanks,

Ralf Lici
Mandelbit Srl

---
Changes since v1 https://lore.kernel.org/netdev/cover.1793806941.git.ralf@mandelbit.com/
- Fix kdoc return statements in patch 2/3. (NIPA)

Ralf Lici (3):
  ipv6: stamp the default route node with a fresh serial on link failure
  ipv6: add ip6_dst_lookup_flow_cookie
  ipv6: use the lookup cookie when caching the dst in sockets

 include/net/ip6_route.h | 18 +++++++++----
 include/net/ipv6.h      | 13 ++++++++++
 net/ipv6/datagram.c     |  6 +++--
 net/ipv6/ip6_output.c   | 57 ++++++++++++++++++++++++++++++++++++++---
 net/ipv6/route.c        | 24 ++++++++---------
 net/ipv6/tcp_ipv6.c     |  5 ++--
 6 files changed, 98 insertions(+), 25 deletions(-)


base-commit: c4c470912125ec8a1a427a70f4085d8a731b00e4
-- 
2.56.0

[PATCH net-next v2 1/3] ipv6: stamp the default route node with a fresh serial on link failure

[Ralf Lici]

ip6_link_failure invalidates the cached dsts of an RTF_DEFAULT route by
writing -1 into the node's fn_sernum. Unlike every other fn_sernum
update, this does not allocate from net->ipv6.fib6_sernum, so the change
is invisible to anyone watching the generation, and a cookie sampled
from the node after the write simply reads -1 and matches it.

The write is also a no-op when the node already holds -1, so dsts cached
after one link failure survive the next one. dsts of nexthop objects are
validated against the generation and ignore fn_sernum, so a link failure
never invalidated them.

Allocate a fresh serial under tb6_lock instead, as for any other FIB
change. A link failure now invalidates the nexthop-object dsts of the
namespace too, as a route insertion does.

Signed-off-by: Ralf Lici <ralf@mandelbit.com>
---
No changes since v1 https://lore.kernel.org/netdev/5b039a5a24808f2678eedd283c29f8dae514ef5e.1793806941.git.ralf@mandelbit.com/

 net/ipv6/route.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 04098cf32eab..89fddc9df55c 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -2867,13 +2867,13 @@ static void ip6_link_failure(struct sk_buff *skb)
 			rt6_remove_exception_rt(rt);
 		} else {
 			struct fib6_info *from;
-			struct fib6_node *fn;
 
 			from = rcu_dereference(rt->from);
-			if (from) {
-				fn = rcu_dereference(from->fib6_node);
-				if (fn && (rt->rt6i_flags & RTF_DEFAULT))
-					WRITE_ONCE(fn->fn_sernum, -1);
+			if (from && from->fib6_table &&
+			    (rt->rt6i_flags & RTF_DEFAULT)) {
+				spin_lock_bh(&from->fib6_table->tb6_lock);
+				fib6_update_sernum(rt6_exception_net(rt), from);
+				spin_unlock_bh(&from->fib6_table->tb6_lock);
 			}
 		}
 		rcu_read_unlock();
-- 
2.56.0

Re: [PATCH net-next v2 1/3] ipv6: stamp the default route node with a fresh serial on link failure

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit "ipv6: stamp the default route node with a fresh
  serial on link failure" fixes a real, long-standing bug but carries no…
- [Low] The commit message's account of nexthop objects is misleading.

--

ipv6: stamp the default route node with a fresh serial on link failure

For an RTF_DEFAULT route, this patch makes ip6_link_failure() allocate a
fresh serial with fib6_update_sernum() under tb6_lock. Before, it wrote
-1 into the node's fn_sernum. With the change, repeated link failures
invalidate cached dsts, and the change becomes visible through the
netns-wide generation.

> ipv6: stamp the default route node with a fresh serial on link failure
>
> [ ... ]
>
> The write is also a no-op when the node already holds -1, so dsts cached
> after one link failure survive the next one. dsts of nexthop objects are
> validated against the generation and ignore fn_sernum, so a link failure
> never invalidated them.
>
> Allocate a fresh serial under tb6_lock instead, as for any other FIB
> change. A link failure now invalidates the nexthop-object dsts of the
> namespace too, as a route insertion does.

[Severity: Low]
Is this account of nexthop objects accurate? The second paragraph lists
"a link failure never invalidated them" as a defect. The third paragraph
then presents invalidating them as part of the fix.

Can a route that uses a nexthop object ever reach the RTF_DEFAULT branch
of ip6_link_failure()? RTF_DEFAULT seems to be set in only three places:
rt6_add_route_info() for a zero prefixlen, rt6_add_dflt_router(), and the
legacy SIOCADDRT ioctl path. None of them attach a nexthop object.

The netlink path is the only one that sets fc_nh_id. It starts from:

net/ipv6/route.c:rtm_to_fib6_config() {
    ...
	.fc_flags = RTF_UP,
    ...
}

and never adds RTF_DEFAULT.

In practice, every default router link failure now goes through
fib6_update_sernum()->fib6_new_sernum(), which advances
net->ipv6.fib6_sernum. After that, rt6_is_valid() fails for every
unrelated nexthop-object dst in the namespace:

	return rt6->sernum == rt_genid_ipv6(dev_net(rt6->dst.dev));

The global bump looks fine in itself, and the follow-up "ipv6: add
ip6_dst_lookup_flow_cookie" depends on the generation moving.

Could the changelog call the nexthop-object invalidation a harmless side
effect, rather than a deficiency being fixed?

> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 04098cf32eabe..89fddc9df55ce 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -2867,13 +2867,13 @@ static void ip6_link_failure(struct sk_buff *skb)
>  			rt6_remove_exception_rt(rt);
>  		} else {
>  			struct fib6_info *from;
> -			struct fib6_node *fn;
>  
>  			from = rcu_dereference(rt->from);
> -			if (from) {
> -				fn = rcu_dereference(from->fib6_node);
> -				if (fn && (rt->rt6i_flags & RTF_DEFAULT))
> -					WRITE_ONCE(fn->fn_sernum, -1);
> +			if (from && from->fib6_table &&
> +			    (rt->rt6i_flags & RTF_DEFAULT)) {
> +				spin_lock_bh(&from->fib6_table->tb6_lock);
> +				fib6_update_sernum(rt6_exception_net(rt), from);
> +				spin_unlock_bh(&from->fib6_table->tb6_lock);

[Severity: Low]
This fixes a long-standing bug in ip6_link_failure(). Should the commit
carry a Fixes: tag, and possibly Cc: stable and a target tree?

With the old code, the first link failure writes -1. A socket that
re-validates its dst then caches it again with cookie 0xffffffff, taken
from:

rt6_get_cookie()
  fib6_get_cookie_safe()
    READ_ONCE(fn->fn_sernum)

The next link failure writes -1 again, so this check in rt6_check() does
not fire:

	if (!from || !fib6_get_cookie_safe(from, &rt_cookie) ||
	    rt_cookie != cookie)
		return NULL;

The socket then keeps using the failed default router until some
unrelated FIB change restamps the node.

git blame on the removed line points at aafc2e3285c2d7, the WRITE_ONCE()
conversion. The surrounding block was shaped by a68886a691804d and
c5cff8561d2d00, but the -1 sentinel itself appears to be older than all
three.

Without a reference to the commit that introduced the -1 write, how would
stable maintainers know which trees need this?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1794384146.git.ralf%40mandelbit.com

[PATCH net-next v2 2/3] ipv6: add ip6_dst_lookup_flow_cookie

[Ralf Lici]

Callers that keep a dst in a persistent cache read its validation cookie
with rt6_get_cookie once the lookup has returned. For a route without
its own sernum that is the current fn_sernum of the node, so a FIB
change landing in between, or an xfrm policy insertion, which restamps
every node, pairs the old dst with the new serial. dst_check then keeps
accepting it until the next change.

Add ip6_dst_lookup_flow_cookie, which returns the cookie together with
the dst. The cookie is taken between two reads of the IPv6 generation:
if it moved during the lookup the cookie is 0, which is never a node
serial, so a route dst can still be used for the packet at hand but is
not accepted again. An xfrm bundle ignores the cookie and is validated
against its own route and path cookies, which this patch does not
change.

dsts of nexthop objects are validated against the global generation and
ignore the cookie. For them the helper returns the generation read
before the lookup, and ip6_dst_check now requires it to match
rt->sernum, so a dst allocated after a FIB change is rejected for a
lookup that began before it. Callers that read the cookie from the dst
keep passing rt->sernum and are not affected.

Signed-off-by: Ralf Lici <ralf@mandelbit.com>
---
Changes since v1 https://lore.kernel.org/netdev/e40f48e925a0950eac99a3f084e485e82cabe88f.1793806941.git.ralf@mandelbit.com/
- Fix kdoc return statement in ip6_dst_lookup_flow and
  ip6_dst_lookup_flow_cookie. (NIPA)

 include/net/ipv6.h    | 13 +++++++++++
 net/ipv6/ip6_output.c | 51 +++++++++++++++++++++++++++++++++++++++++--
 net/ipv6/route.c      |  2 +-
 3 files changed, 63 insertions(+), 3 deletions(-)

diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 579df1516d25..5a6c66504056 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -1062,6 +1062,11 @@ int ip6_dst_lookup(struct net *net, struct sock *sk, struct dst_entry **dst,
 #if IS_ENABLED(CONFIG_IPV6)
 struct dst_entry *ip6_dst_lookup_flow(struct net *net, const struct sock *sk, struct flowi6 *fl6,
 				      const struct in6_addr *final_dst);
+struct dst_entry *ip6_dst_lookup_flow_cookie(struct net *net,
+					     const struct sock *sk,
+					     struct flowi6 *fl6,
+					     const struct in6_addr *final_dst,
+					     u32 *cookie);
 #else
 static inline struct dst_entry *ip6_dst_lookup_flow(struct net *net, const struct sock *sk,
 						    struct flowi6 *fl6,
@@ -1069,6 +1074,14 @@ static inline struct dst_entry *ip6_dst_lookup_flow(struct net *net, const struc
 {
 	return ERR_PTR(-EAFNOSUPPORT);
 }
+
+static inline struct dst_entry *
+ip6_dst_lookup_flow_cookie(struct net *net, const struct sock *sk,
+			   struct flowi6 *fl6, const struct in6_addr *final_dst,
+			   u32 *cookie)
+{
+	return ERR_PTR(-EAFNOSUPPORT);
+}
 #endif
 
 struct dst_entry *ip6_sk_dst_lookup_flow(struct sock *sk, struct flowi6 *fl6,
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 25fe0ba98b1a..1b273714f7cc 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -1279,8 +1279,7 @@ EXPORT_SYMBOL_GPL(ip6_dst_lookup);
  *
  *	This function performs a route lookup on the given flow.
  *
- *	It returns a valid dst pointer on success, or a pointer encoded
- *	error code.
+ *	Return: a valid dst pointer on success or a pointer encoded error code.
  */
 struct dst_entry *ip6_dst_lookup_flow(struct net *net, const struct sock *sk, struct flowi6 *fl6,
 				      const struct in6_addr *final_dst)
@@ -1300,6 +1299,54 @@ struct dst_entry *ip6_dst_lookup_flow(struct net *net, const struct sock *sk, st
 }
 EXPORT_SYMBOL_GPL(ip6_dst_lookup_flow);
 
+/**
+ *	ip6_dst_lookup_flow_cookie - perform route lookup on flow with ipsec
+ *				     and return the matching dst cookie
+ *	@net: Network namespace to perform lookup in
+ *	@sk: socket which provides route info
+ *	@fl6: flow to lookup
+ *	@final_dst: final destination address for ipsec lookup
+ *	@cookie: where to store the cookie for the returned dst
+ *
+ *	Same as ip6_dst_lookup_flow, but also returns the cookie describing
+ *	the routing state the dst was obtained from. A caller caching the dst
+ *	should use this cookie when validating it with dst_check. If the
+ *	IPv6 FIB changed during the lookup, a route dst no longer validates
+ *	with it. An xfrm bundle is validated by xfrm and ignores the cookie.
+ *
+ *	Return: a valid dst pointer on success or a pointer encoded error code.
+ */
+struct dst_entry *ip6_dst_lookup_flow_cookie(struct net *net,
+					     const struct sock *sk,
+					     struct flowi6 *fl6,
+					     const struct in6_addr *final_dst,
+					     u32 *cookie)
+{
+	const struct rt6_info *rt;
+	struct dst_entry *dst;
+	int gen0, gen1;
+
+	gen0 = rt_genid_ipv6(net);
+	/* read the generation before looking at the FIB or xfrm policies */
+	smp_rmb();
+
+	dst = ip6_dst_lookup_flow(net, sk, fl6, final_dst);
+	if (IS_ERR(dst))
+		return dst;
+
+	rt = dst_rt6_info(dst);
+	*cookie = rt->sernum ? gen0 : rt6_get_cookie(rt);
+
+	/* keep the FIB and cookie reads before the final generation read */
+	smp_rmb();
+	gen1 = rt_genid_ipv6(net);
+	if (unlikely(gen0 != gen1))
+		*cookie = 0;
+
+	return dst;
+}
+EXPORT_SYMBOL_GPL(ip6_dst_lookup_flow_cookie);
+
 /**
  *	ip6_sk_dst_lookup_flow - perform socket cached route lookup on flow
  *	@sk: socket which provides the dst cache and route info
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 89fddc9df55c..84af9626c738 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -2811,7 +2811,7 @@ INDIRECT_CALLABLE_SCOPE struct dst_entry *ip6_dst_check(struct dst_entry *dst,
 	rt = dst_rt6_info(dst);
 
 	if (rt->sernum)
-		return rt6_is_valid(rt) ? dst : NULL;
+		return (rt6_is_valid(rt) && cookie == rt->sernum) ? dst : NULL;
 
 	rcu_read_lock();
 
-- 
2.56.0

Re: [PATCH net-next v2 2/3] ipv6: add ip6_dst_lookup_flow_cookie

[netdev-bot+sashiko]

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

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

- [High] The stale-cookie race that this patch describes as a general
  flaw of 'callers that keep a dst in a persistent cache' is still…
- [Medium] The gen0/gen1 check in ip6_dst_lookup_flow_cookie() only
  notices a FIB change if the writer updates routing state before…

--

ipv6: add ip6_dst_lookup_flow_cookie

This commit adds ip6_dst_lookup_flow_cookie(). It returns the dst
together with a validation cookie that is sampled between two reads of
the IPv6 generation, and the cookie is 0 if the generation moved during
the lookup. For nexthop-object dsts, ip6_dst_check() now also requires
the cookie to match rt->sernum.

> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index 25fe0ba98b1a6..1b273714f7cc3 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c

[ ... ]

> @@ -1300,6 +1299,54 @@ struct dst_entry *ip6_dst_lookup_flow(struct net *net, const struct sock *sk, st
>  }
>  EXPORT_SYMBOL_GPL(ip6_dst_lookup_flow);
>  
> +/**
> + *	ip6_dst_lookup_flow_cookie - perform route lookup on flow with ipsec
> + *				     and return the matching dst cookie

[ ... ]

> + *	Same as ip6_dst_lookup_flow, but also returns the cookie describing
> + *	the routing state the dst was obtained from. A caller caching the dst
> + *	should use this cookie when validating it with dst_check. If the
> + *	IPv6 FIB changed during the lookup, a route dst no longer validates
> + *	with it. An xfrm bundle is validated by xfrm and ignores the cookie.

[Severity: High]
The commit message describes the stale cookie as a problem for "Callers
that keep a dst in a persistent cache", and names an xfrm policy
insertion as one trigger. This patch converts no callers.

At the end of the series, only tcp_v6_connect(), ip6_datagram_dst_update()
and ip6_sk_dst_lookup_flow() are converted, by "ipv6: use the lookup
cookie when caching the dst in sockets".

Are the remaining persistent caches still exposed to the same race? The
TCP re-route path, for example, still does this:

net/ipv6/inet6_connection_sock.c:inet6_csk_route_socket() {
    ...
	dst = ip6_dst_lookup_flow(sock_net(sk), sk, fl6, final_p);

	if (!IS_ERR(dst))
		ip6_dst_store(sk, dst, false, false);
    ...
}

ip6_dst_store() reads rt6_get_cookie() only after the lookup has
returned. This path runs from inet6_csk_xmit() whenever __sk_dst_check()
fails, and that happens right after a rt_genid_bump_ipv6().

tcp_v6_syn_recv_sock() has the same pattern for accepted sockets. Its
window runs from inet6_csk_route_req() to ip6_dst_store().
inet6_sk_rebuild_header() has the same pattern too.

dst_cache_set_ip6() computes the cookie itself:

net/core/dst_cache.c:dst_cache_set_ip6() {
    ...
	dst_cache_per_cpu_dst_set(idst, dst,
				  rt6_get_cookie(dst_rt6_info(dst)));
    ...
}

It has no parameter for a cookie from the caller. Without a new dst_cache
API, these users can't adopt the new helper:

- udp_tunnel6_dst_lookup() (vxlan, geneve, bareudp)
- ip6_tunnel
- wireguard
- ovpn
- the seg6, rpl, ioam6 and ila lwtunnels
- tipc

Suppose this sequence lands between ip6_dst_lookup_flow() and
rt6_get_cookie() in any of these paths:

xfrm_policy_insert()->rt_genid_bump_ipv6()->__fib6_flush_trees()

The plain route would then be cached with the post-insertion serial and
keep passing rt6_check().

Wouldn't that socket or tunnel keep sending without the new IPsec policy
until the next FIB change? That is the plaintext leak this series sets
out to close.

> + *
> + *	Return: a valid dst pointer on success or a pointer encoded error code.
> + */
> +struct dst_entry *ip6_dst_lookup_flow_cookie(struct net *net,
> +					     const struct sock *sk,
> +					     struct flowi6 *fl6,
> +					     const struct in6_addr *final_dst,
> +					     u32 *cookie)
> +{
> +	const struct rt6_info *rt;
> +	struct dst_entry *dst;
> +	int gen0, gen1;
> +
> +	gen0 = rt_genid_ipv6(net);
> +	/* read the generation before looking at the FIB or xfrm policies */
> +	smp_rmb();
> +
> +	dst = ip6_dst_lookup_flow(net, sk, fl6, final_dst);
> +	if (IS_ERR(dst))
> +		return dst;
> +
> +	rt = dst_rt6_info(dst);
> +	*cookie = rt->sernum ? gen0 : rt6_get_cookie(rt);
> +
> +	/* keep the FIB and cookie reads before the final generation read */
> +	smp_rmb();
> +	gen1 = rt_genid_ipv6(net);
> +	if (unlikely(gen0 != gen1))
> +		*cookie = 0;

[Severity: Medium]
Can the gen0/gen1 check catch writers that bump the serial before they
finish updating the routing state?

fib6_ifup() clears the nh flags, bumps the serial, and only then
rebalances:

net/ipv6/route.c:fib6_ifup() {
    ...
		rt->fib6_nh->fib_nh_flags &= ~arg->nh_flags;
		fib6_update_sernum_upto_root(net, rt);
		rt6_multipath_rebalance(rt);
    ...
}

fib6_ifdown() uses the same order for NETDEV_DOWN on multipath routes,
and also for NETDEV_CHANGE:

net/ipv6/route.c:fib6_ifdown() {
    ...
			rt6_multipath_nh_flags_set(rt, dev, RTNH_F_DEAD |
						   RTNH_F_LINKDOWN);
			fib6_update_sernum(net, rt);
			rt6_multipath_rebalance(rt);
		}
		return -2;
    ...
}

The -2 return makes the walker skip the other siblings, so no second
bump follows the rebalance.

Suppose gen0 reads the new serial N+1 and fib6_select_path() runs before
rt6_multipath_rebalance(). The lookup then sees the old
fib_nh_upper_bound values. rt6_get_cookie() returns fn_sernum N+1 and
gen1 is also N+1, so the returned cookie is valid. rt6_check() keeps
accepting the dst after that.

rt6_score_route() doesn't check RTNH_F_DEAD or RTNH_F_LINKDOWN. Could
the cached dst then stay pinned to a nexthop that was just marked dead
or link-down, or to a stale weight split, until some unrelated FIB
change?

The writer ordering predates this patch. Still, the kdoc above says "If
the IPv6 FIB changed during the lookup, a route dst no longer validates
with it", and that doesn't seem to hold for these writers.

> +
> +	return dst;
> +}
> +EXPORT_SYMBOL_GPL(ip6_dst_lookup_flow_cookie);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1794384146.git.ralf%40mandelbit.com

[PATCH net-next v2 3/3] ipv6: use the lookup cookie when caching the dst in sockets

[Ralf Lici]

tcp_v6_connect, ip6_datagram_dst_update and ip6_sk_dst_lookup_flow store
the dst in the socket and read its cookie afterwards. If an xfrm policy
matching the flow is added after the route and policy lookup but before
the cookie is read, the plain route found without the policy is cached
with a post-insertion serial. Later packets pass the validation in
__sk_dst_check and skip xfrm, so a socket that should be using IPsec
keeps sending plaintext until the FIB changes again. It affects TCP and
connected UDP sockets alike.

Store the cookie returned by ip6_dst_lookup_flow_cookie instead. Add
ip6_dst_store_cookie for this and keep ip6_dst_store for callers that
have no lookup cookie.

Signed-off-by: Ralf Lici <ralf@mandelbit.com>
---
No changes since v1 https://lore.kernel.org/netdev/832892eaa7d2371f20459c3134c72914e42970b0.1793806941.git.ralf@mandelbit.com/

 include/net/ip6_route.h | 18 +++++++++++++-----
 net/ipv6/datagram.c     |  6 ++++--
 net/ipv6/ip6_output.c   |  6 ++++--
 net/ipv6/route.c        | 12 ++++++------
 net/ipv6/tcp_ipv6.c     |  5 +++--
 5 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
index 95f936be32c6..2684f6069a5e 100644
--- a/include/net/ip6_route.h
+++ b/include/net/ip6_route.h
@@ -252,13 +252,13 @@ static inline const struct rt6_info *skb_rt6_info(const struct sk_buff *skb)
 /*
  *	Store a destination cache entry in a socket
  */
-static inline void ip6_dst_store(struct sock *sk, struct dst_entry *dst,
-				 bool daddr_set,
-				 bool saddr_set)
+static inline void ip6_dst_store_cookie(struct sock *sk, struct dst_entry *dst,
+					u32 cookie, bool daddr_set,
+					bool saddr_set)
 {
 	struct ipv6_pinfo *np = inet6_sk(sk);
 
-	np->dst_cookie = rt6_get_cookie(dst_rt6_info(dst));
+	np->dst_cookie = cookie;
 	sk_setup_caps(sk, dst);
 	np->daddr_cache = daddr_set;
 #ifdef CONFIG_IPV6_SUBTREES
@@ -266,8 +266,16 @@ static inline void ip6_dst_store(struct sock *sk, struct dst_entry *dst,
 #endif
 }
 
+static inline void ip6_dst_store(struct sock *sk, struct dst_entry *dst,
+				 bool daddr_set,
+				 bool saddr_set)
+{
+	ip6_dst_store_cookie(sk, dst, rt6_get_cookie(dst_rt6_info(dst)),
+			     daddr_set, saddr_set);
+}
+
 void ip6_sk_dst_store_flow(struct sock *sk, struct dst_entry *dst,
-			   const struct flowi6 *fl6);
+			   const struct flowi6 *fl6, u32 cookie);
 
 static inline bool ipv6_unicast_destination(const struct sk_buff *skb)
 {
diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index ff7a7ffbf983..c18609bab366 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -78,6 +78,7 @@ int ip6_datagram_dst_update(struct sock *sk, bool fix_sk_saddr)
 	struct in6_addr *final_p;
 	struct dst_entry *dst;
 	struct flowi6 *fl6;
+	u32 cookie;
 	int err = 0;
 
 	if (inet6_test_bit(SNDFLOW, sk) &&
@@ -94,7 +95,8 @@ int ip6_datagram_dst_update(struct sock *sk, bool fix_sk_saddr)
 	final_p = fl6_update_dst(fl6, opt, &np->final);
 	rcu_read_unlock();
 
-	dst = ip6_dst_lookup_flow(sock_net(sk), sk, fl6, final_p);
+	dst = ip6_dst_lookup_flow_cookie(sock_net(sk), sk, fl6, final_p,
+					 &cookie);
 	if (IS_ERR(dst)) {
 		err = PTR_ERR(dst);
 		goto out;
@@ -112,7 +114,7 @@ int ip6_datagram_dst_update(struct sock *sk, bool fix_sk_saddr)
 		}
 	}
 
-	ip6_sk_dst_store_flow(sk, dst, fl6);
+	ip6_sk_dst_store_flow(sk, dst, fl6, cookie);
 
 out:
 	fl6_sock_release(flowlabel);
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 1b273714f7cc..2288102710b3 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -1370,14 +1370,16 @@ struct dst_entry *ip6_sk_dst_lookup_flow(struct sock *sk, struct flowi6 *fl6,
 					 bool connected)
 {
 	struct dst_entry *dst = sk_dst_check(sk, inet6_sk(sk)->dst_cookie);
+	u32 cookie;
 
 	dst = ip6_sk_dst_check(sk, dst, fl6);
 	if (dst)
 		return dst;
 
-	dst = ip6_dst_lookup_flow(sock_net(sk), sk, fl6, final_dst);
+	dst = ip6_dst_lookup_flow_cookie(sock_net(sk), sk, fl6, final_dst,
+					 &cookie);
 	if (connected && !IS_ERR(dst))
-		ip6_sk_dst_store_flow(sk, dst_clone(dst), fl6);
+		ip6_sk_dst_store_flow(sk, dst_clone(dst), fl6, cookie);
 
 	return dst;
 }
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 84af9626c738..ec7433b4fac8 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3042,19 +3042,19 @@ void ip6_sk_update_pmtu(struct sk_buff *skb, struct sock *sk, __be32 mtu)
 }
 
 void ip6_sk_dst_store_flow(struct sock *sk, struct dst_entry *dst,
-			   const struct flowi6 *fl6)
+			   const struct flowi6 *fl6, u32 cookie)
 {
 #ifdef CONFIG_IPV6_SUBTREES
 	struct ipv6_pinfo *np = inet6_sk(sk);
 #endif
 
-	ip6_dst_store(sk, dst,
-		      ipv6_addr_equal(&fl6->daddr, &sk->sk_v6_daddr),
+	ip6_dst_store_cookie(sk, dst, cookie,
+			     ipv6_addr_equal(&fl6->daddr, &sk->sk_v6_daddr),
 #ifdef CONFIG_IPV6_SUBTREES
-		      ipv6_addr_equal(&fl6->saddr, &np->saddr) ?
-		      true :
+			     ipv6_addr_equal(&fl6->saddr, &np->saddr) ?
+			     true :
 #endif
-		      false);
+			     false);
 }
 
 static bool ip6_redirect_nh_match(const struct fib6_result *res,
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index b1dec5d86d63..25404656069c 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -145,6 +145,7 @@ static int tcp_v6_connect(struct sock *sk, struct sockaddr_unsized *uaddr,
 	struct dst_entry *dst;
 	struct flowi6 *fl6;
 	int addr_type;
+	u32 cookie;
 	int err;
 
 	if (addr_len < SIN6_LEN_RFC2133)
@@ -288,7 +289,7 @@ static int tcp_v6_connect(struct sock *sk, struct sockaddr_unsized *uaddr,
 	 */
 	ip6_ecmp_set_mp_hash(net, fl6, sk->sk_txhash);
 
-	dst = ip6_dst_lookup_flow(net, sk, fl6, final_p);
+	dst = ip6_dst_lookup_flow_cookie(net, sk, fl6, final_p, &cookie);
 	if (IS_ERR(dst)) {
 		err = PTR_ERR(dst);
 		goto failure;
@@ -312,7 +313,7 @@ static int tcp_v6_connect(struct sock *sk, struct sockaddr_unsized *uaddr,
 	inet->inet_rcv_saddr = LOOPBACK4_IPV6;
 
 	sk->sk_gso_type = SKB_GSO_TCPV6;
-	ip6_dst_store(sk, dst, false, false);
+	ip6_dst_store_cookie(sk, dst, cookie, false, false);
 
 	icsk->icsk_ext_hdr_len = 0;
 	if (opt)
-- 
2.56.0

Re: [PATCH net-next v2 3/3] ipv6: use the lookup cookie when caching the dst in sockets

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 1 · 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] Incomplete fix: on the TCP side only tcp_v6_connect() was
  converted.
- [Medium] The same bug class remains in the SCTP per-transport dst
  cache.
- [Low] The commit fixes a user-visible security problem: 'a socket that
  should be using IPsec keeps sending plaintext until the FIB changes…

--

ipv6: use the lookup cookie when caching the dst in sockets

This commit changes tcp_v6_connect(), ip6_datagram_dst_update() and
ip6_sk_dst_lookup_flow() to store the cookie returned by
ip6_dst_lookup_flow_cookie(). Before, they read rt6_get_cookie() after
the route and xfrm lookup. It adds ip6_dst_store_cookie() for this, and
ip6_dst_store() stays as a wrapper that still reads the cookie after the
lookup.

> Store the cookie returned by ip6_dst_lookup_flow_cookie instead. Add
> ip6_dst_store_cookie for this and keep ip6_dst_store for callers that
> have no lookup cookie.
>
> Signed-off-by: Ralf Lici <ralf@mandelbit.com>

[Severity: Low]
This isn't a bug, but should this have a Fixes: tag and Cc: stable? The
commit message describes a socket that "keeps sending plaintext" when it
should be using IPsec.

The patch also calls ip6_dst_lookup_flow_cookie(). That helper comes from
the previous patch in the series ("ipv6: add ip6_dst_lookup_flow_cookie"),
which also changes ip6_dst_check() behaviour for nexthop-object dsts. So
this patch can't be backported on its own. Could the dependency be noted
for stable?

> diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
> index 95f936be32c6c..2684f6069a5e1 100644
> --- a/include/net/ip6_route.h
> +++ b/include/net/ip6_route.h

[ ... ]

> @@ -266,8 +266,16 @@ static inline void ip6_dst_store(struct sock *sk, struct dst_entry *dst,
>  #endif
>  }
>  
> +static inline void ip6_dst_store(struct sock *sk, struct dst_entry *dst,
> +				 bool daddr_set,
> +				 bool saddr_set)
> +{
> +	ip6_dst_store_cookie(sk, dst, rt6_get_cookie(dst_rt6_info(dst)),
> +			     daddr_set, saddr_set);
> +}

[Severity: High]
Is the TCP side of this fix complete? The commit message says this
"affects TCP and connected UDP sockets alike", but tcp_v6_connect() is
the only TCP path converted. Other TCPv6 paths still call
ip6_dst_lookup_flow() and then ip6_dst_store(), so they still read the
cookie after xfrm_lookup_route() has returned.

Here is inet6_csk_route_socket() in net/ipv6/inet6_connection_sock.c:

	dst = ip6_dst_lookup_flow(sock_net(sk), sk, fl6, final_p);

	if (!IS_ERR(dst))
		ip6_dst_store(sk, dst, false, false);

inet6_csk_xmit() reaches this whenever __sk_dst_check(sk, np->dst_cookie)
fails. inet6_csk_update_pmtu() also reaches it through
tcp_v6_mtu_reduced(), and a remote peer can trigger that path with
ICMPv6 Packet Too Big.

inet6_sk_rebuild_header() in net/ipv6/af_inet6.c has the same
lookup/store pair. It is ipv6_specific.rebuild_header, and tcp_connect()
calls it right after tcp_v6_connect(). Suppose
ip6_dst_lookup_flow_cookie() saw the generation move and returned cookie
0. Then __sk_dst_check() fails immediately, and the lookup is redone
through the unconverted path inside the same connect() call. The
retransmit path also reaches inet6_sk_rebuild_header().

tcp_v6_syn_recv_sock() in net/ipv6/tcp_ipv6.c gets its dst from
inet6_csk_route_req(). That function calls ip6_dst_lookup_flow() without
capturing a cookie. tcp_v6_syn_recv_sock() then runs
tcp_create_openreq_child() before it does:

	ip6_dst_store(newsk, dst, false, false);

So every passively opened IPv6 TCP socket has the same window.

An xfrm policy insertion goes through
rt_genid_bump_ipv6()->__fib6_flush_trees(), which invalidates every
cached socket dst. Established sockets therefore re-route through
inet6_csk_route_socket() right after a policy change.

Suppose another policy is inserted during that re-lookup, which can
happen when an IKE daemon installs several policies back to back. Can
the plain route then be cached with the post-insertion serial? If so,
inet6_csk_xmit() would keep accepting it and send plaintext.

The commit message says ip6_dst_store is kept "for callers that have no
lookup cookie". Each of these callers does its own ip6_dst_lookup_flow(),
though, so each could use ip6_dst_lookup_flow_cookie() with
ip6_dst_store_cookie(). For the passive-open path, inet6_csk_route_req()
would need to return the cookie.

The kernel-doc of ip6_dst_lookup_flow_cookie() also says "A caller caching
the dst should use this cookie when validating it with dst_check."

[Severity: Medium]
Does the same window remain in the SCTP per-transport dst cache?
sctp_v6_get_dst() in net/sctp/ipv6.c does its route and policy lookup
through ip6_dst_lookup_flow() and stores the result in t->dst. Only after
that does it read the cookie:

	rt = dst_rt6_info(dst);
	t->dst_cookie = rt6_get_cookie(rt);

sctp_transport_dst_check() later validates t->dst against that cookie.
If an xfrm policy insertion restamps fn_sernum between the lookup and
this read, could an SCTP association over IPv6 keep using the route that
was found without the policy?

dst_cache_set_ip6() in net/core/dst_cache.c has the same pattern. IPv6
UDP tunnels and ip6_tunnel use it:

	dst_cache_per_cpu_dst_set(idst, dst,
				  rt6_get_cookie(dst_rt6_info(dst)));

The previous patch in the series describes the problem as affecting all
"callers that keep a dst in a persistent cache". Should these sites also
be converted to ip6_dst_lookup_flow_cookie(), or at least mentioned in
the commit message?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1794384146.git.ralf%40mandelbit.com