[PATCH v3 00/26] compat_ioctl: cleanups

[PATCH v3 00/26] compat_ioctl: cleanups

[Arnd Bergmann]

Hi Al,

It took me way longer than I had hoped to revisit this series, see
https://lore.kernel.org/lkml/20180912150142.157913-1-arnd@arndb.de/
for the previously posted version.

I've come to the point where all conversion handlers and most
COMPATIBLE_IOCTL() entries are gone from this file, but for
now, this series only has the parts that have either been reviewed
previously, or that are simple enough to include.

The main missing piece is the SG_IO/SG_GET_REQUEST_TABLE conversion.
I'll post the patches I made for that later, as they need more
testing and review from the scsi maintainers.

I hope you can still take these for the coming merge window, unless
new problems come up.

      Arnd

Arnd Bergmann (26):
  compat_ioctl: pppoe: fix PPPOEIOCSFWD handling
  compat_ioctl: move simple ppp command handling into driver
  compat_ioctl: avoid unused function warning for do_ioctl
  compat_ioctl: move PPPIOCSCOMPRESS32 to ppp-generic.c
  compat_ioctl: move PPPIOCSPASS32/PPPIOCSACTIVE32 to ppp_generic.c
  compat_ioctl: handle PPPIOCGIDLE for 64-bit time_t
  compat_ioctl: move rtc handling into rtc-dev.c
  compat_ioctl: add compat_ptr_ioctl()
  compat_ioctl: move drivers to compat_ptr_ioctl
  compat_ioctl: use correct compat_ptr() translation in drivers
  ceph: fix compat_ioctl for ceph_dir_operations
  compat_ioctl: move more drivers to compat_ptr_ioctl
  compat_ioctl: move tape handling into drivers
  compat_ioctl: move ATYFB_CLK handling to atyfb driver
  compat_ioctl: move isdn/capi ioctl translation into driver
  compat_ioctl: move rfcomm handlers into driver
  compat_ioctl: move hci_sock handlers into driver
  compat_ioctl: remove HCIUART handling
  compat_ioctl: remove HIDIO translation
  compat_ioctl: remove translation for sound ioctls
  compat_ioctl: remove IGNORE_IOCTL()
  compat_ioctl: remove /dev/random commands
  compat_ioctl: remove joystick ioctl translation
  compat_ioctl: remove PCI ioctl translation
  compat_ioctl: remove /dev/raw ioctl translation
  compat_ioctl: remove last RAID handling code

 Documentation/networking/ppp_generic.txt    |   2 +
 arch/um/drivers/hostaudio_kern.c            |   1 +
 drivers/android/binder.c                    |   2 +-
 drivers/char/ppdev.c                        |  12 +-
 drivers/char/random.c                       |   1 +
 drivers/char/tpm/tpm_vtpm_proxy.c           |  12 +-
 drivers/crypto/qat/qat_common/adf_ctl_drv.c |   2 +-
 drivers/dma-buf/dma-buf.c                   |   4 +-
 drivers/dma-buf/sw_sync.c                   |   2 +-
 drivers/dma-buf/sync_file.c                 |   2 +-
 drivers/firewire/core-cdev.c                |  12 +-
 drivers/gpu/drm/amd/amdkfd/kfd_chardev.c    |   2 +-
 drivers/hid/hidraw.c                        |   4 +-
 drivers/hid/usbhid/hiddev.c                 |  11 +-
 drivers/hwtracing/stm/core.c                |  12 +-
 drivers/ide/ide-tape.c                      |  31 +-
 drivers/iio/industrialio-core.c             |   2 +-
 drivers/infiniband/core/uverbs_main.c       |   4 +-
 drivers/isdn/capi/capi.c                    |  31 +
 drivers/isdn/i4l/isdn_ppp.c                 |  14 +-
 drivers/media/rc/lirc_dev.c                 |   4 +-
 drivers/mfd/cros_ec_dev.c                   |   4 +-
 drivers/misc/cxl/flash.c                    |   8 +-
 drivers/misc/genwqe/card_dev.c              |  23 +-
 drivers/misc/mei/main.c                     |  22 +-
 drivers/misc/vmw_vmci/vmci_host.c           |   2 +-
 drivers/mtd/ubi/cdev.c                      |  36 +-
 drivers/net/ppp/ppp_generic.c               |  99 +++-
 drivers/net/ppp/pppoe.c                     |   7 +
 drivers/net/ppp/pptp.c                      |   3 +
 drivers/net/tap.c                           |  12 +-
 drivers/nvdimm/bus.c                        |   4 +-
 drivers/nvme/host/core.c                    |   2 +-
 drivers/pci/switch/switchtec.c              |   2 +-
 drivers/platform/x86/wmi.c                  |   2 +-
 drivers/rpmsg/rpmsg_char.c                  |   4 +-
 drivers/rtc/dev.c                           |  13 +-
 drivers/rtc/rtc-vr41xx.c                    |  10 +
 drivers/s390/char/tape_char.c               |  41 +-
 drivers/sbus/char/display7seg.c             |   2 +-
 drivers/sbus/char/envctrl.c                 |   4 +-
 drivers/scsi/3w-xxxx.c                      |   4 +-
 drivers/scsi/cxlflash/main.c                |   2 +-
 drivers/scsi/esas2r/esas2r_main.c           |   2 +-
 drivers/scsi/megaraid/megaraid_mm.c         |  28 +-
 drivers/scsi/osst.c                         |  34 +-
 drivers/scsi/pmcraid.c                      |   4 +-
 drivers/scsi/st.c                           |  35 +-
 drivers/staging/android/ion/ion.c           |   4 +-
 drivers/staging/pi433/pi433_if.c            |  12 +-
 drivers/staging/vme/devices/vme_user.c      |   2 +-
 drivers/tee/tee_core.c                      |   2 +-
 drivers/usb/class/cdc-wdm.c                 |   2 +-
 drivers/usb/class/usbtmc.c                  |   4 +-
 drivers/usb/core/devio.c                    |  16 +-
 drivers/usb/gadget/function/f_fs.c          |  12 +-
 drivers/vfio/vfio.c                         |  39 +-
 drivers/vhost/net.c                         |  12 +-
 drivers/vhost/scsi.c                        |  12 +-
 drivers/vhost/test.c                        |  12 +-
 drivers/vhost/vsock.c                       |  12 +-
 drivers/video/fbdev/aty/atyfb_base.c        |  12 +-
 drivers/virt/fsl_hypervisor.c               |   2 +-
 fs/btrfs/super.c                            |   2 +-
 fs/ceph/dir.c                               |   1 +
 fs/ceph/file.c                              |   2 +-
 fs/compat_ioctl.c                           | 602 +-------------------
 fs/fat/file.c                               |  13 +-
 fs/fuse/dev.c                               |   2 +-
 fs/notify/fanotify/fanotify_user.c          |   2 +-
 fs/userfaultfd.c                            |   2 +-
 include/linux/fs.h                          |   7 +
 include/linux/if_pppox.h                    |   2 +
 include/linux/mtio.h                        |  58 ++
 include/uapi/linux/ppp-ioctl.h              |   2 +
 include/uapi/linux/ppp_defs.h               |  14 +
 net/bluetooth/hci_sock.c                    |  21 +-
 net/bluetooth/rfcomm/sock.c                 |  14 +-
 net/l2tp/l2tp_ppp.c                         |   3 +
 net/rfkill/core.c                           |   2 +-
 sound/core/oss/pcm_oss.c                    |   4 +
 sound/oss/dmasound/dmasound_core.c          |   2 +
 82 files changed, 452 insertions(+), 1034 deletions(-)
 create mode 100644 include/linux/mtio.h

