[PATCH v2] xfrm: iptfs: hold a device reference while packets are queued

[PATCH v2] xfrm: iptfs: hold a device reference while packets are queued

[Roshan Kumar]

IPTFS receive may retain skbs past the return of iptfs_input(): out of
order outer packets are parked in the reorder window (w_saved) and a
partially received inner packet is kept in ra_newskb until more data
arrives or the drop timer fires.  The retained skbs keep the pointer
to the ingress net_device that was copied from the tunnel packet, but
no reference is taken on it.  If the ingress device is unregistered
while any of these skbs is queued, the later processing in
iptfs_drop_timer() and the xfrm_input() restart path dereference a
freed net_device:

  BUG: KASAN: use-after-free in xfrm_input+0x45d6/0x59c0
   iptfs_complete_inner_skb
   __input_process_payload
   iptfs_input_ordered
   iptfs_drop_timer

Take a reference on skb->dev when a packet is placed in the reorder
window or kept as the in-progress reassembly skb, and drop it when the
queued skb is delivered back into the stack or freed.  The drop timer
and the reassembly queues are bounded by the configured drop time, so
the extra reference delays device unregistration by at most that
amount.

Reported-by: Roshan Kumar <roshaen09@gmail.com>
Reported-by: Shubham Antil <shubham@octane.security>
Fixes: 6c82d2433671 ("xfrm: iptfs: add basic receive packet (tunnel egress) handling")
Signed-off-by: Roshan Kumar <roshaen09@gmail.com>
---
v2: take the reference on the incoming packet at the reorder dispatch
site instead of inside __reorder_this().  __reorder_this() is also
called with an already referenced buffer from the window shift path,
so taking it there could leak a reference.

 net/xfrm/xfrm_iptfs.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
index 597aedeac..0a1a823f6 100644
--- a/net/xfrm/xfrm_iptfs.c
+++ b/net/xfrm/xfrm_iptfs.c
@@ -701,6 +701,8 @@ 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 (xtfs->ra_newskb)
+		dev_put(xtfs->ra_newskb->dev);
 	if (free)
 		kfree_skb(xtfs->ra_newskb);
 	xtfs->ra_newskb = NULL;
@@ -837,6 +839,7 @@ static u32 iptfs_reassem_cont(struct xfrm_iptfs_data *xtfs, u64 seq,
 			goto abandon;
 		}
 		xtfs->ra_newskb = newskb;
+		dev_hold(newskb->dev);
 
 		/* Copy the runt data into the buffer, but leave data
 		 * pointers the same as normal non-runt case. The extra `rrem`
@@ -1153,6 +1156,7 @@ static bool __input_process_payload(struct xfrm_state *x, u32 data,
 			spin_lock(&xtfs->drop_lock);
 
 			xtfs->ra_newskb = skb;
+			dev_hold(skb->dev);
 			xtfs->ra_wantseq = seq + 1;
 			if (!hrtimer_is_queued(&xtfs->drop_timer)) {
 				/* softirq blocked lest the timer fire and interrupt us */
@@ -1467,6 +1471,7 @@ static void __reorder_future_fits(struct xfrm_iptfs_data *xtfs,
 	}
 
 	xtfs->w_saved[index].skb = inskb;
+	dev_hold(inskb->dev);
 	xtfs->w_savedlen = max(savedlen, index + 1);
 	iptfs_set_window_drop_times(xtfs, index);
 }
