| 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
| ||
|
Message-ID: <CAM0EoMkPHKW8WPG4t2V-6wpCcnnuV5y1fs0OmVAaBYcPnbMmkQ@mail.gmail.com>
Date: Wed, 22 Jan 2025 13:40:15 -0500
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: Cong Wang <xiyou.wangcong@...il.com>, Petr Machata <petrm@...dia.com>, netdev@...r.kernel.org,
jiri@...nulli.us, davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
petrm@...lanox.com, security@...nel.org, g1042620637@...il.com
Subject: Re: [PATCH net v4 1/1] net: sched: fix ets qdisc OOB Indexing
On Thu, Jan 16, 2025 at 8:35 AM Jamal Hadi Salim <jhs@...atatu.com> wrote:
>
> On Thu, Jan 16, 2025 at 3:30 AM Paolo Abeni <pabeni@...hat.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@...dia.com> wrote:
> > >>>
> > >>>
> > >>> Cong Wang <xiyou.wangcong@...il.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
Powered by blists - more mailing lists