[PATCH net v4 0/3] xfrm: fix vti tunnel device use-after-free across async crypto resumption

[PATCH net v4 0/3] xfrm: fix vti tunnel device use-after-free across async crypto resumption

[Qihang]

This replaces the v1/v2 vti/vti6 series and the earlier reference-transfer
rework.

vti_input() and vti6_input_proto() cache the tunnel pointer in the skb
control buffer without taking a reference.  With async crypto the
receive callback runs from the crypto completion, after a possible
device teardown, and dereferences the freed tunnel device.

The input hook takes a reference on the tunnel device when it caches
the tunnel.  A successful receive callback leaves the cached tunnel in
place; xfrm_input() releases the reference on every exit, including
after delivery and on the drop paths.  The error callback only updates
the counters, so the common drop path releases the reference after the
skb has been freed.  IPTFS, which frees the outer skb itself, releases
it through xfrm_input_drop()/xfrm_input_consume().

Patch 3 depends on the first two: it releases the tunnel reference from
the drop path, which needs the receive callback ordering and the address
family set up by patches 1 and 2.

Changes since v3:
- the cached pointer is always a tunnel; the address family selects its
  type, and a non-NULL slot means one device reference is held, so a
  nested or pre-installed tunnel device cannot leak it
- xfrm_input() takes the device out of the skb before
  gro_cells_receive()/transport_finish(), which may consume the skb
- the device reference is dropped only after the skb has been freed,
  including on the error path
- IPTFS passes the state's address family explicitly instead of
  inferring it from the secpath; the consumed skb keeps consume_skb()
  semantics and the drop paths keep their skb drop reason
- drop the xfrmi changes: it re-looks-up the interface and does not
  take part in the reference transfer
- merge the reference and IPTFS changes into one patch so the series
  stays bisectable
- the gro_cells NAPI window is pre-existing and is documented, not
  changed

Runtime evidence (KASAN, aarch64 QEMU):
- VTI async success and error: RTM_DELLINK completes without a KASAN
  report; the unfixed tree reports slab-use-after-free in vti_rcv_cb.
- Async resume with the SA deleted in the crypto window: the reference
  is released and the device is removed; with patch 2 reverted,
  xfrm_input_take_tunnel_dev() warns and unregister_netdevice() waits
  for vti0.
- IPTFS outer-skb free: RTM_DELLINK completes; with the !first_skb
  xfrm_input_drop() reverted to kfree_skb(), the reference leaks and
  unregister_netdevice() waits for vti0.
The IPTFS timer/reorder/state-teardown paths are covered by code audit,
not by a runtime trigger.

Previous versions:
  https://lore.kernel.org/netdev/20261013074610.17808-1-q.h.hack.winter@gmail.com/ (v2)
  https://lore.kernel.org/netdev/20260930090813.73901-1-q.h.hack.winter@gmail.com/ (v1)

Qihang


Qihang (3):
  xfrm: run the receive callback before dropping the skb->dev reference
  xfrm: set the address family before the early invalid-state drop
  xfrm: hold the tunnel device reference across the input path

 net/ipv4/ip_vti.c      |  3 ++
 net/ipv6/ip6_vti.c     |  4 +++
 net/ipv6/xfrm6_input.c |  3 ++
 net/xfrm/xfrm_inout.h  |  4 +++
 net/xfrm/xfrm_input.c  | 74 +++++++++++++++++++++++++++++++++++++++---
 net/xfrm/xfrm_iptfs.c  | 25 +++++++++-----
 6 files changed, 99 insertions(+), 14 deletions(-)

-- 
2.54.0 (Apple Git-157)

[PATCH net v4 1/3] xfrm: run the receive callback before dropping the skb->dev reference

[Qihang]

The drop path in xfrm_input() releases the skb->dev reference that is
held across an async resumption before it runs the receive callbacks.
The callbacks may still dereference skb->dev, xfrmi_rcv_cb() reads
skb->dev->ifindex, so they can race a concurrent teardown of the
device.  Run the callbacks first and drop the reference afterwards,
like the success path does.

The early drop for a state that is no longer valid releases the
reference on its own; let the drop label handle that reference too.

