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

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

[Zhengchao Shao]

As shown in [1], out-of-bounds access occurs in two cases:
1)when the qdisc of the taprio type is used to replace the previously
configured taprio, count and offset in tc_to_txq can be set to 0. In this
case, the value of *txq in taprio_next_tc_txq() will increases
continuously. When the number of accessed queues exceeds the number of
queues on the device, out-of-bounds access occurs.
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.

Now the restriction on the queue number is added.

[1] https://groups.google.com/g/syzkaller-bugs/c/_lYOKgkBVMg
Fixes: 2f530df76c8c ("net/sched: taprio: give higher priority to higher TCs in software dequeue mode")
Reported-by: syzbot+04afcb3d2c840447559a@syzkaller.appspotmail.com
Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
---
v2: set q->cur_txq[tc] to prevent out-of-bounds access during next dequeue
---
 net/sched/sch_taprio.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 3c4c2c334878..82983a6eb8f8 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -799,6 +799,9 @@ static struct sk_buff *taprio_dequeue_tc_priority(struct Qdisc *sch,
 
 			taprio_next_tc_txq(dev, tc, &q->cur_txq[tc]);
 
+			if (q->cur_txq[tc] >= dev->num_tx_queues)
+				q->cur_txq[tc] = first_txq;
+
 			if (skb)
 				return skb;
 		} while (q->cur_txq[tc] != first_txq);
-- 
2.34.1

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

[Pedro Tammela]

On 08/06/2023 03:27, Zhengchao Shao wrote:
> As shown in [1], out-of-bounds access occurs in two cases:
> 1)when the qdisc of the taprio type is used to replace the previously
> configured taprio, count and offset in tc_to_txq can be set to 0. In this
> case, the value of *txq in taprio_next_tc_txq() will increases
> continuously. When the number of accessed queues exceeds the number of
> queues on the device, out-of-bounds access occurs.
> 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.
> 
> Now the restriction on the queue number is added.
> 
> [1] https://groups.google.com/g/syzkaller-bugs/c/_lYOKgkBVMg
> Fixes: 2f530df76c8c ("net/sched: taprio: give higher priority to higher TCs in software dequeue mode")
> Reported-by: syzbot+04afcb3d2c840447559a@syzkaller.appspotmail.com
> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>

Tested-by: Pedro Tammela <pctammela@mojatatu.com>

> ---
> v2: set q->cur_txq[tc] to prevent out-of-bounds access during next dequeue
> ---
>   net/sched/sch_taprio.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index 3c4c2c334878..82983a6eb8f8 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -799,6 +799,9 @@ static struct sk_buff *taprio_dequeue_tc_priority(struct Qdisc *sch,
>   
>   			taprio_next_tc_txq(dev, tc, &q->cur_txq[tc]);
>   
> +			if (q->cur_txq[tc] >= dev->num_tx_queues)
> +				q->cur_txq[tc] = first_txq;
> +
>   			if (skb)
>   				return skb;
>   		} while (q->cur_txq[tc] != first_txq);

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

[Vinicius Costa Gomes]

Zhengchao Shao <shaozhengchao@huawei.com> writes:

> As shown in [1], out-of-bounds access occurs in two cases:
> 1)when the qdisc of the taprio type is used to replace the previously
> configured taprio, count and offset in tc_to_txq can be set to 0. In this
> case, the value of *txq in taprio_next_tc_txq() will increases
> continuously. When the number of accessed queues exceeds the number of
> queues on the device, out-of-bounds access occurs.

The more I think about this, the more I think the problem is somewhere
else, i.e. even enqueuing a packet from a TC with zero queues associated
with it doesn't make much sense.

The behaviors that make more sense to me are:
  1. reject configurations with '0@0' as invalid;
  2. drop the packets from TCs mapped to the "empty set" queue (0@0)
  during enqueue();

btw, (2) sounds better to me at this point.

Or is there another valid/sensible interpretation to '0@0' that I am missing? 

> 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.

>
> Now the restriction on the queue number is added.
>
> [1] https://groups.google.com/g/syzkaller-bugs/c/_lYOKgkBVMg
> Fixes: 2f530df76c8c ("net/sched: taprio: give higher priority to higher TCs in software dequeue mode")
> Reported-by: syzbot+04afcb3d2c840447559a@syzkaller.appspotmail.com
> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
> ---
> v2: set q->cur_txq[tc] to prevent out-of-bounds access during next dequeue
> ---
>  net/sched/sch_taprio.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index 3c4c2c334878..82983a6eb8f8 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -799,6 +799,9 @@ static struct sk_buff *taprio_dequeue_tc_priority(struct Qdisc *sch,
>  
>  			taprio_next_tc_txq(dev, tc, &q->cur_txq[tc]);
>  
> +			if (q->cur_txq[tc] >= dev->num_tx_queues)
> +				q->cur_txq[tc] = first_txq;
> +
>  			if (skb)
>  				return skb;
>  		} while (q->cur_txq[tc] != first_txq);
> -- 
> 2.34.1
>
>

