Dell supplied me with the following test:
#include<stdio.h>
#include<errno.h>
#include<sys/ioctl.h>
#include<fcntl.h>
#include<linux/usbdevice_fs.h>
main(int argc,char*argv[])
{
struct usbdevfs_hub_portinfo hubPortInfo = {0};
struct usbdevfs_ioctl command = {0};
command.ifno = 0;
command.ioctl_code = USBDEVFS_HUB_PORTINFO;
command.data = (void*)&hubPortInfo;
int fd, ret;
if(argc != 2) {
fprintf(stderr,"Usage: %s /proc/bus/usb/<BusNo>/<HubID>\n",argv[0]);
fprintf(stderr,"Example: %s /proc/bus/usb/001/001\n",argv[0]);
exit(1);
}
errno = 0;
fd = open(argv[1],O_RDWR);
if(fd < 0) {
perror("open failed:");
exit(errno);
}
errno = 0;
ret = ioctl(fd,USBDEVFS_IOCTL,&command);
printf("IOCTL return status:%d\n",ret);
if(ret<0) {
perror("IOCTL failed:");
close(fd);
exit(3);
} else {
printf("IOCTL passed:Num of ports %d\n",hubPortInfo.nports);
close(fd);
exit(0);
}
return 0;
}
I have verified that it breaks if built in 32 bit mode on x86_64 and that
the patch below fixes it.
Signed-off-by: Pete Zaitcev <[email protected]>
---
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.
-- Pete
diff -urp -X dontdiff linux-2.6.14-rc3/drivers/usb/core/devio.c linux-2.6.14-rc3-lem/drivers/usb/core/devio.c
--- linux-2.6.14-rc3/drivers/usb/core/devio.c 2005-10-07 21:28:12.000000000 -0700
+++ linux-2.6.14-rc3-lem/drivers/usb/core/devio.c 2005-10-07 21:36:02.000000000 -0700
@@ -1230,23 +1230,20 @@ static int proc_releaseinterface(struct
return 0;
}
-static int proc_ioctl (struct dev_state *ps, void __user *arg)
+static int proc_ioctl(struct dev_state *ps, struct usbdevfs_ioctl *ctl)
{
- struct usbdevfs_ioctl ctrl;
int size;
void *buf = NULL;
int retval = 0;
struct usb_interface *intf = NULL;
struct usb_driver *driver = NULL;
- /* get input parameters and alloc buffer */
- if (copy_from_user(&ctrl, arg, sizeof (ctrl)))
- return -EFAULT;
- if ((size = _IOC_SIZE (ctrl.ioctl_code)) > 0) {
+ /* alloc buffer */
+ if ((size = _IOC_SIZE (ctl->ioctl_code)) > 0) {
if ((buf = kmalloc (size, GFP_KERNEL)) == NULL)
return -ENOMEM;
- if ((_IOC_DIR(ctrl.ioctl_code) & _IOC_WRITE)) {
- if (copy_from_user (buf, ctrl.data, size)) {
+ if ((_IOC_DIR(ctl->ioctl_code) & _IOC_WRITE)) {
+ if (copy_from_user (buf, ctl->data, size)) {
kfree(buf);
return -EFAULT;
}
@@ -1262,9 +1259,9 @@ static int proc_ioctl (struct dev_state
if (ps->dev->state != USB_STATE_CONFIGURED)
retval = -EHOSTUNREACH;
- else if (!(intf = usb_ifnum_to_if (ps->dev, ctrl.ifno)))
+ else if (!(intf = usb_ifnum_to_if (ps->dev, ctl->ifno)))
retval = -EINVAL;
- else switch (ctrl.ioctl_code) {
+ else switch (ctl->ioctl_code) {
/* disconnect kernel driver from interface */
case USBDEVFS_DISCONNECT:
@@ -1296,7 +1293,7 @@ static int proc_ioctl (struct dev_state
if (driver == NULL || driver->ioctl == NULL) {
retval = -ENOTTY;
} else {
- retval = driver->ioctl (intf, ctrl.ioctl_code, buf);
+ retval = driver->ioctl (intf, ctl->ioctl_code, buf);
if (retval == -ENOIOCTLCMD)
retval = -ENOTTY;
}
@@ -1305,15 +1302,42 @@ static int proc_ioctl (struct dev_state
/* cleanup and return */
if (retval >= 0
- && (_IOC_DIR (ctrl.ioctl_code) & _IOC_READ) != 0
+ && (_IOC_DIR (ctl->ioctl_code) & _IOC_READ) != 0
&& size > 0
- && copy_to_user (ctrl.data, buf, size) != 0)
+ && copy_to_user (ctl->data, buf, size) != 0)
retval = -EFAULT;
kfree(buf);
return retval;
}
+static int proc_ioctl_default(struct dev_state *ps, void __user *arg)
+{
+ struct usbdevfs_ioctl ctrl;
+
+ if (copy_from_user(&ctrl, arg, sizeof (ctrl)))
+ return -EFAULT;
+ return proc_ioctl(ps, &ctrl);
+}
+
+#ifdef CONFIG_COMPAT
+static int proc_ioctl_compat(struct dev_state *ps, void __user *arg)
+{
+ struct usbdevfs_ioctl32 __user *uioc;
+ struct usbdevfs_ioctl ctrl;
+ u32 udata;
+
+ uioc = compat_ptr(arg);
+ if (get_user(ctrl.ifno, &uioc->ifno) ||
+ get_user(ctrl.ioctl_code, &uioc->ioctl_code) ||
+ __get_user(udata, &uioc->data))
+ return -EFAULT;
+ ctrl.data = compat_ptr(udata);
+
+ return proc_ioctl(ps, &ctrl);
+}
+#endif
+
/*
* NOTE: All requests here that have interface numbers as parameters
* are assuming that somehow the configuration has been prevented from
@@ -1414,6 +1438,10 @@ static int usbdev_ioctl(struct inode *in
ret = proc_reapurbnonblock_compat(ps, p);
break;
+ case USBDEVFS_IOCTL32:
+ snoop(&dev->dev, "%s: IOCTL\n", __FUNCTION__);
+ ret = proc_ioctl_compat(ps, p);
+ break;
#endif
case USBDEVFS_DISCARDURB:
@@ -1448,7 +1476,7 @@ static int usbdev_ioctl(struct inode *in
case USBDEVFS_IOCTL:
snoop(&dev->dev, "%s: IOCTL\n", __FUNCTION__);
- ret = proc_ioctl(ps, p);
+ ret = proc_ioctl_default(ps, p);
break;
}
usb_unlock_device(dev);
diff -urp -X dontdiff linux-2.6.14-rc3/fs/compat_ioctl.c linux-2.6.14-rc3-lem/fs/compat_ioctl.c
--- linux-2.6.14-rc3/fs/compat_ioctl.c 2005-10-07 21:28:15.000000000 -0700
+++ linux-2.6.14-rc3-lem/fs/compat_ioctl.c 2005-10-17 18:06:40.000000000 -0700
@@ -3050,6 +3050,7 @@ HANDLE_IOCTL(TIOCSSERIAL, serial_struct_
HANDLE_IOCTL(USBDEVFS_CONTROL32, do_usbdevfs_control)
HANDLE_IOCTL(USBDEVFS_BULK32, do_usbdevfs_bulk)
HANDLE_IOCTL(USBDEVFS_DISCSIGNAL32, do_usbdevfs_discsignal)
+COMPATIBLE_IOCTL(USBDEVFS_IOCTL32)
/* i2c */
HANDLE_IOCTL(I2C_FUNCS, w_long)
HANDLE_IOCTL(I2C_RDWR, do_i2c_rdwr_ioctl)
diff -urp -X dontdiff linux-2.6.14-rc3/include/linux/usbdevice_fs.h linux-2.6.14-rc3-lem/include/linux/usbdevice_fs.h
--- linux-2.6.14-rc3/include/linux/usbdevice_fs.h 2005-10-07 21:28:26.000000000 -0700
+++ linux-2.6.14-rc3-lem/include/linux/usbdevice_fs.h 2005-10-07 21:36:02.000000000 -0700
@@ -140,6 +140,12 @@ struct usbdevfs_urb32 {
compat_caddr_t usercontext; /* unused */
struct usbdevfs_iso_packet_desc iso_frame_desc[0];
};
+
+struct usbdevfs_ioctl32 {
+ s32 ifno;
+ s32 ioctl_code;
+ compat_caddr_t data;
+};
#endif
#define USBDEVFS_CONTROL _IOWR('U', 0, struct usbdevfs_ctrltransfer)
@@ -160,6 +166,7 @@ struct usbdevfs_urb32 {
#define USBDEVFS_RELEASEINTERFACE _IOR('U', 16, unsigned int)
#define USBDEVFS_CONNECTINFO _IOW('U', 17, struct usbdevfs_connectinfo)
#define USBDEVFS_IOCTL _IOWR('U', 18, struct usbdevfs_ioctl)
+#define USBDEVFS_IOCTL32 _IOWR('U', 18, struct usbdevfs_ioctl32)
#define USBDEVFS_HUB_PORTINFO _IOR('U', 19, struct usbdevfs_hub_portinfo)
#define USBDEVFS_RESET _IO('U', 20)
#define USBDEVFS_CLEAR_HALT _IOR('U', 21, unsigned int)
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?
thanks,
greg k-h
On Tue, Oct 18, 2005 at 10:13:33AM -0700, Greg KH 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?
>
There are a few exception like submit urb and reap urb is in devio.c
because it contain user space pointer which need some state if put
under compat_ioctl.c.
I want to know why it is better in devio.c in this case as well.
Another comment regarding the change, the USBDEVFS_IOCTL is passing an ioctl
buffer inside another ioctl wrapper.
I am a little nervous no check have been done to what ioctl it is
passing. Most of the case you don't need to convert the buffer, but
what if there is some ioctl need to do something special.
On the safe side, I am expecting a big switch to list all the ioctl
we known can safely pass, can grow the list when needed.
BTW, the other common case is the disconnect ioctl.
Chris
On Tue, 18 Oct 2005 10:13:33 -0700, Greg KH <[email protected]> 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
On Tue, 2005-10-18 at 11:49 -0700, Pete Zaitcev wrote:
>
> 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.
there is one more option; allocate on the user stack space for a 64 bit
struct, then copy_in_user() the fields to that and then pass the new
pointer to the 64 bit struct to the ioctl.....
Not saying this is better or worse than what you did, it's just another
option.
Arjan van de Ven writes:
> On Tue, 2005-10-18 at 11:49 -0700, Pete Zaitcev wrote:
>
> >
> > 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.
>
>
> there is one more option; allocate on the user stack space for a 64 bit
> struct, then copy_in_user() the fields to that and then pass the new
> pointer to the 64 bit struct to the ioctl.....
Doesn't work and it would break user-space.
Case 1 is when the user stack pointer is at or very near the limit
of the available address space for the user stack, and the next page
is not available for expanding the stack.
Case 2 is when user-space manages its own threads and stacks, using
sigaltstack() to ensure that signal handlers execute elsewhere. Assume
the user stack pointer is at or near the limit of the currently allocated
stack. Having the kernel write to memory beyond the user's stack pointer
can then easily clobber some unrelated user data structure.
So please don't do this.
/Mikael
On Tue, 18 Oct 2005 11:05:33 -0400, Christopher Li <[email protected]> wrote:
> On Tue, Oct 18, 2005 at 10:13:33AM -0700, Greg KH wrote:
> I am a little nervous no check have been done to what ioctl it is
> passing. Most of the case you don't need to convert the buffer, but
> what if there is some ioctl need to do something special.
You do not have to be nervous, because USB ioctls are not unstructured.
These ioctls do not receive __user pointers, unsigned longs, or things
freely interpreted. They are defined to receive one buffer, and they
are not expected to perform any copy_from_user/copy_to_user.
The struct file_operations has ioctl which looks like this:
int (*ioctl) (struct inode *, struct file *, unsigned int, unsigned long);
The unsigned long here is often a pointer to user buffer.
However, in USB case, struct usb_driver has ioctl like this:
int (*ioctl) (struct usb_interface *intf, unsigned int code, void *buf);
The *buf is not passed from user mode. It's an honest to goodness
kmalloc'ed buffer.
> On the safe side, I am expecting a big switch to list all the ioctl
> we known can safely pass, can grow the list when needed.
>From the above it should be obvious why such checking is entirely
unnecessary for the case of USB.
To be sure, one can write a driver which looks _inside_ the passed
*buf, and interpret the contents as a bunch of user pointers, in a
misguided attempt to create a DIY s/g capability, for instance.
If such a thing happens, we'd only have to identify the programmers
and impale them on a pole, then feed their intestines to crows.
That ought to take care of the problem without any checking lists.
-- Pete