Fixes: 1c428b038400 ("xfrm: hold dev ref until after transport_finish NF_HOOK")
Cc: stable@vger.kernel.org
Signed-off-by: Qihang <q.h.hack.winter@gmail.com>
---
 net/xfrm/xfrm_input.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
index 5ed87d51392a..94de4f698c33 100644
--- a/net/xfrm/xfrm_input.c
+++ b/net/xfrm/xfrm_input.c
@@ -496,7 +496,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 					       LINUX_MIB_XFRMINSTATEINVALID);
 
 			if (encap_type == -1)
-				dev_put(dev);
+				async = 1;
 			goto drop;
 		}
 
@@ -779,9 +779,9 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 drop_unlock:
 	spin_unlock(&x->lock);
 drop:
+	xfrm_rcv_cb(skb, family, xfrm_proto, -1);
 	if (async)
 		dev_put(dev);
-	xfrm_rcv_cb(skb, family, xfrm_proto, -1);
 	kfree_skb(skb);
 	return 0;
 }
-- 
2.54.0 (Apple Git-157)

Re: [PATCH net v4 1/3] xfrm: run the receive callback before dropping the skb->dev reference

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · 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] This patch makes the async drop path in xfrm_input()
  (net/xfrm/xfrm_input.c) run xfrm_rcv_cb() before the async…

--

xfrm: run the receive callback before dropping the skb->dev reference

This patch reorders the drop label in xfrm_input() so that xfrm_rcv_cb()
runs before the async dev_put(dev). It also makes the early drop for an
invalid state let the drop label release the reference. The goal is to
keep callbacks such as xfrmi_rcv_cb() from touching skb->dev after its
reference has been dropped.

> diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> index 5ed87d51392a5..94de4f698c331 100644
> --- a/net/xfrm/xfrm_input.c
> +++ b/net/xfrm/xfrm_input.c

[ ... ]

> @@ -779,9 +779,9 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
>  drop_unlock:
>  	spin_unlock(&x->lock);
>  drop:
> +	xfrm_rcv_cb(skb, family, xfrm_proto, -1);
>  	if (async)
>  		dev_put(dev);
> -	xfrm_rcv_cb(skb, family, xfrm_proto, -1);
>  	kfree_skb(skb);
>  	return 0;
>  }

[Severity: High]
The commit message says the drop path now drops the reference "like the
success path does". Is that true for the tunnel-mode (decaps) success
path in xfrm_input()? That branch still drops the async reference before
handing the skb to GRO:

net/xfrm/xfrm_input.c:xfrm_input() {
    ...
	if (decaps) {
		...
		if (async)
			dev_put(dev);
		gro_cells_receive(&gro_cells, skb);
		rcu_read_unlock();
		return 0;
	}
    ...
}

gro_cells_receive() reads skb->dev right away:

