[PATCH net] net/sched: fix rate estimator use-after-free when clearing NOLOCK

[PATCH net] net/sched: fix rate estimator use-after-free when clearing NOLOCK

[Cen Zhang (Microsoft)]

qdisc_create() creates the rate estimator requested by TCA_RATE before
qdisc_graft() converts a lockless child of a locking parent. The
conversion in qdisc_clear_nolock() frees the per-CPU statistics, but
the estimator keeps its own copy of the cpu_bstats pointer and its
timer keeps reading the freed per-CPU memory on every estimator tick.

An unprivileged local user can trigger this. The estimator timer can
access freed per-CPU statistics in softirq context and panic the kernel:

  BUG: unable to handle page fault for address: ff9d8b20ffa47920
  RIP: 0010:gnet_stats_add_basic+0x77/0xc0
  Call Trace:
   <IRQ>
   est_fetch_counters+0x31/0x50
   est_timer+0x52/0x170
   call_timer_fn+0x9f/0x210
   run_timer_softirq+0x6e/0xb0
   </IRQ>
  Kernel panic - not syncing: Fatal exception in interrupt

Fix by doing the NOLOCK conversion in qdisc_create(), right before
the estimator is created, instead of in qdisc_graft(), so that the
estimator captures the final statistics pointer. Removing the
conversion from qdisc_graft() is safe: a new qdisc is created by
qdisc_create() with the parent it is grafted under, and since
commit bc50835e83f6 ("net: sched: Disallow replacing of child qdisc
from one parent to another") an existing qdisc can only be grafted
again under its original parent. Either way the qdisc was already
converted for that parent at creation.

Fixes: 8a53e616de29 ("net: sched: when clearing NOLOCK, clear TCQ_F_CPUSTATS, too")
Cc: stable@vger.kernel.org
Cc: AutonomousCodeSecurity@microsoft.com
Signed-off-by: Cen Zhang (Microsoft) <cenzhang@linux.microsoft.com>
---

The bug was found and this fix was developed with the assistance of an
LLM-based code analysis tool.

Backport note: trees without commit bc50835e83f6 (v6.14) still allow
regrafting an existing child qdisc under a different parent and need
an additional fix for that path, for example cherry-picking that
commit as well.

 net/sched/sch_api.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index f36c41224bcc..d89f3f0a3c5f 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1247,10 +1247,6 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
 		unsigned long cl;
 		int err;
 
-		/* Only support running class lockless if parent is lockless */
-		if (new && (new->flags & TCQ_F_NOLOCK) && !(parent->flags & TCQ_F_NOLOCK))
-			qdisc_clear_nolock(new);
-
 		if (!cops || !cops->graft)
 			return -EOPNOTSUPP;
 
@@ -1327,6 +1323,7 @@ static int qdisc_block_indexes_set(struct Qdisc *sch, struct nlattr **tca,
 
 static struct Qdisc *qdisc_create(struct net_device *dev,
 				  struct netdev_queue *dev_queue,
+				  struct Qdisc *parent_qdisc,
 				  u32 parent, u32 handle,
 				  struct nlattr **tca, int *errp,
 				  struct netlink_ext_ack *extack)
@@ -1405,6 +1402,11 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
 			goto err_out4;
 	}
 
+	/* Only support running class lockless if parent is lockless */
+	if ((sch->flags & TCQ_F_NOLOCK) && parent_qdisc &&
+	    !(parent_qdisc->flags & TCQ_F_NOLOCK))
+		qdisc_clear_nolock(sch);
+
 	if (tca[TCA_RATE]) {
 		err = -EOPNOTSUPP;
 		if (sch->flags & TCQ_F_MQROOT) {
@@ -1817,7 +1819,7 @@ static int __tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
 create_n_graft2:
 	if (clid == TC_H_INGRESS) {
 		if (dev_ingress_queue(dev)) {
-			q = qdisc_create(dev, dev_ingress_queue(dev),
+			q = qdisc_create(dev, dev_ingress_queue(dev), NULL,
 					 tcm->tcm_parent, tcm->tcm_parent,
 					 tca, &err, extack);
 		} else {
@@ -1834,7 +1836,7 @@ static int __tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
 		else
 			dev_queue = netdev_get_tx_queue(dev, 0);
 
-		q = qdisc_create(dev, dev_queue,
+		q = qdisc_create(dev, dev_queue, p,
 				 tcm->tcm_parent, tcm->tcm_handle,
 				 tca, &err, extack);
 	}
-- 
2.55.0