Re: [PATCH] ipv6: Protect the cork MTU read with RCU

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 2 · 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.
- [High] The new rcu_read_lock() in ip6_setup_cork() only helps if the
  final free of the old shared metrics is deferred by a grace period…

--

ipv6: Protect the cork MTU read with RCU

This patch wraps the MTU selection in ip6_setup_cork() in an RCU read-side
section. The goal is to keep a metrics image that is shared with a
Redirect-created exception alive until dst6_mtu() has finished reading
it. The target is a KASAN use-after-free in ip6_mtu() on the UDP send
path, seen when a Packet Too Big update and route deletion run at the
same time as the send.

> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index 5509650589915..0440acc3dec0b 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -1421,12 +1421,14 @@ static int ip6_setup_cork(struct sock *sk, struct inet_cork_full *cork,
>  	v6_cork->hop_limit = ipc6->hlimit;
>  	v6_cork->tclass = ipc6->tclass;
>  	v6_cork->dontfrag = ipc6->dontfrag;
> +	rcu_read_lock();
>  	if (rt->dst.flags & DST_XFRM_TUNNEL)
>  		mtu = READ_ONCE(np->pmtudisc) >= IPV6_PMTUDISC_PROBE ?
>  		      READ_ONCE(rt->dst.dev->mtu) : dst6_mtu(&rt->dst);
>  	else
>  		mtu = READ_ONCE(np->pmtudisc) >= IPV6_PMTUDISC_PROBE ?
>  			READ_ONCE(rt->dst.dev->mtu) : dst6_mtu(xfrm_dst_path(&rt->dst));
> +	rcu_read_unlock();

[Severity: High]
Does this fully close the window the commit message describes? The
message names the defect as "The MTU helper's existing RCU section
starts after the raw metrics read". The helper itself is not changed:

include/net/ip6_route.h:ip6_dst_mtu_maybe_forward() {
    ...
	if (!forwarding || dst_metric_locked(dst, RTAX_MTU)) {
		mtu = dst_metric_raw(dst, RTAX_MTU);
		if (mtu)
			goto out;
	}

	mtu = IPV6_MIN_MTU;
	rcu_read_lock();
    ...
}

ip6_dst_hoplimit() in net/ipv6/ip6_output.c has the same shape:

int ip6_dst_hoplimit(struct dst_entry *dst)
{
	int hoplimit = dst_metric_raw(dst, RTAX_HOPLIMIT);

	rcu_read_lock();
    ...

udpv6_sendmsg() calls it on the same send path and the same exception
dst. The call happens before ip6_make_skb() and without RCU held:

	if (ipc6.hlimit < 0)
		ipc6.hlimit = ip6_sk_dst_hoplimit(np, fl6, dst);

np->hop_limit defaults to -1, so this reaches ip6_dst_hoplimit(). Suppose
the PTB COW and route deletion from the commit message happen while the
sender sits between dst_metric_raw() and the read, for example while
preempted outside any RCU section.

Can the sender then read RTAX_HOPLIMIT, not RTAX_MTU, from the freed
kmalloc-96 metrics? Raw, ping and l2tp_ip6 senders call the same helper.

Other dst6_mtu() callers that run without RCU look exposed too. One
example is tcp_v6_mtu_reduced() in net/ipv6/tcp_ipv6.c:

	dmtu = dst6_mtu(dst);

It runs from tcp_release_cb right after inet6_csk_update_pmtu() may have
COWed the metrics.

Would it be better to widen the RCU section inside
ip6_dst_mtu_maybe_forward() and ip6_dst_hoplimit() so it covers the
dst_metric_raw() read, instead of adding it at single call sites?

[Severity: High]
The commit message says:

    Since COW replaces the metrics pointer before route deletion, the
    existing FIB and destination RCU callbacks must wait for this read
    before releasing the remaining old metrics owners.

Does anything enforce that ordering? __ip6_rt_update_pmtu() in
net/ipv6/route.c COWs the metrics on any RTF_CACHE dst and never checks
rt->from:

	if (!rt6_cache_allowed_for_pmtu(rt6)) {
		rt6_do_update_pmtu(rt6, mtu);

So the COW can run on an exception that route deletion has already
purged. One way is inet6_csk_update_pmtu() or skb_dst_update_pmtu()
acting on a dst that is still held. ip6_dst_ifdown() clears rt->from and
releases the fib6_info, but dst->_metrics still points at the shared
image.

First ordering: the route is deleted, and fib6_info_destroy_rcu() drops
the FIB's metrics reference. The exception dst now holds the last
reference. A COW then frees the old image synchronously in
dst_cow_metrics_generic():

	} else if (prev & DST_METRICS_REFCOUNTED) {
		if (refcount_dec_and_test(&old_p->refcnt))
			kfree(old_p);

Can a sender inside the new rcu_read_lock() section in ip6_setup_cork()
load old_p just before this and then read freed memory? A read-side RCU
section cannot delay a plain kfree().

Second ordering: route deletion queues call_rcu_hurry(&f6i->rcu,
fib6_info_destroy_rcu) before the sender enters rcu_read_lock(). The
sender then loads the old pointer, which dst->_metrics still publishes.
A later COW drops the dst's reference from 2 to 1.

The grace period started before the sender's read-side section, so it
does not wait for the sender. Could
fib6_info_destroy_rcu()->ip_fib_metrics_put() then kfree() the metrics
while ip6_mtu() is still reading them?

dst->_metrics is not an RCU-published pointer. ip_fib_metrics_put(),
ip_dst_metrics_put() and dst_cow_metrics_generic() all free immediately.

For the reader-side section to be enough, would the metrics need
RCU-deferred freeing, such as kfree_rcu() in those put and COW paths? The
alternative would be to enforce the COW-before-purge ordering.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/pm-ip-core-objects-candidate-0007-v2-46f5bb8aa2cc9824a047%40gmail.com