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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89iKHfi=kQY1FC=07COJfVX4ROTnGkM_1uKvOfPfdhqt4Ow@mail.gmail.com>
Date:   Fri, 10 Jun 2022 01:40:06 -0700
From:   Eric Dumazet <edumazet@...gle.com>
To:     Jianhao Xu <jianhao_xu@...il.nju.edu.cn>
Cc:     Daniel Borkmann <daniel@...earbox.net>, jhs <jhs@...atatu.com>,
        "xiyou.wangcong" <xiyou.wangcong@...il.com>,
        jiri <jiri@...nulli.us>, davem <davem@...emloft.net>,
        kuba <kuba@...nel.org>, pabeni <pabeni@...hat.com>,
        netdev <netdev@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] net: sched: fix potential null pointer deref

On Fri, Jun 10, 2022 at 1:09 AM Jianhao Xu <jianhao_xu@...il.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@...earbox.net>;
> Date:  Fri, Jun 10, 2022 09:14 AM
> To:  "Jianhao Xu"<jianhao_xu@...il.nju.edu.cn>; "jhs"<jhs@...atatu.com>; "xiyou.wangcong"<xiyou.wangcong@...il.com>; "jiri"<jiri@...nulli.us>; "davem"<davem@...emloft.net>; "edumazet"<edumazet@...gle.com>; "kuba"<kuba@...nel.org>; "pabeni"<pabeni@...hat.com>;
> Cc:  "netdev"<netdev@...r.kernel.org>; "linux-kernel"<linux-kernel@...r.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@...il.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 ||
> >
>