* [PATCH] ppp: remove the PPPIOCDETACH ioctl [not found] <20180523032958.GE658@sol.localdomain> @ 2018-05-23 3:59 ` Eric Biggers 2018-05-23 13:57 ` Guillaume Nault 2018-05-23 21:37 ` [PATCH v2] " Eric Biggers 0 siblings, 2 replies; 9+ messages in thread From: Eric Biggers @ 2018-05-23 3:59 UTC (permalink / raw) To: linux-ppp, Paul Mackerras Cc: netdev, linux-fsdevel, linux-kernel, Guillaume Nault, syzkaller-bugs, Eric Biggers From: Eric Biggers <ebiggers@google.com> 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. Reported-by: syzbot+16363c99d4134717c05b@syzkaller.appspotmail.com Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Cc: stable@vger.kernel.org Signed-off-by: Eric Biggers <ebiggers@google.com> --- Documentation/networking/ppp_generic.txt | 6 ----- drivers/net/ppp/ppp_generic.c | 29 ------------------------ fs/compat_ioctl.c | 1 - include/uapi/linux/ppp-ioctl.h | 1 - 4 files changed, 37 deletions(-) diff --git a/Documentation/networking/ppp_generic.txt b/Documentation/networking/ppp_generic.txt index 091d20273dcb..61daf4b39600 100644 --- a/Documentation/networking/ppp_generic.txt +++ b/Documentation/networking/ppp_generic.txt @@ -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 diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c index dc7c7ec43202..dce8812fe802 100644 --- a/drivers/net/ppp/ppp_generic.c +++ b/drivers/net/ppp/ppp_generic.c @@ -603,35 +603,6 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg) goto out; } - 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. - */ - 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; - } - if (pf->kind == CHANNEL) { struct channel *pch; struct ppp_channel *chan; diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c index ef80085ed564..8285b570d635 100644 --- a/fs/compat_ioctl.c +++ b/fs/compat_ioctl.c @@ -917,7 +917,6 @@ COMPATIBLE_IOCTL(PPPIOCSDEBUG) /* PPPIOCGIDLE is translated */ COMPATIBLE_IOCTL(PPPIOCNEWUNIT) COMPATIBLE_IOCTL(PPPIOCATTACH) -COMPATIBLE_IOCTL(PPPIOCDETACH) COMPATIBLE_IOCTL(PPPIOCSMRRU) COMPATIBLE_IOCTL(PPPIOCCONNECT) COMPATIBLE_IOCTL(PPPIOCDISCONN) diff --git a/include/uapi/linux/ppp-ioctl.h b/include/uapi/linux/ppp-ioctl.h index b19a9c249b15..d46caf217ea4 100644 --- a/include/uapi/linux/ppp-ioctl.h +++ b/include/uapi/linux/ppp-ioctl.h @@ -106,7 +106,6 @@ 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 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 */ -- 2.17.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] ppp: remove the PPPIOCDETACH ioctl 2018-05-23 3:59 ` [PATCH] ppp: remove the PPPIOCDETACH ioctl Eric Biggers @ 2018-05-23 13:57 ` Guillaume Nault 2018-05-23 15:56 ` David Miller 2018-05-23 21:37 ` [PATCH v2] " Eric Biggers 1 sibling, 1 reply; 9+ messages in thread From: Guillaume Nault @ 2018-05-23 13:57 UTC (permalink / raw) To: Eric Biggers Cc: linux-ppp, Paul Mackerras, netdev, linux-fsdevel, linux-kernel, syzkaller-bugs, Eric Biggers On Tue, May 22, 2018 at 08:59:52PM -0700, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > 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. > > Reported-by: syzbot+16363c99d4134717c05b@syzkaller.appspotmail.com > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Cc: stable@vger.kernel.org > Signed-off-by: Eric Biggers <ebiggers@google.com> > --- > Documentation/networking/ppp_generic.txt | 6 ----- > drivers/net/ppp/ppp_generic.c | 29 ------------------------ > fs/compat_ioctl.c | 1 - > include/uapi/linux/ppp-ioctl.h | 1 - > 4 files changed, 37 deletions(-) > > diff --git a/Documentation/networking/ppp_generic.txt b/Documentation/networking/ppp_generic.txt > index 091d20273dcb..61daf4b39600 100644 > --- a/Documentation/networking/ppp_generic.txt > +++ b/Documentation/networking/ppp_generic.txt > @@ -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 > diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c > index dc7c7ec43202..dce8812fe802 100644 > --- a/drivers/net/ppp/ppp_generic.c > +++ b/drivers/net/ppp/ppp_generic.c > @@ -603,35 +603,6 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > goto out; > } > > - 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. > - */ > - 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; > - } > - I'd rather add + if (cmd == PPPIOCDETACH) { + err = -EINVAL; + goto out; + } Making PPPIOCDETACH unknown to ppp_generic means that the ioctl would be handled by the underlying channel when pf->kind == CHANNEL (see the chan->ops->ioctl() call further down). That shouldn't be a problem per se, but even though PPPIOCDETACH is unsupported, I feel that it should remain a ppp_generic thing. I don't really want its value to be reused for other purposes in the future or have different behaviour depending on the underlying channel. Also PPPIOCDETACH can already fail with -EINVAL. Therefore, if ever there really were programs out there using this call, they'd already have to handle this case. Unconditionally returning -EINVAL would further minimise possibilities for breakage. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ppp: remove the PPPIOCDETACH ioctl 2018-05-23 13:57 ` Guillaume Nault @ 2018-05-23 15:56 ` David Miller 2018-05-23 21:17 ` Eric Biggers 0 siblings, 1 reply; 9+ messages in thread From: David Miller @ 2018-05-23 15:56 UTC (permalink / raw) To: g.nault Cc: ebiggers3, linux-ppp, paulus, netdev, linux-fsdevel, linux-kernel, syzkaller-bugs, ebiggers From: Guillaume Nault <g.nault@alphalink.fr> Date: Wed, 23 May 2018 15:57:08 +0200 > I'd rather add > + if (cmd == PPPIOCDETACH) { > + err = -EINVAL; > + goto out; > + } > > Making PPPIOCDETACH unknown to ppp_generic means that the ioctl would > be handled by the underlying channel when pf->kind == CHANNEL (see the > chan->ops->ioctl() call further down). That shouldn't be a problem per > se, but even though PPPIOCDETACH is unsupported, I feel that it should > remain a ppp_generic thing. I don't really want its value to be reused > for other purposes in the future or have different behaviour depending > on the underlying channel. > > Also PPPIOCDETACH can already fail with -EINVAL. Therefore, if ever > there really were programs out there using this call, they'd already > have to handle this case. Unconditionally returning -EINVAL would > further minimise possibilities for breakage. I agree. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ppp: remove the PPPIOCDETACH ioctl 2018-05-23 15:56 ` David Miller @ 2018-05-23 21:17 ` Eric Biggers 0 siblings, 0 replies; 9+ messages in thread From: Eric Biggers @ 2018-05-23 21:17 UTC (permalink / raw) To: David Miller Cc: g.nault, linux-ppp, paulus, netdev, linux-fsdevel, linux-kernel, syzkaller-bugs, ebiggers On Wed, May 23, 2018 at 11:56:36AM -0400, David Miller wrote: > From: Guillaume Nault <g.nault@alphalink.fr> > Date: Wed, 23 May 2018 15:57:08 +0200 > > > I'd rather add > > + if (cmd == PPPIOCDETACH) { > > + err = -EINVAL; > > + goto out; > > + } > > > > Making PPPIOCDETACH unknown to ppp_generic means that the ioctl would > > be handled by the underlying channel when pf->kind == CHANNEL (see the > > chan->ops->ioctl() call further down). That shouldn't be a problem per > > se, but even though PPPIOCDETACH is unsupported, I feel that it should > > remain a ppp_generic thing. I don't really want its value to be reused > > for other purposes in the future or have different behaviour depending > > on the underlying channel. > > > > Also PPPIOCDETACH can already fail with -EINVAL. Therefore, if ever > > there really were programs out there using this call, they'd already > > have to handle this case. Unconditionally returning -EINVAL would > > further minimise possibilities for breakage. > > I agree. Okay, I'll do that and leave the ioctl number reserved. I will add a pr_warn_once() too. Thanks, - Eric ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] ppp: remove the PPPIOCDETACH ioctl 2018-05-23 3:59 ` [PATCH] ppp: remove the PPPIOCDETACH ioctl Eric Biggers 2018-05-23 13:57 ` Guillaume Nault @ 2018-05-23 21:37 ` Eric Biggers 2018-05-23 23:04 ` Paul Mackerras ` (3 more replies) 1 sibling, 4 replies; 9+ messages in thread From: Eric Biggers @ 2018-05-23 21:37 UTC (permalink / raw) To: linux-ppp, Paul Mackerras Cc: netdev, linux-fsdevel, linux-kernel, Guillaume Nault, syzkaller-bugs, Eric Biggers From: Eric Biggers <ebiggers@google.com> 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: 1da177e4c3f4 ("Linux-2.6.12-rc2") Cc: stable@vger.kernel.org Signed-off-by: Eric Biggers <ebiggers@google.com> --- v2: leave a stub in place, rather than removing the ioctl completely. Documentation/networking/ppp_generic.txt | 6 ------ drivers/net/ppp/ppp_generic.c | 27 +++++------------------- include/uapi/linux/ppp-ioctl.h | 2 +- 3 files changed, 6 insertions(+), 29 deletions(-) diff --git a/Documentation/networking/ppp_generic.txt b/Documentation/networking/ppp_generic.txt index 091d20273dcb..61daf4b39600 100644 --- a/Documentation/networking/ppp_generic.txt +++ b/Documentation/networking/ppp_generic.txt @@ -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 diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c index dc7c7ec43202..02ad03a2fab7 100644 --- a/drivers/net/ppp/ppp_generic.c +++ b/drivers/net/ppp/ppp_generic.c @@ -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; } diff --git a/include/uapi/linux/ppp-ioctl.h b/include/uapi/linux/ppp-ioctl.h index b19a9c249b15..784c2e3e572e 100644 --- a/include/uapi/linux/ppp-ioctl.h +++ b/include/uapi/linux/ppp-ioctl.h @@ -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 */ -- 2.17.0.441.gb46fe60e1d-goog ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] ppp: remove the PPPIOCDETACH ioctl 2018-05-23 21:37 ` [PATCH v2] " Eric Biggers @ 2018-05-23 23:04 ` Paul Mackerras 2018-05-24 14:04 ` Guillaume Nault ` (2 subsequent siblings) 3 siblings, 0 replies; 9+ messages in thread From: Paul Mackerras @ 2018-05-23 23:04 UTC (permalink / raw) To: Eric Biggers Cc: linux-ppp, netdev, linux-fsdevel, linux-kernel, Guillaume Nault, syzkaller-bugs, Eric Biggers On Wed, May 23, 2018 at 02:37:38PM -0700, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > 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: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Cc: stable@vger.kernel.org > Signed-off-by: Eric Biggers <ebiggers@google.com> Acked-by: Paul Mackerras <paulus@ozlabs.org> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] ppp: remove the PPPIOCDETACH ioctl 2018-05-23 21:37 ` [PATCH v2] " Eric Biggers 2018-05-23 23:04 ` Paul Mackerras @ 2018-05-24 14:04 ` Guillaume Nault 2018-05-25 2:55 ` David Miller 2018-06-06 9:01 ` Walter Harms 3 siblings, 0 replies; 9+ messages in thread From: Guillaume Nault @ 2018-05-24 14:04 UTC (permalink / raw) To: Eric Biggers Cc: linux-ppp, Paul Mackerras, netdev, linux-fsdevel, linux-kernel, syzkaller-bugs, Eric Biggers On Wed, May 23, 2018 at 02:37:38PM -0700, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > 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: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Cc: stable@vger.kernel.org > Signed-off-by: Eric Biggers <ebiggers@google.com> > --- > > v2: leave a stub in place, rather than removing the ioctl completely. > Thanks a lot for your help on this matter. BTW, netdev has its own rules wrt. stable backports. You didn't need to CC: stable@. David handles -stable submissions himself. Using a 'PATCH net' subject prefix would have made it clear that this patch was fixing some released code and should be considered for -stable backport. Reviewed-by: Guillaume Nault <g.nault@alphalink.fr> Tested-by: Guillaume Nault <g.nault@alphalink.fr> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] ppp: remove the PPPIOCDETACH ioctl 2018-05-23 21:37 ` [PATCH v2] " Eric Biggers 2018-05-23 23:04 ` Paul Mackerras 2018-05-24 14:04 ` Guillaume Nault @ 2018-05-25 2:55 ` David Miller 2018-06-06 9:01 ` Walter Harms 3 siblings, 0 replies; 9+ messages in thread From: David Miller @ 2018-05-25 2:55 UTC (permalink / raw) To: ebiggers3 Cc: linux-ppp, paulus, netdev, linux-fsdevel, linux-kernel, g.nault, syzkaller-bugs, ebiggers From: Eric Biggers <ebiggers3@gmail.com> Date: Wed, 23 May 2018 14:37:38 -0700 > From: Eric Biggers <ebiggers@google.com> > > 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: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Signed-off-by: Eric Biggers <ebiggers@google.com> Applied and queued up for -stable. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] ppp: remove the PPPIOCDETACH ioctl 2018-05-23 21:37 ` [PATCH v2] " Eric Biggers ` (2 preceding siblings ...) 2018-05-25 2:55 ` David Miller @ 2018-06-06 9:01 ` Walter Harms 3 siblings, 0 replies; 9+ messages in thread From: Walter Harms @ 2018-06-06 9:01 UTC (permalink / raw) To: linux-ppp, Eric Biggers, Paul Mackerras Cc: syzkaller-bugs, Guillaume Nault, Eric Biggers, netdev, linux-kernel, linux-fsdevel > Eric Biggers <ebiggers3@gmail.com> hat am 23. Mai 2018 um 23:37 geschrieben: > > > From: Eric Biggers <ebiggers@google.com> > > 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: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Cc: stable@vger.kernel.org > Signed-off-by: Eric Biggers <ebiggers@google.com> > --- > > v2: leave a stub in place, rather than removing the ioctl completely. > > Documentation/networking/ppp_generic.txt | 6 ------ > drivers/net/ppp/ppp_generic.c | 27 +++++------------------- > include/uapi/linux/ppp-ioctl.h | 2 +- > 3 files changed, 6 insertions(+), 29 deletions(-) > > diff --git a/Documentation/networking/ppp_generic.txt > b/Documentation/networking/ppp_generic.txt > index 091d20273dcb..61daf4b39600 100644 > --- a/Documentation/networking/ppp_generic.txt > +++ b/Documentation/networking/ppp_generic.txt > @@ -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 > diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c > index dc7c7ec43202..02ad03a2fab7 100644 > --- a/drivers/net/ppp/ppp_generic.c > +++ b/drivers/net/ppp/ppp_generic.c > @@ -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; > } > > diff --git a/include/uapi/linux/ppp-ioctl.h b/include/uapi/linux/ppp-ioctl.h > index b19a9c249b15..784c2e3e572e 100644 > --- a/include/uapi/linux/ppp-ioctl.h > +++ b/include/uapi/linux/ppp-ioctl.h > @@ -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 */ It's a bit late but ... i would suggest a stronger wording here. like /* removed 2003 */ for me "obsolete" sounds like "maybe it will be removed in a distant future" just my 2 cents, re, wh > #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 */ > -- > 2.17.0.441.gb46fe60e1d-goog > > -- > To unsubscribe from this list: send the line "unsubscribe linux-ppp" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-06-06 9:10 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20180523032958.GE658@sol.localdomain> 2018-05-23 3:59 ` [PATCH] ppp: remove the PPPIOCDETACH ioctl Eric Biggers 2018-05-23 13:57 ` Guillaume Nault 2018-05-23 15:56 ` David Miller 2018-05-23 21:17 ` Eric Biggers 2018-05-23 21:37 ` [PATCH v2] " Eric Biggers 2018-05-23 23:04 ` Paul Mackerras 2018-05-24 14:04 ` Guillaume Nault 2018-05-25 2:55 ` David Miller 2018-06-06 9:01 ` Walter Harms
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).