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

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

[Qihang]

This replaces the v1 vti/vti6 series.

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 reference cannot simply be held in the input path and dropped in
the callback: the callback is not reached on every path (IPTFS frees
the outer skb itself, and the early drop for a non-valid state runs
with family still AF_UNSPEC), and on the success path the callback may
drop the last reference before xfrm_input() is done with skb->dev.

Make the reference travel with the skb instead: the receive callbacks
install the device in skb->dev and leave their reference with it, and
xfrm_input() releases it once the packet has been delivered.

Patch 3 covers vti, vti6 and xfrmi, the only receive callbacks that
replace skb->dev (esp, ah and ipcomp do not).

For Chris: patch 2 covers the outer-skb free paths in
__input_process_payload(), iptfs_input_ordered() and the reorder-window
free list.  The deferred fragment (consume_skb(defer)), the reassembly
drop timer / __iptfs_reassem_done() and the state teardown still free
the skb directly; I left those alone for now.

Changes since v1:
- split into three patches: the first two fix the paths where the
  receive callback is not dispatched at all, the third is the device
  reference handling
- the reference is taken before delivery and released through a saved
  pointer afterwards, instead of holding skb->dev after the callback
- v1: https://lore.kernel.org/netdev/20260930090813.73901-1-q.h.hack.winter@gmail.com/

Qihang (3):
  xfrm: set the address family before the early invalid-state drop
  xfrm: iptfs: run the receive callbacks before freeing the skb
  xfrm: transfer the skb->dev reference to the input path

 net/ipv4/ip_vti.c              |  6 ++++++
 net/ipv6/ip6_vti.c             |  7 +++++++
 net/xfrm/xfrm_inout.h          |  2 ++
 net/xfrm/xfrm_input.c          | 35 ++++++++++++++++++++++++++++++++--
 net/xfrm/xfrm_interface_core.c |  9 ++++++++-
 net/xfrm/xfrm_iptfs.c          |  6 +++---
 6 files changed, 59 insertions(+), 6 deletions(-)

-- 
2.54.0 (Apple Git-157)

[PATCH net v2 1/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.

Fixes: 5f24f41e8ea6 ("xfrm: Remove inner/outer modes from input path")
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 5ed87d51392a..ddf8fd75fd20 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 v2 1/3] xfrm: set the address family before the early invalid-state drop

[netdev-bot+sashiko]

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

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

- [High] This patch makes the early invalid-state drop in xfrm_input()
  (net/xfrm/xfrm_input.c) dispatch the receive callbacks.
