[PATCH net] net/sched: sch_teql: restore skb->dev on the slave failure path

[PATCH net] net/sched: sch_teql: restore skb->dev on the slave failure path

[Victor Nogueira]

teql_master_xmit() sets skb->dev = slave before calling the slave's
ndo_start_xmit(), but never restores it when that transmit fails. The
skb then walks on to the next slave still pointing at the previous one.

If a later slave has no resolved neighbour, teql_resolve() hands the skb
to neigh_event_send(), which queues it on that neighbour's arp_queue
with the stale skb->dev. skb->dev holds no reference, so deleting the
previous slave frees the net_device while the skb is still queued.
Whatever runs next on that skb - arp_error_report() on timeout, or
neigh_direct_output() -> dev_queue_xmit() once the neighbour resolves -
causes a UAF like the one below:

BUG: KASAN: slab-use-after-free in __icmp_send (net/ipv4/icmp.c:914 (discriminator 2))
Read of size 4 at addr ffff888106e100b0 by task flood_packet/527
CPU: 0 UID: 0 PID: 527 Comm: flood_packet Not tainted 7.2.0-rc6-g594d90519502 #1 PREEMPT(lazy)
Hardware name: QEMU Ubuntu 24.04 PC v2 (i440FX + PIIX, arch_caps fix, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
Call Trace:
<IRQ>
dump_stack_lvl (lib/dump_stack.c:94 lib/dump_stack.c:120)
print_report (mm/kasan/report.c:378 mm/kasan/report.c:482)
? __pfx__raw_spin_lock_irqsave (./include/asm-generic/qrwlock.h:122 (discriminator 4))
? __icmp_send (net/ipv4/icmp.c:914 (discriminator 2))
kasan_report (mm/kasan/report.c:595)
? __icmp_send (net/ipv4/icmp.c:914 (discriminator 2))
__icmp_send (net/ipv4/icmp.c:914 (discriminator 2))
[...]
ipv4_link_failure (net/ipv4/route.c:1251 net/ipv4/route.c:1258)
? __pfx_ipv4_link_failure (./include/linux/skbuff.h:4327)
? _raw_write_lock (./include/linux/instrumented.h:55 ./include/linux/atomic/atomic-instrumented.h:1301 ./include/asm-generic/qrwlock.h:98 ./include/linux/rwlock_api_smp.h:230 kernel/locking/spinlock.c:304)
? __pfx__raw_write_lock (kernel/locking/spinlock.c:175)
arp_error_report (./include/net/dst.h:438 net/ipv4/arp.c:296)
neigh_invalidate (net/core/neighbour.c:1077)
neigh_timer_handler (net/core/neighbour.c:1169)
[...]
Allocated by task 505:
kasan_save_stack (mm/kasan/common.c:57)
kasan_save_track (mm/kasan/common.c:78)
__kasan_kmalloc (mm/kasan/common.c:398 mm/kasan/common.c:415)
__kvmalloc_node_noprof (./include/linux/kasan.h:263 mm/slub.c:5334 mm/slub.c:6905)
alloc_netdev_mqs (net/core/dev.c:12055 (discriminator 2))
rtnl_create_link (net/core/rtnetlink.c:3721)
rtnl_newlink (net/core/rtnetlink.c:3903 net/core/rtnetlink.c:4044 net/core/rtnetlink.c:4159)
rtnetlink_rcv_msg (net/core/rtnetlink.c:7076)
[...]
Freed by task 536:
kasan_save_stack (mm/kasan/common.c:57)
kasan_save_track (mm/kasan/common.c:78)
kasan_save_free_info (mm/kasan/generic.c:584)
__kasan_slab_free (mm/kasan/common.c:253 mm/kasan/common.c:285)
kfree (./include/linux/kasan.h:235 mm/slub.c:2677 mm/slub.c:6377 mm/slub.c:6692)
device_release (drivers/base/core.c:2636)
kobject_put (lib/kobject.c:689 lib/kobject.c:720 ./include/linux/kref.h:65 lib/kobject.c:737)
netdev_run_todo (net/core/dev.c:11756)
rtnl_dellink (net/core/rtnetlink.c:157 ./include/linux/rtnetlink.h:135 net/core/rtnetlink.c:3651)
rtnetlink_rcv_msg (net/core/rtnetlink.c:7076)
[...]

Fix this by restoring skb->dev to the master at the end of each slave's
iteration.

Fixes: 0cc0c2e661af ("net/sched: teql: fix NULL pointer dereference in iptunnel_xmit on TEQL slave xmit")
Reported-by: Vega <vega@nebusec.ai>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Victor Nogueira <victor@mojatatu.com>
---
 net/sched/sch_teql.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c
index 5c42a29a981c..9e52afc2d980 100644
--- a/net/sched/sch_teql.c
+++ b/net/sched/sch_teql.c
@@ -377,6 +377,7 @@ static netdev_tx_t teql_master_xmit(struct sk_buff *skb, struct net_device *dev)
 			nores = 1;
 			break;
 		}
