Re: [PATCH] net: sched: teql: fix use-after-free in teql_master_xmit

[Simon Horman]

From: 'Simon Horman' <horms@xxxxxxxxxx>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
net: sched: teql: fix use-after-free in teql_master_xmit

This patch attempts to fix a use-after-free in teql_master_xmit() by
saving master->slaves to a local head variable during teql_destroy(),
and by adding READ_ONCE()/WRITE_ONCE() to datapath accesses.

> diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c
> index ec4039a201a2c..2e86397a5219c 100644
> --- a/net/sched/sch_teql.c
> +++ b/net/sched/sch_teql.c
> @@ -101,7 +101,7 @@ teql_dequeue(struct Qdisc *sch)
>  	if (skb == NULL) {
>  		struct net_device *m = qdisc_dev(q);
>  		if (m) {
> -			dat->m->slaves = sch;
> +			WRITE_ONCE(dat->m->slaves, sch);

Can this lockless publication write a soon-to-be-freed pointer to
master->slaves?

If teql_dequeue() concurrently executes this write while teql_destroy()
is unlinking sch and scheduling it for deletion via call_rcu(), sch could
be published globally just as it is being destroyed.

When the RCU grace period ends, sch would be freed but still globally
referenced. The WRITE_ONCE() prevents store-tearing but does not prevent
this race condition.

>  		}
>  	} else {
[ ... ]
> @@ -136,19 +136,23 @@ teql_destroy(struct Qdisc *sch)
>  	if (!master)
>  		return;
>  
> -	prev = master->slaves;
> +	prev = READ_ONCE(master->slaves);
>  	if (prev) {
> +		struct Qdisc *head = prev;
> +
>  		do {
>  			q = NEXT_SLAVE(prev);
>  			if (q == sch) {
>  				NEXT_SLAVE(prev) = NEXT_SLAVE(q);
> -				if (q == master->slaves) {
> -					master->slaves = NEXT_SLAVE(q);
> -					if (q == master->slaves) {
> +				if (q == head) {

Could changing this condition introduce a use-after-free if master->slaves
is concurrently advanced?

If the datapath (teql_master_xmit() or teql_dequeue()) advances
master->slaves to point to sch after head was snapshot, head will not
equal sch.

When teql_destroy() unlinks sch, the q == head check would evaluate
to false, meaning master->slaves is not updated. This would leave
master->slaves pointing to the unlinked sch, which will then be freed.

> +					WRITE_ONCE(master->slaves,
> +						   NEXT_SLAVE(q));
[ ... ]
> @@ -317,7 +321,7 @@ static netdev_tx_t teql_master_xmit(struct sk_buff *skb, struct net_device *dev)
>  				    netdev_start_xmit(skb, slave, slave_txq, false) ==
>  				    NETDEV_TX_OK) {
>  					__netif_tx_unlock(slave_txq);
> -					master->slaves = NEXT_SLAVE(q);
> +					WRITE_ONCE(master->slaves, NEXT_SLAVE(q));

Similarly to teql_dequeue() above, does this risk publishing a dying qdisc
globally?

If NEXT_SLAVE(q) is read before teql_destroy() unlinks it, but the
WRITE_ONCE() executes immediately after the unlink, it appears the
unlinked qdisc would be written to the global master->slaves pointer.

[ ... ]

While looking at the concurrency issues addressed by this patch, it appears
teql_qdisc_init() might also be affected by concurrent updates to
m->slaves:

net/sched/sch_teql.c:teql_qdisc_init() {
    ...
		q->next = NEXT_SLAVE(m->slaves);
		NEXT_SLAVE(m->slaves) = sch;
    ...
}

Because NEXT_SLAVE(m->slaves) evaluates m->slaves twice, if m->slaves
is concurrently advanced between these two statements, could this break
the list linkage and cause lost elements or infinite loops?