- [Medium] The Fixes: tag points to 5f24f41e8ea6 ("xfrm: Remove
  inner/outer modes from input path", v6.4), but that commit did not…

--

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

This patch moves the family = x->props.family assignment in xfrm_input()
above the XFRM_STATE_VALID check. The early invalid-state drop then
reaches xfrm_rcv_cb() with a real family, not AF_UNSPEC, so the
per-protocol receive callbacks get called for these packets.

> Fixes: 5f24f41e8ea6 ("xfrm: Remove inner/outer modes from input path")

[Severity: Medium]
Is this the right commit for the Fixes: tag?

5f24f41e8ea6 only changed one line, from this:

    family = x->outer_mode.family;

to this:

    family = x->props.family;

That line was already after the invalid-state check.

The ordering that leaves family as AF_UNSPEC on the early drop seems to
come from 4ce3dbe397d7 ("xfrm: Fix xfrm_input() to verify state is valid
when (encap_type < 0)"). That commit added the AF_UNSPEC initialization
and put the "x->km.state != XFRM_STATE_VALID ... goto drop" block ahead
of the family assignment.

With the current tag, 6.1.y, 5.15.y and 5.10.y have the same early drop
but would not pick up this fix automatically.

A later patch in this series, "xfrm: transfer the skb->dev reference to
the input path", is tagged for all stable branches. In that patch,
vti_input() and vti6_input_proto() take dev_hold(tunnel->dev). It relies
on the receive callback to drop that reference on every drop path,
including this early one.

In a stable branch that gets that patch but not this one, the early drop
would skip vti_rcv_cb() and leak the tunnel device reference.
unregister_netdevice would then wait forever.

Should this be:

    Fixes: 4ce3dbe397d7 ("xfrm: Fix xfrm_input() to verify state is valid when (encap_type < 0)")

and also be marked as a prerequisite of the reference-transfer patch?

> diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> index 5ed87d51392a5..ddf8fd75fd207 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;
>  		}

[Severity: High]
On async resume (encap_type == -1), can this new callback dispatch touch
skb->dev after its device reference has been dropped?

On the first pass, xfrm_input() takes a reference before calling the
type's input handler. That reference is the only thing keeping skb->dev
alive while async crypto runs:

    dev_hold(dev);

    nexthdr = x->type->input(x, skb);
    if (nexthdr == -EINPROGRESS) {

When crypto completes, xfrm_input_resume() calls
xfrm_input(skb, nexthdr, 0, -1). The SA may have left
XFRM_STATE_VALID while crypto was running, for example through hard
expiry, XFRM_MSG_DELSA or a flush. In that case the early branch drops
the reference:

    if (encap_type == -1)
        dev_put(dev);
    goto drop;

The drop label then calls the callback:

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

Before this patch, family was still AF_UNSPEC at this point. The afinfo
lookup failed and nothing touched skb->dev after the dev_put().

With family set, xfrm4_rcv_cb()/xfrm6_rcv_cb() now walk the handlers.
vti_rcv_cb() returns 1 when there is no tunnel, so the walk reaches
xfrmi_rcv_cb().

The resumed skb has a secpath, so the "err && !secpath_exists(skb)"
early return is not taken. If xfrmi_lookup() matches the SA's if_id,
this runs:

net/xfrm/xfrm_interface_core.c:xfrmi_rcv_cb() {
    ...
	link = skb->dev->ifindex;
    ...
}

At that point the last extra reference on that device has just been
dropped.

The rcu_read_lock() in xfrm_input_get_afinfo() does not seem to protect
this. netdev_wait_allrefs_any() goes ahead as soon as the refcount drops
to 1. netdev_release() then frees the device with kvfree() and does not
wait for an RCU grace period:

net/core/net-sysfs.c:netdev_release() {
    ...
	/* no need to wait for rcu grace period:
	 * device is dead and about to be freed.
	 */
	kfree(rcu_access_pointer(dev->ifalias));
	kvfree(dev);
}

The success path uses the safe order: callback under rcu_read_lock(),
then dev_put(dev). The commit message says the early drop now dispatches
the callback "like every other drop does". The drop label's order is
put first, then callback, so this patch adds a new reachable path with
that order.

The later patch "xfrm: transfer the skb->dev reference to the input
path" in this series fixes the vti/vti6 side of this. It adds
dev_hold(tunnel->dev) in vti_input() and vti6_input_proto(). It leaves
the early-branch dev_put() and the drop label's dev_put()-then-callback
order unchanged, so the xfrmi_rcv_cb() read of skb->dev->ifindex is
still reachable at the end of the series.

Would it work to let the drop label own the reference in the early
branch (for example, by setting async = 1 instead of calling dev_put()
directly)? The drop label could then call xfrm_rcv_cb() before
dev_put(dev).

> -		family = x->props.family;
> -
>  		/* An encap_type of -2 indicates reconstructed inner packet */
>  		if (encap_type == -2)
>  			goto resume_decapped;

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

[PATCH net v2 2/3] xfrm: iptfs: run the receive callbacks before freeing the skb

[Qihang]

IPTFS consumes the outer skb itself and frees it directly on several
paths, so the receive callbacks registered for the protocol never run
for those packets and whatever they cached in the skb control buffer is
never cleaned up.  Add xfrm_input_drop(), which dispatches the receive
callback with err = -1 before freeing the skb, and use it for the
outer-skb free paths in __input_process_payload(),
iptfs_input_ordered() and the reorder-window free list of iptfs_input().

Fixes: 4b3faf610cc6 ("xfrm: iptfs: add new iptfs xfrm mode impl")
Cc: stable@vger.kernel.org
Signed-off-by: Qihang <q.h.hack.winter@gmail.com>
---
 net/xfrm/xfrm_inout.h |  2 ++
 net/xfrm/xfrm_input.c | 19 +++++++++++++++++++
 net/xfrm/xfrm_iptfs.c |  6 +++---
 3 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/net/xfrm/xfrm_inout.h b/net/xfrm/xfrm_inout.h
index efc5e6b2e87b..be1f5e569ae7 100644
--- a/net/xfrm/xfrm_inout.h
+++ b/net/xfrm/xfrm_inout.h
@@ -6,6 +6,8 @@
 #ifndef XFRM_INOUT_H
 #define XFRM_INOUT_H 1
 
+void xfrm_input_drop(struct sk_buff *skb);
+
 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 ddf8fd75fd20..f293d5e124d4 100644
--- a/net/xfrm/xfrm_input.c
+++ b/net/xfrm/xfrm_input.c
@@ -120,6 +120,25 @@ static int xfrm_rcv_cb(struct sk_buff *skb, unsigned int family, u8 protocol,
 	return ret;
 }
 
+/* Finish an skb that is freed outside of xfrm_input()'s own drop path, e.g.
+ * from IPTFS.  The receive callbacks must still run before the skb goes away.
+ */
+void xfrm_input_drop(struct sk_buff *skb)
+{
+	struct sec_path *sp = skb_sec_path(skb);
+	struct xfrm_state *x;
+
+	if (!sp || !sp->len) {
+		kfree_skb(skb);
+		return;
+	}
+
+	x = sp->xvec[sp->len - 1];
+	xfrm_rcv_cb(skb, x->props.family, x->type ? x->type->proto : 0, -1);
+	kfree_skb(skb);
+}
+EXPORT_SYMBOL(xfrm_input_drop);
+
 struct sec_path *secpath_set(struct sk_buff *skb)
 {
 	struct sec_path *sp, *tmp = skb_ext_find(skb, SKB_EXT_SEC_PATH);
diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
index 6920940a35b4..4b3d04e531a5 100644
--- a/net/xfrm/xfrm_iptfs.c
+++ b/net/xfrm/xfrm_iptfs.c
@@ -1211,7 +1211,7 @@ static bool __input_process_payload(struct xfrm_state *x, u32 data,
 		 * 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);
 	}
 	return true;
 }
@@ -1298,7 +1298,7 @@ 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);
 	}
 }
 
