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

[netdev-bot+sashiko@kernel.org]

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