Message-ID: <66ba9652-5f9e-4a15-9eec-58ad78cbd745@redhat.com>
Date: Thu, 16 Jan 2025 09:30:26 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Cong Wang <xiyou.wangcong@...il.com>, Jamal Hadi Salim <jhs@...atatu.com>
Cc: 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

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?

Thanks,

Paolo

Powered by blists - more mailing lists