From: Vinicius Costa Gomes <vinicius.gomes@intel•com>
To: shaozhengchao <shaozhengchao@huawei•com>,
Pedro Tammela <pctammela@mojatatu•com>,
netdev@vger•kernel.org, jhs@mojatatu•com,
xiyou.wangcong@gmail•com, jiri@resnulli•us, davem@davemloft•net,
edumazet@google•com, kuba@kernel•org, pabeni@redhat•com
Cc: vladimir.oltean@nxp•com, weiyongjun1@huawei•com, yuehaibing@huawei•com
Subject: Re: [PATCH net] net/sched: taprio: fix slab-out-of-bounds Read in taprio_dequeue_from_txq
Date: Wed, 07 Jun 2023 12:05:00 -0700 [thread overview]
Message-ID: <87o7lrqejn.fsf@intel.com> (raw)
In-Reply-To: <4cbeb947-5230-4343-1380-95b2d81153d3@huawei.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
next prev parent reply other threads:[~2023-06-07 19:05 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-06 12:10 [PATCH net] net/sched: taprio: fix slab-out-of-bounds Read in taprio_dequeue_from_txq Zhengchao Shao
2023-06-06 15:10 ` Pedro Tammela
2023-06-07 1:15 ` shaozhengchao
2023-06-07 1:38 ` shaozhengchao
2023-06-07 19:05 ` Vinicius Costa Gomes [this message]
2023-06-08 2:06 ` shaozhengchao
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87o7lrqejn.fsf@intel.com \
--to=vinicius.gomes@intel$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=edumazet@google$(echo .)com \
--cc=jhs@mojatatu$(echo .)com \
--cc=jiri@resnulli$(echo .)us \
--cc=kuba@kernel$(echo .)org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pabeni@redhat$(echo .)com \
--cc=pctammela@mojatatu$(echo .)com \
--cc=shaozhengchao@huawei$(echo .)com \
--cc=vladimir.oltean@nxp$(echo .)com \
--cc=weiyongjun1@huawei$(echo .)com \
--cc=xiyou.wangcong@gmail$(echo .)com \
--cc=yuehaibing@huawei$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox