Re: [PATCH net v4 1/1] net: sched: fix ets qdisc OOB Indexing

[Cong Wang <xiyou.wangcong@gmail.com>]

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 ||