@@ -1751,7 +1751,7 @@ 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);
 	}
 done:
 	/* We always have dealt with the input SKB, either we are re-using it,
-- 
2.54.0 (Apple Git-157)

Re: [PATCH net v2 2/3] xfrm: iptfs: run the receive callbacks before freeing the skb

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 · Medium: 1 · 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 converts only some of the IPTFS paths that free the
  outer ESP skb, so the bug its commit message describes is still…
- [High] xfrm_input_drop() now runs the per-protocol receive callbacks on
  outer IPTFS skbs that may have been parked in the reorder window.
- [Medium] xfrm_input_drop() always calls xfrm_rcv_cb() with err = -1.
- [Low] The commit message says the receive callbacks must run because
  "whatever they cached in the skb control buffer is never cleaned up".

--

xfrm: iptfs: run the receive callbacks before freeing the skb

This patch adds xfrm_input_drop(). It runs the protocol receive callback
with err = -1 and then frees the skb. The patch uses it in three places
where IPTFS frees the outer skb: __input_process_payload(),
iptfs_input_ordered() and the reorder-window freelist in iptfs_input().

> diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> index ddf8fd75fd207..f293d5e124d47 100644
> --- a/net/xfrm/xfrm_input.c
> +++ b/net/xfrm/xfrm_input.c
> @@ -120,6 +120,25 @@ static int xfrm_rcv_cb(struct sk_buff *skb, unsigned int family, u8 protocol,
>  	return ret;
>  }
>  
> +/* Finish an skb that is freed outside of xfrm_input()'s own drop path, e.g.
> + * from IPTFS.  The receive callbacks must still run before the skb goes away.
> + */

[Severity: Low]
The commit message says the callbacks need to run because "whatever they
cached in the skb control buffer is never cleaned up". Is that accurate at
this commit?

None of the cb_handlers release anything when err is nonzero.
vti_rcv_cb(), vti6_rcv_cb() and xfrmi_rcv_cb() only bump rx_errors and
rx_dropped. esp4_rcv_cb() and esp6_rcv_cb() just return 0:

net/ipv4/ip_vti.c:vti_rcv_cb() {
    ...
	if (err) {
		DEV_STATS_INC(dev, rx_errors);
		DEV_STATS_INC(dev, rx_dropped);

		return 0;
	}
    ...
}

A later patch in the series, "xfrm: transfer the skb->dev reference to the
input path", adds dev_hold() in vti_input()/vti6_input_proto() and
dev_put() in these err branches. Once that patch is applied, the
description is accurate.

This patch carries Fixes: and Cc: stable. If it were backported without
that follow-up, its only effect would be on device drop accounting. Could
the commit message mention the dependency, or be reworded?