net/core/gro_cells.c:gro_cells_receive() {
	struct net_device *dev = skb->dev;
	...
	if (unlikely(!(dev->flags & IFF_UP)))
		goto drop;
	...
	if (!gcells->cells || skb_cloned(skb) || netif_elide_gro(dev)) {
	...
drop:
	dev_core_stats_rx_dropped_inc(dev);
    ...
}

For a plain tunnel-mode SA where no xfrmi or vti callback rewrote
skb->dev, skb->dev is still the local dev. The async reference is the
only thing pinning that device while the crypto request is pending.

During unregister, netdev_wait_allrefs_any() returns as soon as
netdev_refcnt_read(dev) == 1. netdev_run_todo() then calls
netdev_do_free_pcpu_stats(), priv_destructor and free_netdev() without
waiting for another RCU grace period. The rcu_read_lock() held in
xfrm_input() therefore does not protect the device after the dev_put().

Can this sequence free the netdev while gro_cells_receive() is still
using it?

esp_input() returns -EINPROGRESS with dev_hold(dev) taken
  underlay device is deleted, unregister waits for refs
crypto completes
  xfrm_input_resume()->xfrm_input(skb, nexthdr, 0, -1), async = 1
    resume_decapped sets decaps = 1
    xfrm_rcv_cb() succeeds, skb->dev == dev
    dev_put(dev)                 <- last outside reference dropped
      waiter sees refcnt == 1, frees dev
    gro_cells_receive()          <- dereferences skb->dev

On the IFF_UP-cleared branch, dev_core_stats_rx_dropped_inc() also
writes through the freed dev->core_stats pointer.

The decaps ordering comes from the same commit named in the Fixes tag,
1c428b038400. The rest of this series does not change it either: in the
final tree the async dev_put(dev) still comes before
xfrm_input_take_tunnel_dev() and gro_cells_receive(). The transport and
xfrm_gro branch already drops dev after transport_finish() and
gro_cells_receive().

Should this patch also move the decaps dev_put(dev) after
gro_cells_receive(), and should the commit message be updated so it
doesn't say the drop path matches all of the success paths?

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

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

[Qihang]

The early drop in xfrm_input() for a state that is no longer valid runs
while family is still AF_UNSPEC, so xfrm_rcv_cb() cannot find the
afinfo and the receive callback is never invoked for such a packet.
Set the family from the state before the check so that the drop path
dispatches the callback like every other drop does.

This is also a prerequisite for releasing the tunnel device reference
on that drop path in the next patch: without a family, the release
cannot tell an ip_tunnel from an ip6_tnl.

Fixes: 4ce3dbe397d7 ("xfrm: Fix xfrm_input() to verify state is valid when (encap_type < 0)")
Cc: stable@vger.kernel.org
Signed-off-by: Qihang <q.h.hack.winter@gmail.com>
---
 net/xfrm/xfrm_input.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
index 94de4f698c33..517369ba9efe 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)
@@ -500,8 +501,6 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 			goto drop;
 		}
 
-		family = x->props.family;
-
 		/* An encap_type of -2 indicates reconstructed inner packet */
 		if (encap_type == -2)
 			goto resume_decapped;
-- 
2.54.0 (Apple Git-157)

Re: [PATCH net v4 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] 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.

> diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> index 94de4f698c331..517369ba9efea 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]

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

[PATCH net v4 3/3] xfrm: hold the tunnel device reference across the input path

[Qihang]

vti_input() and vti6_input_proto() cache the tunnel pointer in the skb
control buffer without taking a reference.  With async crypto the
receive callback runs from the crypto completion, after a possible
device teardown, and dereferences the freed tunnel device:

    BUG: KASAN: slab-use-after-free in vti_rcv_cb

Take a reference on the tunnel device when the pointer is cached.  The
tunnel is the device's private area, so the reference keeps the tunnel
structure alive as well.  xfrm6_rcv_spi() takes the same reference for
the tunnel it is given; it is a separate entry point, so vti6 does not
hold the reference twice.

A successful vti/vti6 callback leaves the cached tunnel in place for
xfrm_input() to release after delivery.  A callback that fails only
updates the error counters; the common drop path releases the reference
after the skb has been freed.  The helper takes the device out of the
skb before gro_cells_receive() and transport_finish(), which may already
consume the skb.  The device reference is therefore always dropped after
the skb is gone.

IPTFS consumes the outer skb itself and frees it directly, so it must
release the reference as well.  The IPTFS state carries the address
family the tunnel was resolved for, and the caller passes it in.  Add
xfrm_input_drop() and xfrm_input_consume() for the outer-skb free paths
in __input_process_payload(), iptfs_input_ordered(), iptfs_input(),
iptfs_drop_timer() and iptfs_destroy_state().  Only the outer skb
carries the cached tunnel; the inner skbs built by iptfs_alloc_skb()
have a zeroed control buffer, so they do not release it.  The drop paths
keep their skb drop reason and the consumed skb keeps consume_skb()
semantics.

The reference is released right after gro_cells_receive(); the NAPI
that processes the queued skb later still uses skb->dev without a
reference.  That window is pre-existing and is not changed here.

This depends on the previous two patches: the receive callbacks must
run before the async dev_put(), and the early drop must know the
address family to release the reference.