-- 
2.20.0

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Karsten Keil <isdn@linux-pingi.de>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: devel@driverdev.osuosl.org
Cc: linux-integrity@vger.kernel.org
Cc: qat-linux@intel.com
Cc: linux-crypto@vger.kernel.org
Cc: linux-media@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux1394-devel@lists.sourceforge.net
Cc: amd-gfx@lists.freedesktop.org
Cc: linux-input@vger.kernel.org
Cc: linux-usb@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-ide@vger.kernel.org
Cc: linux-iio@vger.kernel.org
Cc: linux-rdma@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-mtd@lists.infradead.org
Cc: linux-ppp@vger.kernel.org
Cc: linux-nvme@lists.infradead.org
Cc: platform-driver-x86@vger.kernel.org
Cc: linux-remoteproc@vger.kernel.org
Cc: linux-rtc@vger.kernel.org
Cc: linux-s390@vger.kernel.org
Cc: sparclinux@vger.kernel.org
Cc: linux-scsi@vger.kernel.org
Cc: linux-fbdev@vger.kernel.org
Cc: linux-btrfs@vger.kernel.org
Cc: ceph-devel@vger.kernel.org
Cc: linux-fsdevel@vger.kernel.org
Cc: linux-bluetooth@vger.kernel.org
Cc: linux-wireless@vger.kernel.org
Cc: alsa-devel@alsa-project.org
Cc: y2038@lists.linaro.org

[PATCH v3 02/26] compat_ioctl: move simple ppp command handling into driver

[Arnd Bergmann]

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 c708400fff4a..04252c3492ee 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 c5e7435db86c..5eccb49bcd2e 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c
@@ -1124,6 +1124,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 50c60550f295..7cf56ad50d07 100644
--- a/drivers/net/ppp/pptp.c
+++ b/drivers/net/ppp/pptp.c
@@ -628,6 +628,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 f1065d116b55..54f26a9fa9f2 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -608,39 +608,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 = {
-- 
2.20.0

[PATCH v3 04/26] compat_ioctl: move PPPIOCSCOMPRESS32 to ppp-generic.c

[Arnd Bergmann]

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 04252c3492ee..8d211c9c2e4e 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))
@@ -2739,7 +2754,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;
@@ -2748,8 +2763,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 2772b539674d..a7cea8f9c771 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -305,13 +305,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;
@@ -339,29 +332,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;
@@ -904,8 +874,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);
-- 
2.20.0

[PATCH v3 05/26] compat_ioctl: move PPPIOCSPASS32/PPPIOCSACTIVE32 to ppp_generic.c

