All of lore.kernel.org
 help / color / mirror / Atom feed
From: Al Viro <viro@zeniv.linux.org.uk>
To: Ian Abbott <abbotti@mev.co.uk>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCHES] uaccess comedi compat
Date: Fri, 29 May 2020 15:15:55 +0100	[thread overview]
Message-ID: <20200529141555.GC23230@ZenIV.linux.org.uk> (raw)
In-Reply-To: <b18a9407-8124-ff94-8c9b-333a32e0a137@mev.co.uk>

On Fri, May 29, 2020 at 11:48:51AM +0100, Ian Abbott wrote:

> > Al Viro (10):
> >        comedi: move compat ioctl handling to native fops
> >        comedi: get rid of indirection via translated_ioctl()
> >        comedi: get rid of compat_alloc_user_space() mess in COMEDI_CHANINFO compat
> >        comedi: get rid of compat_alloc_user_space() mess in COMEDI_RANGEINFO compat
> >        comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSN compat
> >        comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSNLIST compat
> >        comedi: lift copy_from_user() into callers of __comedi_get_user_cmd()
> >        comedi: do_cmdtest_ioctl(): lift copyin/copyout into the caller
> >        comedi: do_cmd_ioctl(): lift copyin/copyout into the caller
> >        comedi: get rid of compat_alloc_user_space() mess in COMEDI_CMD{,TEST} compat
> 
> There is a bug in patch 05. Patch 10 doesn't seem to have been sent yet (I
> didn't receive it and I can't see it in the thread in the LKML archives).
> I've signed off on 01-04, 06-09.

#5 fixed, force-pushed to the same branch.  As for s-o-b... are you sure that's
the header you have in mind?  Normally it's for the chain of transmission...

Do you offer to take that series through comedi (or staging, or...) git tree?
In that case s-o-b would make sense and I'd be happy to have it taken off
my hands.  Otherwise it probably should be Acked-by: or Reviewed-by: or
Read-through-and-managed-not-to-throw-up: - up to you...

> These should be Cc'd to Greg KH and to devel@driverdev.osuosl.org.

FWIW, 10/10 seems to have been really lost; follows here:

From 88833127a8f00da422ddef03425ad9b19eb65558 Mon Sep 17 00:00:00 2001
From: Al Viro <viro@zeniv.linux.org.uk>
Date: Sun, 26 Apr 2020 09:27:23 -0400
Subject: [PATCH 10/10] comedi: get rid of compat_alloc_user_space() mess in
 COMEDI_CMD{,TEST} compat

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/staging/comedi/comedi_fops.c | 181 +++++++++++++----------------------
 1 file changed, 66 insertions(+), 115 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index f5ecfbfcdaf5..bcdb059e6bb6 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2930,155 +2930,106 @@ static int compat_rangeinfo(struct file *file, unsigned long arg)
 }
 
 /* Copy 32-bit cmd structure to native cmd structure. */
