netdev - Re: [PATCH net] 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: Wed, 7 Jun 2023 09:15:38 +0800
From: shaozhengchao <shaozhengchao@...wei.com>
To: Pedro Tammela <pctammela@...atatu.com>, <netdev@...r.kernel.org>,
	<vinicius.gomes@...el.com>, <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] net/sched: taprio: fix slab-out-of-bounds Read in
 taprio_dequeue_from_txq


On 2023/6/6 23:10, Pedro Tammela wrote:
> On 06/06/2023 09:10, Zhengchao Shao wrote:
>> As shown in [1], when qdisc of the taprio type is set, 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. 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>
>> ---
>>   net/sched/sch_taprio.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
>> index 3c4c2c334878..dccb64425852 100644
>> --- a/net/sched/sch_taprio.c
>> +++ b/net/sched/sch_taprio.c
>> @@ -801,7 +801,7 @@ static struct sk_buff 
>> *taprio_dequeue_tc_priority(struct Qdisc *sch,
>>               if (skb)
>>                   return skb;
>> -        } while (q->cur_txq[tc] != first_txq);
>> +        } while (q->cur_txq[tc] != first_txq && q->cur_txq[tc] < 
>> dev->num_tx_queues);
> 
Hi Pedro:
	Thank you for youe reply.
> I'm not sure this is the correct fix.
> If q->cur_txg[tc] == dev->num_tx_queues the next call to 
> taprio_dequeue_tc_priority() for the same tc index will have
> first_txq set to dev->num_tx_queues (no wrap around to first_txq happens).
yes, maybe the same problem will occur at the next dequeue skb. It can
be changed to the following:
			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);
However, I prefer to limit the value of count in taprio_change(), so 
that I don't add extra judgment to the data path.

Hi Vinicius,
	Do you have any better suggestions?
> If count and offset are 0 it will increment q->cur_txg[tc] and then bail 
> on the while condition but still growing unbounded (just slower than 
> before).
> 
>>       }
>>   taprio_dequeue_tc_priority
>>       return NULL;
> 
>