[PATCH] net: sched: fix potential null pointer deref

[PATCH] net: sched: fix potential null pointer deref

[Jianhao Xu]

mq_queue_get() may return NULL, a check is needed to avoid using
the NULL pointer.

Signed-off-by: Jianhao Xu <jianhao_xu@smail.nju.edu.cn>
---
 net/sched/sch_mq.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/sched/sch_mq.c b/net/sched/sch_mq.c
index 83d2e54bf303..9aca4ca82947 100644
--- a/net/sched/sch_mq.c
+++ b/net/sched/sch_mq.c
@@ -201,6 +201,8 @@ static int mq_graft(struct Qdisc *sch, unsigned long cl, struct Qdisc *new,
 static struct Qdisc *mq_leaf(struct Qdisc *sch, unsigned long cl)
 {
 	struct netdev_queue *dev_queue = mq_queue_get(sch, cl);
+	if (!dev_queue)
+		return NULL;
 
 	return dev_queue->qdisc_sleeping;
 }
@@ -218,6 +220,8 @@ static int mq_dump_class(struct Qdisc *sch, unsigned long cl,
 			 struct sk_buff *skb, struct tcmsg *tcm)
 {
 	struct netdev_queue *dev_queue = mq_queue_get(sch, cl);
+	if (!dev_queue)
+		return -1;
 
 	tcm->tcm_parent = TC_H_ROOT;
 	tcm->tcm_handle |= TC_H_MIN(cl);
@@ -229,6 +233,8 @@ static int mq_dump_class_stats(struct Qdisc *sch, unsigned long cl,
 			       struct gnet_dump *d)
 {
 	struct netdev_queue *dev_queue = mq_queue_get(sch, cl);
+	if (!dev_queue)
+		return -1;
 
 	sch = dev_queue->qdisc_sleeping;
 	if (gnet_stats_copy_basic(d, sch->cpu_bstats, &sch->bstats, true) < 0 ||
-- 
2.25.1

Re: [PATCH] net: sched: fix potential null pointer deref

[Daniel Borkmann]

Hi Jianhao,

On 6/10/22 4:14 AM, Jianhao Xu wrote:
> mq_queue_get() may return NULL, a check is needed to avoid using
> the NULL pointer.
> 
> Signed-off-by: Jianhao Xu <jianhao_xu@smail.nju.edu.cn>

Do you have a reproducer where this is triggered?

> ---
>   net/sched/sch_mq.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/net/sched/sch_mq.c b/net/sched/sch_mq.c
> index 83d2e54bf303..9aca4ca82947 100644
> --- a/net/sched/sch_mq.c
> +++ b/net/sched/sch_mq.c
> @@ -201,6 +201,8 @@ static int mq_graft(struct Qdisc *sch, unsigned long cl, struct Qdisc *new,
>   static struct Qdisc *mq_leaf(struct Qdisc *sch, unsigned long cl)
>   {
>   	struct netdev_queue *dev_queue = mq_queue_get(sch, cl);
> +	if (!dev_queue)
> +		return NULL;
>   
>   	return dev_queue->qdisc_sleeping;
>   }
> @@ -218,6 +220,8 @@ static int mq_dump_class(struct Qdisc *sch, unsigned long cl,
>   			 struct sk_buff *skb, struct tcmsg *tcm)
>   {
>   	struct netdev_queue *dev_queue = mq_queue_get(sch, cl);
> +	if (!dev_queue)
> +		return -1;
>   
>   	tcm->tcm_parent = TC_H_ROOT;
>   	tcm->tcm_handle |= TC_H_MIN(cl);
> @@ -229,6 +233,8 @@ static int mq_dump_class_stats(struct Qdisc *sch, unsigned long cl,
>   			       struct gnet_dump *d)
>   {
>   	struct netdev_queue *dev_queue = mq_queue_get(sch, cl);
> +	if (!dev_queue)
> +		return -1;
>   
>   	sch = dev_queue->qdisc_sleeping;
>   	if (gnet_stats_copy_basic(d, sch->cpu_bstats, &sch->bstats, true) < 0 ||
> 

Re: [PATCH] net: sched: fix potential null pointer deref

[Eric Dumazet]

On Fri, Jun 10, 2022 at 1:09 AM Jianhao Xu <jianhao_xu@smail.nju.edu.cn> wrote:
>
> Hi,
>
> TBH, We do not have a reproducer. This is found by our static analysis tool. We can not see any clue of the context here of mq_queue_get() to ensure it never returns NULL.
>


All netdev devices have their dev->_tx allocated in netif_alloc_netdev_queues()

There is absolutely no way MQ qdisc could be attached to a device that
has failed netif_alloc_netdev_queues() step.



> I would appreciate it if you could tell me why when you found out it was our false positive. It will be helpful for us to improve our tool.


Please do not send patches before you can provide a detailed
explanation of a real bug.

If you need help, post instead a [RFC] with a message explaining how
far you went into your analysis.

A patch should be sent only once you are absolutely sure that there is
a real bug to fix.

Thank you.


>
> Thanks.
> ------------------ Original ------------------
> From:  "Daniel Borkmann"<daniel@iogearbox.net>;
> Date:  Fri, Jun 10, 2022 09:14 AM
> To:  "Jianhao Xu"<jianhao_xu@smail.nju.edu.cn>; "jhs"<jhs@mojatatu.com>; "xiyou.wangcong"<xiyou.wangcong@gmail.com>; "jiri"<jiri@resnulli.us>; "davem"<davem@davemloft.net>; "edumazet"<edumazet@google.com>; "kuba"<kuba@kernel.org>; "pabeni"<pabeni@redhat.com>;
> Cc:  "netdev"<netdev@vger.kernel.org>; "linux-kernel"<linux-kernel@vger.kernel.org>;
> Subject:  Re: [PATCH] net: sched: fix potential null pointer deref
>
> Hi Jianhao,
>
> On 6/10/22 4:14 AM, Jianhao Xu wrote:
> > mq_queue_get() may return NULL, a check is needed to avoid using
> > the NULL pointer.
> >
> > Signed-off-by: Jianhao Xu <jianhao_xu@smail.nju.edu.cn>
>
> Do you have a reproducer where this is triggered?
>
> > ---
> >   net/sched/sch_mq.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/net/sched/sch_mq.c b/net/sched/sch_mq.c
> > index 83d2e54bf303..9aca4ca82947 100644
> > --- a/net/sched/sch_mq.c
> > +++ b/net/sched/sch_mq.c
> > @@ -201,6 +201,8 @@ static int mq_graft(struct Qdisc *sch, unsigned long cl, struct Qdisc *new,
> >   static struct Qdisc *mq_leaf(struct Qdisc *sch, unsigned long cl)
> >   {
> >  struct netdev_queue *dev_queue = mq_queue_get(sch, cl);
> > + if (!dev_queue)
> > +return NULL;
> >
> >  return dev_queue->qdisc_sleeping;
> >   }
> > @@ -218,6 +220,8 @@ static int mq_dump_class(struct Qdisc *sch, unsigned long cl,
> >   struct sk_buff *skb, struct tcmsg *tcm)
> >   {
> >  struct netdev_queue *dev_queue = mq_queue_get(sch, cl);
> > + if (!dev_queue)
> > +return -1;
> >
> >  tcm->tcm_parent = TC_H_ROOT;
> >  tcm->tcm_handle |= TC_H_MIN(cl);
> > @@ -229,6 +233,8 @@ static int mq_dump_class_stats(struct Qdisc *sch, unsigned long cl,
> >         struct gnet_dump *d)
> >   {
> >  struct netdev_queue *dev_queue = mq_queue_get(sch, cl);
> > + if (!dev_queue)
> > +return -1;
> >
> >  sch = dev_queue->qdisc_sleeping;
> >  if (gnet_stats_copy_basic(d, sch->cpu_bstats, &sch->bstats, true) < 0 ||
> >
>

Re: [PATCH] net: sched: fix potential null pointer deref

[Jianhao Xu]

Thanks for your advice and sorry for the noise.

> All netdev devices have their dev->_tx allocated in netif_alloc_netdev_queues()
> 
> There is absolutely no way MQ qdisc could be attached to a device that
> has failed netif_alloc_netdev_queues() step.

I believe this makes sense. But I am still a bit confused, especially after we 
cross-checked the similar context of mq, mqprio, taprio. To be specific, we 
cross-checked whether `mq_class_ops`, `mqprio_class_ops`, and 
`taprio_class_ops` check the return value of  their respective version of 
`_queue_get` before dereferencing it. 

--------------- ----- --------- --------- 
class_ops      whether check the ret value of _queue_get
                     mq |  mqprio | taprio   
--------------- ----- --------- ---------  
select_queue    -     -         -         
graft               no    yes      yes       
leaf                 no    yes      yes       
find                yes    -         yes       
walk                -      -          -         
dump              no    no       no        
dump_stats     no    no       no     
--------------- ----- --------- --------- 
  
As shown in this table, `mq_leaf()` does not check the return value of 
`mq__queue_get()` before using the pointer, while `mqprio_leaf()` and 
`taprio_leaf()` do have such a NULL check. 

FYI, here is the code of `mqprio_leaf()` and we can find the NULL check.
```
//net/sched/sch_mqprio.c
static struct Qdisc *mqprio_leaf(struct Qdisc *sch, unsigned long cl)
{
	struct netdev_queue *dev_queue = mqprio_queue_get(sch, cl);

	if (!dev_queue)
		return NULL;

	return dev_queue->qdisc_sleeping;
}
```

That is also the situation of `mq_graft()`, `mqprio_graft()` and `taprio_graft()`. 
I am not sure whether it is reasonable to expect the class_ops of mq, mqprio, 
and taprio to be consistent in this way. If so, does it mean that it is possible 
that`mq_leaf()`and `mq_graft` may miss a check here, or mqprio, taprio have 
redundant checks?

Thanks again for your time. I apologize if my question is stupid.