From: Haimin Zhang <redacted>
syzbot report an array-index-out-of-bounds in taprio_change
index 16 is out of range for type '__u16 [16]'
that's because mqprio->num_tc is lager than TC_MAX_QUEUE,so we check
the return value of netdev_set_num_tc.
Reported-by: syzbot+2b3e5fb6c7ef285a94f6@syzkaller.appspotmail.com
Signed-off-by: Haimin Zhang <redacted>
---
net/sched/sch_taprio.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
From: Eric Dumazet <hidden> Date: 2021-08-11 07:44:51
On 8/11/21 7:10 AM, tcs.kernel@gmail.com wrote:
quoted hunk
From: Haimin Zhang <redacted>
syzbot report an array-index-out-of-bounds in taprio_change
index 16 is out of range for type '__u16 [16]'
that's because mqprio->num_tc is lager than TC_MAX_QUEUE,so we check
the return value of netdev_set_num_tc.
Reported-by: syzbot+2b3e5fb6c7ef285a94f6@syzkaller.appspotmail.com
Signed-off-by: Haimin Zhang <redacted>
---
net/sched/sch_taprio.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
When was the bug added ?
Hint: Please provide a Fixes: tag
taprio_parse_mqprio_opt() already checks :
/* Verify num_tc is not out of max range */
if (qopt->num_tc > TC_MAX_QUEUE) {
NL_SET_ERR_MSG(extack, "Number of traffic classes is outside valid range");
return -EINVAL;
}
So what is happening exactly ?
From: Pavel Skripkin <hidden> Date: 2021-08-11 08:30:15
On 8/11/21 10:44 AM, Eric Dumazet wrote:
On 8/11/21 7:10 AM, tcs.kernel@gmail.com wrote:
quoted
From: Haimin Zhang <redacted>
syzbot report an array-index-out-of-bounds in taprio_change
index 16 is out of range for type '__u16 [16]'
that's because mqprio->num_tc is lager than TC_MAX_QUEUE,so we check
the return value of netdev_set_num_tc.
Reported-by: syzbot+2b3e5fb6c7ef285a94f6@syzkaller.appspotmail.com
Signed-off-by: Haimin Zhang <redacted>
---
net/sched/sch_taprio.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
When was the bug added ?
Hint: Please provide a Fixes: tag
taprio_parse_mqprio_opt() already checks :
/* Verify num_tc is not out of max range */
if (qopt->num_tc > TC_MAX_QUEUE) {
NL_SET_ERR_MSG(extack, "Number of traffic classes is outside valid range");
return -EINVAL;
}
So what is happening exactly ?
Hi, Eric!
I've looked into this bug, but I decided to write a reproducer for it
first. Unfortunately, I didn't finish it yesterday, but I have an idea
about what happened:
taprio_parse_mqprio_opt() may return 0 before qopt->num_tc check:
/* If num_tc is already set, it means that the user already
* configured the mqprio part
*/
if (dev->num_tc)
return 0;
Then taprio_mqprio_cmp() fails here:
if (!mqprio || mqprio->num_tc != dev->num_tc)
return -1;
That's why we won't get shift-out-of-bound in taprio_mqprio_cmp().
And finally taprio_change() gets to buggy for with wrong mqprio->num_tc.
I don't know how to reproduce it, but I'll try to finish my reproducer
this evening.
Does above makes any sense?
With regards,
Pavel Skripkin
From: Pavel Skripkin <hidden> Date: 2021-08-11 08:35:44
On 8/11/21 11:30 AM, Pavel Skripkin wrote:
On 8/11/21 10:44 AM, Eric Dumazet wrote:
quoted
On 8/11/21 7:10 AM, tcs.kernel@gmail.com wrote:
quoted
From: Haimin Zhang <redacted>
syzbot report an array-index-out-of-bounds in taprio_change
index 16 is out of range for type '__u16 [16]'
that's because mqprio->num_tc is lager than TC_MAX_QUEUE,so we check
the return value of netdev_set_num_tc.
Reported-by: syzbot+2b3e5fb6c7ef285a94f6@syzkaller.appspotmail.com
Signed-off-by: Haimin Zhang <redacted>
---
net/sched/sch_taprio.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
When was the bug added ?
Hint: Please provide a Fixes: tag
taprio_parse_mqprio_opt() already checks :
/* Verify num_tc is not out of max range */
if (qopt->num_tc > TC_MAX_QUEUE) {
NL_SET_ERR_MSG(extack, "Number of traffic classes is outside valid range");
return -EINVAL;
}
So what is happening exactly ?
Hi, Eric!
I've looked into this bug, but I decided to write a reproducer for it
first. Unfortunately, I didn't finish it yesterday, but I have an idea
about what happened:
taprio_parse_mqprio_opt() may return 0 before qopt->num_tc check:
/* If num_tc is already set, it means that the user already
* configured the mqprio part
*/
if (dev->num_tc)
return 0;
Then taprio_mqprio_cmp() fails here:
if (!mqprio || mqprio->num_tc != dev->num_tc)
return -1;
That's why we won't get shift-out-of-bound in taprio_mqprio_cmp().
^^^^^^^^^^^^^^^^^
array-index-out-of-bounds
Sorry for confusion
With regards,
Pavel Skripkin
From: Haimin Zhang <redacted>
syzbot report an array-index-out-of-bounds in taprio_change
index 16 is out of range for type '__u16 [16]'
that's because mqprio->num_tc is lager than TC_MAX_QUEUE,so we check
the return value of netdev_set_num_tc.
Reported-by: syzbot+2b3e5fb6c7ef285a94f6@syzkaller.appspotmail.com
Signed-off-by: Haimin Zhang <redacted>
---
net/sched/sch_taprio.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
When was the bug added ?
Hint: Please provide a Fixes: tag
taprio_parse_mqprio_opt() already checks :
/* Verify num_tc is not out of max range */
if (qopt->num_tc > TC_MAX_QUEUE) {
NL_SET_ERR_MSG(extack, "Number of traffic classes is outside valid range");
return -EINVAL;
}
So what is happening exactly ?
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-08-12 17:53:24
On Wed, 11 Aug 2021 13:10:09 +0800 tcs.kernel@gmail.com wrote:
quoted hunk
From: Haimin Zhang <redacted>
syzbot report an array-index-out-of-bounds in taprio_change
index 16 is out of range for type '__u16 [16]'
that's because mqprio->num_tc is lager than TC_MAX_QUEUE,so we check
the return value of netdev_set_num_tc.
Reported-by: syzbot+2b3e5fb6c7ef285a94f6@syzkaller.appspotmail.com
Signed-off-by: Haimin Zhang <redacted>
---
net/sched/sch_taprio.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
taprio_set_picos_per_byte() already got called and applied some of the
changes. It seems like the early return from taprio_parse_mqprio_opt()
if dev->num_tc is non-zero is incorrect. That function is supposed to
validate that mqprio_opt() is correct AFAIU. That would mean:
Fixes: a3d43c0d56f1 ("taprio: Add support adding an admin schedule")
Vinicius - WDYT?
for (i = 0; i < mqprio->num_tc; i++)
netdev_set_tc_queue(dev, i,
mqprio->count[i],
hi，i wonder to know what‘s going on with this patch ?
are you plannig to merge it ?
在 2021/8/11 16:36, Haimin Zhang 写道:
在 2021/8/11 15:44, Eric Dumazet 写道:
quoted
On 8/11/21 7:10 AM, tcs.kernel@gmail.com wrote:
quoted
From: Haimin Zhang <redacted>
syzbot report an array-index-out-of-bounds in taprio_change
index 16 is out of range for type '__u16 [16]'
that's because mqprio->num_tc is lager than TC_MAX_QUEUE,so we check
the return value of netdev_set_num_tc.
Reported-by: syzbot+2b3e5fb6c7ef285a94f6@syzkaller.appspotmail.com
Signed-off-by: Haimin Zhang <redacted>
---
net/sched/sch_taprio.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
@@ -1513,7 +1513,9 @@ static int taprio_change(struct Qdisc *sch,
struct nlattr *opt,
taprio_set_picos_per_byte(dev, q);
if (mqprio) {
- netdev_set_num_tc(dev, mqprio->num_tc);
+ err = netdev_set_num_tc(dev, mqprio->num_tc);
+ if (err)
+ goto free_sched;
for (i = 0; i < mqprio->num_tc; i++)
netdev_set_tc_queue(dev, i,
mqprio->count[i],
When was the bug added ?
Hint: Please provide a Fixes: tag
taprio_parse_mqprio_opt() already checks :
/* Verify num_tc is not out of max range */
if (qopt->num_tc > TC_MAX_QUEUE) {
NL_SET_ERR_MSG(extack, "Number of traffic classes is outside
valid range");
return -EINVAL;
}
So what is happening exactly ?
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-08-30 16:10:50
On Mon, 30 Aug 2021 10:09:38 +0800 Haimin Zhang wrote:
hi，i wonder to know what‘s going on with this patch ?
are you plannig to merge it ?
Please don't top post. As I told you I think that fix belongs in
taprio_parse_mqprio_opt() which is validating inputs, not in the
middle of the code applying the changes.