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]

Message-ID: <87o7lrqejn.fsf@intel.com>
Date: Wed, 07 Jun 2023 12:05:00 -0700
From: Vinicius Costa Gomes <vinicius.gomes@...el.com>
To: shaozhengchao <shaozhengchao@...wei.com>, Pedro Tammela
 <pctammela@...atatu.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] net/sched: taprio: fix slab-out-of-bounds Read in
 taprio_dequeue_from_txq

shaozhengchao <shaozhengchao@...wei.com> writes:

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

>From a very quick look at the syzkaller report, I couldn't come up with
a config to trigger the issue.

But reading your report, the problematic case is having a bunch of
'0@0' in the "queues" map in the config, right?

A '0@0' would mean, in my opinion, that the user wants that a specific
TC to not have any queues associated with it, i.e. that it should be
ignored. Either that, or a configuration error.

Am I missing something?

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

-- 
Vinicius