Re: [PATCH net-next 7/9] sit: dynamically allocate struct ip_tunnel_parm_kern

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

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

- [Medium] Intermediate-state use-after-free in net/ipv6/sit.c:
  ipip6_tunnel_update() publishes the freshly allocated parameter block…
- [Low] Missing publication barrier / unannotated lockless pointer: in
  this commit `sit_parms` is a plain (non-__rcu) pointer in struct…

--

sit: dynamically allocate struct ip_tunnel_parm_kern

This moves the SIT configuration parameters out of the embedded
ip_tunnel.parms into a separately allocated struct ip_tunnel_parm_kern
reachable through the new ip_tunnel.sit_parms pointer, in preparation for
converting them to RCU.  All readers and writers in net/ipv6/sit.c are
switched over to the new pointer.

A couple of questions about the intermediate state this commit leaves the
tree in.

      ^^^^

[Severity: Low]
Should this member already carry the __rcu annotation in this commit?  It
is read from softirq packet paths (ipip6_tunnel_lookup(), ipip6_rcv(),
sit_tunnel_rcv(), ipip6_tunnel_xmit(), sit_tunnel_xmit__()) without RTNL,
while ipip6_tunnel_update() replaces it with a plain store.  As a plain
pointer, sparse cannot flag the unprotected dereferences, the publish has
no release semantics, and the loads are plain data races for KCSAN.

The following patch in the series, "sit: convert configuration to RCU
protection", changes this to struct ip_tunnel_parm_kern __rcu *sit_parms
and converts publication to rcu_assign_pointer(), so the window is limited
to this single commit.  Would it be worth folding the annotation in here?

        ^^^^

[Severity: Low]
Can this mix fields from two different parameter blocks?  t->sit_parms is
reloaded up to four times inside one match decision here, and
ipip6_tunnel_update() swaps the pointer concurrently, so saddr/daddr could
come from the old block and link from the new one.

The next patch caches a single rcu_dereference(t->sit_parms) in a local
parms variable per candidate, which removes this; could that be done here
instead?

 		    (t->dev->flags & IFF_UP))
 			return t;
 	}

[ ... ]

        ^^^^

[Severity: Medium]
Can this read freed memory?  ipip6_tunnel_xmit() reaches the tunnel via
netdev_priv(dev), never through the sitn->tunnels[] buckets, and it keeps
the interior pointer tiph across the route lookup, the PMTU handling, the
headroom reallocation and iptunnel_xmit().

Meanwhile ipip6_tunnel_update() frees the block the pointer points into:

net/ipv6/sit.c:ipip6_tunnel_update() {
	...
	ipip6_tunnel_unlink(sitn, t);
	synchronize_net();
	t->sit_parms = new_p;
	...
	kfree(old_p);
	return 0;
}

The synchronize_net() runs before the new pointer is published and only
drains readers that came in through the hash buckets, so it does not cover
this path.  dev->lltx is set for sit, so nothing else serializes a
SIOCCHGTUNNEL or RTM_NEWLINK change against traffic on the same device.
The values read out of the freed block (tiph->saddr, tos, ttl, frag_off)
end up in the outer IPv4 header.

The immediately following patch, "sit: convert configuration to RCU
protection", wraps the transmit path in rcu_read_lock() and uses
kfree_rcu(old_p, rcu), so this hazard exists only between the two commits
(a concern for bisection and partial backports).

 	__be16 df = tiph->frag_off;
 	struct rtable *rt;		/* Route to the other host */
 	struct net_device *tdev;	/* Device to other host */

[ ... ]

        ^^^^

[Severity: Medium]
Same question here: this hands an interior pointer of the freeable
parameter block to ip_tunnel_xmit() with no reference and no RCU section
held, so a concurrent ipip6_tunnel_update() can kfree() it underneath.

 
 	if (iptunnel_handle_offloads(skb, SKB_GSO_IPXIP4))
 		goto tx_error;

[ ... ]

        ^^^^

[Severity: Medium]
Is the ordering here intentional?  The grace period happens before the new
pointer is published, so nothing separates the store of new_p from the
kfree(old_p) below for readers that already loaded the old pointer.

+	t->parms.iph = new_p->iph;
+	WRITE_ONCE(t->parms.link, new_p->link);
 	__dev_addr_set(t->dev, &p->iph.saddr, 4);
 	memcpy(t->dev->broadcast, &p->iph.daddr, 4);
 	ipip6_tunnel_link(sitn, t);
-	t->parms.iph.ttl = p->iph.ttl;
-	t->parms.iph.tos = p->iph.tos;
-	t->parms.iph.frag_off = p->iph.frag_off;
-	if (t->parms.link != p->link || t->fwmark != fwmark) {
-		t->parms.link = p->link;
+	if (old_p->link != p->link || t->fwmark != fwmark) {
 		WRITE_ONCE(t->fwmark, fwmark);
 		ipip6_tunnel_bind_dev(t->dev);
 	}
 	dst_cache_reset(&t->dst_cache);
 	netdev_state_change(t->dev);
+	kfree(old_p);

        ^^^^

[Severity: Medium]
Would kfree_rcu() be needed here, given that ipip6_tunnel_xmit(),
sit_tunnel_xmit__() and ipip6_get_iflink() all reach sit_parms through
netdev_priv(dev) rather than through the hash buckets?

The next patch in the series adds a struct rcu_head to
ip_tunnel_parm_kern and turns this into kfree_rcu(old_p, rcu).

+	return 0;
 }

[ ... ]

        ^^^^

[Severity: Medium]
Here the READ_ONCE() now covers only the link field, not the pointer load
itself.  Since this accessor can run without RTNL, can it dereference the
block that ipip6_tunnel_update() is about to kfree()?  The following patch
adds an rcu_read_lock() section and an rcu_dereference() here.

 }
 
 static const struct net_device_ops ipip6_netdev_ops = {

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907075846.2913645-1-edumazet%40google.com