Re: [PATCH net v4 2/3] xfrm: set the address family before the early invalid-state drop

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

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

- [Medium] Taken alone, this patch exposes a tunnel pointer with no
  reference held to a new drop path.

--

xfrm: set the address family before the early invalid-state drop

This patch moves the family = x->props.family assignment in xfrm_input()
so it runs before the km.state != XFRM_STATE_VALID check. The early
invalid-state drop then calls xfrm_rcv_cb() with a real family, so the
per-family receive callback runs on that path too. The patch is tagged for
stable, and a later patch in the series needs it to release the tunnel
device reference.

[Severity: Medium]

With family set here, the early invalid-state drop now calls xfrm_rcv_cb()
with AF_INET or AF_INET6 instead of AF_UNSPEC:

net/xfrm/xfrm_input.c:xfrm_input() {
    ...
		if (unlikely(x->km.state != XFRM_STATE_VALID)) {
			...
			if (encap_type == -1)
				async = 1;
			goto drop;
		}
    ...
drop:
	xfrm_rcv_cb(skb, family, xfrm_proto, -1);
	if (async)
		dev_put(dev);
	kfree_skb(skb);
    ...
}

Before this patch, xfrm_input_get_afinfo() found no afinfo for AF_UNSPEC,
so no handler ran on this path. Now vti_rcv_cb() and vti6_rcv_cb() run
here, and both write to the tunnel netdev stats when err is set:

net/ipv4/ip_vti.c:vti_rcv_cb() {
    ...
	dev = tunnel->dev;

	if (err) {
		DEV_STATS_INC(dev, rx_errors);
		DEV_STATS_INC(dev, rx_dropped);
    ...
}

At this commit, the tunnel pointer in XFRM_TUNNEL_SKB_CB(skb) is stored
without taking a reference. vti_input() saves tunnel.ip4 directly.
vti6_input_proto() saves tunnel.ip6 after it has already called
rcu_read_unlock():

net/ipv6/ip6_vti.c:vti6_input_proto() {
    ...
		rcu_read_unlock();

		XFRM_TUNNEL_SKB_CB(skb)->tunnel.ip6 = t;
    ...
}

The only device xfrm_input() pins across async crypto is the local dev,
which is skb->dev at entry. For vti_input_proto() (update_skb_dev=false)
and for vti6_input_proto(), that is the lower device, not the vti netdev.

Can this become a use-after-free write on the vti/vti6 net_device? One
possible sequence:

  async crypto (e.g. cryptd) is in flight
  the SA is deleted and km.state becomes XFRM_STATE_DEAD
  the vti/vti6 netdev is unregistered and freed
  crypto completes:
    xfrm_input_resume()
      xfrm_input(..., -1)
        goto drop
          xfrm_rcv_cb()
            vti_rcv_cb() / vti6_rcv_cb()
              DEV_STATS_INC(tunnel->dev, ...)

The async success path and drop_unlock already use the same unreferenced
pointer before this patch. The new path is the one most closely tied to SA
and tunnel teardown.

The last patch in this series, "xfrm: hold the tunnel device reference
across the input path", seems to fix this. It adds dev_hold() in
vti_input() and vti6_input_proto(), and adds a dev_put() after
xfrm_rcv_cb() on the drop label. Its commit message says it depends on
this patch.

This commit has a Fixes: tag and Cc: stable. Should the series be
reordered so the reference is taken first? The alternative is to make sure
the follow-up is always backported together with this one.

Otherwise, a bisect that stops at this commit, or a stable tree that takes
only this patch, would get the new drop path without the reference.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261018090638.55686-1-q.h.hack.winter%40gmail.com