[Arnd Bergmann]

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 8d211c9c2e4e..b8a867fdd5ad 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 a7cea8f9c771..d507b7189958 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -271,40 +271,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;
@@ -874,9 +840,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);
-- 
2.20.0

[PATCH v3 06/26] compat_ioctl: handle PPPIOCGIDLE for 64-bit time_t

[Arnd Bergmann]

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            | 19 +++++++++++----
 fs/compat_ioctl.c                        | 31 ------------------------
 include/uapi/linux/ppp-ioctl.h           |  2 ++
 include/uapi/linux/ppp_defs.h            | 14 +++++++++++
 6 files changed, 43 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 b8a867fdd5ad..712fa94650fe 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,18 @@ 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)))
+			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, &idle64, sizeof(idle64)))
 			break;
 		err = 0;
 		break;
diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index d507b7189958..e6dbd956cf66 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -269,36 +269,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;
@@ -838,8 +809,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_ */
-- 
2.20.0

Re: [PATCH v3 00/26] compat_ioctl: cleanups

[Douglas Gilbert]

On 2019-04-16 4:19 p.m., Arnd Bergmann wrote:
> Hi Al,
> 
> It took me way longer than I had hoped to revisit this series, see
> https://lore.kernel.org/lkml/20180912150142.157913-1-arnd@arndb.de/
> for the previously posted version.
> 
> I've come to the point where all conversion handlers and most
> COMPATIBLE_IOCTL() entries are gone from this file, but for
> now, this series only has the parts that have either been reviewed
> previously, or that are simple enough to include.
> 
> The main missing piece is the SG_IO/SG_GET_REQUEST_TABLE conversion.
> I'll post the patches I made for that later, as they need more
> testing and review from the scsi maintainers.

Perhaps you could look at the document in this url:
     http://sg.danny.cz/sg/sg_v40.html

It is work-in-progress to modernize the SCSI generic driver. It
extends ioctl(sg_fd, SG_IO, &pt_obj) to additionally accept the sg v4
interface as defined in include/uapi/linux/bsg.h . Currently only the
bsg driver uses the sg v4 interface. Since struct sg_io_v4 is all
explicitly sized integers, I'm guessing it is immune "compat" problems.
[I can see no reference to bsg nor struct sg_io_v4 in the current
fs/compat_ioctl.c file.]

Other additions described in the that document are these new ioctls:
   - SG_IOSUBMIT    ultimately to replace write(sg_fd, ...)
   - SG_IORECEIVE              to replace read(sg_fd, ...)
   - SG_IOABORT     abort SCSI cmd in progress; new functionality
   - SG_SET_GET_EXTENDED   has associated struct sg_extended_info

The first three take a pointer to a struct sg_io_hdr (v3 interface) or
a struct sg_io_v4 object. Both objects start with a 32 bit integer:
'S' identifies the v3 interface while 'Q' identifies the v4 interface.

The SG_SET_GET_EXTENDED ioctl takes a pointer to a struct
sg_extended_info object which contains explicitly sized integers so it
may also be immune from "compat" problems. The ioctls section (13) of
that document referenced above has a table showing how many "sets and
gets" are hiding in the SG_SET_GET_EXTENDED ioctl.

BTW No change is proposed for this case:
     ioctl(normal_block_device, SG_IO, &sg_v3_obj)
which is handled by block/scsi_ioctl.c


This would be a good time for me to address any "compat" concerns in the
proposed sg driver update.

Doug Gilbert


