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.