> +void xfrm_input_drop(struct sk_buff *skb)
> +{
> +	struct sec_path *sp = skb_sec_path(skb);
> +	struct xfrm_state *x;
> +
> +	if (!sp || !sp->len) {
> +		kfree_skb(skb);
> +		return;
> +	}
> +
> +	x = sp->xvec[sp->len - 1];
> +	xfrm_rcv_cb(skb, x->props.family, x->type ? x->type->proto : 0, -1);

[Severity: Medium]
err is always -1 here, whatever the reason the skb is being freed.

In __input_process_payload(), the else if (!first_skb) branch is reached
after real errors. It is also reached after valid outer packets that were
fully consumed without starting a new inner packet. One example is a
pad-only payload:

net/xfrm/xfrm_iptfs.c:__input_process_payload() {
    ...
		} else if (iph->version == 0x0) {
			/* pad */
			data = tail;
			break;
    ...
}

Two other valid cases also end with first_skb == NULL:

- A continuation-only payload, such as the middle fragment of a large
  inner packet. Here iptfs_reassem_cont() returns data + fraglen == tail.
- A payload whose only new data is a runt saved by
  iptfs_input_save_runt().

For these valid packets, will vti_rcv_cb(), vti6_rcv_cb() and
xfrmi_rcv_cb() take their err branch and bump rx_errors and rx_dropped on
the vti/vti6/xfrm interface? Before this patch, these paths ran no
callback.

The later patch in the series still passes -1 here, so the counts would
still be wrong at the end of the series.

> +	kfree_skb(skb);
> +}
> +EXPORT_SYMBOL(xfrm_input_drop);

[ ... ]

> diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
> index 6920940a35b49..4b3d04e531a51 100644
> --- a/net/xfrm/xfrm_iptfs.c
> +++ b/net/xfrm/xfrm_iptfs.c
> @@ -1211,7 +1211,7 @@ static bool __input_process_payload(struct xfrm_state *x, u32 data,
>  		 * 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);

[Severity: High]
This converts the !first_skb branch. Does the branch next to it still free
the outer skb without running the callbacks?

	if (defer) {
		consume_skb(defer);
	} else if (!first_skb) {

defer is the outer root skb whenever the first inner packet can't reuse
it. That covers the skb_has_frag_list() case, the head_frag/fragwalk case
and the allocate-new fallback.

defer is also the reused outer skb when pskb_trim() fails:

		if (pskb_trim(first_skb, first_iplen)) {
			/* error trimming */
			list_del(&first_skb->list);
			defer = first_skb;
		}

In both cases the skb is freed by consume_skb(defer) and no callback
runs. The inner skbs come from iptfs_alloc_skb(), which zeroes cb, so no
callback ever sees the outer skb's cb.

Other outer-skb free paths also skip xfrm_rcv_cb():

- When the reused outer skb becomes xtfs->ra_newskb (first_skb_partial),
  it can be freed by iptfs_drop_timer() via kfree_skb_reason(skb,
  SKB_DROP_REASON_FRAG_REASM_TIMEOUT).
- It can also be freed by __iptfs_reassem_done() with free == true, via
  iptfs_reassem_abort().
- It can also be freed by iptfs_destroy_state().
- iptfs_destroy_state() frees the outer skbs still in xtfs->w_saved[] with
  plain kfree_skb().

The commit message says IPTFS "frees it directly on several paths, so the
receive callbacks ... never run". The new comment says "The receive
callbacks must still run before the skb goes away."

The last patch in the series adds dev_hold(tunnel->dev) in
vti_input()/vti6_input_proto(). It relies on vti_rcv_cb()/vti6_rcv_cb()
calling dev_put() on the error path, and it doesn't touch xfrm_iptfs.c.

At the end of the series, would each outer skb freed on these paths leak
a reference on the vti/vti6 net_device? Deleting the device or its netns
would then hang in netdev_wait_allrefs().

>  	}
>  	return true;
>  }
> @@ -1298,7 +1298,7 @@ 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);

[Severity: High]
iptfs_input_ordered() also processes outer skbs that
__reorder_future_fits() and __reorder_future_shifts() parked in the
reorder window. They are drained later, either from another softirq or
from the iptfs_drop_timer() soft hrtimer:

iptfs_drop_timer()
    iptfs_input_ordered()
        __input_process_payload() / !consumed
            xfrm_input_drop()
                xfrm_rcv_cb()
                    xfrmi_rcv_cb()

Nothing holds a reference on skb->dev or on the cached
XFRM_TUNNEL_SKB_CB()->tunnel while the skb is parked. With synchronous
crypto, xfrm_input() calls dev_put(dev) right after x->type->input()
returns -EINPROGRESS.

xfrmi_rcv_cb() dereferences skb->dev before it checks err:

