Re: ppp/pppoe, still panic 4.15.3 in ppp_push

[Guillaume Nault <g.nault@alphalink.fr>]

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.