@@ -1602,6 +1607,7 @@ static void __reorder_future_shifts(struct xfrm_iptfs_data *xtfs,
 	/* We've shifted. plug the packet in at the end. */
 	xtfs->w_savedlen = nslots - 1;
 	xtfs->w_saved[xtfs->w_savedlen - 1].skb = inskb;
+	dev_hold(inskb->dev);
 	iptfs_set_window_drop_times(xtfs, xtfs->w_savedlen - 1);
 
 	/* if we don't have a slot0 then we must wait for it */
@@ -1633,8 +1639,10 @@ static void iptfs_input_reorder(struct xfrm_iptfs_data *xtfs,
 	}
 	wantseq = xtfs->w_wantseq;
 
-	if (likely(inseq == wantseq))
+	if (likely(inseq == wantseq)) {
+		dev_hold(inskb->dev);
 		__reorder_this(xtfs, inskb, list);
+	}
 	else if (inseq < wantseq)
 		__reorder_past(xtfs, inskb, freelist);
 	else if ((inseq - wantseq) < nslots)
@@ -1691,13 +1699,18 @@ static enum hrtimer_restart iptfs_drop_timer(struct hrtimer *me)
 
 	spin_unlock(&xtfs->drop_lock);
 
-	if (skb)
+	if (skb) {
+		dev_put(skb->dev);
 		kfree_skb_reason(skb, SKB_DROP_REASON_FRAG_REASM_TIMEOUT);
+	}
 
 	if (count) {
 		list_for_each_entry_safe(skb, next, &list, list) {
+			struct net_device *dev = skb->dev;
+
 			skb_list_del_init(skb);
 			iptfs_input_ordered(x, skb);
+			dev_put(dev);
 		}
 	}
 
@@ -1737,8 +1750,11 @@ static int iptfs_input(struct xfrm_state *x, struct sk_buff *skb)
 	spin_unlock(&xtfs->drop_lock);
 
 	list_for_each_entry_safe(skb, next, &list, list) {
+		struct net_device *dev = skb->dev;
+
 		skb_list_del_init(skb);
 		iptfs_input_ordered(x, skb);
+		dev_put(dev);
 	}
 
 	list_for_each_entry_safe(skb, next, &freelist, list) {
@@ -2743,12 +2759,16 @@ static void iptfs_destroy_state(struct xfrm_state *x)
 
 	hrtimer_cancel(&xtfs->drop_timer);
 
-	if (xtfs->ra_newskb)
+	if (xtfs->ra_newskb) {
+		dev_put(xtfs->ra_newskb->dev);
 		kfree_skb(xtfs->ra_newskb);
+	}
 
 	for (s = xtfs->w_saved, se = s + xtfs->w_savedlen; s < se; s++) {
-		if (s->skb)
+		if (s->skb) {
+			dev_put(s->skb->dev);
 			kfree_skb(s->skb);
+		}
 	}
 
 	kfree_sensitive(xtfs->w_saved);
-- 
2.43.0

Re: [PATCH v2] xfrm: iptfs: hold a device reference while packets are queued

[netdev-bot+sashiko]

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

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

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] Unbounded net_device reference retention: the patch pins the
  ingress net_device for every skb parked in the reorder window…
- [High] Refcount/UAF race: the newly added device reference for the in-
  progress reassembly skb is released before the skb's last use.
- [Medium] The KASAN backtrace pasted in the commit message cannot have
  been produced by this code.
- [Low] Undisclosed behavioural/hot-path change and split ownership
  convention.
- [Low] Coding style (checkpatch error) in the modified hunk:
  iptfs_input_reorder() now braces the first arm of the if/else-if chain…

--

xfrm: iptfs: hold a device reference while packets are queued

This takes a net_device reference on skb->dev when an outer packet is
parked in the IPTFS reorder window (w_saved) or kept as the in-progress
reassembly skb (ra_newskb), and drops it when the retained skb is
delivered or freed.  Release sites are added in iptfs_drop_timer(),
iptfs_input(), __iptfs_reassem_done() and iptfs_destroy_state().

A few questions about the commit message and the reference accounting
below.

>   BUG: KASAN: use-after-free in xfrm_input+0x45d6/0x59c0
>    iptfs_complete_inner_skb
>    __input_process_payload
>    iptfs_input_ordered
>    iptfs_drop_timer

[Severity: Medium]
Can this backtrace be produced by the current code?  The frame order
presents iptfs_complete_inner_skb() as the caller of xfrm_input(), but
iptfs_complete_inner_skb() in net/xfrm/xfrm_iptfs.c only resets the
network and transport headers, sets ip_summed and fixes up DSCP/ECN and
skb->protocol:

net/xfrm/xfrm_iptfs.c:iptfs_complete_inner_skb() {
	skb_reset_network_header(skb);
	skb_reset_transport_header(skb);
	skb->ip_summed = CHECKSUM_NONE;
	...
		skb->protocol = htons(ETH_P_IPV6);
	}
}

The only xfrm_input() call on that path is in the delivery loop of
__input_process_payload():

		if (xfrm_input(skb, 0, 0, -2))
			kfree_skb(skb);

Both Reported-by: tags also name the patch author and there is no
Closes: tag or reproducer reference.  Could the trace be replaced with
one that matches the code, or with a reproducer?

> diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
> index 6920940a35b49..ce5c6170d249d 100644
> --- a/net/xfrm/xfrm_iptfs.c
> +++ b/net/xfrm/xfrm_iptfs.c
> @@ -709,6 +709,8 @@ 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 (xtfs->ra_newskb)
> +		dev_put(xtfs->ra_newskb->dev);
>  	if (free)
>  		kfree_skb(xtfs->ra_newskb);
>  	xtfs->ra_newskb = NULL;

[Severity: High]
The dev_put() here runs for both values of free, but in the free ==
false case the skb is not freed, it is handed on for delivery:

net/xfrm/xfrm_iptfs.c:iptfs_reassem_cont() {
	...
	} else {
		/* We are done with packet reassembly! */
		iptfs_reassem_done(xtfs);	/* __iptfs_reassem_done(xtfs, false) */
		iptfs_complete_inner_skb(xtfs->x, newskb);
		list_add_tail(&newskb->list, list);
	}
	...
}

and the list is then delivered in __input_process_payload():

		if (xfrm_input(skb, 0, 0, -2))
			kfree_skb(skb);

xfrm_input() dereferences the device immediately:

net/xfrm/xfrm_input.c:xfrm_input() {
	struct net *net = dev_net(skb->dev);
	struct net_device *dev = skb->dev;
	...
}

ra_newskb->dev was copied from an older outer packet in
iptfs_alloc_skb() ("xfrm_input resume needs dev and xfrm ext from tunnel
pkt"), so it need not be the device of the packet currently being
processed, and in the reorder_win_size == 0 fast path no reference is
held on the current outer skb either.

Can the dev_put() here therefore drop the last reference, so that
dev_net(skb->dev) in xfrm_input() reads a freed net_device?  Would
moving it under the free branch, matching the ordering used in the new
iptfs_input() loop, keep the reference alive until after delivery?

		struct net_device *dev = skb->dev;

		skb_list_del_init(skb);
		iptfs_input_ordered(x, skb);
		dev_put(dev);

> @@ -845,6 +847,7 @@ static u32 iptfs_reassem_cont(struct xfrm_iptfs_data *xtfs, u64 seq,
>  			goto abandon;
>  		}
>  		xtfs->ra_newskb = newskb;
> +		dev_hold(newskb->dev);
>  
>  		/* Copy the runt data into the buffer, but leave data
>  		 * pointers the same as normal non-runt case. The extra `rrem`
> @@ -1161,6 +1164,7 @@ static bool __input_process_payload(struct xfrm_state *x, u32 data,
>  			spin_lock(&xtfs->drop_lock);
>  
>  			xtfs->ra_newskb = skb;
> +			dev_hold(skb->dev);
>  			xtfs->ra_wantseq = seq + 1;
>  			if (!hrtimer_is_queued(&xtfs->drop_timer)) {
>  				/* softirq blocked lest the timer fire and interrupt us */
> @@ -1475,6 +1479,7 @@ static void __reorder_future_fits(struct xfrm_iptfs_data *xtfs,
>  	}
>  
>  	xtfs->w_saved[index].skb = inskb;
> +	dev_hold(inskb->dev);
>  	xtfs->w_savedlen = max(savedlen, index + 1);
>  	iptfs_set_window_drop_times(xtfs, index);
>  }

[Severity: High]
The commit message says:

  "The drop timer and the reassembly queues are bounded by the
  configured drop time, so the extra reference delays device
  unregistration by at most that amount."

Does that bound hold for the window slots?  __iptfs_reassem_done()
cancels the drop timer unconditionally, and that single hrtimer is
shared between reassembly and the reorder window:

	hrtimer_try_to_cancel(&xtfs->drop_timer);

Once cancelled, iptfs_set_window_drop_times() only re-arms it when the
walk back reaches an empty slot 0:

	if (savedlen > index + 1) {
		/* we are below another, our drop time and the timer are already set */
		return;
	}
	...
	if (index == -1 && !hrtimer_is_queued(&xtfs->drop_timer))
		hrtimer_start(&xtfs->drop_timer, xtfs->drop_time_ns,
			      IPTFS_HRTIMER_MODE);

So with an occupied lower slot, is the timer ever re-armed?

