linux-kernel - Re: net/core: BUG in unregister_netdevice

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

Message-ID: <CA+55aFwg=OtMWmU153uM27Fwk5eVv5ZBGBA9phQqVqn3nNAy6Q@mail.gmail.com>
Date:   Fri, 21 Apr 2017 10:25:33 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Andrey Konovalov <andreyknvl@...gle.com>,
        Eric Dumazet <edumazet@...gle.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Alexey Kuznetsov <kuznet@....inr.ac.ru>,
        James Morris <jmorris@...ei.org>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        Patrick McHardy <kaber@...sh.net>,
        netdev <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Alexander Duyck <alexander.h.duyck@...el.com>,
        David Ahern <dsa@...ulusnetworks.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        tcharding <me@...in.cc>, Jiri Pirko <jiri@...lanox.com>,
        stephen hemminger <stephen@...workplumber.org>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Kostya Serebryany <kcc@...gle.com>,
        syzkaller <syzkaller@...glegroups.com>
Subject: Re: net/core: BUG in unregister_netdevice_many

On Fri, Apr 21, 2017 at 5:48 AM, Andrey Konovalov <andreyknvl@...gle.com> wrote:
>
> I've got the following error report while fuzzing the kernel with syzkaller.
>
> ------------[ cut here ]------------
> kernel BUG at net/core/dev.c:6813!

Another useless BUG_ON() that

 (a) kills the machine
 (b) doesn't tell the actual useful information.

Grr.

But that BUG_ON() is ancient, and actually goes back to pre-git days.
So you do seem to have triggered some code-path that doesn't ever
happen in any normal use.

This code looks odd:

  void unregister_netdevice_many(struct list_head *head)
  {
        struct net_device *dev;

        if (!list_empty(head)) {
                rollback_registered_many(head);
                list_for_each_entry(dev, head, unreg_list)
                        net_set_todo(dev);
                list_del(head);
        }
  }

In particular the pattern of "look if list is empty, do something to
the list, and then delete the list" is a rather nasty pattern.

Why? That final "delete the list" looks like garbage to me.

Either that list is never used again - in which case the list_del() is
pointless - or it _is_ used again, in which case the list_del is
wrong.

Why is it wrong? Because "list_del()" only removes the list from the
head, but leaves the head itself untouched. So it will still end up
pointing to the list entries that we've just walked.

Now, almost every single case I looked at, the head is always a
temporary list that was created just for this
"unregister_netdevice_many()", so the code works fine, and it's a case
of "that list_del() is just pointless".

But it's a very dangerous pattern. Either the list head should be left
alone, or it should be cleaned up with list_del_init()

Anyway, because each user seems fine, and really just uses it as a
temporary list, this is not the cause of the bug.

I'm assuming that the real cause is simply that "dev->reg_state" ends
up being NETREG_UNREGISTERING or something. Maybe the BUG_ON() could
be just removed, and replaced by the previous warning about
NETREG_UNINITIALIZED.

Something like the attached (TOTALLY UNTESTED) patch.

We really shouldn't have BUG_ON()'s in the kernel, particularly for
cases that we already have error handling for and are ignoring. But
whatever.

Eric Dumazet seems to be the main person to look at this.

                  Linus

View attachment "patch.diff" of type "text/plain" (905 bytes)