net/xfrm/xfrm_interface_core.c:xfrmi_rcv_cb() {
    ...
	link = skb->dev->ifindex;
	dev = xi->dev;
	skb->dev = dev;

	if (err) {
    ...
}

If the ingress netdev is unregistered while the skb sits in w_saved, can
this read a freed net_device? IPTFS has no netdev notifier, and
unregistering the device doesn't flush w_saved. Before this patch, these
paths only called kfree_skb(), which doesn't dereference skb->dev.

At this commit, vti_rcv_cb()/vti6_rcv_cb() also call DEV_STATS_INC()
through tunnel->dev without holding a reference. If the vti device was
deleted in the meantime, that would write to freed memory.

The later patch "xfrm: transfer the skb->dev reference to the input path"
fixes the vti part with dev_hold() in vti_input()/vti6_input_proto(). The
xfrmi_rcv_cb() case still looks open at the end of the series.

The success path, xfrm_input(skb, 0, 0, -2) -> dev_net(skb->dev), already
has the same dangling skb->dev problem. That part is pre-existing, but
this patch extends it to the drop paths.

The freelist in iptfs_input() only ever holds the current inskb, so that
hunk isn't affected.

>  	}
>  }

[ ... ]

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

[PATCH net v2 3/3] xfrm: transfer the skb->dev reference to 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

Taking a reference while the pointer is cached and dropping it in the
callback does not fix that: on the success path the callback drops what
may be the last reference before xfrm_input() hands the skb to
gro_cells_receive(), and taking a new reference on skb->dev at that
point is already too late, the device may have been freed in between:

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

Make the reference travel with the skb instead: the receive callbacks
install the device in skb->dev and leave their reference with it, and
xfrm_input() releases it once the packet has been delivered.  vti,
vti6 and xfrmi -- the only callbacks that replace skb->dev -- take the
reference when installing the device and drop it on their error paths.

Tested on arm64 with KASAN and an async AEAD: no use-after-free on
either completion path, and RTM_DELLINK on a VTI device with pending
async crypto now waits for the completion.

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              |  6 ++++++
 net/ipv6/ip6_vti.c             |  7 +++++++
 net/xfrm/xfrm_input.c          | 13 +++++++++++++
 net/xfrm/xfrm_interface_core.c |  9 ++++++++-
 4 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/ip_vti.c b/net/ipv4/ip_vti.c
index 3b80929994a0..1964482064c1 100644
--- a/net/ipv4/ip_vti.c
+++ b/net/ipv4/ip_vti.c
@@ -63,6 +63,8 @@ static int vti_input(struct sk_buff *skb, int nexthdr, __be32 spi,
 
 		XFRM_TUNNEL_SKB_CB(skb)->tunnel.ip4 = tunnel;
 
+		dev_hold(tunnel->dev);
+
 		if (update_skb_dev)
 			skb->dev = tunnel->dev;
 
@@ -109,10 +111,14 @@ static int vti_rcv_cb(struct sk_buff *skb, int err)
 
 	dev = tunnel->dev;
 
+	/* Drop the reference taken in vti_input(); on success it is
+	 * transferred with skb->dev and released by xfrm_input().
+	 */
 	if (err) {
 		DEV_STATS_INC(dev, rx_errors);
 		DEV_STATS_INC(dev, rx_dropped);
 
+		dev_put(dev);
 		return 0;
 	}
 
diff --git a/net/ipv6/ip6_vti.c b/net/ipv6/ip6_vti.c
index ab94b3a4ba9c..a47acae4756c 100644
--- a/net/ipv6/ip6_vti.c
+++ b/net/ipv6/ip6_vti.c
@@ -322,6 +322,9 @@ static int vti6_input_proto(struct sk_buff *skb, int nexthdr, __be32 spi,
 		rcu_read_unlock();
 
 		XFRM_TUNNEL_SKB_CB(skb)->tunnel.ip6 = 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, encap_type);
@@ -355,10 +358,14 @@ static int vti6_rcv_cb(struct sk_buff *skb, int err)
 
 	dev = t->dev;
 
+	/* Drop the reference taken in vti6_input_proto(); on success it is
+	 * transferred with skb->dev and released by xfrm_input().
+	 */
 	if (err) {
 		DEV_STATS_INC(dev, rx_errors);
 		DEV_STATS_INC(dev, rx_dropped);
 
+		dev_put(dev);
 		return 0;
 	}
 
diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
index f293d5e124d4..bdd493f9d877 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)
 	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;
@@ -767,7 +768,13 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 			skb_dst_drop(skb);
 		if (async)
 			dev_put(dev);
+		/* Release the reference a receive callback left with
+		 * skb->dev, after the packet has been delivered.
+		 */
+		tdev = skb->dev != dev ? skb->dev : NULL;
 		gro_cells_receive(&gro_cells, skb);
