netdev - Re: [PATCH net] net/sched: fix NULL pointer dereference in qdisc

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-prev] [day] [month] [year] [list]

Date:   Fri, 27 Oct 2017 16:19:55 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Davide Caratti <dcaratti@...hat.com>
Cc:     Jiri Pirko <jiri@...nulli.us>,
        "David S. Miller" <davem@...emloft.net>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Kosina <jkosina@...e.cz>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH net] net/sched: fix NULL pointer dereference in qdisc_alloc()

On Thu, Oct 26, 2017 at 11:26 AM, Davide Caratti <dcaratti@...hat.com> wrote:
> the following script generates a NULL pointer dereference error:
>
> ip l a name eth0 type dummy
> tc q a dev eth0 parent :1 handle 1: htb
>
> upon creation of classful qdiscs, qdisc_alloc() dereferences dev_queue->dev
> assuming it is not NULL. This is not true when eth0 has been added, but not
> yet set administratively up; a bisect test proved that Linux started making
> NULL exception with the above two commands after commit 59cc1f61f09c ("net:
> sched:convert qdisc linked list to hashtable"). Let qdisc_alloc() return -1
> (-ENOENT) when a NULL value of dev_queue->dev is seen, so that non-crashing
> behaviour observable in Linux 4.8 is restored.

This analysis is wrong, you just hit noop_qdisc in this case.

Parent :1 doesn't exist in this case, so the second command
should be rejected since parent can't be found.

The the following patch works and better than your patch, but
still I think it should fail even earlier in this path, so there is probably
a even better fix.

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index c6deb74e3d2f..6a3033c528a8 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1413,7 +1413,7 @@ static int tc_modify_qdisc(struct sk_buff *skb,
struct nlmsghdr *n,

                if (p && p->ops->cl_ops && p->ops->cl_ops->select_queue)
                        dev_queue = p->ops->cl_ops->select_queue(p, tcm);
-               else if (p)
+               else if (p && p != &noop_qdisc)
                        dev_queue = p->dev_queue;
                else
                        dev_queue = netdev_get_tx_queue(dev, 0);