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

[netdev-bot+sashiko]

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] This commit taken by itself: moving `family = x->props.family`
  above the `x->km.state != XFRM_STATE_VALID` check in xfrm_input()…

--

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

This patch moves the family assignment in xfrm_input() above the check
for a state that is no longer valid. As a result, the early drop path
calls xfrm_rcv_cb() with a real address family instead of AF_UNSPEC. The
commit message describes this as a prerequisite for releasing the tunnel
device reference on that drop path in the next patch.

> diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> index cfd5cea033127..ab391258a5028 100644
> --- a/net/xfrm/xfrm_input.c
> +++ b/net/xfrm/xfrm_input.c
> @@ -487,6 +487,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
>  				      encap_type == UDP_ENCAP_ESPINUDP))) {
>  		x = xfrm_input_state(skb);
>  		xfrm_proto = x->type ? x->type->proto : nexthdr;
> +		family = x->props.family;
>  
>  		if (unlikely(x->km.state != XFRM_STATE_VALID)) {
>  			if (x->km.state == XFRM_STATE_ACQ)

[Severity: Medium]
On its own, can this commit expose a use-after-free on the vti and vti6
tunnel devices?

When an async-resumed skb (encap_type == -1) fails this check, it jumps
to the drop label in xfrm_input():

drop:
	xfrm_rcv_cb(skb, family, xfrm_proto, -1);
	if (async)
		dev_put(dev);
	kfree_skb(skb);

Before this patch, family was still AF_UNSPEC at this point. That made
xfrm_input_get_afinfo() return NULL, so no callback ran. Now
xfrm4_rcv_cb() or xfrm6_rcv_cb() runs, and that reaches vti_rcv_cb() or
vti6_rcv_cb() with err == -1:

net/ipv4/ip_vti.c:vti_rcv_cb() {
	struct ip_tunnel *tunnel = XFRM_TUNNEL_SKB_CB(skb)->tunnel.ip4;
	...
	dev = tunnel->dev;

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

vti_input() stores the tunnel pointer in skb->cb without taking a
reference. vti6_input_proto() does the same for IPv6, after
rcu_read_unlock().

During async crypto, xfrm_input() calls dev_hold() only on skb->dev. On
the vti proto path that is the underlay device, not the vti net_device.

A state that becomes invalid while crypto is pending is the teardown
case: an SA delete or flush, often together with tunnel deletion or
netns teardown. If the vti device has already been unregistered and
freed by then (vti6_dev_uninit() unlinks the tunnel and drops its
reference without waiting for in-flight crypto), then the
DEV_STATS_INC() calls would write into freed memory.

The same unreferenced dereference already exists on the async success
and drop_unlock paths. So this patch widens an existing problem rather
than creating a new one.

The last patch of this series, "xfrm: hold the tunnel device reference
across the input path", adds dev_hold() on the tunnel device in
vti_input(), vti6_input_proto() and xfrm6_rcv_spi(). It then releases
that reference after xfrm_rcv_cb() on the drop path, which appears to
fix this.

Both patches are Cc: stable. Would it make sense to reorder the series,
or fold the two patches together? That would avoid an intermediate
state that bisect could land on, or that a stable backport taking only
this patch would pick up.

[ ... ]

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