> I hope you can still take these for the coming merge window, unless
> new problems come up.
> 
>        Arnd
> 
> Arnd Bergmann (26):
>    compat_ioctl: pppoe: fix PPPOEIOCSFWD handling
>    compat_ioctl: move simple ppp command handling into driver
>    compat_ioctl: avoid unused function warning for do_ioctl
>    compat_ioctl: move PPPIOCSCOMPRESS32 to ppp-generic.c
>    compat_ioctl: move PPPIOCSPASS32/PPPIOCSACTIVE32 to ppp_generic.c
>    compat_ioctl: handle PPPIOCGIDLE for 64-bit time_t
>    compat_ioctl: move rtc handling into rtc-dev.c
>    compat_ioctl: add compat_ptr_ioctl()
>    compat_ioctl: move drivers to compat_ptr_ioctl
>    compat_ioctl: use correct compat_ptr() translation in drivers
>    ceph: fix compat_ioctl for ceph_dir_operations
>    compat_ioctl: move more drivers to compat_ptr_ioctl
>    compat_ioctl: move tape handling into drivers
>    compat_ioctl: move ATYFB_CLK handling to atyfb driver
>    compat_ioctl: move isdn/capi ioctl translation into driver
>    compat_ioctl: move rfcomm handlers into driver
>    compat_ioctl: move hci_sock handlers into driver
>    compat_ioctl: remove HCIUART handling
>    compat_ioctl: remove HIDIO translation
>    compat_ioctl: remove translation for sound ioctls
>    compat_ioctl: remove IGNORE_IOCTL()
>    compat_ioctl: remove /dev/random commands
>    compat_ioctl: remove joystick ioctl translation
>    compat_ioctl: remove PCI ioctl translation
>    compat_ioctl: remove /dev/raw ioctl translation
>    compat_ioctl: remove last RAID handling code
> 
>   Documentation/networking/ppp_generic.txt    |   2 +
>   arch/um/drivers/hostaudio_kern.c            |   1 +
>   drivers/android/binder.c                    |   2 +-
>   drivers/char/ppdev.c                        |  12 +-
>   drivers/char/random.c                       |   1 +
>   drivers/char/tpm/tpm_vtpm_proxy.c           |  12 +-
>   drivers/crypto/qat/qat_common/adf_ctl_drv.c |   2 +-
>   drivers/dma-buf/dma-buf.c                   |   4 +-
>   drivers/dma-buf/sw_sync.c                   |   2 +-
>   drivers/dma-buf/sync_file.c                 |   2 +-
>   drivers/firewire/core-cdev.c                |  12 +-
>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c    |   2 +-
>   drivers/hid/hidraw.c                        |   4 +-
>   drivers/hid/usbhid/hiddev.c                 |  11 +-
>   drivers/hwtracing/stm/core.c                |  12 +-
>   drivers/ide/ide-tape.c                      |  31 +-
>   drivers/iio/industrialio-core.c             |   2 +-
>   drivers/infiniband/core/uverbs_main.c       |   4 +-
>   drivers/isdn/capi/capi.c                    |  31 +
>   drivers/isdn/i4l/isdn_ppp.c                 |  14 +-
>   drivers/media/rc/lirc_dev.c                 |   4 +-
>   drivers/mfd/cros_ec_dev.c                   |   4 +-
>   drivers/misc/cxl/flash.c                    |   8 +-
>   drivers/misc/genwqe/card_dev.c              |  23 +-
>   drivers/misc/mei/main.c                     |  22 +-
>   drivers/misc/vmw_vmci/vmci_host.c           |   2 +-
>   drivers/mtd/ubi/cdev.c                      |  36 +-
>   drivers/net/ppp/ppp_generic.c               |  99 +++-
>   drivers/net/ppp/pppoe.c                     |   7 +
>   drivers/net/ppp/pptp.c                      |   3 +
>   drivers/net/tap.c                           |  12 +-
>   drivers/nvdimm/bus.c                        |   4 +-
>   drivers/nvme/host/core.c                    |   2 +-
>   drivers/pci/switch/switchtec.c              |   2 +-
>   drivers/platform/x86/wmi.c                  |   2 +-
>   drivers/rpmsg/rpmsg_char.c                  |   4 +-
>   drivers/rtc/dev.c                           |  13 +-
>   drivers/rtc/rtc-vr41xx.c                    |  10 +
>   drivers/s390/char/tape_char.c               |  41 +-
>   drivers/sbus/char/display7seg.c             |   2 +-
>   drivers/sbus/char/envctrl.c                 |   4 +-
>   drivers/scsi/3w-xxxx.c                      |   4 +-
>   drivers/scsi/cxlflash/main.c                |   2 +-
>   drivers/scsi/esas2r/esas2r_main.c           |   2 +-
>   drivers/scsi/megaraid/megaraid_mm.c         |  28 +-
>   drivers/scsi/osst.c                         |  34 +-
>   drivers/scsi/pmcraid.c                      |   4 +-
>   drivers/scsi/st.c                           |  35 +-
>   drivers/staging/android/ion/ion.c           |   4 +-
>   drivers/staging/pi433/pi433_if.c            |  12 +-
>   drivers/staging/vme/devices/vme_user.c      |   2 +-
>   drivers/tee/tee_core.c                      |   2 +-
>   drivers/usb/class/cdc-wdm.c                 |   2 +-
>   drivers/usb/class/usbtmc.c                  |   4 +-
>   drivers/usb/core/devio.c                    |  16 +-
>   drivers/usb/gadget/function/f_fs.c          |  12 +-
>   drivers/vfio/vfio.c                         |  39 +-
>   drivers/vhost/net.c                         |  12 +-
>   drivers/vhost/scsi.c                        |  12 +-
>   drivers/vhost/test.c                        |  12 +-
>   drivers/vhost/vsock.c                       |  12 +-
>   drivers/video/fbdev/aty/atyfb_base.c        |  12 +-
>   drivers/virt/fsl_hypervisor.c               |   2 +-
>   fs/btrfs/super.c                            |   2 +-
>   fs/ceph/dir.c                               |   1 +
>   fs/ceph/file.c                              |   2 +-
>   fs/compat_ioctl.c                           | 602 +-------------------
>   fs/fat/file.c                               |  13 +-
>   fs/fuse/dev.c                               |   2 +-
>   fs/notify/fanotify/fanotify_user.c          |   2 +-
>   fs/userfaultfd.c                            |   2 +-
>   include/linux/fs.h                          |   7 +
>   include/linux/if_pppox.h                    |   2 +
>   include/linux/mtio.h                        |  58 ++
>   include/uapi/linux/ppp-ioctl.h              |   2 +
>   include/uapi/linux/ppp_defs.h               |  14 +
>   net/bluetooth/hci_sock.c                    |  21 +-
>   net/bluetooth/rfcomm/sock.c                 |  14 +-
>   net/l2tp/l2tp_ppp.c                         |   3 +
>   net/rfkill/core.c                           |   2 +-
>   sound/core/oss/pcm_oss.c                    |   4 +
>   sound/oss/dmasound/dmasound_core.c          |   2 +
>   82 files changed, 452 insertions(+), 1034 deletions(-)
>   create mode 100644 include/linux/mtio.h
> 