+		if (tdev)
+			dev_put(tdev);
 		rcu_read_unlock();
 		return 0;
 	} else {
@@ -775,6 +782,10 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 		if (xo)
 			xfrm_gro = xo->flags & XFRM_GRO;
 
+		/* Release the reference a receive callback left with
+		 * skb->dev, after the packet has been delivered.
+		 */
+		tdev = skb->dev != dev ? skb->dev : NULL;
 		err = -EAFNOSUPPORT;
 		afinfo = xfrm_state_afinfo_get_rcu(family);
 		if (likely(afinfo))
@@ -788,6 +799,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();
diff --git a/net/xfrm/xfrm_interface_core.c b/net/xfrm/xfrm_interface_core.c
index 688306bf62c5..f4859be7ed18 100644
--- a/net/xfrm/xfrm_interface_core.c
+++ b/net/xfrm/xfrm_interface_core.c
@@ -375,11 +375,18 @@ static int xfrmi_rcv_cb(struct sk_buff *skb, int err)
 
 	link = skb->dev->ifindex;
 	dev = xi->dev;
-	skb->dev = dev;
+	if (skb->dev != dev) {
+		/* Reference travels with skb->dev; released by xfrm_input()
+		 * after delivery.  Guard makes a re-invocation a no-op.
+		 */
+		dev_hold(dev);
+		skb->dev = dev;
+	}
 
 	if (err) {
 		DEV_STATS_INC(dev, rx_errors);
 		DEV_STATS_INC(dev, rx_dropped);
+		dev_put(dev);
 
 		return 0;
 	}
-- 
2.54.0 (Apple Git-157)

Re: [PATCH net v2 3/3] xfrm: transfer the skb->dev reference to the input path

[netdev-bot+sashiko]

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

Critical: 0 · High: 4 · Medium: 2 · 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] In xfrmi_rcv_cb() (net/xfrm/xfrm_interface_core.c),
  dev_hold(dev) only runs when skb->dev != xi->dev (line 378-383), but…
- [High] vti_input() (net/ipv4/ip_vti.c:66) and vti6_input_proto()
  (net/ipv6/ip6_vti.c:326) always call dev_hold(tunnel->dev).
- [High] The dev_hold() added in vti_input()/vti6_input_proto() is only
  released if a receive callback later runs for the same skb.