+		skb->dev = dev;
 		__skb_pull(skb, skb_network_offset(skb));
 	} while ((q = rcu_dereference(NEXT_SLAVE(q))) != start);
 
-- 
2.55.0

Re: [PATCH net] net/sched: sch_teql: restore skb->dev on the slave failure path

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/sched: sch_teql: restore skb->dev on the slave failure path

teql_master_xmit() used to leave skb->dev pointing at a slave after that
slave's ndo_start_xmit() failed, so the skb could end up queued on a later
slave's neighbour arp_queue while referring to the earlier slave.  The
patch restores skb->dev to the master device at the end of each slave
iteration.

> diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c
> index 5c42a29a981c..9e52afc2d980 100644
> --- a/net/sched/sch_teql.c
> +++ b/net/sched/sch_teql.c
> @@ -377,6 +377,7 @@ static netdev_tx_t teql_master_xmit(struct sk_buff *skb, struct net_device *dev)
>  			nores = 1;
>  			break;
>  		}
> +		skb->dev = dev;

[Severity: High]
This isn't a bug introduced by this patch, but does it leave a smaller
version of the same dangling skb->dev behind, with the master netdev in
place of the previous slave?

After the restore, skb->dev is the teql master, and the skb can still be
parked on the arp_queue of a neighbour that belongs to a slave device.  On
the second pass (skb_res == skb after the goto restart), teql_resolve() ->
__teql_resolve() creates the neighbour on the slave because dst->dev is the
master:

net/sched/sch_teql.c:__teql_resolve() {
	...
	if (dst->dev != dev) {
		struct neighbour *mn;

		mn = __neigh_lookup_errno(n->tbl, n->primary_key, dev);
	...
	if (neigh_event_send(n, skb_res) == 0) {
	...
	} else {
		err = (skb_res == NULL) ? -EAGAIN : 1;
	}
}

__neigh_event_send() then keeps the skb:

net/core/neighbour.c:__neigh_event_send() {
	...
		skb_dst_force(skb);
		__skb_queue_tail(&neigh->arp_queue, skb);
	...
}

teql_master_xmit() returns NETDEV_TX_OK for the err == 1 case and loses
ownership.  At that point the queued skb's skb->dev (master) is not the
neighbour's device (slave), so the arp_queue flushing done by
arp_ifdown()/neigh_ifdown() for the master's device never sees this skb.
skb->dev also takes no netdev reference.

Is the held dst enough to pin the master here?  On NETDEV_UNREGISTER,
rt_flush_dev() and dst_dev_put() swap dst->dev to blackhole_netdev via
netdev_ref_replace(), which drops the master's refcount, so
netdev_wait_allrefs() does not hold off free_netdev().  The master has no
rtnl_link_ops, so the free comes from teql_exit():

net/sched/sch_teql.c:teql_exit() {
	...
		unregister_qdisc(&master->qops);
		unregister_netdev(master->dev);
		free_netdev(master->dev);
	...
}

which means rmmod sch_teql after the last teql qdisc is deleted, racing a
pending neighbour resolution (the retransmit window is multiple seconds by
default).  The next use of the queued skb dereferences the freed master
through the same path as the KASAN report quoted in the commit message,
neigh_invalidate() -> arp_error_report() -> dst_link_failure() ->
ipv4_link_failure() -> __icmp_send():

net/ipv4/icmp.c:__icmp_send() {
	...
	if (!(skb_in->dev && (skb_in->dev->flags&IFF_LOOPBACK)) &&
	      !icmpv4_global_allow(net, type, code, &apply_ratelimit))
	...
}

and on the successful resolution path,
neigh_update_process_arp_queue() -> n1->output() -> dev_queue_xmit() uses
the freed device's netdev_ops and _tx.

Would setting skb->dev to the device of the neighbour that actually takes
the skb (the slave used by __teql_resolve()), or declining to hand the skb
to a neighbour whose device differs from skb->dev, close the remaining
window?

> \t\t__skb_pull(skb, skb_network_offset(skb));
>  	} while ((q = rcu_dereference(NEXT_SLAVE(q))) != start);
>
-- 
pw-bot: cr