netdev - Re: general protection fault 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:   Mon, 9 Sep 2019 16:14:36 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     syzbot <syzbot+d5870a903591faaca4ae@...kaller.appspotmail.com>,
        Akinobu Mita <akinobu.mita@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        David Miller <davem@...emloft.net>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Linux List Kernel Mailing <linux-kernel@...r.kernel.org>,
        Michal Hocko <mhocko@...nel.org>,
        Netdev <netdev@...r.kernel.org>,
        syzkaller-bugs <syzkaller-bugs@...glegroups.com>
Subject: Re: general protection fault in qdisc_put

On Sun, Sep 8, 2019 at 10:19 AM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
> I see two solutions:
>
>  (a) move the
>
>         q->qdisc = &noop_qdisc;
>
>      up earlier in sfb_init(), so that qdisc is always initialized
> after sfb_init(), even on failure.
>
>  (b) just make qdisc_put(NULL) just silently work as a no-op.
>
>  (c) change all the semantics to not call ->destroy if ->init failed.
>
> Honestly, (a) seems very fragile - do all the other init routines do
> this? And (c) sounds like a big change, and very fragile too.
>
> So I'd suggest that qdisc_put() be made to just ignore a NULL pointer
> (and maybe an error pointer too?).

I think (a) is the best solution here.

(c) changes too much, we already rely on this behavior.

(b) is not bad either, just very slightly more risky.

Alternatively, we can add a quick NULL check inside
sfb_destroy().

I can send out a patch if you don't.

Thanks for looking at this!