Re: [PATCH net,v2] net/sched: taprio: fix slab-out-of-bounds Read in taprio_dequeue_from_txq

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Fri, Jun 09, 2023 at 09:57:20AM +0800, shaozhengchao wrote:
> > btw, (2) sounds better to me at this point.
> > 
> > Or is there another valid/sensible interpretation to '0@0' that I am missing?
> I think I know what you mean. Your intention is to make judgments
> simultaneously during the enqueue process, as shown below?
> 
> static int taprio_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>                           struct sk_buff **to_free)
>  {
>         struct taprio_sched *q = qdisc_priv(sch);
> +       struct net_device *dev = qdisc_dev(sch);
>         struct Qdisc *child;
>         int queue;
> +       int i;
> +
> +       for (i = 0; i < dev->num_tc; i++) {
> +               if (unlikely(!dev->tc_to_txq[i].count))
> +                       return qdisc_drop(skb, sch, to_free);
> +       }
> 
>         queue = skb_get_queue_mapping(skb);
> 
> Is it like this?

No. If we go down this path (not saying that we should), you should only
validate the queue count of the packet's traffic class, not all queue counts...

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 978c3504fbaa..d1d10341278d 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -633,11 +633,16 @@ static int taprio_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 			  struct sk_buff **to_free)
 {
 	struct taprio_sched *q = qdisc_priv(sch);
+	struct net_device *dev = qdisc_dev(sch);
+	int tc, queue, prio = skb->priority;
 	struct Qdisc *child;
-	int queue;
 
 	queue = skb_get_queue_mapping(skb);
 
+	tc = netdev_get_prio_tc_map(dev, prio);
+	if (!dev->tc_to_txq[tc].count)
+		return qdisc_drop(skb, sch, to_free);
+
 	child = q->qdiscs[queue];
 	if (unlikely(!child))
 		return qdisc_drop(skb, sch, to_free);

> 
> > 
> > > 2)When packets are dequeued, taprio can be deleted. In this case, the tc
> > > rule of dev is cleared. The count and offset values are also set to 0. In
> > > this case, out-of-bounds access is also caused.
> > 
> > This looks like more like working around the issue than fixing it, and
> > it just happens, it's a coincidence, that both issues have the same
> > symptoms.
> > 
> There are many trigger paths for this problem, and I worry that there
> may be missing scenarios after I modify taprio_change and
> taprio_destroy, so I modify the dequeue process.

Many other trigger paths like what?

The main code path leading to 0 TXQs for a traffic class that Vinicius
seems to worry about ("queues 0@0" in configuration) should already be
rejected by mqprio_validate_queue_counts():

tc qdisc replace dev eno0 handle 8001: parent root stab overhead 24 taprio \
	num_tc 3 map 0 1 2 queues 0@0 0@0 0@0 base-time 200 \
	sched-entry S 80 20000 sched-entry S a0 20000 sched-entry S 5f 60000 clockid CLOCK_TAI
Error: sch_mqprio_lib: No queues for TC 0.

We should thus concentrate on the other (involuntary) code paths that
can lead to there being 0 TXQs for a TC. Modifying the data path because
we can't figure out the control path seems desperate.

Is there a reproducer for the bug?