- [High] In the decaps and GRO paths, xfrm_input() calls dev_put(tdev)
  right after gro_cells_receive() returns (net/xfrm/xfrm_input.c:775-777…
- [Medium] In xfrmi_rcv_cb(), the err==0 call takes the reference
  (dev_hold(xi->dev), skb->dev = xi->dev) and can still fail afterwards:…
- [Medium] For a cross-namespace collect_md xfrm interface (xnet true,
  xi->p.collect_md), xfrmi_rcv_cb() takes dev_hold(xi->dev) and calls…

--

xfrm: transfer the skb->dev reference to the input path

This patch has vti_input() and vti6_input_proto() take a reference on the
tunnel device. xfrmi_rcv_cb() takes one when it installs the device in
skb->dev. xfrm_input() then releases that reference after the skb has been
handed to gro_cells_receive() or transport_finish(), and the callbacks
drop it on their error paths.

> diff --git a/net/ipv4/ip_vti.c b/net/ipv4/ip_vti.c
> index 3b80929994a00..1964482064c16 100644
> --- a/net/ipv4/ip_vti.c
> +++ b/net/ipv4/ip_vti.c
> @@ -63,6 +63,8 @@ static int vti_input(struct sk_buff *skb, int nexthdr, __be32 spi,
>  
>  		XFRM_TUNNEL_SKB_CB(skb)->tunnel.ip4 = tunnel;
>  
> +		dev_hold(tunnel->dev);
> +
>  		if (update_skb_dev)
>  			skb->dev = tunnel->dev;
>  

[Severity: High]
This hold is unconditional. On the success path, xfrm_input() only
releases it if skb->dev changed during the call:

net/xfrm/xfrm_input.c:xfrm_input() {
    struct net_device *dev = skb->dev;
    ...
    tdev = skb->dev != dev ? skb->dev : NULL;
    gro_cells_receive(&gro_cells, skb);
    if (tdev)
        dev_put(tdev);
    ...
}

What happens if skb->dev already is tunnel->dev when xfrm_input() is
entered? In that case the skb->dev = dev assignment in vti_rcv_cb()
changes nothing and tdev stays NULL. The reference taken here would then
leak for every packet that is delivered successfully.

Nested ESP decapsulated twice by the same VTI looks like one way to get
there. For a keyless inner packet, ip_tunnel_lookup() can return the same
tunnel through a wildcard VTI (saddr=daddr=0), the ip_vti0 fallback, or
matching inner endpoints, because the link is only a preference.
vti6_tnl_lookup() matches only on addresses plus tnls_wc, so the IPv6 side
looks the same.

The update_skb_dev branch just below also sets skb->dev = tunnel->dev
before xfrm_input() runs. No in-tree caller passes true today, but if one
did, wouldn't every packet leak a reference?

Would ip link del or netns teardown then hang in netdev_wait_allrefs_any()
on this device?

In this case the new comment in vti_rcv_cb(), "on success it is
transferred with skb->dev and released by xfrm_input()", doesn't hold.
xfrmi_rcv_cb() also only takes its reference when skb->dev changes, so
vti/vti6 and xfrmi follow different rules for the same release check.

[ ... ]

> diff --git a/net/ipv6/ip6_vti.c b/net/ipv6/ip6_vti.c
> index ab94b3a4ba9c3..a47acae4756c2 100644
> --- a/net/ipv6/ip6_vti.c
> +++ b/net/ipv6/ip6_vti.c
> @@ -322,6 +322,9 @@ static int vti6_input_proto(struct sk_buff *skb, int nexthdr, __be32 spi,
>  		rcu_read_unlock();
>  
>  		XFRM_TUNNEL_SKB_CB(skb)->tunnel.ip6 = 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, encap_type);

[Severity: High]
The reference taken here, and the one in vti_input(), is only dropped if a
receive callback later runs for this same skb. Is that guaranteed when the
SA is in IPTFS mode?

iptfs_input() always returns -EINPROGRESS. xfrm_input() then returns
without calling xfrm_rcv_cb() and without reaching the tdev release:

	err = xfrm_inner_mode_input(x, skb);
	if (err == -EINPROGRESS) {
		if (async)
			dev_put(dev);
		return 0;

IPTFS then frees the outer skb on several paths that skip the callbacks:

net/xfrm/xfrm_iptfs.c:__input_process_payload() {
    ...
	if (defer) {
		consume_skb(defer);
    ...
}

If the original skb is reused for a partial inner packet, it becomes
xtfs->ra_newskb. That skb is later freed in one of three places:

  kfree_skb() in __iptfs_reassem_done()
  kfree_skb_reason(skb, SKB_DROP_REASON_FRAG_REASM_TIMEOUT) in
  iptfs_drop_timer()
  kfree_skb() in iptfs_delete_state()

iptfs_delete_state() also frees the reorder-window skbs in w_saved with
kfree_skb().

The inner skbs built by iptfs_alloc_skb() copy dev and the skb extensions
but not cb. Their tunnel pointer is therefore NULL, and vti_rcv_cb() and
vti6_rcv_cb() return 1 for them without dropping the outer skb's
reference.

Could each of these outer packets leak one reference on the VTI device? A
reassembly timeout after ordinary packet loss looks like enough to trigger
it. This still seems to be true with the preceding commit "xfrm: iptfs:
run the receive callbacks before freeing the skb" applied.

[ ... ]

> diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> index f293d5e124d47..bdd493f9d877a 100644
> --- a/net/xfrm/xfrm_input.c
> +++ b/net/xfrm/xfrm_input.c

[ ... ]

> @@ -767,7 +768,13 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
>  			skb_dst_drop(skb);
>  		if (async)
>  			dev_put(dev);
> +		/* Release the reference a receive callback left with
> +		 * skb->dev, after the packet has been delivered.
> +		 */
> +		tdev = skb->dev != dev ? skb->dev : NULL;
>  		gro_cells_receive(&gro_cells, skb);
> +		if (tdev)
> +			dev_put(tdev);

[Severity: High]
Has the packet actually been delivered at this point?
gro_cells_receive() only queues the skb on the per-CPU xfrm gro_cells and
calls napi_schedule(). The skb is processed later, in
gro_cell_poll()->napi_gro_receive(), which dereferences skb->dev.

Nothing purges the global xfrm gro_cells when a vti or xfrmi device is
unregistered. netdev_run_todo() frees the device as soon as
netdev_wait_allrefs_any() sees the refcount at 1.

Suppose the gro_cell NAPI poll is deferred (to ksoftirqd, on PREEMPT_RT,
or after a crypto completion in process context). Can a concurrent
RTM_DELLINK then free the device before gro_cell_poll() touches skb->dev?
The xfrm_gro branch below drops tdev the same way.

xfrm_trans_queue_net() in this file holds skb->dev across deferred
delivery:

	if (skb->dev)
		dev_hold(skb->dev);

This window existed before the patch. However, the commit message says the
reference is released "once the packet has been delivered" and that
RTM_DELLINK "now waits for the completion". Neither seems to be true for
this gro_cells path, which is the tunnel-mode path from the KASAN report in
the commit message.

>  		rcu_read_unlock();
>  		return 0;
>  	} else {

[ ... ]

> diff --git a/net/xfrm/xfrm_interface_core.c b/net/xfrm/xfrm_interface_core.c
> index 688306bf62c5a..f4859be7ed189 100644
> --- a/net/xfrm/xfrm_interface_core.c
> +++ b/net/xfrm/xfrm_interface_core.c
> @@ -375,11 +375,18 @@ static int xfrmi_rcv_cb(struct sk_buff *skb, int err)
>  
>  	link = skb->dev->ifindex;
>  	dev = xi->dev;
> -	skb->dev = dev;
> +	if (skb->dev != dev) {
> +		/* Reference travels with skb->dev; released by xfrm_input()
> +		 * after delivery.  Guard makes a re-invocation a no-op.
> +		 */
> +		dev_hold(dev);
> +		skb->dev = dev;
> +	}

[Severity: High]
The hold only happens when skb->dev != xi->dev, but the dev_put() added to
the err branch below always runs. What happens with nested IPsec that
terminates on the same xfrm interface?

After the outer tunnel-mode SA is decapsulated, skb->dev is the xfrmi
device. The inner ESP then comes back in through:

xfrm4_rcv()->xfrmi4_rcv()->xfrmi_input()->xfrm_input()

At that point skb->dev == xi->dev, and the secpath is still there because
the non-xnet scrub does not reset it. xfrmi_lookup() can map the inner SA
to the same xi, either through a matching if_id or through the
collect_md_xfrmi fallback:

	xi = rcu_dereference(xfrmn->collect_md_xfrmi);
	if (xi && (xi->dev->flags & IFF_UP))
		return xi;

In that case this block takes no hold. Any drop after the inner SA has been
added to the secpath (replay check, ICV failure, hard lifetime expiry and
so on) goes through the drop label in xfrm_input():

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

That reaches the dev_put() below with no matching hold. The same happens
when the err == 0 call returns -EPERM, -EINVAL or -ENOMEM and the callback
is called again with err == -1.

Can this underflow the xfrmi refcount by one for every dropped packet? On
unregister, netdev_wait_allrefs_any() could then return while real holders
still exist. The "Guard makes a re-invocation a no-op" comment only seems
true when the first call took the reference.

>  
>  	if (err) {
>  		DEV_STATS_INC(dev, rx_errors);
>  		DEV_STATS_INC(dev, rx_dropped);
> +		dev_put(dev);
>  
>  		return 0;
>  	}

[Severity: Medium]
Suppose the err == 0 call takes the reference and then fails: -EINVAL for
a NULL inner mode or -EPERM from xfrm_policy_check() in the xnet case, or
-ENOMEM from metadata_dst_alloc(). The release then depends on the
err == -1 call from the drop label reaching this dev_put().

That second call repeats xfrmi_lookup() after xfrm_input() has done
rcu_read_unlock(), and no lock is held between the two calls:

	xi = xfrmi_lookup(xs_net(x), x);
	if (!xi)
		return 1;

If the interface is set down (IFF_UP cleared), or deleted through
xfrmi_dev_uninit()->xfrmi_unlink(), inside that window, the lookup returns
NULL and the dev_put() is skipped. If the lookup falls back to
collect_md_xfrmi instead, a different device gets held and put.

Since kfree_skb() does not release skb->dev, is the reference on the
original device leaked in either case?

[Severity: Medium]
Can the -ENOMEM path also skip this dev_put() for a cross-netns
collect_md interface? With xnet true, xfrmi_scrub_packet() runs after the
hold and resets the secpath:

	if (!xnet)
		return;

	ipvs_reset(skb);
	secpath_reset(skb);

Now suppose metadata_dst_alloc() fails:

		md_dst = metadata_dst_alloc(0, METADATA_XFRM, GFP_ATOMIC);
		if (!md_dst)
			return -ENOMEM;

The err == -1 call from xfrm_input() then returns early at the top of
xfrmi_rcv_cb(), because the secpath is gone:

	if (err && !secpath_exists(skb))
		return 0;

Would the xfrmi reference then never be released?

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

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