Re: [PATCH v3 02/26] compat_ioctl: move simple ppp command handling into driver

[Al Viro]

On Tue, Apr 16, 2019 at 10:19:40PM +0200, Arnd Bergmann wrote:
> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index c708400fff4a..04252c3492ee 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,

Oh?  What happens on e.g. s390 with something like PPPIOCNEWUNIT?
Current kernel:
	* no ->compat_ioctl()
	* ->unlock_ioctl() is present
	* found by compat_ioctl_check_table()
	* pass (unsigned long)compat_ptr(arg) to do_vfs_ioctl()
	* pass that to ppp_ioctl()
	* pass that to ppp_unattached_ioctl()
	* fetch int from (int __user *)compat_ptr(arg)

With your patch:
	* call ppp_ioctl()
	* pass arg to ppp_unattached_ioctl()
	* fetch int from (int __user *)arg

AFAICS, that's broken...  Looking at that ppp_ioctl(),
pointer to arch-independent type or ignored:
	PPPIOCNEWUNIT PPPIOCATTACH PPPIOCATTCHAN PPPIOCSMRU PPPIOCSFLAGS
	PPPIOCGFLAGS PPPIOCGUNIT PPPIOCSDEBUG PPPIOCSMAXCID PPPIOCCONNECT
	PPPIOCGDEBUG PPPIOCSMAXCID PPPIOCSMRRU
	PPPIOCDETACH PPPIOCDISCONN
	PPPIOCGASYNCMAP PPPIOCSASYNCMAP PPPIOCGRASYNCMAP PPPIOCSRASYNCMAP
	PPPIOCGXASYNCMAP PPPIOCSXASYNCMAP
	PPPIOCGNPMODE PPPIOCSNPMODE
pointer to struct ppp_option_data (with further pointer-chasing in it):
	PPPIOCSCOMPRESS
pointer to struct ppp_idle:
	PPPIOCGIDLE
pointer to struct sock_filter (with hidden pointer-chasing, AFAICS):
	PPPIOCSPASS PPPIOCSACTIVE

Pretty much all of them take pointers.  What's more, reaction to
unknown is -ENOTTY, not -ENOIOCTLCM, so that patch will have
prevent the translated ones from reaching do_ioctl_trans()

What am I missing here?  Why not simply do

compat_ppp_ioctl()
{
	PPPIOCSCOMPRESS32 => deal with it
	PPPIOCGIDLE32 => deal with it
	PPPIOCSPASS32 / PPPIOCSACTIVE32 => deal with it
	default: pass compat_ptr(arg) to ppp_ioctl() and be done with that
}

with BPF-related bits (both compat and native) taken to e.g. net/core/bpf-ppp.c,
picked by both generic and isdn?  IDGI...

Re: [PATCH v3 04/26] compat_ioctl: move PPPIOCSCOMPRESS32 to ppp-generic.c

[Al Viro]

On Tue, Apr 16, 2019 at 10:19:42PM +0200, Arnd Bergmann wrote:
> +#ifdef CONFIG_COMPAT
> +struct ppp_option_data32 {
> +	compat_caddr_t	ptr;

Huh?  compat_uptr_t, surely?  I realize that compat_ioctl.c is bogus
that way right now, but let's not spread that crap into the places
where it's harder to find...

>  	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;
> +	}

*UGH*

Do that in caller, please.  And sod the flag argument...

Re: [PATCH v3 04/26] compat_ioctl: move PPPIOCSCOMPRESS32 to ppp-generic.c

[Arnd Bergmann]

On Wed, Apr 17, 2019 at 11:16 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Tue, Apr 16, 2019 at 10:19:42PM +0200, Arnd Bergmann wrote:
> > +#ifdef CONFIG_COMPAT
> > +struct ppp_option_data32 {
> > +     compat_caddr_t  ptr;
>
> Huh?  compat_uptr_t, surely?  I realize that compat_ioctl.c is bogus
> that way right now, but let's not spread that crap into the places
> where it's harder to find...

Ok, done.

> >       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;
> > +     }
>
> *UGH*
>
> Do that in caller, please.  And sod the flag argument...

Ack, changed it now.

      Arnd

Re: [PATCH v3 02/26] compat_ioctl: move simple ppp command handling into driver

[Arnd Bergmann]

On Wed, Apr 17, 2019 at 11:13 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Tue, Apr 16, 2019 at 10:19:40PM +0200, Arnd Bergmann wrote:
> > diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> > index c708400fff4a..04252c3492ee 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,
>
> Oh?  What happens on e.g. s390 with something like PPPIOCNEWUNIT?
> Current kernel:
>         * no ->compat_ioctl()
>         * ->unlock_ioctl() is present
>         * found by compat_ioctl_check_table()
>         * pass (unsigned long)compat_ptr(arg) to do_vfs_ioctl()
>         * pass that to ppp_ioctl()
>         * pass that to ppp_unattached_ioctl()
>         * fetch int from (int __user *)compat_ptr(arg)
>
> With your patch:
>         * call ppp_ioctl()
>         * pass arg to ppp_unattached_ioctl()
>         * fetch int from (int __user *)arg
>
> AFAICS, that's broken...

