mq_queue_get() may return NULL, a check is needed to avoid using
the NULL pointer.
Signed-off-by: Jianhao Xu <[email protected]>
---
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
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 <[email protected]>
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 ||
>
On Fri, Jun 10, 2022 at 1:09 AM Jianhao Xu <[email protected]> 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"<[email protected]>;
> Date: Fri, Jun 10, 2022 09:14 AM
> To: "Jianhao Xu"<[email protected]>; "jhs"<[email protected]>; "xiyou.wangcong"<[email protected]>; "jiri"<[email protected]>; "davem"<[email protected]>; "edumazet"<[email protected]>; "kuba"<[email protected]>; "pabeni"<[email protected]>;
> Cc: "netdev"<[email protected]>; "linux-kernel"<[email protected]>;
> 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 <[email protected]>
>
> 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 ||
> >
>
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.