Support for handling the PPPOEIOCSFWD ioctl in compat mode was added in linux-2.5.69 along with hundreds of other commands, but was always broken sincen only the structure is compatible, but the command number is not, due to the size being sizeof(size_t), or at first sizeof(sizeof((struct sockaddr_pppox)), which is different on 64-bit architectures.
Fix it by defining a separate command code that matches the 32-bit version, and marking that one as compatible.
This should apply to all stable kernels.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/net/ppp/pppoe.c | 4 ++++ fs/compat_ioctl.c | 2 +- include/linux/if_pppox.h | 2 ++ 3 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c index ce61231e96ea..d1c3f9292c54 100644 --- a/drivers/net/ppp/pppoe.c +++ b/drivers/net/ppp/pppoe.c @@ -57,6 +57,7 @@ * */
+#include <linux/compat.h> #include <linux/string.h> #include <linux/module.h> #include <linux/kernel.h> @@ -780,6 +781,9 @@ static int pppoe_ioctl(struct socket *sock, unsigned int cmd, err = 0; break;
+#ifdef CONFIG_COMPAT + case PPPOEIOCSFWD32: +#endif case PPPOEIOCSFWD: { struct pppox_sock *relay_po; diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c index a9b00942e87d..a8bb193fdfd5 100644 --- a/fs/compat_ioctl.c +++ b/fs/compat_ioctl.c @@ -895,7 +895,7 @@ COMPATIBLE_IOCTL(PPPIOCATTCHAN) COMPATIBLE_IOCTL(PPPIOCGCHAN) COMPATIBLE_IOCTL(PPPIOCGL2TPSTATS) /* PPPOX */ -COMPATIBLE_IOCTL(PPPOEIOCSFWD) +COMPATIBLE_IOCTL(PPPOEIOCSFWD32) COMPATIBLE_IOCTL(PPPOEIOCDFWD) /* Big A */ /* sparc only */ diff --git a/include/linux/if_pppox.h b/include/linux/if_pppox.h index ba7a9b0c7c57..d221f1465f41 100644 --- a/include/linux/if_pppox.h +++ b/include/linux/if_pppox.h @@ -85,6 +85,8 @@ extern void unregister_pppox_proto(int proto_num); extern void pppox_unbind_sock(struct sock *sk);/* delete ppp-channel binding */ extern int pppox_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg);
+#define PPPOEIOCSFWD32 _IOW(0xB1 ,0, compat_size_t) + /* PPPoX socket states */ enum { PPPOX_NONE = 0, /* initial state */
There are multiple implementations of the PPP ioctl interface in the kernel:
- drivers/net/ppp/ppp_generic.c implements a generic interface for the /dev/ppp chardev used by some subdrivers. - drivers/net/ppp/pppox.c implements a socket based interface for pppoe, pptp and l2tp. - drivers/isdn/i4l/isdn_ppp.c is for the i4l ISDN stack
All ioctl commands in the respective functions are compatible between 32-bit and 64-bit kernels, so we can simply mark the handlers themselves as compatible and stop listing the commands individually.
Four commands (PPPIOCSCOMPRESS, PPPIOCSPASS, PPPIOCSACTIVE, and PPPIOCGIDLE) are incompatible on the user level but have a translation handler to make them compatible. I'm simplifying that compat handling in separate patches.
The PPPIOCGUNIT and PPPIOCGCHAN ioctl commands are special, they are implemented on various other file descriptors, so we have to keep them listed as COMPATIBLE_IOCTL().
For the isdn_ppp code, additional ioctl commands are needed that have never had working compat handling, so I'm leaving that part out: If they are remaining users of i4l's ippp, they are not using compat mode today, and are highly unlikely in the future before the last ISDN network gets shut down. I4L has been deprecated since 2002.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/net/ppp/ppp_generic.c | 1 + drivers/net/ppp/pppoe.c | 3 +++ drivers/net/ppp/pptp.c | 3 +++ fs/compat_ioctl.c | 31 ------------------------------- net/l2tp/l2tp_ppp.c | 3 +++ 5 files changed, 10 insertions(+), 31 deletions(-)
diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c index 02ad03a2fab7..41a6e9851a4a 100644 --- a/drivers/net/ppp/ppp_generic.c +++ b/drivers/net/ppp/ppp_generic.c @@ -899,6 +899,7 @@ static const struct file_operations ppp_device_fops = { .write = ppp_write, .poll = ppp_poll, .unlocked_ioctl = ppp_ioctl, + .compat_ioctl = ppp_ioctl, .open = ppp_open, .release = ppp_release, .llseek = noop_llseek, diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c index d1c3f9292c54..25174fa7a470 100644 --- a/drivers/net/ppp/pppoe.c +++ b/drivers/net/ppp/pppoe.c @@ -1120,6 +1120,9 @@ static const struct proto_ops pppoe_ops = { .recvmsg = pppoe_recvmsg, .mmap = sock_no_mmap, .ioctl = pppox_ioctl, +#ifdef CONFIG_COMPAT + .compat_ioctl = pppox_ioctl, +#endif };
static const struct pppox_proto pppoe_proto = { diff --git a/drivers/net/ppp/pptp.c b/drivers/net/ppp/pptp.c index 67ffe74747a1..3b5ab3f6745c 100644 --- a/drivers/net/ppp/pptp.c +++ b/drivers/net/ppp/pptp.c @@ -632,6 +632,9 @@ static const struct proto_ops pptp_ops = { .recvmsg = sock_no_recvmsg, .mmap = sock_no_mmap, .ioctl = pppox_ioctl, +#ifdef CONFIG_COMPAT + .compat_ioctl = pppox_ioctl, +#endif };
static const struct pppox_proto pppox_pptp_proto = { diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c index a8bb193fdfd5..142ca673b9cc 100644 --- a/fs/compat_ioctl.c +++ b/fs/compat_ioctl.c @@ -864,39 +864,8 @@ COMPATIBLE_IOCTL(SG_SET_KEEP_ORPHAN) COMPATIBLE_IOCTL(SG_GET_KEEP_ORPHAN) #endif /* PPP stuff */ -COMPATIBLE_IOCTL(PPPIOCGFLAGS) -COMPATIBLE_IOCTL(PPPIOCSFLAGS) -COMPATIBLE_IOCTL(PPPIOCGASYNCMAP) -COMPATIBLE_IOCTL(PPPIOCSASYNCMAP) COMPATIBLE_IOCTL(PPPIOCGUNIT) -COMPATIBLE_IOCTL(PPPIOCGRASYNCMAP) -COMPATIBLE_IOCTL(PPPIOCSRASYNCMAP) -COMPATIBLE_IOCTL(PPPIOCGMRU) -COMPATIBLE_IOCTL(PPPIOCSMRU) -COMPATIBLE_IOCTL(PPPIOCSMAXCID) -COMPATIBLE_IOCTL(PPPIOCGXASYNCMAP) -COMPATIBLE_IOCTL(PPPIOCSXASYNCMAP) -COMPATIBLE_IOCTL(PPPIOCXFERUNIT) -/* PPPIOCSCOMPRESS is translated */ -COMPATIBLE_IOCTL(PPPIOCGNPMODE) -COMPATIBLE_IOCTL(PPPIOCSNPMODE) -COMPATIBLE_IOCTL(PPPIOCGDEBUG) -COMPATIBLE_IOCTL(PPPIOCSDEBUG) -/* PPPIOCSPASS is translated */ -/* PPPIOCSACTIVE is translated */ -/* PPPIOCGIDLE is translated */ -COMPATIBLE_IOCTL(PPPIOCNEWUNIT) -COMPATIBLE_IOCTL(PPPIOCATTACH) -COMPATIBLE_IOCTL(PPPIOCDETACH) -COMPATIBLE_IOCTL(PPPIOCSMRRU) -COMPATIBLE_IOCTL(PPPIOCCONNECT) -COMPATIBLE_IOCTL(PPPIOCDISCONN) -COMPATIBLE_IOCTL(PPPIOCATTCHAN) COMPATIBLE_IOCTL(PPPIOCGCHAN) -COMPATIBLE_IOCTL(PPPIOCGL2TPSTATS) -/* PPPOX */ -COMPATIBLE_IOCTL(PPPOEIOCSFWD32) -COMPATIBLE_IOCTL(PPPOEIOCDFWD) /* Big A */ /* sparc only */ /* Big Q for sound/OSS */ diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c index 04d9946dcdba..8ef66513fbe0 100644 --- a/net/l2tp/l2tp_ppp.c +++ b/net/l2tp/l2tp_ppp.c @@ -1686,6 +1686,9 @@ static const struct proto_ops pppol2tp_ops = { .recvmsg = pppol2tp_recvmsg, .mmap = sock_no_mmap, .ioctl = pppox_ioctl, +#ifdef CONFIG_COMPAT + .compat_ioctl = pppox_ioctl, +#endif };
static const struct pppox_proto pppol2tp_proto = {
PPPIOCSCOMPRESS is only implemented in ppp_generic, so it's best to move the compat handling there. My first approach was to keep it in a new ppp_compat_ioctl() function, but it turned out to be much simpler to do it in the regular ioctl handler, by allowing both structure layouts to be handled directly there.
Aside from moving the code to the right place, this also avoids a round-trip through compat_alloc_user_space() allocated memory.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/net/ppp/ppp_generic.c | 40 ++++++++++++++++++++++++++++++----- fs/compat_ioctl.c | 32 ---------------------------- 2 files changed, 35 insertions(+), 37 deletions(-)
diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c index 41a6e9851a4a..8dfe8d47df95 100644 --- a/drivers/net/ppp/ppp_generic.c +++ b/drivers/net/ppp/ppp_generic.c @@ -274,7 +274,7 @@ static void ppp_mp_insert(struct ppp *ppp, struct sk_buff *skb); static struct sk_buff *ppp_mp_reconstruct(struct ppp *ppp); static int ppp_mp_explode(struct ppp *ppp, struct sk_buff *skb); #endif /* CONFIG_PPP_MULTILINK */ -static int ppp_set_compress(struct ppp *ppp, unsigned long arg); +static int ppp_set_compress(struct ppp *ppp, unsigned long arg, bool compat); static void ppp_ccp_peek(struct ppp *ppp, struct sk_buff *skb, int inbound); static void ppp_ccp_closed(struct ppp *ppp); static struct compressor *find_compressor(int type); @@ -557,6 +557,15 @@ static __poll_t ppp_poll(struct file *file, poll_table *wait) return mask; }
+#ifdef CONFIG_COMPAT +struct ppp_option_data32 { + compat_caddr_t ptr; + u32 length; + compat_int_t transmit; +}; +#define PPPIOCSCOMPRESS32 _IOW('t', 77, struct ppp_option_data32) +#endif + #ifdef CONFIG_PPP_FILTER static int get_filter(void __user *arg, struct sock_filter **p) { @@ -683,8 +692,14 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg) break;
case PPPIOCSCOMPRESS: - err = ppp_set_compress(ppp, arg); + err = ppp_set_compress(ppp, arg, false); + break; + +#ifdef CONFIG_COMPAT + case PPPIOCSCOMPRESS32: + err = ppp_set_compress(ppp, arg, true); break; +#endif
case PPPIOCGUNIT: if (put_user(ppp->file.index, p)) @@ -2691,7 +2706,7 @@ ppp_output_wakeup(struct ppp_channel *chan)
/* Process the PPPIOCSCOMPRESS ioctl. */ static int -ppp_set_compress(struct ppp *ppp, unsigned long arg) +ppp_set_compress(struct ppp *ppp, unsigned long arg, bool compat) { int err; struct compressor *cp, *ocomp; @@ -2700,8 +2715,23 @@ ppp_set_compress(struct ppp *ppp, unsigned long arg) unsigned char ccp_option[CCP_MAX_OPTION_LENGTH];
err = -EFAULT; - if (copy_from_user(&data, (void __user *) arg, sizeof(data))) - goto out; +#ifdef CONFIG_COMPAT + if (compat) { + struct ppp_option_data32 data32; + + if (copy_from_user(&data32, (void __user *) arg, + sizeof(data32))) + goto out; + + data.ptr = compat_ptr(data32.ptr); + data.length = data32.length; + data.transmit = data32.transmit; + } else +#endif + { + if (copy_from_user(&data, (void __user *) arg, sizeof(data))) + goto out; + } if (data.length > CCP_MAX_OPTION_LENGTH) goto out; if (copy_from_user(ccp_option, (void __user *) data.ptr, data.length)) diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c index 142ca673b9cc..f518dc174dc7 100644 --- a/fs/compat_ioctl.c +++ b/fs/compat_ioctl.c @@ -413,13 +413,6 @@ static int ppp_sock_fprog_ioctl_trans(struct file *file, return do_ioctl(file, cmd, (unsigned long) u_fprog64); }
-struct ppp_option_data32 { - compat_caddr_t ptr; - u32 length; - compat_int_t transmit; -}; -#define PPPIOCSCOMPRESS32 _IOW('t', 77, struct ppp_option_data32) - struct ppp_idle32 { compat_time_t xmit_idle; compat_time_t recv_idle; @@ -447,29 +440,6 @@ static int ppp_gidle(struct file *file, unsigned int cmd, return err; }
-static int ppp_scompress(struct file *file, unsigned int cmd, - struct ppp_option_data32 __user *odata32) -{ - struct ppp_option_data __user *odata; - __u32 data; - void __user *datap; - - odata = compat_alloc_user_space(sizeof(*odata)); - - if (get_user(data, &odata32->ptr)) - return -EFAULT; - - datap = compat_ptr(data); - if (put_user(datap, &odata->ptr)) - return -EFAULT; - - if (copy_in_user(&odata->length, &odata32->length, - sizeof(__u32) + sizeof(int))) - return -EFAULT; - - return do_ioctl(file, PPPIOCSCOMPRESS, (unsigned long) odata); -} - #ifdef CONFIG_BLOCK struct mtget32 { compat_long_t mt_type; @@ -1248,8 +1218,6 @@ static long do_ioctl_trans(unsigned int cmd, switch (cmd) { case PPPIOCGIDLE32: return ppp_gidle(file, cmd, argp); - case PPPIOCSCOMPRESS32: - return ppp_scompress(file, cmd, argp); case PPPIOCSPASS32: case PPPIOCSACTIVE32: return ppp_sock_fprog_ioctl_trans(file, cmd, argp);
On Wed, Aug 29, 2018 at 04:03:28PM +0200, Arnd Bergmann wrote:
PPPIOCSCOMPRESS is only implemented in ppp_generic, so it's best to move the compat handling there. My first approach was to keep it in a new ppp_compat_ioctl() function, but it turned out to be much simpler to do it in the regular ioctl handler, by allowing both structure layouts to be handled directly there.
Acked-by: Guillaume Nault g.nault@alphalink.fr
PPPIOCSPASS and PPPIOCSACTIVE are implemented in ppp_generic and isdn_ppp, but the latter one doesn't work for compat mode in general, so we can move these two into the generic code.
Again, the best implementation I could come up with was to merge the compat handling into the regular ppp_ioctl() function and treating all ioctl commands as compatible.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/net/ppp/ppp_generic.c | 39 ++++++++++++++++++++++++++++++----- fs/compat_ioctl.c | 37 --------------------------------- 2 files changed, 34 insertions(+), 42 deletions(-)
diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c index 8dfe8d47df95..3a7aa2eed415 100644 --- a/drivers/net/ppp/ppp_generic.c +++ b/drivers/net/ppp/ppp_generic.c @@ -22,6 +22,7 @@ * ==FILEVERSION 20041108== */
+#include <linux/compat.h> #include <linux/module.h> #include <linux/kernel.h> #include <linux/sched/signal.h> @@ -567,14 +568,36 @@ struct ppp_option_data32 { #endif
#ifdef CONFIG_PPP_FILTER -static int get_filter(void __user *arg, struct sock_filter **p) +#ifdef CONFIG_COMPAT +struct sock_fprog32 { + unsigned short len; + compat_caddr_t filter; +}; +#define PPPIOCSPASS32 _IOW('t', 71, struct sock_fprog32) +#define PPPIOCSACTIVE32 _IOW('t', 70, struct sock_fprog32) +#endif + +static int get_filter(void __user *arg, struct sock_filter **p, bool compat) { struct sock_fprog uprog; struct sock_filter *code = NULL; int len;
- if (copy_from_user(&uprog, arg, sizeof(uprog))) - return -EFAULT; +#ifdef CONFIG_COMPAT + if (compat) { + struct sock_fprog32 uprog32; + + if (copy_from_user(&uprog32, arg, sizeof(uprog32))) + return -EFAULT; + + uprog.len = uprog32.len; + uprog.filter = compat_ptr(uprog32.filter); + } else +#endif + { + if (copy_from_user(&uprog, arg, sizeof(uprog))) + return -EFAULT; + }
if (!uprog.len) { *p = NULL; @@ -772,10 +795,13 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
#ifdef CONFIG_PPP_FILTER case PPPIOCSPASS: +#ifdef CONFIG_COMPAT + case PPPIOCSPASS32: +#endif { struct sock_filter *code;
- err = get_filter(argp, &code); + err = get_filter(argp, &code, cmd != PPPIOCSPASS); if (err >= 0) { struct bpf_prog *pass_filter = NULL; struct sock_fprog_kern fprog = { @@ -798,10 +824,13 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg) break; } case PPPIOCSACTIVE: +#ifdef CONFIG_COMPAT + case PPPIOCSACTIVE32: +#endif { struct sock_filter *code;
- err = get_filter(argp, &code); + err = get_filter(argp, &code, cmd != PPPIOCSACTIVE); if (err >= 0) { struct bpf_prog *active_filter = NULL; struct sock_fprog_kern fprog = { diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c index f518dc174dc7..258c6938e80a 100644 --- a/fs/compat_ioctl.c +++ b/fs/compat_ioctl.c @@ -379,40 +379,6 @@ static int sg_grt_trans(struct file *file, } #endif /* CONFIG_BLOCK */
-struct sock_fprog32 { - unsigned short len; - compat_caddr_t filter; -}; - -#define PPPIOCSPASS32 _IOW('t', 71, struct sock_fprog32) -#define PPPIOCSACTIVE32 _IOW('t', 70, struct sock_fprog32) - -static int ppp_sock_fprog_ioctl_trans(struct file *file, - unsigned int cmd, struct sock_fprog32 __user *u_fprog32) -{ - struct sock_fprog __user *u_fprog64 = compat_alloc_user_space(sizeof(struct sock_fprog)); - void __user *fptr64; - u32 fptr32; - u16 flen; - - if (get_user(flen, &u_fprog32->len) || - get_user(fptr32, &u_fprog32->filter)) - return -EFAULT; - - fptr64 = compat_ptr(fptr32); - - if (put_user(flen, &u_fprog64->len) || - put_user(fptr64, &u_fprog64->filter)) - return -EFAULT; - - if (cmd == PPPIOCSPASS32) - cmd = PPPIOCSPASS; - else - cmd = PPPIOCSACTIVE; - - return do_ioctl(file, cmd, (unsigned long) u_fprog64); -} - struct ppp_idle32 { compat_time_t xmit_idle; compat_time_t recv_idle; @@ -1218,9 +1184,6 @@ static long do_ioctl_trans(unsigned int cmd, switch (cmd) { case PPPIOCGIDLE32: return ppp_gidle(file, cmd, argp); - case PPPIOCSPASS32: - case PPPIOCSACTIVE32: - return ppp_sock_fprog_ioctl_trans(file, cmd, argp); #ifdef CONFIG_BLOCK case SG_IO: return sg_ioctl_trans(file, cmd, argp);
On Wed, Aug 29, 2018 at 04:03:29PM +0200, Arnd Bergmann wrote:
PPPIOCSPASS and PPPIOCSACTIVE are implemented in ppp_generic and isdn_ppp, but the latter one doesn't work for compat mode in general, so we can move these two into the generic code.
Again, the best implementation I could come up with was to merge the compat handling into the regular ppp_ioctl() function and treating all ioctl commands as compatible.
Acked-by: Guillaume Nault g.nault@alphalink.fr
The ppp_idle structure is defined in terms of __kernel_time_t, which is defined as 'long' on all architectures, and this usage is not affected by the y2038 problem since it transports a time interval rather than an absolute time.
However, the ppp user space defines the same structure as time_t, which may be 64-bit wide on new libc versions even on 32-bit architectures.
It's easy enough to just handle both possible structure layouts on all architectures, to deal with the possibility that a user space ppp implementation comes with its own ppp_idle structure definition, as well as to document the fact that the driver is y2038-safe.
Doing this also avoids the need for a special compat mode translation, since 32-bit and 64-bit kernels now support the same interfaces.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- Documentation/networking/ppp_generic.txt | 2 ++ drivers/isdn/i4l/isdn_ppp.c | 14 ++++++++--- drivers/net/ppp/ppp_generic.c | 18 ++++++++++---- fs/compat_ioctl.c | 31 ------------------------ include/uapi/linux/ppp-ioctl.h | 2 ++ include/uapi/linux/ppp_defs.h | 14 +++++++++++ 6 files changed, 42 insertions(+), 39 deletions(-)
diff --git a/Documentation/networking/ppp_generic.txt b/Documentation/networking/ppp_generic.txt index 61daf4b39600..fd563aff5fc9 100644 --- a/Documentation/networking/ppp_generic.txt +++ b/Documentation/networking/ppp_generic.txt @@ -378,6 +378,8 @@ an interface unit are: CONFIG_PPP_FILTER option is enabled, the set of packets which reset the transmit and receive idle timers is restricted to those which pass the `active' packet filter. + Two versions of this command exist, to deal with user space + expecting times as either 32-bit or 64-bit time_t seconds.
* PPPIOCSMAXCID sets the maximum connection-ID parameter (and thus the number of connection slots) for the TCP header compressor and diff --git a/drivers/isdn/i4l/isdn_ppp.c b/drivers/isdn/i4l/isdn_ppp.c index a7b275ea5de1..1f17126c5fa4 100644 --- a/drivers/isdn/i4l/isdn_ppp.c +++ b/drivers/isdn/i4l/isdn_ppp.c @@ -543,11 +543,19 @@ isdn_ppp_ioctl(int min, struct file *file, unsigned int cmd, unsigned long arg) } is->pppcfg = val; break; - case PPPIOCGIDLE: /* get idle time information */ + case PPPIOCGIDLE32: /* get idle time information */ if (lp) { - struct ppp_idle pidle; + struct ppp_idle32 pidle; pidle.xmit_idle = pidle.recv_idle = lp->huptimer; - if ((r = set_arg(argp, &pidle, sizeof(struct ppp_idle)))) + if ((r = set_arg(argp, &pidle, sizeof(struct ppp_idle32)))) + return r; + } + break; + case PPPIOCGIDLE64: /* get idle time information */ + if (lp) { + struct ppp_idle64 pidle; + pidle.xmit_idle = pidle.recv_idle = lp->huptimer; + if ((r = set_arg(argp, &pidle, sizeof(struct ppp_idle64)))) return r; } break; diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c index 3a7aa2eed415..c8b8aa071140 100644 --- a/drivers/net/ppp/ppp_generic.c +++ b/drivers/net/ppp/ppp_generic.c @@ -619,7 +619,8 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg) struct ppp_file *pf; struct ppp *ppp; int err = -EFAULT, val, val2, i; - struct ppp_idle idle; + struct ppp_idle32 idle32; + struct ppp_idle64 idle64; struct npioctl npi; int unit, cflags; struct slcompress *vj; @@ -743,10 +744,17 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg) err = 0; break;
- case PPPIOCGIDLE: - idle.xmit_idle = (jiffies - ppp->last_xmit) / HZ; - idle.recv_idle = (jiffies - ppp->last_recv) / HZ; - if (copy_to_user(argp, &idle, sizeof(idle))) + case PPPIOCGIDLE32: + idle32.xmit_idle = (jiffies - ppp->last_xmit) / HZ; + idle32.recv_idle = (jiffies - ppp->last_recv) / HZ; + if (copy_to_user(argp, &idle32, sizeof(idle32))) + err = 0; + break; + + case PPPIOCGIDLE64: + idle64.xmit_idle = (jiffies - ppp->last_xmit) / HZ; + idle64.recv_idle = (jiffies - ppp->last_recv) / HZ; + if (copy_to_user(argp, &idle32, sizeof(idle32))) break; err = 0; break; diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c index 258c6938e80a..208ff51f3ed9 100644 --- a/fs/compat_ioctl.c +++ b/fs/compat_ioctl.c @@ -377,36 +377,7 @@ static int sg_grt_trans(struct file *file, } return err; } -#endif /* CONFIG_BLOCK */ - -struct ppp_idle32 { - compat_time_t xmit_idle; - compat_time_t recv_idle; -}; -#define PPPIOCGIDLE32 _IOR('t', 63, struct ppp_idle32) - -static int ppp_gidle(struct file *file, unsigned int cmd, - struct ppp_idle32 __user *idle32) -{ - struct ppp_idle __user *idle; - __kernel_time_t xmit, recv; - int err; - - idle = compat_alloc_user_space(sizeof(*idle)); - - err = do_ioctl(file, PPPIOCGIDLE, (unsigned long) idle);
- if (!err) { - if (get_user(xmit, &idle->xmit_idle) || - get_user(recv, &idle->recv_idle) || - put_user(xmit, &idle32->xmit_idle) || - put_user(recv, &idle32->recv_idle)) - err = -EFAULT; - } - return err; -} - -#ifdef CONFIG_BLOCK struct mtget32 { compat_long_t mt_type; compat_long_t mt_resid; @@ -1182,8 +1153,6 @@ static long do_ioctl_trans(unsigned int cmd, void __user *argp = compat_ptr(arg);
switch (cmd) { - case PPPIOCGIDLE32: - return ppp_gidle(file, cmd, argp); #ifdef CONFIG_BLOCK case SG_IO: return sg_ioctl_trans(file, cmd, argp); diff --git a/include/uapi/linux/ppp-ioctl.h b/include/uapi/linux/ppp-ioctl.h index 88b5f9990320..7bd2a5a75348 100644 --- a/include/uapi/linux/ppp-ioctl.h +++ b/include/uapi/linux/ppp-ioctl.h @@ -104,6 +104,8 @@ struct pppol2tp_ioc_stats { #define PPPIOCGDEBUG _IOR('t', 65, int) /* Read debug level */ #define PPPIOCSDEBUG _IOW('t', 64, int) /* Set debug level */ #define PPPIOCGIDLE _IOR('t', 63, struct ppp_idle) /* get idle time */ +#define PPPIOCGIDLE32 _IOR('t', 63, struct ppp_idle32) /* 32-bit times */ +#define PPPIOCGIDLE64 _IOR('t', 63, struct ppp_idle64) /* 64-bit times */ #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) /* obsolete, do not use */ diff --git a/include/uapi/linux/ppp_defs.h b/include/uapi/linux/ppp_defs.h index fff51b91b409..0039fa39a358 100644 --- a/include/uapi/linux/ppp_defs.h +++ b/include/uapi/linux/ppp_defs.h @@ -142,10 +142,24 @@ struct ppp_comp_stats { /* * The following structure records the time in seconds since * the last NP packet was sent or received. + * + * Linux implements both 32-bit and 64-bit time_t versions + * for compatibility with user space that defines ppp_idle + * based on the libc time_t. */ struct ppp_idle { __kernel_time_t xmit_idle; /* time since last NP packet sent */ __kernel_time_t recv_idle; /* time since last NP packet received */ };
+struct ppp_idle32 { + __s32 xmit_idle; /* time since last NP packet sent */ + __s32 recv_idle; /* time since last NP packet received */ +}; + +struct ppp_idle64 { + __s64 xmit_idle; /* time since last NP packet sent */ + __s64 recv_idle; /* time since last NP packet received */ +}; + #endif /* _UAPI_PPP_DEFS_H_ */
On Wed, Aug 29, 2018 at 04:03:30PM +0200, Arnd Bergmann wrote:
The ppp_idle structure is defined in terms of __kernel_time_t, which is defined as 'long' on all architectures, and this usage is not affected by the y2038 problem since it transports a time interval rather than an absolute time.
However, the ppp user space defines the same structure as time_t, which may be 64-bit wide on new libc versions even on 32-bit architectures.
It's easy enough to just handle both possible structure layouts on all architectures, to deal with the possibility that a user space ppp implementation comes with its own ppp_idle structure definition, as well as to document the fact that the driver is y2038-safe.
Doing this also avoids the need for a special compat mode translation, since 32-bit and 64-bit kernels now support the same interfaces.
Signed-off-by: Arnd Bergmann arnd@arndb.de
Documentation/networking/ppp_generic.txt | 2 ++ drivers/isdn/i4l/isdn_ppp.c | 14 ++++++++--- drivers/net/ppp/ppp_generic.c | 18 ++++++++++---- fs/compat_ioctl.c | 31 ------------------------ include/uapi/linux/ppp-ioctl.h | 2 ++ include/uapi/linux/ppp_defs.h | 14 +++++++++++ 6 files changed, 42 insertions(+), 39 deletions(-)
diff --git a/Documentation/networking/ppp_generic.txt b/Documentation/networking/ppp_generic.txt index 61daf4b39600..fd563aff5fc9 100644 --- a/Documentation/networking/ppp_generic.txt +++ b/Documentation/networking/ppp_generic.txt @@ -378,6 +378,8 @@ an interface unit are: CONFIG_PPP_FILTER option is enabled, the set of packets which reset the transmit and receive idle timers is restricted to those which pass the `active' packet filter.
- Two versions of this command exist, to deal with user space
- expecting times as either 32-bit or 64-bit time_t seconds.
- PPPIOCSMAXCID sets the maximum connection-ID parameter (and thus the number of connection slots) for the TCP header compressor and
diff --git a/drivers/isdn/i4l/isdn_ppp.c b/drivers/isdn/i4l/isdn_ppp.c index a7b275ea5de1..1f17126c5fa4 100644 --- a/drivers/isdn/i4l/isdn_ppp.c +++ b/drivers/isdn/i4l/isdn_ppp.c @@ -543,11 +543,19 @@ isdn_ppp_ioctl(int min, struct file *file, unsigned int cmd, unsigned long arg) } is->pppcfg = val; break;
- case PPPIOCGIDLE: /* get idle time information */
- case PPPIOCGIDLE32: /* get idle time information */ if (lp) {
struct ppp_idle pidle;
struct ppp_idle32 pidle; pidle.xmit_idle = pidle.recv_idle = lp->huptimer;
if ((r = set_arg(argp, &pidle, sizeof(struct ppp_idle))))
if ((r = set_arg(argp, &pidle, sizeof(struct ppp_idle32))))return r;}break;- case PPPIOCGIDLE64: /* get idle time information */
if (lp) {struct ppp_idle64 pidle;pidle.xmit_idle = pidle.recv_idle = lp->huptimer;if ((r = set_arg(argp, &pidle, sizeof(struct ppp_idle64)))) return r;diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c index 3a7aa2eed415..c8b8aa071140 100644 --- a/drivers/net/ppp/ppp_generic.c +++ b/drivers/net/ppp/ppp_generic.c @@ -619,7 +619,8 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg) struct ppp_file *pf; struct ppp *ppp; int err = -EFAULT, val, val2, i;
- struct ppp_idle idle;
- struct ppp_idle32 idle32;
- struct ppp_idle64 idle64; struct npioctl npi; int unit, cflags; struct slcompress *vj;
@@ -743,10 +744,17 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg) err = 0; break;
- case PPPIOCGIDLE:
idle.xmit_idle = (jiffies - ppp->last_xmit) / HZ;idle.recv_idle = (jiffies - ppp->last_recv) / HZ;if (copy_to_user(argp, &idle, sizeof(idle)))
- case PPPIOCGIDLE32:
idle32.xmit_idle = (jiffies - ppp->last_xmit) / HZ;idle32.recv_idle = (jiffies - ppp->last_recv) / HZ;if (copy_to_user(argp, &idle32, sizeof(idle32)))
Missing 'break;'
err = 0;break;- case PPPIOCGIDLE64:
idle64.xmit_idle = (jiffies - ppp->last_xmit) / HZ;idle64.recv_idle = (jiffies - ppp->last_recv) / HZ;if (copy_to_user(argp, &idle32, sizeof(idle32)))
I guess you meant 'idle64' instead of 'idle32'.
On Thu, Aug 30, 2018 at 1:06 PM Guillaume Nault g.nault@alphalink.fr wrote:
On Wed, Aug 29, 2018 at 04:03:30PM +0200, Arnd Bergmann wrote:
@@ -743,10 +744,17 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg) err = 0; break;
case PPPIOCGIDLE:idle.xmit_idle = (jiffies - ppp->last_xmit) / HZ;idle.recv_idle = (jiffies - ppp->last_recv) / HZ;if (copy_to_user(argp, &idle, sizeof(idle)))
case PPPIOCGIDLE32:idle32.xmit_idle = (jiffies - ppp->last_xmit) / HZ;idle32.recv_idle = (jiffies - ppp->last_recv) / HZ;if (copy_to_user(argp, &idle32, sizeof(idle32)))Missing 'break;'
err = 0;break;case PPPIOCGIDLE64:idle64.xmit_idle = (jiffies - ppp->last_xmit) / HZ;idle64.recv_idle = (jiffies - ppp->last_recv) / HZ;if (copy_to_user(argp, &idle32, sizeof(idle32)))I guess you meant 'idle64' instead of 'idle32'.
Good catch, fixing up both now.
Thanks for the review!
Arnd
On Wed, Aug 29, 2018 at 04:03:26PM +0200, Arnd Bergmann wrote:
Support for handling the PPPOEIOCSFWD ioctl in compat mode was added in linux-2.5.69 along with hundreds of other commands, but was always broken sincen only the structure is compatible, but the command number is not, due to the size being sizeof(size_t), or at first sizeof(sizeof((struct sockaddr_pppox)), which is different on 64-bit architectures.
And the implementation was broken until 2016 (see 29e73269aa4d ("pppoe: fix reference counting in PPPoE proxy")), and nobody ever noticed. I should probably have removed this ioctl entirely instead of fixing it. Clearly, it has never been used.
If you think it's worth fixing (as opposed to dropping this ioctl or its compat mode), then, Acked-by: Guillaume Nault g.nault@alphalink.fr
On Thu, Aug 30, 2018 at 1:04 PM Guillaume Nault g.nault@alphalink.fr wrote:
On Wed, Aug 29, 2018 at 04:03:26PM +0200, Arnd Bergmann wrote:
Support for handling the PPPOEIOCSFWD ioctl in compat mode was added in linux-2.5.69 along with hundreds of other commands, but was always broken sincen only the structure is compatible, but the command number is not, due to the size being sizeof(size_t), or at first sizeof(sizeof((struct sockaddr_pppox)), which is different on 64-bit architectures.
And the implementation was broken until 2016 (see 29e73269aa4d ("pppoe: fix reference counting in PPPoE proxy")), and nobody ever noticed. I should probably have removed this ioctl entirely instead of fixing it. Clearly, it has never been used.
If you think it's worth fixing (as opposed to dropping this ioctl or its compat mode), then, Acked-by: Guillaume Nault g.nault@alphalink.fr
I don't care much, but fixing it seems seems easier than coming up with a convincing rationale for dropping.
I'll update the changelog text to include your additional background information though, unless someone else prefers to have it dropped.
Arnd
On Thu, Aug 30, 2018 at 01:54:48PM +0200, Arnd Bergmann wrote:
On Thu, Aug 30, 2018 at 1:04 PM Guillaume Nault g.nault@alphalink.fr wrote:
On Wed, Aug 29, 2018 at 04:03:26PM +0200, Arnd Bergmann wrote:
Support for handling the PPPOEIOCSFWD ioctl in compat mode was added in linux-2.5.69 along with hundreds of other commands, but was always broken sincen only the structure is compatible, but the command number is not, due to the size being sizeof(size_t), or at first sizeof(sizeof((struct sockaddr_pppox)), which is different on 64-bit architectures.
And the implementation was broken until 2016 (see 29e73269aa4d ("pppoe: fix reference counting in PPPoE proxy")), and nobody ever noticed. I should probably have removed this ioctl entirely instead of fixing it. Clearly, it has never been used.
If you think it's worth fixing (as opposed to dropping this ioctl or its compat mode), then, Acked-by: Guillaume Nault g.nault@alphalink.fr
I don't care much, but fixing it seems seems easier than coming up with a convincing rationale for dropping.
I'll update the changelog text to include your additional background information though, unless someone else prefers to have it dropped.
Sounds good. Thanks.
-
Arnd Bergmann -
Guillaume Nault