-- 
Vinicius

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

[shaozhengchao]

On 2023/6/9 8:42, Vinicius Costa Gomes wrote:
> Zhengchao Shao <shaozhengchao@huawei.com> writes:
> 
>> As shown in [1], out-of-bounds access occurs in two cases:
>> 1)when the qdisc of the taprio type is used to replace the previously
>> configured taprio, count and offset in tc_to_txq can be set to 0. In this
>> case, the value of *txq in taprio_next_tc_txq() will increases
>> continuously. When the number of accessed queues exceeds the number of
>> queues on the device, out-of-bounds access occurs.
> 
Hi Vinicius:
	Thank you for your reply.
> The more I think about this, the more I think the problem is somewhere
> else, i.e. even enqueuing a packet from a TC with zero queues associated
> with it doesn't make much sense.
> 
> The behaviors that make more sense to me are:
>    1. reject configurations with '0@0' as invalid;
>    2. drop the packets from TCs mapped to the "empty set" queue (0@0)
>    during enqueue();
> 
> 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?

> 
>> 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.

Do you have any other ideas? Thanks.

Zhengchao Shao
>>
>> Now the restriction on the queue number is added.
>>
>> [1] https://groups.google.com/g/syzkaller-bugs/c/_lYOKgkBVMg
>> Fixes: 2f530df76c8c ("net/sched: taprio: give higher priority to higher TCs in software dequeue mode")
>> Reported-by: syzbot+04afcb3d2c840447559a@syzkaller.appspotmail.com
>> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
>> ---
>> v2: set q->cur_txq[tc] to prevent out-of-bounds access during next dequeue
>> ---
>>   net/sched/sch_taprio.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
>> index 3c4c2c334878..82983a6eb8f8 100644
>> --- a/net/sched/sch_taprio.c
>> +++ b/net/sched/sch_taprio.c
>> @@ -799,6 +799,9 @@ static struct sk_buff *taprio_dequeue_tc_priority(struct Qdisc *sch,
>>   
>>   			taprio_next_tc_txq(dev, tc, &q->cur_txq[tc]);
>>   
>> +			if (q->cur_txq[tc] >= dev->num_tx_queues)
>> +				q->cur_txq[tc] = first_txq;
>> +
>>   			if (skb)
>>   				return skb;
>>   		} while (q->cur_txq[tc] != first_txq);
>> -- 
>> 2.34.1
>>
>>
> 

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

[Vladimir Oltean]

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?

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Thu, 8 Jun 2023 14:27:56 +0800 you wrote:
> As shown in [1], out-of-bounds access occurs in two cases:
> 1)when the qdisc of the taprio type is used to replace the previously
> configured taprio, count and offset in tc_to_txq can be set to 0. In this
> case, the value of *txq in taprio_next_tc_txq() will increases
> continuously. When the number of accessed queues exceeds the number of
> queues on the device, out-of-bounds access occurs.
> 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.
> 
> [...]

Here is the summary with links:
  - [net,v2] net/sched: taprio: fix slab-out-of-bounds Read in taprio_dequeue_from_txq
    https://git.kernel.org/netdev/net/c/be3618d96510

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

[shaozhengchao]

On 2023/6/9 17:45, Vladimir Oltean wrote:
> 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?
> 

Hi Vladimir:
	Thank you for your reply.
> 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);
> +

It looks good to me. I'll add it in subsequent patch.
>   	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():
> 
I added the local print information to confirm that some scenarios
cannot be filtered by mqprio_validate_queue_counts. But I can't find a
command line that can reproduce the problem.
> 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?
Only the syz reproduction program.
https://groups.google.com/g/syzkaller-bugs/c/_lYOKgkBVMg
Thank you.

Zhengchao Shao

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

[Vladimir Oltean]

On Mon, Jun 12, 2023 at 08:49:53AM +0800, shaozhengchao wrote:
> > Is there a reproducer for the bug?
> Only the syz reproduction program.
> https://groups.google.com/g/syzkaller-bugs/c/_lYOKgkBVMg
> Thank you.

Sorry, I don't really have time to become familiar with the syzbot right
now. Can someone help me translate that syz repro into a C program?