Correct. I had added this patch to the series from an older set of
patches (predating
the compat_ptr_ioctl() series) , and did not check for this issue again. When
I originally created the patch, I assumed that even on s390 it would be no
problem.

> Looking at that ppp_ioctl(),
> pointer to arch-independent type or ignored:
>         PPPIOCNEWUNIT PPPIOCATTACH PPPIOCATTCHAN PPPIOCSMRU PPPIOCSFLAGS
>         PPPIOCGFLAGS PPPIOCGUNIT PPPIOCSDEBUG PPPIOCSMAXCID PPPIOCCONNECT
>         PPPIOCGDEBUG PPPIOCSMAXCID PPPIOCSMRRU
>         PPPIOCDETACH PPPIOCDISCONN
>         PPPIOCGASYNCMAP PPPIOCSASYNCMAP PPPIOCGRASYNCMAP PPPIOCSRASYNCMAP
>         PPPIOCGXASYNCMAP PPPIOCSXASYNCMAP
>         PPPIOCGNPMODE PPPIOCSNPMODE
> pointer to struct ppp_option_data (with further pointer-chasing in it):
>         PPPIOCSCOMPRESS
> pointer to struct ppp_idle:
>         PPPIOCGIDLE
> pointer to struct sock_filter (with hidden pointer-chasing, AFAICS):
>         PPPIOCSPASS PPPIOCSACTIVE
>
> Pretty much all of them take pointers.  What's more, reaction to
> unknown is -ENOTTY, not -ENOIOCTLCM, so that patch will have
> prevent the translated ones from reaching do_ioctl_trans()

Good point, this patch sequence does break bisection.

> What am I missing here?  Why not simply do
>
> compat_ppp_ioctl()
> {
>         PPPIOCSCOMPRESS32 => deal with it
>         PPPIOCGIDLE32 => deal with it
>         PPPIOCSPASS32 / PPPIOCSACTIVE32 => deal with it
>         default: pass compat_ptr(arg) to ppp_ioctl() and be done with that
> }
>
> with BPF-related bits (both compat and native) taken to e.g. net/core/bpf-ppp.c,
> picked by both generic and isdn?  IDGI...

I was trying to unify the native and compat code paths as much
as possible here. Handling the four PPPIO*32 commands in
compat_ppp_ioctl would either require duplicating large chunks
of ppp_ioctl, or keeping the extra compat_alloc_user_space()
copy from the existing implementation.

I'll try to come up with a different way to structure the patches.

      Arnd

Re: [PATCH v3 02/26] compat_ioctl: move simple ppp command handling into driver

[Al Viro]

On Thu, Apr 18, 2019 at 12:03:07AM +0200, Arnd Bergmann wrote:
> On Wed, Apr 17, 2019 at 11:13 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > On Tue, Apr 16, 2019 at 10:19:40PM +0200, Arnd Bergmann wrote:
> > > diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> > > index c708400fff4a..04252c3492ee 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,
> >
> > Oh?  What happens on e.g. s390 with something like PPPIOCNEWUNIT?
> > Current kernel:
> >         * no ->compat_ioctl()
> >         * ->unlock_ioctl() is present
> >         * found by compat_ioctl_check_table()
> >         * pass (unsigned long)compat_ptr(arg) to do_vfs_ioctl()
> >         * pass that to ppp_ioctl()
> >         * pass that to ppp_unattached_ioctl()
> >         * fetch int from (int __user *)compat_ptr(arg)
> >
> > With your patch:
> >         * call ppp_ioctl()
> >         * pass arg to ppp_unattached_ioctl()
> >         * fetch int from (int __user *)arg
> >
> > AFAICS, that's broken...
> 
> Correct. I had added this patch to the series from an older set of
> patches (predating
> the compat_ptr_ioctl() series) , and did not check for this issue again. When
> I originally created the patch, I assumed that even on s390 it would be no
> problem.
> 
> > Looking at that ppp_ioctl(),
> > pointer to arch-independent type or ignored:
> >         PPPIOCNEWUNIT PPPIOCATTACH PPPIOCATTCHAN PPPIOCSMRU PPPIOCSFLAGS
> >         PPPIOCGFLAGS PPPIOCGUNIT PPPIOCSDEBUG PPPIOCSMAXCID PPPIOCCONNECT
> >         PPPIOCGDEBUG PPPIOCSMAXCID PPPIOCSMRRU
> >         PPPIOCDETACH PPPIOCDISCONN
> >         PPPIOCGASYNCMAP PPPIOCSASYNCMAP PPPIOCGRASYNCMAP PPPIOCSRASYNCMAP
> >         PPPIOCGXASYNCMAP PPPIOCSXASYNCMAP
> >         PPPIOCGNPMODE PPPIOCSNPMODE
> > pointer to struct ppp_option_data (with further pointer-chasing in it):
> >         PPPIOCSCOMPRESS
> > pointer to struct ppp_idle:
> >         PPPIOCGIDLE
> > pointer to struct sock_filter (with hidden pointer-chasing, AFAICS):
> >         PPPIOCSPASS PPPIOCSACTIVE
> >
> > Pretty much all of them take pointers.  What's more, reaction to
> > unknown is -ENOTTY, not -ENOIOCTLCM, so that patch will have
> > prevent the translated ones from reaching do_ioctl_trans()
> 
> Good point, this patch sequence does break bisection.
> 
> > What am I missing here?  Why not simply do
> >
> > compat_ppp_ioctl()
> > {
> >         PPPIOCSCOMPRESS32 => deal with it
> >         PPPIOCGIDLE32 => deal with it
> >         PPPIOCSPASS32 / PPPIOCSACTIVE32 => deal with it
> >         default: pass compat_ptr(arg) to ppp_ioctl() and be done with that
> > }
> >
> > with BPF-related bits (both compat and native) taken to e.g. net/core/bpf-ppp.c,
> > picked by both generic and isdn?  IDGI...
> 
> I was trying to unify the native and compat code paths as much
> as possible here. Handling the four PPPIO*32 commands in
> compat_ppp_ioctl would either require duplicating large chunks
> of ppp_ioctl, or keeping the extra compat_alloc_user_space()
> copy from the existing implementation.
> 
> I'll try to come up with a different way to structure the patches.

