netdev - Re: ppp/pppoe, still panic 4.15.3 in ppp

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: <20180227185438.GH1322@alphalink.fr>
Date:   Tue, 27 Feb 2018 19:54:38 +0100
From:   Guillaume Nault <g.nault@...halink.fr>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     Denys Fedoryshchenko <nuclearcat@...learcat.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: ppp/pppoe, still panic 4.15.3 in ppp_push

On Thu, Feb 22, 2018 at 07:30:38PM +0100, Guillaume Nault wrote:
> On Wed, Feb 21, 2018 at 12:04:30PM -0800, Cong Wang wrote:
> > For me it looks like pch->clist is not removed from the list ppp->channels
> > when destroyed via ppp_release(). But I don't want to pretend I understand
> > ppp logic.
> > 
> I've thought about that too, but couldn't find a scenario that could
> trigger the bug.
> 
> To get ->private_data pointing to a struct channel pointer, a file needs
> to ioctl(PPPIOCATTCHAN) first. For this call to succeed, the channel
> must have been registered with ppp_register_net_channel(). Both
> operations take a reference on the channel, which means that, before
> adding pch->clist to a ppp->channels list (with ppp_connect_channel()),
> the channel is already held by a /dev/ppp file and by the code that
> registered the channel in the first place.
> 
> Therefore, closing the /dev/ppp file can't be enough to make
> ppp_release() free the channel. We need to unregister the channel for
> the refcount to drop to 0. And ppp_unregister_channel(), removes
> pch->clist from ppp->channels before decrementing the reference
> counter.
> 
And this is where my reasoning failed... If pch->clist hasn't been
added to a ppp->channels list (that is, there was no
ppp_connect_channel() call for this channel), then
ppp_unregister_channel() only decrements the reference counter.

Therefore, we now have an unregistered channel which is only held by a
/dev/ppp file. But ioctl(PPPIOCCONNECT) still works on such a file, so
one can add pch->clist to a ppp->channels list. When the file
descriptor closes, we fall in Cong's scenario and the channel is freed,
leaving dangling pointers in ppp->channels.
Then, it's just a matter of calling ioctl(PPPIOCCONNECT) on this ppp
unit again to make list_add_tail() follow those invalid pointers and
crash.

Thank you Cong for putting me on the right track.