Fixes: df3893c176e9 ("vti: Update the ipv4 side to use it's own receive hook.")
Fixes: fa9ad96d4905 ("vti6: Update the ipv6 side to use its own receive hook.")
Cc: stable@vger.kernel.org
Signed-off-by: Qihang <q.h.hack.winter@gmail.com>
---
 net/ipv4/ip_vti.c      |  3 ++
 net/ipv6/ip6_vti.c     |  4 +++
 net/ipv6/xfrm6_input.c |  3 ++
 net/xfrm/xfrm_inout.h  |  4 +++
 net/xfrm/xfrm_input.c  | 67 +++++++++++++++++++++++++++++++++++++++++-
 net/xfrm/xfrm_iptfs.c  | 25 ++++++++++------
 6 files changed, 96 insertions(+), 10 deletions(-)

diff --git a/net/ipv4/ip_vti.c b/net/ipv4/ip_vti.c
index 3b80929994a0..cbe7906a4a1b 100644
--- a/net/ipv4/ip_vti.c
+++ b/net/ipv4/ip_vti.c
@@ -63,6 +63,9 @@ static int vti_input(struct sk_buff *skb, int nexthdr, __be32 spi,
 
 		XFRM_TUNNEL_SKB_CB(skb)->tunnel.ip4 = tunnel;
 
+		/* Kept until xfrm_input() is done with the skb. */
+		dev_hold(tunnel->dev);
+
 		if (update_skb_dev)
 			skb->dev = tunnel->dev;
 
diff --git a/net/ipv6/ip6_vti.c b/net/ipv6/ip6_vti.c
index ab94b3a4ba9c..73a65b853be0 100644
--- a/net/ipv6/ip6_vti.c
+++ b/net/ipv6/ip6_vti.c
@@ -319,6 +319,10 @@ static int vti6_input_proto(struct sk_buff *skb, int nexthdr, __be32 spi,
 			goto discard;
 		}
 
+		/* Keep the tunnel device alive until xfrm_input() is done
+		 * with the skb, including an async crypto resumption.
+		 */
+		dev_hold(t->dev);
 		rcu_read_unlock();
 
 		XFRM_TUNNEL_SKB_CB(skb)->tunnel.ip6 = t;
diff --git a/net/ipv6/xfrm6_input.c b/net/ipv6/xfrm6_input.c
index 07edef258984..d27bc24f1b16 100644
--- a/net/ipv6/xfrm6_input.c
+++ b/net/ipv6/xfrm6_input.c
@@ -15,6 +15,7 @@
 #include <linux/netfilter.h>
 #include <linux/netfilter_ipv6.h>
 #include <net/ipv6.h>
+#include <net/ip6_tunnel.h>
 #include <net/xfrm.h>
 #include <net/protocol.h>
 #include <net/gro.h>
@@ -23,6 +24,8 @@ int xfrm6_rcv_spi(struct sk_buff *skb, int nexthdr, __be32 spi,
 		  struct ip6_tnl *t)
 {
 	XFRM_TUNNEL_SKB_CB(skb)->tunnel.ip6 = t;
+	if (t)
+		dev_hold(t->dev);
 	XFRM_SPI_SKB_CB(skb)->family = AF_INET6;
 	XFRM_SPI_SKB_CB(skb)->daddroff = offsetof(struct ipv6hdr, daddr);
 	return xfrm_input(skb, nexthdr, spi, 0);
diff --git a/net/xfrm/xfrm_inout.h b/net/xfrm/xfrm_inout.h
index efc5e6b2e87b..8bd05f62ad56 100644
--- a/net/xfrm/xfrm_inout.h
+++ b/net/xfrm/xfrm_inout.h
@@ -6,6 +6,10 @@
 #ifndef XFRM_INOUT_H
 #define XFRM_INOUT_H 1
 
+void xfrm_input_drop(struct sk_buff *skb, unsigned int family,
+		     enum skb_drop_reason reason);
+void xfrm_input_consume(struct sk_buff *skb, unsigned int family);
+
 static inline void xfrm4_extract_header(struct sk_buff *skb)
 {
 	const struct iphdr *iph = ip_hdr(skb);
diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
index 517369ba9efe..209fd7ffb5c8 100644
--- a/net/xfrm/xfrm_input.c
+++ b/net/xfrm/xfrm_input.c
@@ -120,6 +120,60 @@ static int xfrm_rcv_cb(struct sk_buff *skb, unsigned int family, u8 protocol,
 	return ret;
 }
 
+/*
+ * vti/vti6 cache the tunnel in the skb control buffer and keep a
+ * reference on its net_device.  The tunnel is the device's private
+ * area, so the reference keeps both alive.  xfrm_input() releases it
+ * once the packet has been delivered; the drop paths release it when
+ * no receive callback ran.  The caller passes the address family of the
+ * cached tunnel.
+ */
+static struct net_device *xfrm_input_take_tunnel_dev(struct sk_buff *skb,
+						     unsigned int family)
+{
+	void *slot = (void *)XFRM_TUNNEL_SKB_CB(skb)->tunnel.ip4;
+	struct net_device *dev;
+
+	if (!slot)
+		return NULL;
+
+	switch (family) {
+	case AF_INET:
+		dev = ((struct ip_tunnel *)slot)->dev;
+		break;
+	case AF_INET6:
+		dev = ((struct ip6_tnl *)slot)->dev;
+		break;
+	default:
+		WARN_ON_ONCE(1);
+		return NULL;
+	}
+
+	XFRM_TUNNEL_SKB_CB(skb)->tunnel.ip4 = NULL;
+	return dev;
+}
+
+void xfrm_input_drop(struct sk_buff *skb, unsigned int family,
+		     enum skb_drop_reason reason)
+{
+	struct net_device *dev = xfrm_input_take_tunnel_dev(skb, family);
+
+	kfree_skb_reason(skb, reason);
+	if (dev)
+		dev_put(dev);
+}
+EXPORT_SYMBOL(xfrm_input_drop);
+
+void xfrm_input_consume(struct sk_buff *skb, unsigned int family)
+{
+	struct net_device *dev = xfrm_input_take_tunnel_dev(skb, family);
+
+	consume_skb(skb);
+	if (dev)
+		dev_put(dev);
+}
+EXPORT_SYMBOL(xfrm_input_consume);
+
 struct sec_path *secpath_set(struct sk_buff *skb)
 {
 	struct sec_path *sp, *tmp = skb_ext_find(skb, SKB_EXT_SEC_PATH);
@@ -468,6 +522,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 	const struct xfrm_state_afinfo *afinfo;
 	struct net *net = dev_net(skb->dev);
 	struct net_device *dev = skb->dev;
+	struct net_device *tdev;
 	int err;
 	__be32 seq;
 	__be32 seq_hi;
@@ -748,7 +803,11 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 			skb_dst_drop(skb);
 		if (async)
 			dev_put(dev);
+		/* gro_cells_receive() may free the skb. */
+		tdev = xfrm_input_take_tunnel_dev(skb, family);
 		gro_cells_receive(&gro_cells, skb);
+		if (tdev)
+			dev_put(tdev);
 		rcu_read_unlock();
 		return 0;
 	} else {
@@ -756,6 +815,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 		if (xo)
 			xfrm_gro = xo->flags & XFRM_GRO;
 
+		tdev = xfrm_input_take_tunnel_dev(skb, family);
 		err = -EAFNOSUPPORT;
 		afinfo = xfrm_state_afinfo_get_rcu(family);
 		if (likely(afinfo))
@@ -769,6 +829,8 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 			gro_cells_receive(&gro_cells, skb);
 		}
 
+		if (tdev)
+			dev_put(tdev);
 		if (async)
 			dev_put(dev);
 		rcu_read_unlock();
@@ -779,9 +841,12 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 	spin_unlock(&x->lock);
 drop:
 	xfrm_rcv_cb(skb, family, xfrm_proto, -1);
+	tdev = xfrm_input_take_tunnel_dev(skb, family);
+	kfree_skb(skb);
+	if (tdev)
+		dev_put(tdev);
 	if (async)
 		dev_put(dev);
-	kfree_skb(skb);
 	return 0;
 }
 EXPORT_SYMBOL(xfrm_input);
diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
index 6920940a35b4..657dce0c9fdf 100644
--- a/net/xfrm/xfrm_iptfs.c
+++ b/net/xfrm/xfrm_iptfs.c
@@ -709,8 +709,9 @@ static void __iptfs_reassem_done(struct xfrm_iptfs_data *xtfs, bool free)
 
 	/* We don't care if it works locking takes care of things */
 	hrtimer_try_to_cancel(&xtfs->drop_timer);
-	if (free)
-		kfree_skb(xtfs->ra_newskb);
+	if (free && xtfs->ra_newskb)
+		xfrm_input_drop(xtfs->ra_newskb, xtfs->x->props.family,
+				SKB_DROP_REASON_NOT_SPECIFIED);
 	xtfs->ra_newskb = NULL;
 }
 
@@ -1204,14 +1205,15 @@ static bool __input_process_payload(struct xfrm_state *x, u32 data,
 	skb_abort_seq_read(skbseq);
 
 	if (defer) {
-		consume_skb(defer);
+		xfrm_input_consume(defer, x->props.family);
 	} else if (!first_skb) {
 		/* skb is the original passed in skb, but we didn't get far
 		 * enough to process it as the first_skb, if we had it would
 		 * either be save in ra_newskb, trimmed and sent on as an skb or
 		 * placed in defer to be freed.
 		 */
-		kfree_skb(skb);
+		xfrm_input_drop(skb, x->props.family,
+				SKB_DROP_REASON_NOT_SPECIFIED);
 	}
 	return true;
 }
@@ -1298,7 +1300,8 @@ static void iptfs_input_ordered(struct xfrm_state *x, struct sk_buff *skb)
 	if (!consumed) {
 		skb = skbseq.root_skb;
 		skb_abort_seq_read(&skbseq);
-		kfree_skb(skb);
+		xfrm_input_drop(skb, x->props.family,
+				SKB_DROP_REASON_NOT_SPECIFIED);
 	}
 }
 
