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

[Vinicius Costa Gomes <vinicius.gomes@intel.com>]

shaozhengchao <shaozhengchao@huawei.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@syzkaller.appspotmail.com
>>> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.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