Re: usb: Patch for USBDEVFS_IOCTL from 32-bit programs

[Pete Zaitcev <zaitcev@redhat.com>]

On Tue, 18 Oct 2005 10:13:33 -0700, Greg KH <greg@kroah.com> wrote:

> On Mon, Oct 17, 2005 at 06:15:54PM -0700, Pete Zaitcev wrote:
> > I'm cross-posting to l-k because someone I know was making sounds at
> > a notion of #ifdef CONFIG_COMPAT. But I think this solutions is superior
> > to adding anything outside of devio.c.
> 
> Why not put this in fs/compat_ioctl.c where the other usbfs 32bit ioctls
> are?

This is what Dell people did originally. Here is their code:

+static int do_usbdevfs_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg)
+{
+  struct usbdevfs_ioctl kioc;
+  struct usbdevfs_ioctl32 __user *uioc;
+  mm_segment_t old_fs;
+  u32 udata;
+  int err;
+
+  uioc = compat_ptr(arg);
+  if (get_user(kioc.ifno, &uioc->ifno) ||
+      get_user(kioc.ioctl_code, &uioc->ioctl_code) ||
+      __get_user(udata, &uioc->data))
+    return -EFAULT;
+  
+  kioc.data = compat_ptr(udata);
+
+  old_fs = get_fs();
+  set_fs(KERNEL_DS);
+  err = sys_ioctl(fd, USBDEVFS_IOCTL, (unsigned long)&kioc);
+  set_fs(old_fs);
+
+  return err;
+}

The problem here is that compat_ptr does NOT turn user data pointer
into a kernel pointer. It's still a user pointer, only sized
differently. So, when you do set_fs(KERNEL_DS), this pointer
is invalid (miraclously, it does work on AMD64, so Dell's tests
pass on their new Xeons).

So, you cannot simply to have a small shim. Instead, you have to allocate
the buffer, do copy_from_user(), and then call the ioctl. But then,
it would be a double-copy, when the ioctl allocates the buffer again.

I tweaked this in various ways, and the patch I posted looks like
the cleanest solution. But please tell me if I miss something obvious.

-- Pete