Huh?  Instead of
        case PPPIOCSCOMPRESS:
                err = ppp_set_compress(ppp, arg);
                break;
in native, have
	struct ppp_option_data data;
...
        case PPPIOCSCOMPRESS:
		if (copy_from_user(&data, argp, sizeof(data)))
			err = -EFAULT;
		else
			err = ppp_set_compress(ppp, &data);
                break;

while in compat do
	struct ppp_option_data32 data32;

        case PPPIOCSCOMPRESS32:
		if (copy_from_user(&data32, argp, sizeof(data32)))
			err = -EFAULT;
		else
			err = ppp_set_compress(ppp, &(struct ppp_option_data){
							.ptr = compat_ptr(data32.ptr),
							.length = data32.length,
							.transmit = data32.transmit
						});
		break;

PPPIOCGIDLE is small to start with - not a lot to copy there.
And as for the filters...  What you need is something like

struct bpf_prog *get_ppp_bpf(struct sock_fprog __user *p)
{
        struct sock_fprog uprog;
	struct sock_fprog_kern fprog;
	struct sock_filter *code = NULL;
	struct bpf_prog *res;
        int err;

        if (copy_from_user(&uprog, p, sizeof(uprog)))
                return ERR_PTR(-EFAULT);

        if (!uprog.len)
                return NULL;

        fprog.len = uprog.len * sizeof(struct sock_filter);
        code = fprog.filter = memdup_user(uprog.filter, fprog.len);
        if (IS_ERR(code))
                return ERR_CAST(code);

	err = bpf_prog_create(&res, &fprog);
	kfree(code);

	if (err)
		return ERR_PTR(err);
	return res;
}
in net/core/ppp-filter.c (or in net/core/filter.c, for that matter, under
appropriate ifdef)) with obvious compat counterpart next to it.  Hell,
turn the above into

static struct bpf_prog *__get_ppp_bpf(struct sock_fprog *kp)
{
	struct sock_fprog_kern fprog;
	struct sock_filter *code = NULL;
	struct bpf_prog *res;
        int err;

        if (!kp->len)
                return NULL;

        fprog.len = kp->len * sizeof(struct sock_filter);
        code = fprog.filter = memdup_user(kp->filter, fprog.len);
        if (IS_ERR(code))
                return ERR_CAST(code);

	err = bpf_prog_create(&res, &fprog);
	kfree(code);

	if (err)
		return ERR_PTR(err);
	return res;
}

struct bpf_prog *get_ppp_bpf(struct sock_fprog __user *p)
{
        struct sock_fprog uprog;

        if (copy_from_user(&uprog, p, sizeof(uprog)))
                return ERR_PTR(-EFAULT);
	return __get_ppp_bpf(&uprog);
}

struct bpf_prog *compat_get_ppp_bpf(struct sock_fprog32 __user *p)
{
        struct sock_fprog uprog32;

        if (copy_from_user(&uprog32, p, sizeof(uprog32)))
                return ERR_PTR(-EFAULT);
	return __get_ppp_bpf(&(struct sock_fprog){
			.len = uprog32.len,
			.filter = compat_ptr(uprog32.filter)});
}

Then in native ioctl do
        case PPPIOCSPASS:
        case PPPIOCSACTIVE:
        {
                struct bpf_prog *filter = get_bpf_ppp(argp);

		if (IS_ERR(filter)) {
			err = PTR_ERR(filter);
		} else {
			struct bpf_prog **which;
			if (cmd = PPPIOCSPASS)
				which = &ppp->pass_filter;
			else
				which = &ppp->active_filter;
			ppp_lock(ppp);
			if (*which)
				bpf_prog_destroy(*which);
			*which = filter;
			ppp_unlock(ppp);
			err = 0;
		}
                break;
	}
