netdev - [RFC PATCH net 4/4] net_sched: qfq: Fix double list add in class with netem as child qdisc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250415000316.3122018-5-victor@mojatatu.com>
Date: Mon, 14 Apr 2025 21:03:16 -0300
From: Victor Nogueira <victor@...atatu.com>
To: netdev@...r.kernel.org
Cc: jhs@...atatu.com,
	xiyou.wangcong@...il.com,
	jiri@...nulli.us,
	davem@...emloft.net,
	edumazet@...gle.com,
	kuba@...nel.org,
	pabeni@...hat.com,
	toke@...hat.com,
	gerrard.tai@...rlabs.sg,
	pctammela@...atatu.com
Subject: [RFC PATCH net 4/4] net_sched: qfq: Fix double list add in class with netem as child qdisc

As described in Gerrard's report [1], there are use cases where a netem
child qdisc will make the parent qdisc's enqueue callback reentrant.
In the case of qfq, there won't be a UAF, but the code will add the same
classifier to the list twice, which will cause memory corruption.

This patch checks whether the class was already added to the
agg->active (cl_is_initialised) before doing the addition.

Fixes: 37d9cf1a3ce3 ("sched: Fix detection of empty queues in child qdiscs")

[1] https://lore.kernel.org/netdev/CAHcdcOm+03OD2j6R0=YHKqmy=VgJ8xEOKuP6c7mSgnp-TEJJbw@mail.gmail.com/

Signed-off-by: Victor Nogueira <victor@...atatu.com>
---
 net/sched/sch_qfq.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c
index 687a932eb9b2..6180a5e19859 100644
--- a/net/sched/sch_qfq.c
+++ b/net/sched/sch_qfq.c
@@ -132,6 +132,7 @@ struct qfq_class {
 
 	struct gnet_stats_basic_sync bstats;
 	struct gnet_stats_queue qstats;
+	u8 cl_initialised;
 	struct net_rate_estimator __rcu *rate_est;
 	struct Qdisc *qdisc;
 	struct list_head alist;		/* Link for active-classes list. */
@@ -348,6 +349,7 @@ static void qfq_deactivate_class(struct qfq_sched *q, struct qfq_class *cl)
 
 
 	list_del_init(&cl->alist); /* remove from RR queue of the aggregate */
+	cl->cl_initialised = 0;
 	if (list_empty(&agg->active)) /* agg is now inactive */
 		qfq_deactivate_agg(q, agg);
 }
@@ -982,9 +984,10 @@ static struct sk_buff *agg_dequeue(struct qfq_aggregate *agg,
 
 	cl->deficit -= (int) len;
 
-	if (cl->qdisc->q.qlen == 0) /* no more packets, remove from list */
+	if (cl->qdisc->q.qlen == 0) { /* no more packets, remove from list */
+		cl->cl_initialised = 0;
 		list_del_init(&cl->alist);
-	else if (cl->deficit < qdisc_pkt_len(cl->qdisc->ops->peek(cl->qdisc))) {
+	} else if (cl->deficit < qdisc_pkt_len(cl->qdisc->ops->peek(cl->qdisc))) {
 		cl->deficit += agg->lmax;
 		list_move_tail(&cl->alist, &agg->active);
 	}
@@ -1214,8 +1217,8 @@ static int qfq_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	struct qfq_sched *q = qdisc_priv(sch);
 	struct qfq_class *cl;
 	struct qfq_aggregate *agg;
+	bool is_empty;
 	int err = 0;
-	bool first;
 
 	cl = qfq_classify(skb, sch, &err);
 	if (cl == NULL) {
@@ -1237,7 +1240,7 @@ static int qfq_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	}
 
 	gso_segs = skb_is_gso(skb) ? skb_shinfo(skb)->gso_segs : 1;
-	first = !cl->qdisc->q.qlen;
+	is_empty = !cl->qdisc->q.qlen;
 	err = qdisc_enqueue(skb, cl->qdisc, to_free);
 	if (unlikely(err != NET_XMIT_SUCCESS)) {
 		pr_debug("qfq_enqueue: enqueue failed %d\n", err);
@@ -1254,18 +1257,22 @@ static int qfq_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 
 	agg = cl->agg;
 	/* if the queue was not empty, then done here */
-	if (!first) {
+	if (!is_empty) {
 		if (unlikely(skb == cl->qdisc->ops->peek(cl->qdisc)) &&
 		    list_first_entry(&agg->active, struct qfq_class, alist)
 		    == cl && cl->deficit < len)
 			list_move_tail(&cl->alist, &agg->active);
 
+		return err;
+	/* cater for recursive call */
+	} else if (cl->cl_initialised) {
 		return err;
 	}
 
 	/* schedule class for service within the aggregate */
 	cl->deficit = agg->lmax;
 	list_add_tail(&cl->alist, &agg->active);
+	cl->cl_initialised = 1;
 
 	if (list_first_entry(&agg->active, struct qfq_class, alist) != cl ||
 	    q->in_serv_agg == agg)
-- 
2.34.1