* [PATCH net v4 1/1] net: sched: fix ets qdisc OOB Indexing
@ 2025-01-11 14:57 Jamal Hadi Salim
2025-01-11 21:01 ` Jakub Kicinski
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Jamal Hadi Salim @ 2025-01-11 14:57 UTC (permalink / raw)
To: netdev
Cc: jiri, xiyou.wangcong, davem, edumazet, kuba, petrm, security,
g1042620637
Haowei Yan <g1042620637@gmail.com> found that ets_class_from_arg() can
index an Out-Of-Bound class in ets_class_from_arg() when passed clid of
0. The overflow may cause local privilege escalation.
[ 18.852298] ------------[ cut here ]------------
[ 18.853271] UBSAN: array-index-out-of-bounds in net/sched/sch_ets.c:93:20
[ 18.853743] index 18446744073709551615 is out of range for type 'ets_class [16]'
[ 18.854254] CPU: 0 UID: 0 PID: 1275 Comm: poc Not tainted 6.12.6-dirty #17
[ 18.854821] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
[ 18.856532] Call Trace:
[ 18.857441] <TASK>
[ 18.858227] dump_stack_lvl+0xc2/0xf0
[ 18.859607] dump_stack+0x10/0x20
[ 18.860908] __ubsan_handle_out_of_bounds+0xa7/0xf0
[ 18.864022] ets_class_change+0x3d6/0x3f0
[ 18.864322] tc_ctl_tclass+0x251/0x910
[ 18.864587] ? lock_acquire+0x5e/0x140
[ 18.865113] ? __mutex_lock+0x9c/0xe70
[ 18.866009] ? __mutex_lock+0xa34/0xe70
[ 18.866401] rtnetlink_rcv_msg+0x170/0x6f0
[ 18.866806] ? __lock_acquire+0x578/0xc10
[ 18.867184] ? __pfx_rtnetlink_rcv_msg+0x10/0x10
[ 18.867503] netlink_rcv_skb+0x59/0x110
[ 18.867776] rtnetlink_rcv+0x15/0x30
[ 18.868159] netlink_unicast+0x1c3/0x2b0
[ 18.868440] netlink_sendmsg+0x239/0x4b0
[ 18.868721] ____sys_sendmsg+0x3e2/0x410
[ 18.869012] ___sys_sendmsg+0x88/0xe0
[ 18.869276] ? rseq_ip_fixup+0x198/0x260
[ 18.869563] ? rseq_update_cpu_node_id+0x10a/0x190
[ 18.869900] ? trace_hardirqs_off+0x5a/0xd0
[ 18.870196] ? syscall_exit_to_user_mode+0xcc/0x220
[ 18.870547] ? do_syscall_64+0x93/0x150
[ 18.870821] ? __memcg_slab_free_hook+0x69/0x290
[ 18.871157] __sys_sendmsg+0x69/0xd0
[ 18.871416] __x64_sys_sendmsg+0x1d/0x30
[ 18.871699] x64_sys_call+0x9e2/0x2670
[ 18.871979] do_syscall_64+0x87/0x150
[ 18.873280] ? do_syscall_64+0x93/0x150
[ 18.874742] ? lock_release+0x7b/0x160
[ 18.876157] ? do_user_addr_fault+0x5ce/0x8f0
[ 18.877833] ? irqentry_exit_to_user_mode+0xc2/0x210
[ 18.879608] ? irqentry_exit+0x77/0xb0
[ 18.879808] ? clear_bhb_loop+0x15/0x70
[ 18.880023] ? clear_bhb_loop+0x15/0x70
[ 18.880223] ? clear_bhb_loop+0x15/0x70
[ 18.880426] entry_SYSCALL_64_after_hwframe+0x76/0x7e
[ 18.880683] RIP: 0033:0x44a957
[ 18.880851] Code: ff ff e8 fc 00 00 00 66 2e 0f 1f 84 00 00 00 00 00 66 90 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 89 54 24 1c 48 8974 24 10
[ 18.881766] RSP: 002b:00007ffcdd00fad8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
[ 18.882149] RAX: ffffffffffffffda RBX: 00007ffcdd010db8 RCX: 000000000044a957
[ 18.882507] RDX: 0000000000000000 RSI: 00007ffcdd00fb70 RDI: 0000000000000003
[ 18.885037] RBP: 00007ffcdd010bc0 R08: 000000000703c770 R09: 000000000703c7c0
[ 18.887203] R10: 0000000000000080 R11: 0000000000000246 R12: 0000000000000001
[ 18.888026] R13: 00007ffcdd010da8 R14: 00000000004ca7d0 R15: 0000000000000001
[ 18.888395] </TASK>
[ 18.888610] ---[ end trace ]---
Fixes: dcc68b4d8084 ("net: sch_ets: Add a new Qdisc")
Reported-by: Haowei Yan <g1042620637@gmail.com>
Suggested-by: Haowei Yan <g1042620637@gmail.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
Reviewed-by: Petr Machata <petrm@nvidia.com>
---
net/sched/sch_ets.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/sched/sch_ets.c b/net/sched/sch_ets.c
index f80bc05d4c5a..516038a44163 100644
--- a/net/sched/sch_ets.c
+++ b/net/sched/sch_ets.c
@@ -91,6 +91,8 @@ ets_class_from_arg(struct Qdisc *sch, unsigned long arg)
{
struct ets_sched *q = qdisc_priv(sch);
+ if (arg == 0 || arg > q->nbands)
+ return NULL;
return &q->classes[arg - 1];
}
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH net v4 1/1] net: sched: fix ets qdisc OOB Indexing
2025-01-11 14:57 [PATCH net v4 1/1] net: sched: fix ets qdisc OOB Indexing Jamal Hadi Salim
@ 2025-01-11 21:01 ` Jakub Kicinski
2025-01-11 21:17 ` Jamal Hadi Salim
2025-01-12 23:53 ` Cong Wang
2025-01-23 3:40 ` patchwork-bot+netdevbpf
2 siblings, 1 reply; 13+ messages in thread
From: Jakub Kicinski @ 2025-01-11 21:01 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: netdev, jiri, xiyou.wangcong, davem, edumazet, petrm, security,
g1042620637
On Sat, 11 Jan 2025 09:57:39 -0500 Jamal Hadi Salim wrote:
> Haowei Yan <g1042620637@gmail.com> found that ets_class_from_arg() can
> index an Out-Of-Bound class in ets_class_from_arg() when passed clid of
> 0. The overflow may cause local privilege escalation.
Code is identical to v1 here...
While fixing the code, could you also trim the stack trace?
Like this:
UBSAN: array-index-out-of-bounds in net/sched/sch_ets.c:93:20
index 18446744073709551615 is out of range for type 'ets_class [16]'
CPU: 0 UID: 0 PID: 1275 Comm: poc Not tainted 6.12.6-dirty #17
Call Trace:
<TASK>
ets_class_change+0x3d6/0x3f0
tc_ctl_tclass+0x251/0x910
rtnetlink_rcv_msg+0x170/0x6f0
netlink_rcv_skb+0x59/0x110
rtnetlink_rcv+0x15/0x30
netlink_unicast+0x1c3/0x2b0
netlink_sendmsg+0x239/0x4b0
____sys_sendmsg+0x3e2/0x410
___sys_sendmsg+0x88/0xe0
__sys_sendmsg+0x69/0xd0
the rest has no value.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net v4 1/1] net: sched: fix ets qdisc OOB Indexing
2025-01-11 21:01 ` Jakub Kicinski
@ 2025-01-11 21:17 ` Jamal Hadi Salim
2025-01-11 21:26 ` Jakub Kicinski
0 siblings, 1 reply; 13+ messages in thread
From: Jamal Hadi Salim @ 2025-01-11 21:17 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, jiri, xiyou.wangcong, davem, edumazet, petrm, security,
g1042620637
On Sat, Jan 11, 2025 at 4:01 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Sat, 11 Jan 2025 09:57:39 -0500 Jamal Hadi Salim wrote:
> > Haowei Yan <g1042620637@gmail.com> found that ets_class_from_arg() can
> > index an Out-Of-Bound class in ets_class_from_arg() when passed clid of
> > 0. The overflow may cause local privilege escalation.
>
> Code is identical to v1 here...
>
The inequality changed > vs >=
> While fixing the code, could you also trim the stack trace?
> Like this:
>
> UBSAN: array-index-out-of-bounds in net/sched/sch_ets.c:93:20
> index 18446744073709551615 is out of range for type 'ets_class [16]'
> CPU: 0 UID: 0 PID: 1275 Comm: poc Not tainted 6.12.6-dirty #17
> Call Trace:
> <TASK>
> ets_class_change+0x3d6/0x3f0
> tc_ctl_tclass+0x251/0x910
> rtnetlink_rcv_msg+0x170/0x6f0
> netlink_rcv_skb+0x59/0x110
> rtnetlink_rcv+0x15/0x30
> netlink_unicast+0x1c3/0x2b0
> netlink_sendmsg+0x239/0x4b0
> ____sys_sendmsg+0x3e2/0x410
> ___sys_sendmsg+0x88/0xe0
> __sys_sendmsg+0x69/0xd0
>
> the rest has no value.
Still want this change?
cheers,
jamal
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net v4 1/1] net: sched: fix ets qdisc OOB Indexing
2025-01-11 21:17 ` Jamal Hadi Salim
@ 2025-01-11 21:26 ` Jakub Kicinski
0 siblings, 0 replies; 13+ messages in thread
From: Jakub Kicinski @ 2025-01-11 21:26 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: netdev, jiri, xiyou.wangcong, davem, edumazet, petrm, security,
g1042620637
On Sat, 11 Jan 2025 16:17:45 -0500 Jamal Hadi Salim wrote:
> > Code is identical to v1 here...
>
> The inequality changed > vs >=
Ah, that works!
> > While fixing the code, could you also trim the stack trace?
> > Like this:
> >
> > UBSAN: array-index-out-of-bounds in net/sched/sch_ets.c:93:20
> > index 18446744073709551615 is out of range for type 'ets_class [16]'
> > CPU: 0 UID: 0 PID: 1275 Comm: poc Not tainted 6.12.6-dirty #17
> > Call Trace:
> > <TASK>
> > ets_class_change+0x3d6/0x3f0
> > tc_ctl_tclass+0x251/0x910
> > rtnetlink_rcv_msg+0x170/0x6f0
> > netlink_rcv_skb+0x59/0x110
> > rtnetlink_rcv+0x15/0x30
> > netlink_unicast+0x1c3/0x2b0
> > netlink_sendmsg+0x239/0x4b0
> > ____sys_sendmsg+0x3e2/0x410
> > ___sys_sendmsg+0x88/0xe0
> > __sys_sendmsg+0x69/0xd0
> >
> > the rest has no value.
>
> Still want this change?
No, it's good.
--
pw-bot: under-review
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net v4 1/1] net: sched: fix ets qdisc OOB Indexing
2025-01-11 14:57 [PATCH net v4 1/1] net: sched: fix ets qdisc OOB Indexing Jamal Hadi Salim
2025-01-11 21:01 ` Jakub Kicinski
@ 2025-01-12 23:53 ` Cong Wang
2025-01-13 10:21 ` Petr Machata
2025-01-23 3:40 ` patchwork-bot+netdevbpf
2 siblings, 1 reply; 13+ messages in thread
From: Cong Wang @ 2025-01-12 23:53 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: netdev, jiri, davem, edumazet, kuba, petrm, security, g1042620637
On Sat, Jan 11, 2025 at 09:57:39AM -0500, Jamal Hadi Salim wrote:
> diff --git a/net/sched/sch_ets.c b/net/sched/sch_ets.c
> index f80bc05d4c5a..516038a44163 100644
> --- a/net/sched/sch_ets.c
> +++ b/net/sched/sch_ets.c
> @@ -91,6 +91,8 @@ ets_class_from_arg(struct Qdisc *sch, unsigned long arg)
> {
> struct ets_sched *q = qdisc_priv(sch);
>
> + if (arg == 0 || arg > q->nbands)
> + return NULL;
> return &q->classes[arg - 1];
> }
I must miss something here. Some callers of this function don't handle
NULL at all, so are you sure it is safe to return NULL for all the
callers here??
For one quick example:
322 static int ets_class_dump_stats(struct Qdisc *sch, unsigned long arg,
323 struct gnet_dump *d)
324 {
325 struct ets_class *cl = ets_class_from_arg(sch, arg);
326 struct Qdisc *cl_q = cl->qdisc;
'cl' is not checked against NULL before dereferencing it.
There are other cases too, please ensure _all_ of them handle NULL
correctly.
Thanks!
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net v4 1/1] net: sched: fix ets qdisc OOB Indexing
2025-01-12 23:53 ` Cong Wang
@ 2025-01-13 10:21 ` Petr Machata
2025-01-13 11:47 ` Jamal Hadi Salim
0 siblings, 1 reply; 13+ messages in thread
From: Petr Machata @ 2025-01-13 10:21 UTC (permalink / raw)
To: Cong Wang
Cc: Jamal Hadi Salim, netdev, jiri, davem, edumazet, kuba, petrm,
security, g1042620637
Cong Wang <xiyou.wangcong@gmail.com> writes:
> On Sat, Jan 11, 2025 at 09:57:39AM -0500, Jamal Hadi Salim wrote:
>> diff --git a/net/sched/sch_ets.c b/net/sched/sch_ets.c
>> index f80bc05d4c5a..516038a44163 100644
>> --- a/net/sched/sch_ets.c
>> +++ b/net/sched/sch_ets.c
>> @@ -91,6 +91,8 @@ ets_class_from_arg(struct Qdisc *sch, unsigned long arg)
>> {
>> struct ets_sched *q = qdisc_priv(sch);
>>
>> + if (arg == 0 || arg > q->nbands)
>> + return NULL;
>> return &q->classes[arg - 1];
>> }
>
> I must miss something here. Some callers of this function don't handle
> NULL at all, so are you sure it is safe to return NULL for all the
> callers here??
>
> For one quick example:
>
> 322 static int ets_class_dump_stats(struct Qdisc *sch, unsigned long arg,
> 323 struct gnet_dump *d)
> 324 {
> 325 struct ets_class *cl = ets_class_from_arg(sch, arg);
> 326 struct Qdisc *cl_q = cl->qdisc;
>
> 'cl' is not checked against NULL before dereferencing it.
>
> There are other cases too, please ensure _all_ of them handle NULL
> correctly.
Yeah, I looked through ets_class_from_arg() callers last week and I
think that besides the one call that needs patching, which already
handles NULL, in all other cases the arg passed to ets_class_from_arg()
comes from class_find, and therefore shouldn't cause the NULL return.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net v4 1/1] net: sched: fix ets qdisc OOB Indexing
2025-01-13 10:21 ` Petr Machata
@ 2025-01-13 11:47 ` Jamal Hadi Salim
2025-01-16 4:36 ` Cong Wang
0 siblings, 1 reply; 13+ messages in thread
From: Jamal Hadi Salim @ 2025-01-13 11:47 UTC (permalink / raw)
To: Petr Machata
Cc: Cong Wang, netdev, jiri, davem, edumazet, kuba, petrm, security,
g1042620637
On Mon, Jan 13, 2025 at 5:29 AM Petr Machata <petrm@nvidia.com> wrote:
>
>
> Cong Wang <xiyou.wangcong@gmail.com> writes:
>
> > On Sat, Jan 11, 2025 at 09:57:39AM -0500, Jamal Hadi Salim wrote:
> >> diff --git a/net/sched/sch_ets.c b/net/sched/sch_ets.c
> >> index f80bc05d4c5a..516038a44163 100644
> >> --- a/net/sched/sch_ets.c
> >> +++ b/net/sched/sch_ets.c
> >> @@ -91,6 +91,8 @@ ets_class_from_arg(struct Qdisc *sch, unsigned long arg)
> >> {
> >> struct ets_sched *q = qdisc_priv(sch);
> >>
> >> + if (arg == 0 || arg > q->nbands)
> >> + return NULL;
> >> return &q->classes[arg - 1];
> >> }
> >
> > I must miss something here. Some callers of this function don't handle
> > NULL at all, so are you sure it is safe to return NULL for all the
> > callers here??
> >
> > For one quick example:
> >
> > 322 static int ets_class_dump_stats(struct Qdisc *sch, unsigned long arg,
> > 323 struct gnet_dump *d)
> > 324 {
> > 325 struct ets_class *cl = ets_class_from_arg(sch, arg);
> > 326 struct Qdisc *cl_q = cl->qdisc;
> >
> > 'cl' is not checked against NULL before dereferencing it.
> >
> > There are other cases too, please ensure _all_ of them handle NULL
> > correctly.
>
> Yeah, I looked through ets_class_from_arg() callers last week and I
> think that besides the one call that needs patching, which already
> handles NULL, in all other cases the arg passed to ets_class_from_arg()
> comes from class_find, and therefore shouldn't cause the NULL return.
Exactly.
Regardless - once the nodes are created we are guaranteed non-null.
See other qdiscs, not just ets.
cheers,
jamal
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net v4 1/1] net: sched: fix ets qdisc OOB Indexing
2025-01-13 11:47 ` Jamal Hadi Salim
@ 2025-01-16 4:36 ` Cong Wang
2025-01-16 8:30 ` Paolo Abeni
0 siblings, 1 reply; 13+ messages in thread
From: Cong Wang @ 2025-01-16 4:36 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Petr Machata, netdev, jiri, davem, edumazet, kuba, petrm,
security, g1042620637
On Mon, Jan 13, 2025 at 06:47:02AM -0500, Jamal Hadi Salim wrote:
> On Mon, Jan 13, 2025 at 5:29 AM Petr Machata <petrm@nvidia.com> wrote:
> >
> >
> > Cong Wang <xiyou.wangcong@gmail.com> writes:
> >
> > > On Sat, Jan 11, 2025 at 09:57:39AM -0500, Jamal Hadi Salim wrote:
> > >> diff --git a/net/sched/sch_ets.c b/net/sched/sch_ets.c
> > >> index f80bc05d4c5a..516038a44163 100644
> > >> --- a/net/sched/sch_ets.c
> > >> +++ b/net/sched/sch_ets.c
> > >> @@ -91,6 +91,8 @@ ets_class_from_arg(struct Qdisc *sch, unsigned long arg)
> > >> {
> > >> struct ets_sched *q = qdisc_priv(sch);
> > >>
> > >> + if (arg == 0 || arg > q->nbands)
> > >> + return NULL;
> > >> return &q->classes[arg - 1];
> > >> }
> > >
> > > I must miss something here. Some callers of this function don't handle
> > > NULL at all, so are you sure it is safe to return NULL for all the
> > > callers here??
> > >
> > > For one quick example:
> > >
> > > 322 static int ets_class_dump_stats(struct Qdisc *sch, unsigned long arg,
> > > 323 struct gnet_dump *d)
> > > 324 {
> > > 325 struct ets_class *cl = ets_class_from_arg(sch, arg);
> > > 326 struct Qdisc *cl_q = cl->qdisc;
> > >
> > > 'cl' is not checked against NULL before dereferencing it.
> > >
> > > There are other cases too, please ensure _all_ of them handle NULL
> > > correctly.
> >
> > Yeah, I looked through ets_class_from_arg() callers last week and I
> > think that besides the one call that needs patching, which already
> > handles NULL, in all other cases the arg passed to ets_class_from_arg()
> > comes from class_find, and therefore shouldn't cause the NULL return.
>
> Exactly.
> Regardless - once the nodes are created we are guaranteed non-null.
> See other qdiscs, not just ets.
The anti-pattern part is that we usually pass the pointer instead of
classid with these 'arg', hence it is unsigned long. In fact, for
->change(), classid is passed as the 2nd parameter, not the 5th.
The pointer should come from the return value of ->find().
Something like the untested patch below.
Thanks.
---->
diff --git a/net/sched/sch_ets.c b/net/sched/sch_ets.c
index f80bc05d4c5a..3b7253e8756f 100644
--- a/net/sched/sch_ets.c
+++ b/net/sched/sch_ets.c
@@ -86,12 +86,9 @@ static int ets_quantum_parse(struct Qdisc *sch, const struct nlattr *attr,
return 0;
}
-static struct ets_class *
-ets_class_from_arg(struct Qdisc *sch, unsigned long arg)
+static struct ets_class *ets_class_from_arg(unsigned long arg)
{
- struct ets_sched *q = qdisc_priv(sch);
-
- return &q->classes[arg - 1];
+ return (struct ets_class *) arg;
}
static u32 ets_class_id(struct Qdisc *sch, const struct ets_class *cl)
@@ -198,7 +195,7 @@ static int ets_class_change(struct Qdisc *sch, u32 classid, u32 parentid,
struct nlattr **tca, unsigned long *arg,
struct netlink_ext_ack *extack)
{
- struct ets_class *cl = ets_class_from_arg(sch, *arg);
+ struct ets_class *cl = ets_class_from_arg(*arg);
struct ets_sched *q = qdisc_priv(sch);
struct nlattr *opt = tca[TCA_OPTIONS];
struct nlattr *tb[TCA_ETS_MAX + 1];
@@ -248,7 +245,7 @@ static int ets_class_graft(struct Qdisc *sch, unsigned long arg,
struct Qdisc *new, struct Qdisc **old,
struct netlink_ext_ack *extack)
{
- struct ets_class *cl = ets_class_from_arg(sch, arg);
+ struct ets_class *cl = ets_class_from_arg(arg);
if (!new) {
new = qdisc_create_dflt(sch->dev_queue, &pfifo_qdisc_ops,
@@ -266,7 +263,7 @@ static int ets_class_graft(struct Qdisc *sch, unsigned long arg,
static struct Qdisc *ets_class_leaf(struct Qdisc *sch, unsigned long arg)
{
- struct ets_class *cl = ets_class_from_arg(sch, arg);
+ struct ets_class *cl = ets_class_from_arg(arg);
return cl->qdisc;
}
@@ -278,12 +275,12 @@ static unsigned long ets_class_find(struct Qdisc *sch, u32 classid)
if (band - 1 >= q->nbands)
return 0;
- return band;
+ return (unsigned long)&q->classes[band - 1];
}
static void ets_class_qlen_notify(struct Qdisc *sch, unsigned long arg)
{
- struct ets_class *cl = ets_class_from_arg(sch, arg);
+ struct ets_class *cl = ets_class_from_arg(arg);
struct ets_sched *q = qdisc_priv(sch);
/* We get notified about zero-length child Qdiscs as well if they are
@@ -297,7 +294,7 @@ static void ets_class_qlen_notify(struct Qdisc *sch, unsigned long arg)
static int ets_class_dump(struct Qdisc *sch, unsigned long arg,
struct sk_buff *skb, struct tcmsg *tcm)
{
- struct ets_class *cl = ets_class_from_arg(sch, arg);
+ struct ets_class *cl = ets_class_from_arg(arg);
struct ets_sched *q = qdisc_priv(sch);
struct nlattr *nest;
@@ -322,7 +319,7 @@ static int ets_class_dump(struct Qdisc *sch, unsigned long arg,
static int ets_class_dump_stats(struct Qdisc *sch, unsigned long arg,
struct gnet_dump *d)
{
- struct ets_class *cl = ets_class_from_arg(sch, arg);
+ struct ets_class *cl = ets_class_from_arg(arg);
struct Qdisc *cl_q = cl->qdisc;
if (gnet_stats_copy_basic(d, NULL, &cl_q->bstats, true) < 0 ||
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH net v4 1/1] net: sched: fix ets qdisc OOB Indexing
2025-01-16 4:36 ` Cong Wang
@ 2025-01-16 8:30 ` Paolo Abeni
2025-01-16 13:35 ` Jamal Hadi Salim
0 siblings, 1 reply; 13+ messages in thread
From: Paolo Abeni @ 2025-01-16 8:30 UTC (permalink / raw)
To: Cong Wang, Jamal Hadi Salim
Cc: Petr Machata, netdev, jiri, davem, edumazet, kuba, petrm,
security, g1042620637
Hi,
On 1/16/25 5:36 AM, Cong Wang wrote:
> On Mon, Jan 13, 2025 at 06:47:02AM -0500, Jamal Hadi Salim wrote:
>> On Mon, Jan 13, 2025 at 5:29 AM Petr Machata <petrm@nvidia.com> wrote:
>>>
>>>
>>> Cong Wang <xiyou.wangcong@gmail.com> writes:
>>>
>>>> On Sat, Jan 11, 2025 at 09:57:39AM -0500, Jamal Hadi Salim wrote:
>>>>> diff --git a/net/sched/sch_ets.c b/net/sched/sch_ets.c
>>>>> index f80bc05d4c5a..516038a44163 100644
>>>>> --- a/net/sched/sch_ets.c
>>>>> +++ b/net/sched/sch_ets.c
>>>>> @@ -91,6 +91,8 @@ ets_class_from_arg(struct Qdisc *sch, unsigned long arg)
>>>>> {
>>>>> struct ets_sched *q = qdisc_priv(sch);
>>>>>
>>>>> + if (arg == 0 || arg > q->nbands)
>>>>> + return NULL;
>>>>> return &q->classes[arg - 1];
>>>>> }
>>>>
>>>> I must miss something here. Some callers of this function don't handle
>>>> NULL at all, so are you sure it is safe to return NULL for all the
>>>> callers here??
>>>>
>>>> For one quick example:
>>>>
>>>> 322 static int ets_class_dump_stats(struct Qdisc *sch, unsigned long arg,
>>>> 323 struct gnet_dump *d)
>>>> 324 {
>>>> 325 struct ets_class *cl = ets_class_from_arg(sch, arg);
>>>> 326 struct Qdisc *cl_q = cl->qdisc;
>>>>
>>>> 'cl' is not checked against NULL before dereferencing it.
>>>>
>>>> There are other cases too, please ensure _all_ of them handle NULL
>>>> correctly.
>>>
>>> Yeah, I looked through ets_class_from_arg() callers last week and I
>>> think that besides the one call that needs patching, which already
>>> handles NULL, in all other cases the arg passed to ets_class_from_arg()
>>> comes from class_find, and therefore shouldn't cause the NULL return.
>>
>> Exactly.
>> Regardless - once the nodes are created we are guaranteed non-null.
>> See other qdiscs, not just ets.
>
> The anti-pattern part is that we usually pass the pointer instead of
> classid with these 'arg', hence it is unsigned long. In fact, for
> ->change(), classid is passed as the 2nd parameter, not the 5th.
> The pointer should come from the return value of ->find().
>
> Something like the untested patch below.
>
> Thanks.
>
> ---->
>
> diff --git a/net/sched/sch_ets.c b/net/sched/sch_ets.c
> index f80bc05d4c5a..3b7253e8756f 100644
> --- a/net/sched/sch_ets.c
> +++ b/net/sched/sch_ets.c
> @@ -86,12 +86,9 @@ static int ets_quantum_parse(struct Qdisc *sch, const struct nlattr *attr,
> return 0;
> }
>
> -static struct ets_class *
> -ets_class_from_arg(struct Qdisc *sch, unsigned long arg)
> +static struct ets_class *ets_class_from_arg(unsigned long arg)
> {
> - struct ets_sched *q = qdisc_priv(sch);
> -
> - return &q->classes[arg - 1];
> + return (struct ets_class *) arg;
> }
>
> static u32 ets_class_id(struct Qdisc *sch, const struct ets_class *cl)
> @@ -198,7 +195,7 @@ static int ets_class_change(struct Qdisc *sch, u32 classid, u32 parentid,
> struct nlattr **tca, unsigned long *arg,
> struct netlink_ext_ack *extack)
> {
> - struct ets_class *cl = ets_class_from_arg(sch, *arg);
> + struct ets_class *cl = ets_class_from_arg(*arg);
> struct ets_sched *q = qdisc_priv(sch);
> struct nlattr *opt = tca[TCA_OPTIONS];
> struct nlattr *tb[TCA_ETS_MAX + 1];
> @@ -248,7 +245,7 @@ static int ets_class_graft(struct Qdisc *sch, unsigned long arg,
> struct Qdisc *new, struct Qdisc **old,
> struct netlink_ext_ack *extack)
> {
> - struct ets_class *cl = ets_class_from_arg(sch, arg);
> + struct ets_class *cl = ets_class_from_arg(arg);
>
> if (!new) {
> new = qdisc_create_dflt(sch->dev_queue, &pfifo_qdisc_ops,
> @@ -266,7 +263,7 @@ static int ets_class_graft(struct Qdisc *sch, unsigned long arg,
>
> static struct Qdisc *ets_class_leaf(struct Qdisc *sch, unsigned long arg)
> {
> - struct ets_class *cl = ets_class_from_arg(sch, arg);
> + struct ets_class *cl = ets_class_from_arg(arg);
>
> return cl->qdisc;
> }
> @@ -278,12 +275,12 @@ static unsigned long ets_class_find(struct Qdisc *sch, u32 classid)
>
> if (band - 1 >= q->nbands)
> return 0;
> - return band;
> + return (unsigned long)&q->classes[band - 1];
> }
>
> static void ets_class_qlen_notify(struct Qdisc *sch, unsigned long arg)
> {
> - struct ets_class *cl = ets_class_from_arg(sch, arg);
> + struct ets_class *cl = ets_class_from_arg(arg);
> struct ets_sched *q = qdisc_priv(sch);
>
> /* We get notified about zero-length child Qdiscs as well if they are
> @@ -297,7 +294,7 @@ static void ets_class_qlen_notify(struct Qdisc *sch, unsigned long arg)
> static int ets_class_dump(struct Qdisc *sch, unsigned long arg,
> struct sk_buff *skb, struct tcmsg *tcm)
> {
> - struct ets_class *cl = ets_class_from_arg(sch, arg);
> + struct ets_class *cl = ets_class_from_arg(arg);
> struct ets_sched *q = qdisc_priv(sch);
> struct nlattr *nest;
>
> @@ -322,7 +319,7 @@ static int ets_class_dump(struct Qdisc *sch, unsigned long arg,
> static int ets_class_dump_stats(struct Qdisc *sch, unsigned long arg,
> struct gnet_dump *d)
> {
> - struct ets_class *cl = ets_class_from_arg(sch, arg);
> + struct ets_class *cl = ets_class_from_arg(arg);
> struct Qdisc *cl_q = cl->qdisc;
>
> if (gnet_stats_copy_basic(d, NULL, &cl_q->bstats, true) < 0 ||
The blamed commit is quite old, and the fix will be propagated on
several stable trees. Jamal's option is IMHO more suitable to such goal,
being less invasive and with possibly less conflict.
Would you be fine with Jamal's fix and following-up with the above on
net-next?
Thanks,
Paolo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net v4 1/1] net: sched: fix ets qdisc OOB Indexing
2025-01-16 8:30 ` Paolo Abeni
@ 2025-01-16 13:35 ` Jamal Hadi Salim
2025-01-22 18:40 ` Jamal Hadi Salim
0 siblings, 1 reply; 13+ messages in thread
From: Jamal Hadi Salim @ 2025-01-16 13:35 UTC (permalink / raw)
To: Paolo Abeni
Cc: Cong Wang, Petr Machata, netdev, jiri, davem, edumazet, kuba,
petrm, security, g1042620637
On Thu, Jan 16, 2025 at 3:30 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> Hi,
>
> On 1/16/25 5:36 AM, Cong Wang wrote:
> > On Mon, Jan 13, 2025 at 06:47:02AM -0500, Jamal Hadi Salim wrote:
> >> On Mon, Jan 13, 2025 at 5:29 AM Petr Machata <petrm@nvidia.com> wrote:
> >>>
> >>>
> >>> Cong Wang <xiyou.wangcong@gmail.com> writes:
> >>>
> >>>> On Sat, Jan 11, 2025 at 09:57:39AM -0500, Jamal Hadi Salim wrote:
> >>>>> diff --git a/net/sched/sch_ets.c b/net/sched/sch_ets.c
> >>>>> index f80bc05d4c5a..516038a44163 100644
> >>>>> --- a/net/sched/sch_ets.c
> >>>>> +++ b/net/sched/sch_ets.c
> >>>>> @@ -91,6 +91,8 @@ ets_class_from_arg(struct Qdisc *sch, unsigned long arg)
> >>>>> {
> >>>>> struct ets_sched *q = qdisc_priv(sch);
> >>>>>
> >>>>> + if (arg == 0 || arg > q->nbands)
> >>>>> + return NULL;
> >>>>> return &q->classes[arg - 1];
> >>>>> }
> >>>>
> >>>> I must miss something here. Some callers of this function don't handle
> >>>> NULL at all, so are you sure it is safe to return NULL for all the
> >>>> callers here??
> >>>>
> >>>> For one quick example:
> >>>>
> >>>> 322 static int ets_class_dump_stats(struct Qdisc *sch, unsigned long arg,
> >>>> 323 struct gnet_dump *d)
> >>>> 324 {
> >>>> 325 struct ets_class *cl = ets_class_from_arg(sch, arg);
> >>>> 326 struct Qdisc *cl_q = cl->qdisc;
> >>>>
> >>>> 'cl' is not checked against NULL before dereferencing it.
> >>>>
> >>>> There are other cases too, please ensure _all_ of them handle NULL
> >>>> correctly.
> >>>
> >>> Yeah, I looked through ets_class_from_arg() callers last week and I
> >>> think that besides the one call that needs patching, which already
> >>> handles NULL, in all other cases the arg passed to ets_class_from_arg()
> >>> comes from class_find, and therefore shouldn't cause the NULL return.
> >>
> >> Exactly.
> >> Regardless - once the nodes are created we are guaranteed non-null.
> >> See other qdiscs, not just ets.
> >
> > The anti-pattern part is that we usually pass the pointer instead of
> > classid with these 'arg', hence it is unsigned long. In fact, for
> > ->change(), classid is passed as the 2nd parameter, not the 5th.
> > The pointer should come from the return value of ->find().
> >
> > Something like the untested patch below.
> >
> > Thanks.
> >
> > ---->
> >
> > diff --git a/net/sched/sch_ets.c b/net/sched/sch_ets.c
> > index f80bc05d4c5a..3b7253e8756f 100644
> > --- a/net/sched/sch_ets.c
> > +++ b/net/sched/sch_ets.c
> > @@ -86,12 +86,9 @@ static int ets_quantum_parse(struct Qdisc *sch, const struct nlattr *attr,
> > return 0;
> > }
> >
> > -static struct ets_class *
> > -ets_class_from_arg(struct Qdisc *sch, unsigned long arg)
> > +static struct ets_class *ets_class_from_arg(unsigned long arg)
> > {
> > - struct ets_sched *q = qdisc_priv(sch);
> > -
> > - return &q->classes[arg - 1];
> > + return (struct ets_class *) arg;
> > }
> >
> > static u32 ets_class_id(struct Qdisc *sch, const struct ets_class *cl)
> > @@ -198,7 +195,7 @@ static int ets_class_change(struct Qdisc *sch, u32 classid, u32 parentid,
> > struct nlattr **tca, unsigned long *arg,
> > struct netlink_ext_ack *extack)
> > {
> > - struct ets_class *cl = ets_class_from_arg(sch, *arg);
> > + struct ets_class *cl = ets_class_from_arg(*arg);
> > struct ets_sched *q = qdisc_priv(sch);
> > struct nlattr *opt = tca[TCA_OPTIONS];
> > struct nlattr *tb[TCA_ETS_MAX + 1];
> > @@ -248,7 +245,7 @@ static int ets_class_graft(struct Qdisc *sch, unsigned long arg,
> > struct Qdisc *new, struct Qdisc **old,
> > struct netlink_ext_ack *extack)
> > {
> > - struct ets_class *cl = ets_class_from_arg(sch, arg);
> > + struct ets_class *cl = ets_class_from_arg(arg);
> >
> > if (!new) {
> > new = qdisc_create_dflt(sch->dev_queue, &pfifo_qdisc_ops,
> > @@ -266,7 +263,7 @@ static int ets_class_graft(struct Qdisc *sch, unsigned long arg,
> >
> > static struct Qdisc *ets_class_leaf(struct Qdisc *sch, unsigned long arg)
> > {
> > - struct ets_class *cl = ets_class_from_arg(sch, arg);
> > + struct ets_class *cl = ets_class_from_arg(arg);
> >
> > return cl->qdisc;
> > }
> > @@ -278,12 +275,12 @@ static unsigned long ets_class_find(struct Qdisc *sch, u32 classid)
> >
> > if (band - 1 >= q->nbands)
> > return 0;
> > - return band;
> > + return (unsigned long)&q->classes[band - 1];
> > }
> >
> > static void ets_class_qlen_notify(struct Qdisc *sch, unsigned long arg)
> > {
> > - struct ets_class *cl = ets_class_from_arg(sch, arg);
> > + struct ets_class *cl = ets_class_from_arg(arg);
> > struct ets_sched *q = qdisc_priv(sch);
> >
> > /* We get notified about zero-length child Qdiscs as well if they are
> > @@ -297,7 +294,7 @@ static void ets_class_qlen_notify(struct Qdisc *sch, unsigned long arg)
> > static int ets_class_dump(struct Qdisc *sch, unsigned long arg,
> > struct sk_buff *skb, struct tcmsg *tcm)
> > {
> > - struct ets_class *cl = ets_class_from_arg(sch, arg);
> > + struct ets_class *cl = ets_class_from_arg(arg);
> > struct ets_sched *q = qdisc_priv(sch);
> > struct nlattr *nest;
> >
> > @@ -322,7 +319,7 @@ static int ets_class_dump(struct Qdisc *sch, unsigned long arg,
> > static int ets_class_dump_stats(struct Qdisc *sch, unsigned long arg,
> > struct gnet_dump *d)
> > {
> > - struct ets_class *cl = ets_class_from_arg(sch, arg);
> > + struct ets_class *cl = ets_class_from_arg(arg);
> > struct Qdisc *cl_q = cl->qdisc;
> >
> > if (gnet_stats_copy_basic(d, NULL, &cl_q->bstats, true) < 0 ||
>
> The blamed commit is quite old, and the fix will be propagated on
> several stable trees. Jamal's option is IMHO more suitable to such goal,
> being less invasive and with possibly less conflict.
>
> Would you be fine with Jamal's fix and following-up with the above on
> net-next?
>
Agreed.
The pattern is followed by all qdiscs, not just ets. So if we are
fixing patterns it should be a separate patch.
cheers,
jamal
> Thanks,
>
> Paolo
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net v4 1/1] net: sched: fix ets qdisc OOB Indexing
2025-01-16 13:35 ` Jamal Hadi Salim
@ 2025-01-22 18:40 ` Jamal Hadi Salim
2025-01-23 3:36 ` Jakub Kicinski
0 siblings, 1 reply; 13+ messages in thread
From: Jamal Hadi Salim @ 2025-01-22 18:40 UTC (permalink / raw)
To: Paolo Abeni
Cc: Cong Wang, Petr Machata, netdev, jiri, davem, edumazet, kuba,
petrm, security, g1042620637
On Thu, Jan 16, 2025 at 8:35 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Thu, Jan 16, 2025 at 3:30 AM Paolo Abeni <pabeni@redhat.com> wrote:
> >
> > Hi,
> >
> > On 1/16/25 5:36 AM, Cong Wang wrote:
> > > On Mon, Jan 13, 2025 at 06:47:02AM -0500, Jamal Hadi Salim wrote:
> > >> On Mon, Jan 13, 2025 at 5:29 AM Petr Machata <petrm@nvidia.com> wrote:
> > >>>
> > >>>
> > >>> Cong Wang <xiyou.wangcong@gmail.com> writes:
> > >>>
> > >>>> On Sat, Jan 11, 2025 at 09:57:39AM -0500, Jamal Hadi Salim wrote:
> > >>>>> diff --git a/net/sched/sch_ets.c b/net/sched/sch_ets.c
> > >>>>> index f80bc05d4c5a..516038a44163 100644
> > >>>>> --- a/net/sched/sch_ets.c
> > >>>>> +++ b/net/sched/sch_ets.c
> > >>>>> @@ -91,6 +91,8 @@ ets_class_from_arg(struct Qdisc *sch, unsigned long arg)
> > >>>>> {
> > >>>>> struct ets_sched *q = qdisc_priv(sch);
> > >>>>>
> > >>>>> + if (arg == 0 || arg > q->nbands)
> > >>>>> + return NULL;
> > >>>>> return &q->classes[arg - 1];
> > >>>>> }
> > >>>>
> > >>>> I must miss something here. Some callers of this function don't handle
> > >>>> NULL at all, so are you sure it is safe to return NULL for all the
> > >>>> callers here??
> > >>>>
> > >>>> For one quick example:
> > >>>>
> > >>>> 322 static int ets_class_dump_stats(struct Qdisc *sch, unsigned long arg,
> > >>>> 323 struct gnet_dump *d)
> > >>>> 324 {
> > >>>> 325 struct ets_class *cl = ets_class_from_arg(sch, arg);
> > >>>> 326 struct Qdisc *cl_q = cl->qdisc;
> > >>>>
> > >>>> 'cl' is not checked against NULL before dereferencing it.
> > >>>>
> > >>>> There are other cases too, please ensure _all_ of them handle NULL
> > >>>> correctly.
> > >>>
> > >>> Yeah, I looked through ets_class_from_arg() callers last week and I
> > >>> think that besides the one call that needs patching, which already
> > >>> handles NULL, in all other cases the arg passed to ets_class_from_arg()
> > >>> comes from class_find, and therefore shouldn't cause the NULL return.
> > >>
> > >> Exactly.
> > >> Regardless - once the nodes are created we are guaranteed non-null.
> > >> See other qdiscs, not just ets.
> > >
> > > The anti-pattern part is that we usually pass the pointer instead of
> > > classid with these 'arg', hence it is unsigned long. In fact, for
> > > ->change(), classid is passed as the 2nd parameter, not the 5th.
> > > The pointer should come from the return value of ->find().
> > >
> > > Something like the untested patch below.
> > >
> > > Thanks.
> > >
> > > ---->
> > >
> > > diff --git a/net/sched/sch_ets.c b/net/sched/sch_ets.c
> > > index f80bc05d4c5a..3b7253e8756f 100644
> > > --- a/net/sched/sch_ets.c
> > > +++ b/net/sched/sch_ets.c
> > > @@ -86,12 +86,9 @@ static int ets_quantum_parse(struct Qdisc *sch, const struct nlattr *attr,
> > > return 0;
> > > }
> > >
> > > -static struct ets_class *
> > > -ets_class_from_arg(struct Qdisc *sch, unsigned long arg)
> > > +static struct ets_class *ets_class_from_arg(unsigned long arg)
> > > {
> > > - struct ets_sched *q = qdisc_priv(sch);
> > > -
> > > - return &q->classes[arg - 1];
> > > + return (struct ets_class *) arg;
> > > }
> > >
> > > static u32 ets_class_id(struct Qdisc *sch, const struct ets_class *cl)
> > > @@ -198,7 +195,7 @@ static int ets_class_change(struct Qdisc *sch, u32 classid, u32 parentid,
> > > struct nlattr **tca, unsigned long *arg,
> > > struct netlink_ext_ack *extack)
> > > {
> > > - struct ets_class *cl = ets_class_from_arg(sch, *arg);
> > > + struct ets_class *cl = ets_class_from_arg(*arg);
> > > struct ets_sched *q = qdisc_priv(sch);
> > > struct nlattr *opt = tca[TCA_OPTIONS];
> > > struct nlattr *tb[TCA_ETS_MAX + 1];
> > > @@ -248,7 +245,7 @@ static int ets_class_graft(struct Qdisc *sch, unsigned long arg,
> > > struct Qdisc *new, struct Qdisc **old,
> > > struct netlink_ext_ack *extack)
> > > {
> > > - struct ets_class *cl = ets_class_from_arg(sch, arg);
> > > + struct ets_class *cl = ets_class_from_arg(arg);
> > >
> > > if (!new) {
> > > new = qdisc_create_dflt(sch->dev_queue, &pfifo_qdisc_ops,
> > > @@ -266,7 +263,7 @@ static int ets_class_graft(struct Qdisc *sch, unsigned long arg,
> > >
> > > static struct Qdisc *ets_class_leaf(struct Qdisc *sch, unsigned long arg)
> > > {
> > > - struct ets_class *cl = ets_class_from_arg(sch, arg);
> > > + struct ets_class *cl = ets_class_from_arg(arg);
> > >
> > > return cl->qdisc;
> > > }
> > > @@ -278,12 +275,12 @@ static unsigned long ets_class_find(struct Qdisc *sch, u32 classid)
> > >
> > > if (band - 1 >= q->nbands)
> > > return 0;
> > > - return band;
> > > + return (unsigned long)&q->classes[band - 1];
> > > }
> > >
> > > static void ets_class_qlen_notify(struct Qdisc *sch, unsigned long arg)
> > > {
> > > - struct ets_class *cl = ets_class_from_arg(sch, arg);
> > > + struct ets_class *cl = ets_class_from_arg(arg);
> > > struct ets_sched *q = qdisc_priv(sch);
> > >
> > > /* We get notified about zero-length child Qdiscs as well if they are
> > > @@ -297,7 +294,7 @@ static void ets_class_qlen_notify(struct Qdisc *sch, unsigned long arg)
> > > static int ets_class_dump(struct Qdisc *sch, unsigned long arg,
> > > struct sk_buff *skb, struct tcmsg *tcm)
> > > {
> > > - struct ets_class *cl = ets_class_from_arg(sch, arg);
> > > + struct ets_class *cl = ets_class_from_arg(arg);
> > > struct ets_sched *q = qdisc_priv(sch);
> > > struct nlattr *nest;
> > >
> > > @@ -322,7 +319,7 @@ static int ets_class_dump(struct Qdisc *sch, unsigned long arg,
> > > static int ets_class_dump_stats(struct Qdisc *sch, unsigned long arg,
> > > struct gnet_dump *d)
> > > {
> > > - struct ets_class *cl = ets_class_from_arg(sch, arg);
> > > + struct ets_class *cl = ets_class_from_arg(arg);
> > > struct Qdisc *cl_q = cl->qdisc;
> > >
> > > if (gnet_stats_copy_basic(d, NULL, &cl_q->bstats, true) < 0 ||
> >
> > The blamed commit is quite old, and the fix will be propagated on
> > several stable trees. Jamal's option is IMHO more suitable to such goal,
> > being less invasive and with possibly less conflict.
> >
> > Would you be fine with Jamal's fix and following-up with the above on
> > net-next?
> >
>
> Agreed.
> The pattern is followed by all qdiscs, not just ets. So if we are
> fixing patterns it should be a separate patch.
>
Can we please apply this?
cheers,
jamal
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net v4 1/1] net: sched: fix ets qdisc OOB Indexing
2025-01-22 18:40 ` Jamal Hadi Salim
@ 2025-01-23 3:36 ` Jakub Kicinski
0 siblings, 0 replies; 13+ messages in thread
From: Jakub Kicinski @ 2025-01-23 3:36 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Paolo Abeni, Cong Wang, Petr Machata, netdev, jiri, davem,
edumazet, petrm, security, g1042620637
On Wed, 22 Jan 2025 13:40:15 -0500 Jamal Hadi Salim wrote:
> > Agreed.
> > The pattern is followed by all qdiscs, not just ets. So if we are
> > fixing patterns it should be a separate patch.
>
> Can we please apply this?
Sorry!
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net v4 1/1] net: sched: fix ets qdisc OOB Indexing
2025-01-11 14:57 [PATCH net v4 1/1] net: sched: fix ets qdisc OOB Indexing Jamal Hadi Salim
2025-01-11 21:01 ` Jakub Kicinski
2025-01-12 23:53 ` Cong Wang
@ 2025-01-23 3:40 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 13+ messages in thread
From: patchwork-bot+netdevbpf @ 2025-01-23 3:40 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: netdev, jiri, xiyou.wangcong, davem, edumazet, kuba, petrm,
security, g1042620637
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Sat, 11 Jan 2025 09:57:39 -0500 you wrote:
> Haowei Yan <g1042620637@gmail.com> found that ets_class_from_arg() can
> index an Out-Of-Bound class in ets_class_from_arg() when passed clid of
> 0. The overflow may cause local privilege escalation.
>
> [ 18.852298] ------------[ cut here ]------------
> [ 18.853271] UBSAN: array-index-out-of-bounds in net/sched/sch_ets.c:93:20
> [ 18.853743] index 18446744073709551615 is out of range for type 'ets_class [16]'
> [ 18.854254] CPU: 0 UID: 0 PID: 1275 Comm: poc Not tainted 6.12.6-dirty #17
> [ 18.854821] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
> [ 18.856532] Call Trace:
> [ 18.857441] <TASK>
> [ 18.858227] dump_stack_lvl+0xc2/0xf0
> [ 18.859607] dump_stack+0x10/0x20
> [ 18.860908] __ubsan_handle_out_of_bounds+0xa7/0xf0
> [ 18.864022] ets_class_change+0x3d6/0x3f0
> [ 18.864322] tc_ctl_tclass+0x251/0x910
> [ 18.864587] ? lock_acquire+0x5e/0x140
> [ 18.865113] ? __mutex_lock+0x9c/0xe70
> [ 18.866009] ? __mutex_lock+0xa34/0xe70
> [ 18.866401] rtnetlink_rcv_msg+0x170/0x6f0
> [ 18.866806] ? __lock_acquire+0x578/0xc10
> [ 18.867184] ? __pfx_rtnetlink_rcv_msg+0x10/0x10
> [ 18.867503] netlink_rcv_skb+0x59/0x110
> [ 18.867776] rtnetlink_rcv+0x15/0x30
> [ 18.868159] netlink_unicast+0x1c3/0x2b0
> [ 18.868440] netlink_sendmsg+0x239/0x4b0
> [ 18.868721] ____sys_sendmsg+0x3e2/0x410
> [ 18.869012] ___sys_sendmsg+0x88/0xe0
> [ 18.869276] ? rseq_ip_fixup+0x198/0x260
> [ 18.869563] ? rseq_update_cpu_node_id+0x10a/0x190
> [ 18.869900] ? trace_hardirqs_off+0x5a/0xd0
> [ 18.870196] ? syscall_exit_to_user_mode+0xcc/0x220
> [ 18.870547] ? do_syscall_64+0x93/0x150
> [ 18.870821] ? __memcg_slab_free_hook+0x69/0x290
> [ 18.871157] __sys_sendmsg+0x69/0xd0
> [ 18.871416] __x64_sys_sendmsg+0x1d/0x30
> [ 18.871699] x64_sys_call+0x9e2/0x2670
> [ 18.871979] do_syscall_64+0x87/0x150
> [ 18.873280] ? do_syscall_64+0x93/0x150
> [ 18.874742] ? lock_release+0x7b/0x160
> [ 18.876157] ? do_user_addr_fault+0x5ce/0x8f0
> [ 18.877833] ? irqentry_exit_to_user_mode+0xc2/0x210
> [ 18.879608] ? irqentry_exit+0x77/0xb0
> [ 18.879808] ? clear_bhb_loop+0x15/0x70
> [ 18.880023] ? clear_bhb_loop+0x15/0x70
> [ 18.880223] ? clear_bhb_loop+0x15/0x70
> [ 18.880426] entry_SYSCALL_64_after_hwframe+0x76/0x7e
> [ 18.880683] RIP: 0033:0x44a957
> [ 18.880851] Code: ff ff e8 fc 00 00 00 66 2e 0f 1f 84 00 00 00 00 00 66 90 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 89 54 24 1c 48 8974 24 10
> [ 18.881766] RSP: 002b:00007ffcdd00fad8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
> [ 18.882149] RAX: ffffffffffffffda RBX: 00007ffcdd010db8 RCX: 000000000044a957
> [ 18.882507] RDX: 0000000000000000 RSI: 00007ffcdd00fb70 RDI: 0000000000000003
> [ 18.885037] RBP: 00007ffcdd010bc0 R08: 000000000703c770 R09: 000000000703c7c0
> [ 18.887203] R10: 0000000000000080 R11: 0000000000000246 R12: 0000000000000001
> [ 18.888026] R13: 00007ffcdd010da8 R14: 00000000004ca7d0 R15: 0000000000000001
> [ 18.888395] </TASK>
> [ 18.888610] ---[ end trace ]---
>
> [...]
Here is the summary with links:
- [net,v4,1/1] net: sched: fix ets qdisc OOB Indexing
https://git.kernel.org/netdev/net/c/d62b04fca434
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2025-01-23 3:40 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-11 14:57 [PATCH net v4 1/1] net: sched: fix ets qdisc OOB Indexing Jamal Hadi Salim
2025-01-11 21:01 ` Jakub Kicinski
2025-01-11 21:17 ` Jamal Hadi Salim
2025-01-11 21:26 ` Jakub Kicinski
2025-01-12 23:53 ` Cong Wang
2025-01-13 10:21 ` Petr Machata
2025-01-13 11:47 ` Jamal Hadi Salim
2025-01-16 4:36 ` Cong Wang
2025-01-16 8:30 ` Paolo Abeni
2025-01-16 13:35 ` Jamal Hadi Salim
2025-01-22 18:40 ` Jamal Hadi Salim
2025-01-23 3:36 ` Jakub Kicinski
2025-01-23 3:40 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).