iv/linux
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

ppp: remove the PPPIOCDETACH ioctl

Browse Source 
The PPPIOCDETACH ioctl effectively tries to "close" the given ppp file
before f_count has reached 0, which is fundamentally a bad idea.  It
does check 'f_count < 2', which excludes concurrent operations on the
file since they would only be possible with a shared fd table, in which
case each fdget() would take a file reference.  However, it fails to
account for the fact that even with 'f_count == 1' the file can still be
linked into epoll instances.  As reported by syzbot, this can trivially
be used to cause a use-after-free.

Yet, the only known user of PPPIOCDETACH is pppd versions older than
ppp-2.4.2, which was released almost 15 years ago (November 2003).
Also, PPPIOCDETACH apparently stopped working reliably at around the
same time, when the f_count check was added to the kernel, e.g. see
https://lkml.org/lkml/2002/12/31/83.  Also, the current 'f_count < 2'
check makes PPPIOCDETACH only work in single-threaded applications; it
always fails if called from a multithreaded application.

All pppd versions released in the last 15 years just close() the file
descriptor instead.

Therefore, instead of hacking around this bug by exporting epoll
internals to modules, and probably missing other related bugs, just
remove the PPPIOCDETACH ioctl and see if anyone actually notices.  Leave
a stub in place that prints a one-time warning and returns EINVAL.

Reported-by: syzbot+16363c99d4134717c05b@syzkaller.appspotmail.com
Fixes: 1da177e4c3 ("Linux-2.6.12-rc2")
Signed-off-by: Eric Biggers <ebiggers@google.com>
Acked-by: Paul Mackerras <paulus@ozlabs.org>
Reviewed-by: Guillaume Nault <g.nault@alphalink.fr>
Tested-by: Guillaume Nault <g.nault@alphalink.fr>
Signed-off-by: David S. Miller <davem@davemloft.net>
This commit is contained in:
Eric Biggers 2018-05-23 14:37:38 -07:00 committed by David S. Miller
parent 730c54d594
commit af8d3c7c00
3 changed files with 6 additions and 29 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
Documentation/networking/ppp_generic.txt
View File

@ -300,12 +300,6 @@ unattached instance are:
The ioctl calls available on an instance of /dev/ppp attached to a
channel are:
* PPPIOCDETACH detaches the instance from the channel. This ioctl is
deprecated since the same effect can be achieved by closing the
instance. In order to prevent possible races this ioctl will fail
with an EINVAL error if more than one file descriptor refers to this
instance (i.e. as a result of dup(), dup2() or fork()).
* PPPIOCCONNECT connects this channel to a PPP interface. The
argument should point to an int containing the interface unit
number. It will return an EINVAL error if the channel is already

27
drivers/net/ppp/ppp_generic.c
View File

@ -605,30 +605,13 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
if (cmd == PPPIOCDETACH) {
/*
* We have to be careful here... if the file descriptor
* has been dup'd, we could have another process in the
* middle of a poll using the same file *, so we had
* better not free the interface data structures -
* instead we fail the ioctl. Even in this case, we
* shut down the interface if we are the owner of it.
* Actually, we should get rid of PPPIOCDETACH, userland
* (i.e. pppd) could achieve the same effect by closing
* this fd and reopening /dev/ppp.
* PPPIOCDETACH is no longer supported as it was heavily broken,
* and is only known to have been used by pppd older than
* ppp-2.4.2 (released November 2003).
*/
pr_warn_once("%s (%d) used obsolete PPPIOCDETACH ioctl\n",
current->comm, current->pid);
err = -EINVAL;
if (pf->kind == INTERFACE) {
ppp = PF_TO_PPP(pf);
rtnl_lock();
if (file == ppp->owner)
unregister_netdevice(ppp->dev);
rtnl_unlock();
}
if (atomic_long_read(&file->f_count) < 2) {
ppp_release(NULL, file);
err = 0;
} else
pr_warn("PPPIOCDETACH file->f_count=%ld\n",
atomic_long_read(&file->f_count));
goto out;
}

2
include/uapi/linux/ppp-ioctl.h
View File

@ -106,7 +106,7 @@ struct pppol2tp_ioc_stats {
#define PPPIOCGIDLE _IOR('t', 63, struct ppp_idle) /* get idle time */
#define PPPIOCNEWUNIT _IOWR('t', 62, int) /* create new ppp unit */
#define PPPIOCATTACH _IOW('t', 61, int) /* attach to ppp unit */
#define PPPIOCDETACH _IOW('t', 60, int) /* detach from ppp unit/chan */
#define PPPIOCDETACH _IOW('t', 60, int) /* obsolete, do not use */
#define PPPIOCSMRRU _IOW('t', 59, int) /* set multilink MRU */
#define PPPIOCCONNECT _IOW('t', 58, int) /* connect channel to unit */
#define PPPIOCDISCONN _IO('t', 57) /* disconnect channel */