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 Reported-by: Shubham Antil Fixes: 6c82d2433671 ("xfrm: iptfs: add basic receive packet (tunnel egress) handling") Signed-off-by: Roshan Kumar --- 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