in native and similar in compat, with get_bpf_ppp() replaced
with call of compat_get_bpf_ppp() and ioctl numbers obviously
adjusted.  All there is to it...  Helpers obviously shared
with isdn and yes, all crap gone from fs/compat_ioctl.c...

Why would you want to duplicate large chunks of anything?
The above is not even compile-tested, but...  I can put
together a patch if you wish.  Or am I missing something
here?

Re: [PATCH v3 02/26] compat_ioctl: move simple ppp command handling into driver

[Al Viro]

On Thu, Apr 18, 2019 at 12:53:00AM +0100, Al Viro wrote:

> Why would you want to duplicate large chunks of anything?
> The above is not even compile-tested, but...  I can put
> together a patch if you wish.  Or am I missing something
> here?

Actually, there's another broken part - pppox.  And that one
is nastier - you've just lost everything outside of protocol
family ioctls on those sockets.  Try e.g.  SIOCGIFMTU in 32bit
process on those.

We really can't reuse native ->ioctl() there - proto family
->compat_ioctl() *must* return -ENOIOCTLCMD on everything it
doesn't handle.

Re: [PATCH v3 02/26] compat_ioctl: move simple ppp command handling into driver

[Arnd Bergmann]

On Thu, Apr 18, 2019 at 1:53 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Thu, Apr 18, 2019 at 12:03:07AM +0200, Arnd Bergmann wrote:
> > On Wed, Apr 17, 2019 at 11:13 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> > >
> > > On Tue, Apr 16, 2019 at 10:19:40PM +0200, Arnd Bergmann wrote:
> > > > diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> > > > index c708400fff4a..04252c3492ee 100644
_ptr(arg) to ppp_ioctl() and be done with that
> > > }
> > >
> > > with BPF-related bits (both compat and native) taken to e.g. net/core/bpf-ppp.c,
> > > picked by both generic and isdn?  IDGI...
> >
> > I was trying to unify the native and compat code paths as much
> > as possible here. Handling the four PPPIO*32 commands in
> > compat_ppp_ioctl would either require duplicating large chunks
> > of ppp_ioctl, or keeping the extra compat_alloc_user_space()
> > copy from the existing implementation.
> >
> > I'll try to come up with a different way to structure the patches.
>
> Huh?  Instead of
>         case PPPIOCSCOMPRESS:
>                 err = ppp_set_compress(ppp, arg);
>                 break;
> in native, have
>         struct ppp_option_data data;
> ...
>         case PPPIOCSCOMPRESS:
>                 if (copy_from_user(&data, argp, sizeof(data)))
>                         err = -EFAULT;
>                 else
>                         err = ppp_set_compress(ppp, &data);
>                 break;

Right, I ended up with something similar before I saw your message.

> in native and similar in compat, with get_bpf_ppp() replaced
> with call of compat_get_bpf_ppp() and ioctl numbers obviously
> adjusted.  All there is to it...  Helpers obviously shared
> with isdn and yes, all crap gone from fs/compat_ioctl.c...

I would still leave the ISDN side alone, aside from adding
the 64-bit time_t support.

> Why would you want to duplicate large chunks of anything?
> The above is not even compile-tested, but...  I can put
> together a patch if you wish.  Or am I missing something
> here?

I expected that the ppp_compat_ioctl() function would end up
fairly complex, to duplicate the logic before the switch()/case.

What I have now is

static long ppp_compat_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
{
        struct ppp_file *pf;
        struct ppp *ppp;
        int err = -ENOIOCTLCMD;
        struct ppp_option_data32 data32;
        struct ppp_option_data data;
        void __user *argp = compat_ptr(arg);

        mutex_lock(&ppp_mutex);

        pf = file->private_data;
        if (!pf || pf->kind != INTERFACE)
                goto out;

        ppp = PF_TO_PPP(pf);
        switch (cmd) {
        case PPPIOCSCOMPRESS32:
                if (copy_from_user(&data32, argp, sizeof(data32))) {
                        err = -EFAULT;
                        goto out;
                }

                data.ptr = compat_ptr(data32.ptr);
                data.length = data32.length;
                data.transmit = data32.transmit;

                err = ppp_set_compress(ppp, &data);
                break;

#ifdef CONFIG_PPP_FILTER
        case PPPIOCSPASS32:
                err = compat_get_sock_fprog(&uprog, argp);
                if (err)
                        break;
                err = ppp_set_filter(ppp, &uprog, &ppp->pass_filter);
                break;

        case PPPIOCSACTIVE32:
                err = compat_get_sock_fprog(&uprog, argp);
                if (err)
                        break;
                err = ppp_set_filter(ppp, &uprog, &ppp->active_filter);
                break;
#endif /* CONFIG_PPP_FILTER */

        default:
                break;
        }

out:
        mutex_unlock(&ppp_mutex);

        if (err = -ENOIOCTLCMD)
                err = ppp_ioctl(file, cmd, (unsigned long)compat_ptr(arg));

        return err;
}

Which doesn't look nearly as bad as I had feared, but still is
a larger change to the existing code than what I had before,
so there is a bigger risk that I screwed up somewhere new.

       Arnd