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

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] [thread-next>] [day] [month] [year] [list]

Date: Fri, 9 Jun 2023 09:57:20 +0800
From: shaozhengchao <shaozhengchao@...wei.com>
To: Vinicius Costa Gomes <vinicius.gomes@...el.com>, <netdev@...r.kernel.org>,
	<jhs@...atatu.com>, <xiyou.wangcong@...il.com>, <jiri@...nulli.us>,
	<davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<pabeni@...hat.com>
CC: <vladimir.oltean@....com>, <weiyongjun1@...wei.com>,
	<yuehaibing@...wei.com>
Subject: Re: [PATCH net,v2] net/sched: taprio: fix slab-out-of-bounds Read in
 taprio_dequeue_from_txq



On 2023/6/9 8:42, Vinicius Costa Gomes wrote:
> Zhengchao Shao <shaozhengchao@...wei.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@...kaller.appspotmail.com
>> Signed-off-by: Zhengchao Shao <shaozhengchao@...wei.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
>>
>>
>