For example, with wantseq == N: packet N starts a fragmented inner
packet (ra_newskb set, timer armed), N+2 and N+4 arrive out of order and
are saved with dev_hold(), then N+1 arrives in order and __reorder_this()
flushes N+1 and N+2 and shifts, leaving N+4 in slot 0.  If the
reassembly completes while processing N+1, iptfs_reassem_done() cancels
the timer while N+4 and its device reference are still parked.  If the
peer stops sending, does unregister_netdevice() for that ingress device
then spin forever in netdev_wait_allrefs_any(), which also blocks the
global cleanup_net worker?

The runt path above looks similar: iptfs_reassem_cont() takes
dev_hold(newskb->dev) on the path entered from iptfs_input_save_runt(),
which only sets ra_runtlen and ra_wantseq and does not start the drop
timer, unlike the __input_process_payload() site which does. Is anything
other than iptfs_destroy_state() guaranteed to release that reference?

On the bound itself, drop_time_ns comes from XFRMA_IPTFS_DROP_TIME as
u32 microseconds without a clamp, so even in the well behaved case the
delay can be around 71 minutes.  Is that worth mentioning in the
changelog?

> @@ -1610,6 +1615,7 @@ static void __reorder_future_shifts(struct xfrm_iptfs_data *xtfs,
>  	/* We've shifted. plug the packet in at the end. */
>  	xtfs->w_savedlen = nslots - 1;
>  	xtfs->w_saved[xtfs->w_savedlen - 1].skb = inskb;
> +	dev_hold(inskb->dev);
>  	iptfs_set_window_drop_times(xtfs, xtfs->w_savedlen - 1);
>  
>  	/* if we don't have a slot0 then we must wait for it */
> @@ -1641,8 +1647,10 @@ static void iptfs_input_reorder(struct xfrm_iptfs_data *xtfs,
>  	}
>  	wantseq = xtfs->w_wantseq;
>  
> -	if (likely(inseq == wantseq))
> +	if (likely(inseq == wantseq)) {
> +		dev_hold(inskb->dev);
>  		__reorder_this(xtfs, inskb, list);
> +	}

[Severity: Low]
This isn't a bug, but the changelog only mentions taking a reference
"when a packet is placed in the reorder window or kept as the in-progress
reassembly skb".  This hold is on the in-order packet, which is never
retained past iptfs_input(): it is released a few lines later in the
delivery loop of iptfs_input().  Could the changelog mention that every
in-order received packet now does a dev_hold()/dev_put() pair when
reorder_win_size is non-zero, while the reorder_win_size == 0 fast path
takes none?

The ownership convention also differs between neighbouring helpers:
iptfs_input_reorder() takes the hold for __reorder_this(), the callees
take it in __reorder_future_fits() and __reorder_future_shifts(), and
__reorder_past() plus the duplicate-future case take none, with freelist
entries freed without dev_put().  Releases are spread over both delivery
loops, __iptfs_reassem_done() and iptfs_destroy_state().  Would a short
comment stating who owns the reference help, given the v2 note already
records one near-miss leak from this ambiguity?

>  	else if (inseq < wantseq)
    ^^^^

[Severity: Low]
checkpatch reports an error for this: "else should follow close brace
'}'".  CodingStyle also asks that all arms of the chain use braces once
one arm does.

>  		__reorder_past(xtfs, inskb, freelist);
>  	else if ((inseq - wantseq) < nslots)

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921084743.817859-1-roshaen09%40gmail.com

Re: [PATCH v2] xfrm: iptfs: hold a device reference while packets are queued

[Roshan Kumar]

Thanks for the detailed review. The findings are valid. I have reworked the
next revision to keep the reassembly device reference through the nested
xfrm_input() processing. Reassembly and reorder window deadlines are now
tracked independently, so completing one does not strand the other or apply a
stale deadline to new state. The timer is also armed for reassembly created
from a runt.

The reference is released on every completion, timeout, abort, and teardown
path. The code uses netdev_hold() and netdev_put(), documents ownership, and
fixes the style issues. I replaced the mismatched pasted trace with a concise
description verified against the KASAN reproducer, which remains private. I
also removed the incorrect bounded lifetime claim and the self reporting
credit.

I have split the timer correction into a prerequisite patch. The resulting
series now passes the ten case KASAN and reference tracker lifecycle matrix on
both the regular kernel and the lockdep and RCU debug kernel. It also passes
the stale deadline overlap and runt created reassembly regressions. I will
coordinate the timer prerequisite before posting v3 as a new thread.

pw-bot: cr