-static int get_compat_cmd(struct comedi_cmd __user *cmd,
+static int get_compat_cmd(struct comedi_cmd *cmd,
 			  struct comedi32_cmd_struct __user *cmd32)
 {
-	int err;
-	union {
-		unsigned int uint;
-		compat_uptr_t uptr;
-	} temp;
-
-	/* Copy cmd structure. */
-	if (!access_ok(cmd32, sizeof(*cmd32)) ||
-	    !access_ok(cmd, sizeof(*cmd)))
+	struct comedi32_cmd_struct v32;
+
+	if (copy_from_user(&v32, cmd32, sizeof(v32)))
 		return -EFAULT;
 
-	err = 0;
-	err |= __get_user(temp.uint, &cmd32->subdev);
-	err |= __put_user(temp.uint, &cmd->subdev);
-	err |= __get_user(temp.uint, &cmd32->flags);
-	err |= __put_user(temp.uint, &cmd->flags);
-	err |= __get_user(temp.uint, &cmd32->start_src);
-	err |= __put_user(temp.uint, &cmd->start_src);
-	err |= __get_user(temp.uint, &cmd32->start_arg);
-	err |= __put_user(temp.uint, &cmd->start_arg);
-	err |= __get_user(temp.uint, &cmd32->scan_begin_src);
-	err |= __put_user(temp.uint, &cmd->scan_begin_src);
-	err |= __get_user(temp.uint, &cmd32->scan_begin_arg);
-	err |= __put_user(temp.uint, &cmd->scan_begin_arg);
-	err |= __get_user(temp.uint, &cmd32->convert_src);
-	err |= __put_user(temp.uint, &cmd->convert_src);
-	err |= __get_user(temp.uint, &cmd32->convert_arg);
-	err |= __put_user(temp.uint, &cmd->convert_arg);
-	err |= __get_user(temp.uint, &cmd32->scan_end_src);
-	err |= __put_user(temp.uint, &cmd->scan_end_src);
-	err |= __get_user(temp.uint, &cmd32->scan_end_arg);
-	err |= __put_user(temp.uint, &cmd->scan_end_arg);
-	err |= __get_user(temp.uint, &cmd32->stop_src);
-	err |= __put_user(temp.uint, &cmd->stop_src);
-	err |= __get_user(temp.uint, &cmd32->stop_arg);
-	err |= __put_user(temp.uint, &cmd->stop_arg);
-	err |= __get_user(temp.uptr, &cmd32->chanlist);
-	err |= __put_user((unsigned int __force *)compat_ptr(temp.uptr),
-			  &cmd->chanlist);
-	err |= __get_user(temp.uint, &cmd32->chanlist_len);
-	err |= __put_user(temp.uint, &cmd->chanlist_len);
-	err |= __get_user(temp.uptr, &cmd32->data);
-	err |= __put_user(compat_ptr(temp.uptr), &cmd->data);
-	err |= __get_user(temp.uint, &cmd32->data_len);
-	err |= __put_user(temp.uint, &cmd->data_len);
-	return err ? -EFAULT : 0;
+	cmd->subdev = v32.subdev;
+	cmd->flags = v32.flags;
+	cmd->start_src = v32.start_src;
+	cmd->start_arg = v32.start_arg;
+	cmd->scan_begin_src = v32.scan_begin_src;
+	cmd->scan_begin_arg = v32.scan_begin_arg;
+	cmd->convert_src = v32.convert_src;
+	cmd->convert_arg = v32.convert_arg;
+	cmd->scan_end_src = v32.scan_end_src;
+	cmd->scan_end_arg = v32.scan_end_arg;
+	cmd->stop_src = v32.stop_src;
+	cmd->stop_arg = v32.stop_arg;
+	cmd->chanlist = compat_ptr(v32.chanlist);
+	cmd->chanlist_len = v32.chanlist_len;
+	cmd->data = compat_ptr(v32.data);
+	cmd->data_len = v32.data_len;
+	return 0;
 }
 
 /* Copy native cmd structure to 32-bit cmd structure. */
 static int put_compat_cmd(struct comedi32_cmd_struct __user *cmd32,
-			  struct comedi_cmd __user *cmd)
-{
-	int err;
-	unsigned int temp;
-
-	/*
-	 * Copy back most of cmd structure.
-	 *
-	 * Assume the pointer values are already valid.
-	 * (Could use ptr_to_compat() to set them.)
-	 */
-	if (!access_ok(cmd, sizeof(*cmd)) ||
-	    !access_ok(cmd32, sizeof(*cmd32)))
-		return -EFAULT;
-
-	err = 0;
-	err |= __get_user(temp, &cmd->subdev);
-	err |= __put_user(temp, &cmd32->subdev);
-	err |= __get_user(temp, &cmd->flags);
-	err |= __put_user(temp, &cmd32->flags);
-	err |= __get_user(temp, &cmd->start_src);
-	err |= __put_user(temp, &cmd32->start_src);
-	err |= __get_user(temp, &cmd->start_arg);
-	err |= __put_user(temp, &cmd32->start_arg);
-	err |= __get_user(temp, &cmd->scan_begin_src);
-	err |= __put_user(temp, &cmd32->scan_begin_src);
-	err |= __get_user(temp, &cmd->scan_begin_arg);
-	err |= __put_user(temp, &cmd32->scan_begin_arg);
-	err |= __get_user(temp, &cmd->convert_src);
-	err |= __put_user(temp, &cmd32->convert_src);
-	err |= __get_user(temp, &cmd->convert_arg);
-	err |= __put_user(temp, &cmd32->convert_arg);
-	err |= __get_user(temp, &cmd->scan_end_src);
-	err |= __put_user(temp, &cmd32->scan_end_src);
-	err |= __get_user(temp, &cmd->scan_end_arg);
-	err |= __put_user(temp, &cmd32->scan_end_arg);
-	err |= __get_user(temp, &cmd->stop_src);
-	err |= __put_user(temp, &cmd32->stop_src);
-	err |= __get_user(temp, &cmd->stop_arg);
-	err |= __put_user(temp, &cmd32->stop_arg);
+			  struct comedi_cmd *cmd)
+{
+	struct comedi32_cmd_struct v32;
+
+	memset(&v32, 0, sizeof(v32));
+	v32.subdev = cmd->subdev;
+	v32.flags = cmd->flags;
+	v32.start_src = cmd->start_src;
+	v32.start_arg = cmd->start_arg;
+	v32.scan_begin_src = cmd->scan_begin_src;
+	v32.scan_begin_arg = cmd->scan_begin_arg;
+	v32.convert_src = cmd->convert_src;
+	v32.convert_arg = cmd->convert_arg;
+	v32.scan_end_src = cmd->scan_end_src;
+	v32.scan_end_arg = cmd->scan_end_arg;
+	v32.stop_src = cmd->stop_src;
+	v32.stop_arg = cmd->stop_arg;
 	/* Assume chanlist pointer is unchanged. */
-	err |= __get_user(temp, &cmd->chanlist_len);
-	err |= __put_user(temp, &cmd32->chanlist_len);
-	/* Assume data pointer is unchanged. */
-	err |= __get_user(temp, &cmd->data_len);
-	err |= __put_user(temp, &cmd32->data_len);
-	return err ? -EFAULT : 0;
+	v32.chanlist = ptr_to_compat(cmd->chanlist);
+	v32.chanlist_len = cmd->chanlist_len;
+	v32.data = ptr_to_compat(cmd->data);
+	v32.data_len = cmd->data_len;
+	return copy_to_user(cmd32, &v32, sizeof(v32));
 }
 
 /* Handle 32-bit COMEDI_CMD ioctl. */
 static int compat_cmd(struct file *file, unsigned long arg)
 {
-	struct comedi_cmd __user *cmd;
-	struct comedi32_cmd_struct __user *cmd32;
+	struct comedi_file *cfp = file->private_data;
+	struct comedi_device *dev = cfp->dev;
+	struct comedi_cmd cmd;
+	bool copy = false;
 	int rc, err;
 
-	cmd32 = compat_ptr(arg);
-	cmd = compat_alloc_user_space(sizeof(*cmd));
-
-	rc = get_compat_cmd(cmd, cmd32);
+	rc = get_compat_cmd(&cmd, compat_ptr(arg));
 	if (rc)
 		return rc;
 
-	rc = comedi_unlocked_ioctl(file, COMEDI_CMD, (unsigned long)cmd);
-	if (rc == -EAGAIN) {
+	mutex_lock(&dev->mutex);
+	rc = do_cmd_ioctl(dev, &cmd, &copy, file);
+	mutex_unlock(&dev->mutex);
+	if (copy) {
 		/* Special case: copy cmd back to user. */
-		err = put_compat_cmd(cmd32, cmd);
+		err = put_compat_cmd(compat_ptr(arg), &cmd);
 		if (err)
 			rc = err;
 	}
-
 	return rc;
 }
 
 /* Handle 32-bit COMEDI_CMDTEST ioctl. */
 static int compat_cmdtest(struct file *file, unsigned long arg)
 {
-	struct comedi_cmd __user *cmd;
-	struct comedi32_cmd_struct __user *cmd32;
+	struct comedi_file *cfp = file->private_data;
+	struct comedi_device *dev = cfp->dev;
+	struct comedi_cmd cmd;
+	bool copy = false;
 	int rc, err;
 
-	cmd32 = compat_ptr(arg);
-	cmd = compat_alloc_user_space(sizeof(*cmd));
-
-	rc = get_compat_cmd(cmd, cmd32);
+	rc = get_compat_cmd(&cmd, compat_ptr(arg));
 	if (rc)
 		return rc;
 
-	rc = comedi_unlocked_ioctl(file, COMEDI_CMDTEST, (unsigned long)cmd);
-	if (rc < 0)
-		return rc;
-
-	err = put_compat_cmd(cmd32, cmd);
-	if (err)
-		rc = err;
-
+	mutex_lock(&dev->mutex);
+	rc = do_cmdtest_ioctl(dev, &cmd, &copy, file);
+	mutex_unlock(&dev->mutex);
+	if (copy) {
+		err = put_compat_cmd(compat_ptr(arg), &cmd);
+		if (err)
+			rc = err;
+	}
 	return rc;
 }
 
-- 
2.11.0


  reply	other threads:[~2020-05-29 14:16 UTC|newest]

Thread overview: 94+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-28 23:40 [PATCHES] uaccess base Al Viro
2020-05-28 23:48 ` [PATCHES] uaccess readdir Al Viro
2020-05-28 23:49   ` [PATCH 4/6] switch readdir(2) to unsafe_copy_dirent_name() Al Viro
2020-05-28 23:49     ` [PATCH 5/6] readdir.c: get compat_filldir() more or less in sync with filldir() Al Viro
2020-05-28 23:49     ` [PATCH 6/6] readdir.c: get rid of the last __put_user(), drop now-useless access_ok() Al Viro
2020-05-28 23:49 ` [PATCH 1/6] uaccess: Add user_read_access_begin/end and user_write_access_begin/end Al Viro
2020-05-28 23:49   ` [PATCH 2/6] uaccess: Selectively open read or write user access Al Viro
2020-05-28 23:49   ` [PATCH 3/6] drm/i915/gem: Replace user_access_begin by user_write_access_begin Al Viro
2020-05-28 23:57 ` [PATCHES] uaccess __copy_from_user() Al Viro
2020-05-28 23:58   ` [PATCH 1/2] firewire: switch ioctl_queue_iso to use of copy_from_user() Al Viro
2020-05-28 23:58     ` [PATCH 2/2] pstore: switch to copy_from_user() Al Viro
2020-05-29  0:03 ` [PATCHES] uaccess __copy_to_user() Al Viro
2020-05-29  0:04   ` [PATCH 1/2] esas2r: don't bother with __copy_to_user() Al Viro
2020-05-29  0:04     ` [PATCH 2/2] dlmfs: convert dlmfs_file_read() to copy_to_user() Al Viro
2020-05-29  1:27       ` Linus Torvalds
2020-05-29  1:47         ` Al Viro
2020-05-29  1:54           ` Linus Torvalds
2020-05-29  3:10             ` Al Viro
2020-05-29  3:42               ` Linus Torvalds
2020-05-29 20:46                 ` Al Viro
2020-05-29 20:57                   ` Linus Torvalds
2020-05-29 21:06                     ` Al Viro
2020-05-29  0:09 ` [PATCHES] uaccess __put_user() Al Viro
2020-05-29  0:10   ` [PATCH 1/3] compat sysinfo(2): don't bother with field-by-field copyout Al Viro
2020-05-29  0:10     ` [PATCH 2/3] scsi_ioctl.c: switch SCSI_IOCTL_GET_IDLUN to copy_to_user() Al Viro
2020-05-29  0:10     ` [PATCH 3/3] pcm_native: result of put_user() needs to be checked Al Viro
2020-05-29  0:34 ` [PATCHES] uaccess comedi compat Al Viro
2020-05-29  0:35   ` [PATCH 01/10] comedi: move compat ioctl handling to native fops Al Viro
2020-05-29  0:35     ` [PATCH 02/10] comedi: get rid of indirection via translated_ioctl() Al Viro
2020-05-29 10:34       ` Ian Abbott
2020-05-29  0:35     ` [PATCH 03/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_CHANINFO compat Al Viro
2020-05-29 10:35       ` Ian Abbott
2020-05-29  0:35     ` [PATCH 04/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_RANGEINFO compat Al Viro
2020-05-29 10:35       ` Ian Abbott
2020-05-29  0:35     ` [PATCH 05/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSN compat Al Viro
2020-05-29 10:05       ` Ian Abbott
2020-05-29  0:35     ` [PATCH 06/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSNLIST compat Al Viro
2020-05-29 10:36       ` Ian Abbott
2020-05-29  0:35     ` [PATCH 07/10] comedi: lift copy_from_user() into callers of __comedi_get_user_cmd() Al Viro
2020-05-29 10:37       ` Ian Abbott
2020-05-29  0:35     ` [PATCH 08/10] comedi: do_cmdtest_ioctl(): lift copyin/copyout into the caller Al Viro
2020-05-29 10:37       ` Ian Abbott
2020-05-29  0:35     ` [PATCH 09/10] comedi: do_cmd_ioctl(): " Al Viro
2020-05-29 10:38       ` Ian Abbott
2020-05-29 10:34     ` [PATCH 01/10] comedi: move compat ioctl handling to native fops Ian Abbott
2020-05-29 10:48   ` [PATCHES] uaccess comedi compat Ian Abbott
2020-05-29 14:15     ` Al Viro [this message]
2020-05-29  0:40 ` [PATCHES] uaccess i915 Al Viro
2020-05-29  5:06   ` Jani Nikula
2020-05-29  5:06     ` [Intel-gfx] " Jani Nikula
2020-05-29 14:17     ` Al Viro
2020-05-29 14:17       ` [Intel-gfx] " Al Viro
2020-05-29  0:41 ` [PATCH 1/5] i915: switch query_{topology,engine}_info() to copy_to_user() Al Viro
2020-05-29  0:41   ` [PATCH 2/5] i915: switch copy_perf_config_registers_or_number() to unsafe_put_user() Al Viro
2020-05-29  0:41   ` [PATCH 3/5] i915 compat ioctl(): just use drm_ioctl_kernel() Al Viro
2020-05-29  0:41   ` [PATCH 4/5] i915: alloc_oa_regs(): get rid of pointless access_ok() Al Viro
2020-05-29  0:41   ` [PATCH 5/5] i915:get_engines(): " Al Viro
2020-05-29 23:26 ` [PATCHES] uaccess misc Al Viro
2020-05-29 23:54   ` Linus Torvalds
2020-05-29 23:57     ` Linus Torvalds
2020-05-29 23:27 ` [PATCH 1/9] pselect6() and friends: take handling the combined 6th/7th args into helper Al Viro
2020-05-29 23:27   ` [PATCH 2/9] binfmt_elf: don't bother with __{put,copy_to}_user() Al Viro
2020-05-29 23:27   ` [PATCH 3/9] binfmt_elf_fdpic: don't use __... uaccess primitives Al Viro
2020-05-29 23:27   ` [PATCH 4/9] binfmt_flat: don't use __put_user() Al Viro
2020-05-29 23:27   ` [PATCH 5/9] x86: switch cp_stat64() to unsafe_put_user() Al Viro
2020-05-29 23:27   ` [PATCH 6/9] TEST_ACCESS_OK _never_ had been checked anywhere Al Viro
2020-05-29 23:27   ` [PATCH 7/9] user_regset_copyout_zero(): use clear_user() Al Viro
2020-05-29 23:27   ` [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user() Al Viro
2020-05-29 23:52     ` Linus Torvalds
2020-05-30 14:31       ` Al Viro
2020-05-30 14:52         ` Al Viro
2020-05-30 16:20         ` Paolo Bonzini
2020-05-30 17:57           ` Linus Torvalds
2020-05-30 18:38             ` Al Viro
2020-05-30 18:52               ` Linus Torvalds
2020-05-30 19:14                 ` Al Viro
2020-05-30 19:20                   ` Linus Torvalds
2020-05-30 19:42                     ` Al Viro
2020-05-30 20:43                       ` Al Viro
2020-05-30 19:19                 ` Al Viro
2020-05-30 19:27                   ` Al Viro
2020-05-29 23:28 ` [PATCH 9/9] bpf: make bpf_check_uarg_tail_zero() use check_zeroed_user() Al Viro
2020-05-31 16:35   ` Alexei Starovoitov
2020-05-29 23:39 ` [PATCHES] uaccess hpsa Al Viro
2020-05-29 23:40   ` [PATCH 1/4] hpsa passthrough: lift {BIG_,}IOCTL_Command_struct copy{in,out} into hpsa_ioctl() Al Viro
2020-05-29 23:40     ` [PATCH 2/4] hpsa: don't bother with vmalloc for BIG_IOCTL_Command_struct Al Viro
2020-05-29 23:40     ` [PATCH 3/4] hpsa: get rid of compat_alloc_user_space() Al Viro
2020-05-29 23:40     ` [PATCH 4/4] hpsa_ioctl(): tidy up a bit Al Viro
2020-06-03  1:57   ` [PATCHES] uaccess hpsa Martin K. Petersen
2020-06-03 18:37   ` Don.Brace
2020-06-03 19:17     ` Al Viro
2020-06-03 20:53       ` Martin K. Petersen
2020-06-03 20:54         ` Al Viro
2020-06-04 14:18           ` Martin K. Petersen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200529141555.GC23230@ZenIV.linux.org.uk \
    --to=viro@zeniv.linux.org.uk \
    --cc=abbotti@mev.co.uk \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.