@@ -1700,7 +1703,8 @@ static enum hrtimer_restart iptfs_drop_timer(struct hrtimer *me)
 	spin_unlock(&xtfs->drop_lock);
 
 	if (skb)
-		kfree_skb_reason(skb, SKB_DROP_REASON_FRAG_REASM_TIMEOUT);
+		xfrm_input_drop(skb, x->props.family,
+				SKB_DROP_REASON_FRAG_REASM_TIMEOUT);
 
 	if (count) {
 		list_for_each_entry_safe(skb, next, &list, list) {
@@ -1751,7 +1755,8 @@ static int iptfs_input(struct xfrm_state *x, struct sk_buff *skb)
 
 	list_for_each_entry_safe(skb, next, &freelist, list) {
 		skb_list_del_init(skb);
-		kfree_skb(skb);
+		xfrm_input_drop(skb, x->props.family,
+				SKB_DROP_REASON_NOT_SPECIFIED);
 	}
 done:
 	/* We always have dealt with the input SKB, either we are re-using it,
@@ -2752,11 +2757,13 @@ static void iptfs_destroy_state(struct xfrm_state *x)
 	hrtimer_cancel(&xtfs->drop_timer);
 
 	if (xtfs->ra_newskb)
-		kfree_skb(xtfs->ra_newskb);
+		xfrm_input_drop(xtfs->ra_newskb, xtfs->x->props.family,
+				SKB_DROP_REASON_NOT_SPECIFIED);
 
 	for (s = xtfs->w_saved, se = s + xtfs->w_savedlen; s < se; s++) {
 		if (s->skb)
-			kfree_skb(s->skb);
+			xfrm_input_drop(s->skb, x->props.family,
+					SKB_DROP_REASON_NOT_SPECIFIED);
 	}
 
 	kfree_sensitive(xtfs->w_saved);
-- 
2.54.0 (Apple Git-157)

Re: [PATCH net v4 3/3] xfrm: hold the tunnel device reference across the input path

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 · Medium: 0 · 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] This patch puts a dev_hold() on the vti/vti6 net_device into
  every outer skb (vti_input(), vti6_input_proto()).
- [High] __input_process_payload() can leak the outer skb and, after this
  patch, the vti/vti6 device reference it carries, so the reference is…
- [Low] The new comments describing the tunnel-device reference do not
  match the code.

--

xfrm: hold the tunnel device reference across the input path

vti_input() and vti6_input_proto() now take a reference on the tunnel
net_device when they cache the tunnel pointer in the skb control buffer.
xfrm_input() drops that reference on its delivery and drop paths. IPTFS
drops it through the new xfrm_input_drop() and xfrm_input_consume()
helpers when it frees an outer skb.

> diff --git a/net/ipv4/ip_vti.c b/net/ipv4/ip_vti.c
> index 3b80929994a00..cbe7906a4a1ba 100644
> --- a/net/ipv4/ip_vti.c
> +++ b/net/ipv4/ip_vti.c
> @@ -63,6 +63,9 @@ static int vti_input(struct sk_buff *skb, int nexthdr, __be32 spi,
>  
>  		XFRM_TUNNEL_SKB_CB(skb)->tunnel.ip4 = tunnel;
>  
> +		/* Kept until xfrm_input() is done with the skb. */
> +		dev_hold(tunnel->dev);
> +

[Severity: High]
Can IPTFS keep the vti device pinned for a very long time, or with no
time limit, now that every outer skb holds this reference?

IPTFS can keep the outer skb after xfrm_input() has returned. It keeps it
as xtfs->ra_newskb when __input_process_payload() reuses the outer skb as
first_skb and that skb is partial. It also keeps it in the reorder window
through __reorder_future_fits():

	xtfs->w_saved[index].skb = inskb;

Only iptfs_drop_timer(), later traffic or iptfs_destroy_state() release
these skbs. The drop time comes from XFRMA_IPTFS_DROP_TIME, which has no
upper bound (u32 microseconds, about 71 minutes).

The drop timer can also be lost. __iptfs_reassem_done() calls
hrtimer_try_to_cancel(&xtfs->drop_timer), but the reorder window uses the
same timer. iptfs_set_window_drop_times() only re-arms it here:

	if (index == -1 && !hrtimer_is_queued(&xtfs->drop_timer))
		hrtimer_start(&xtfs->drop_timer, xtfs->drop_time_ns,
			      IPTFS_HRTIMER_MODE);

With the default window of 3, this sequence seems to leave an outer skb
parked with no timer:

  wantseq = N
  N arrives, starts a reassembly, drop_timer is armed
  N+3 arrives, goes into w_saved[1]; the timer is already queued, so it
    is not re-armed
  N+1 arrives, __reorder_this() shifts N+3 into w_saved[0]
  iptfs_input_ordered(N+1) completes the reassembly
    iptfs_reassem_cont()->__iptfs_reassem_done()
      hrtimer_try_to_cancel(&xtfs->drop_timer)

N+3, and its vti device reference, then waits for more traffic or for the
SA to be destroyed.

Nothing in xfrm or IPTFS handles NETDEV_UNREGISTER for the vti device.
Would "ip link del vti0" then block in netdev_wait_allrefs_any()?

Netns teardown looks worse. vti_net_ops and vti6_net_ops are pernet device
ops, so their exit runs before the xfrm_net_ops subsys exit.
xfrm_net_pre_exit() only flushes policies. States are only flushed in
xfrm_net_exit()->xfrm_state_fini()->xfrm_state_flush().

So iptfs_destroy_state() cannot run until the device unregister finishes.
The unregister is waiting for a reference that only iptfs_destroy_state()
would drop. Could this stall cleanup_net with no end?

Before this patch the parked skbs held no device reference. The dev_hold()
added in vti6_input_proto() has the same issue.

[ ... ]

> diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> index 517369ba9efea..209fd7ffb5c82 100644
> --- a/net/xfrm/xfrm_input.c
> +++ b/net/xfrm/xfrm_input.c
> @@ -120,6 +120,60 @@ static int xfrm_rcv_cb(struct sk_buff *skb, unsigned int family, u8 protocol,
>  	return ret;
>  }
>  
> +/*
> + * vti/vti6 cache the tunnel in the skb control buffer and keep a
> + * reference on its net_device.  The tunnel is the device's private
> + * area, so the reference keeps both alive.  xfrm_input() releases it
> + * once the packet has been delivered; the drop paths release it when
> + * no receive callback ran.  The caller passes the address family of the
> + * cached tunnel.
> + */

[Severity: Low]
This isn't a bug, but this comment doesn't seem to match the code.

The comment says "xfrm_input() releases it once the packet has been
delivered". In the decaps branch the reference is dropped right after
gro_cells_receive(), and gro_cells_receive() only queues the skb for the
gro_cells NAPI:

		tdev = xfrm_input_take_tunnel_dev(skb, family);
		gro_cells_receive(&gro_cells, skb);
		if (tdev)
			dev_put(tdev);

It also says "the drop paths release it when no receive callback ran".
The drop: label calls the receive callback first, and vti_rcv_cb() reads
tunnel->dev there:

	xfrm_rcv_cb(skb, family, xfrm_proto, -1);
	tdev = xfrm_input_take_tunnel_dev(skb, family);

The comments in vti_input() ("Kept until xfrm_input() is done with the
skb.") and vti6_input_proto() have a similar problem with IPTFS.
xfrm_input() returns on -EINPROGRESS while the outer skb and its reference
are still in xtfs->w_saved[] or xtfs->ra_newskb.

Could these comments describe when the reference is actually released?

[ ... ]

> diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
> index 6920940a35b49..657dce0c9fdf8 100644
> --- a/net/xfrm/xfrm_iptfs.c
> +++ b/net/xfrm/xfrm_iptfs.c

[ ... ]

> @@ -1204,14 +1205,15 @@ static bool __input_process_payload(struct xfrm_state *x, u32 data,
>  	skb_abort_seq_read(skbseq);
>  
>  	if (defer) {
> -		consume_skb(defer);
> +		xfrm_input_consume(defer, x->props.family);
>  	} else if (!first_skb) {
>  		/* skb is the original passed in skb, but we didn't get far
>  		 * enough to process it as the first_skb, if we had it would
>  		 * either be save in ra_newskb, trimmed and sent on as an skb or
>  		 * placed in defer to be freed.
>  		 */
> -		kfree_skb(skb);
> +		xfrm_input_drop(skb, x->props.family,
> +				SKB_DROP_REASON_NOT_SPECIFIED);
>  	}
>  	return true;
>  }

[Severity: High]
Does this path leak the outer skb, and with it the vti device reference?

On the first loop iteration, __input_process_payload() can reuse the
outer skb as first_skb while XFRM_TUNNEL_SKB_CB(skb)->tunnel is still
set. If the first inner packet is complete, the skb is queued:

		iptfs_complete_inner_skb(x, skb);
		list_add_tail(&skb->list, sublist);

If a later inner block in the same payload is malformed, the loop jumps
to done. For example:

		} else {
			XFRM_INC_STATS(net, LINUX_MIB_XFRMINBUFFERERROR);
			goto done;
		}

The IPv4 iplen < iphlen check and a skb_copy_seq_read() failure take the
same jump.

The done: label comes after the "Send the packets!" loop, so first_skb
never reaches xfrm_input(skb, 0, 0, -2). At done: defer is NULL and
first_skb is non-NULL, so neither xfrm_input_consume() nor
xfrm_input_drop() runs, and the function returns true.
iptfs_input_ordered() then sees consumed == true, and its on-stack
sublist goes out of scope.

The skb leak was already there. With this patch, each such packet also
leaks a reference on the vti/vti6 net_device, so a later unregister of
the tunnel would wait forever.

A completed reassembly skb that iptfs_reassem_cont() put on the same
sublist seems to leak the same way when it is an earlier reused outer
skb.

The commit message says the outer-skb free paths in
__input_process_payload() are covered. Should the goto done path also
free the skbs on sublist, through xfrm_input_drop()?

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

Re: [PATCH net v4 0/3] xfrm: fix vti tunnel device use-after-free across async crypto resumption

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.