Re: [PATCH 06/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSNLIST compat

[Ian Abbott]

On 29/05/2020 01:35, Al Viro wrote:

From: Al Viro 

Signed-off-by: Al Viro 
---
  drivers/staging/comedi/comedi_fops.c | 138 ---
  1 file changed, 48 insertions(+), 90 deletions(-)


Signed-off-by: Ian Abbott 

--
-=( Ian Abbott  || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268.  Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-

[PATCH 06/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSNLIST compat

[Al Viro]

From: Al Viro 

Signed-off-by: Al Viro 
---
 drivers/staging/comedi/comedi_fops.c | 138 ---
 1 file changed, 48 insertions(+), 90 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c 
b/drivers/staging/comedi/comedi_fops.c
index ae0067ab5ead..d80a416e70b2 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -1520,34 +1520,19 @@ static int parse_insn(struct comedi_device *dev, struct 
comedi_insn *insn,
 #define MIN_SAMPLES 16
 #define MAX_SAMPLES 65536
 static int do_insnlist_ioctl(struct comedi_device *dev,
-struct comedi_insnlist __user *arg, void *file)
+struct comedi_insn *insns,
+unsigned int n_insns,
+void *file)
 {
-   struct comedi_insnlist insnlist;
-   struct comedi_insn *insns = NULL;
unsigned int *data = NULL;
unsigned int max_n_data_required = MIN_SAMPLES;
int i = 0;
int ret = 0;
 
lockdep_assert_held(>mutex);
-   if (copy_from_user(, arg, sizeof(insnlist)))
-   return -EFAULT;
-
-   insns = kcalloc(insnlist.n_insns, sizeof(*insns), GFP_KERNEL);
-   if (!insns) {
-   ret = -ENOMEM;
-   goto error;
-   }
-
-   if (copy_from_user(insns, insnlist.insns,
-  sizeof(*insns) * insnlist.n_insns)) {
-   dev_dbg(dev->class_dev, "copy_from_user failed\n");
-   ret = -EFAULT;
-   goto error;
-   }
 
/* Determine maximum memory needed for all instructions. */
-   for (i = 0; i < insnlist.n_insns; ++i) {
+   for (i = 0; i < n_insns; ++i) {
if (insns[i].n > MAX_SAMPLES) {
dev_dbg(dev->class_dev,
"number of samples too large\n");
@@ -1565,7 +1550,7 @@ static int do_insnlist_ioctl(struct comedi_device *dev,
goto error;
}
 
-   for (i = 0; i < insnlist.n_insns; ++i) {
+   for (i = 0; i < n_insns; ++i) {
if (insns[i].insn & INSN_MASK_WRITE) {
if (copy_from_user(data, insns[i].data,
   insns[i].n * sizeof(unsigned int))) {
@@ -1592,7 +1577,6 @@ static int do_insnlist_ioctl(struct comedi_device *dev,
}
 
 error:
-   kfree(insns);
kfree(data);
 
if (ret < 0)
@@ -2236,11 +2220,30 @@ static long comedi_unlocked_ioctl(struct file *file, 
unsigned int cmd,
rc = do_cmdtest_ioctl(dev, (struct comedi_cmd __user *)arg,
  file);
break;
-   case COMEDI_INSNLIST:
-   rc = do_insnlist_ioctl(dev,
-  (struct comedi_insnlist __user *)arg,
-  file);
+   case COMEDI_INSNLIST: {
+   struct comedi_insnlist insnlist;
+   struct comedi_insn *insns = NULL;
+
+   if (copy_from_user(, (void __user *)arg,
+  sizeof(insnlist))) {
+   rc = -EFAULT;
+   break;
+   }
+   insns = kcalloc(insnlist.n_insns, sizeof(*insns), GFP_KERNEL);
+   if (!insns) {
+   rc = -ENOMEM;
+   break;
+   }
+   if (copy_from_user(insns, insnlist.insns,
+  sizeof(*insns) * insnlist.n_insns)) {
+   rc = -EFAULT;
+   kfree(insns);
+   break;
+   }
+   rc = do_insnlist_ioctl(dev, insns, insnlist.n_insns, file);
+   kfree(insns);
break;
+   }
case COMEDI_INSN: {
struct comedi_insn insn;
if (copy_from_user(, (void __user *)arg, sizeof(insn)))
@@ -3094,83 +3097,38 @@ static int get_compat_insn(struct comedi_insn *insn,
return 0;
 }
 
-/* Copy 32-bit insn structure to native insn structure. */
-static int __get_compat_insn(struct comedi_insn __user *insn,
-  struct comedi32_insn_struct __user *insn32)
-{
-   int err;
-   union {
-   unsigned int uint;
-   compat_uptr_t uptr;
-   } temp;
-
-   /* Copy insn structure.  Ignore the unused members. */
-   err = 0;
-   if (!access_ok(insn32, sizeof(*insn32)) ||
-   !access_ok(insn, sizeof(*insn)))
-   return -EFAULT;
-
-   err |= __get_user(temp.uint, >insn);
-   err |= __put_user(temp.uint, >insn);
-   err |= __get_user(temp.uint, >n);
-   err |= __put_user(temp.uint, >n);
-   err |= __get_user(temp.uptr, >data);
-   err |= __put_user(compat_ptr(temp.uptr), >data);
-   err |= __get_user(temp.uint, >subdev);
-   err |= __put_user(temp.uint, >subdev);
-   err |=