2004-04-14 10:47:43

by Duncan Sands

Subject: [PATCH 8/9] USB usbfs: missing lock in proc_getdriver

Protect against driver binding changes while reading the driver name.

devio.c | 14 ++++++++------
1 files changed, 8 insertions(+), 6 deletions(-)


diff -Nru a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
--- a/drivers/usb/core/devio.c Wed Apr 14 12:18:30 2004
+++ b/drivers/usb/core/devio.c Wed Apr 14 12:18:30 2004
@@ -709,12 +709,14 @@
if ((ret = findintfif(ps->dev, gd.interface)) < 0)
return ret;
interface = ps->dev->actconfig->interface[ret];
- if (!interface->dev.driver)
- return -ENODATA;
- strncpy(gd.driver, interface->dev.driver->name, sizeof(gd.driver));
- if (copy_to_user(arg, &gd, sizeof(gd)))
- return -EFAULT;
- return 0;
+ ret = -ENODATA;
+ down_read(&usb_bus_type.subsys.rwsem);
+ if (interface && interface->dev.driver) {
+ strncpy(gd.driver, interface->dev.driver->name, sizeof(gd.driver));
+ ret = copy_to_user(arg, &gd, sizeof(gd)) ? -EFAULT : 0;
+ }
+ up_read(&usb_bus_type.subsys.rwsem);
+ return ret;
}

static int proc_connectinfo(struct dev_state *ps, void __user *arg)

2004-04-14 11:04:58

by Oliver Neukum

Subject: Re: [linux-usb-devel] [PATCH 8/9] USB usbfs: missing lock in proc_getdriver

> + down_read(&usb_bus_type.subsys.rwsem);
> + if (interface && interface->dev.driver) {
> + strncpy(gd.driver, interface->dev.driver->name, sizeof(gd.driver));
> + ret = copy_to_user(arg, &gd, sizeof(gd)) ? -EFAULT : 0;
> + }
> + up_read(&usb_bus_type.subsys.rwsem);
> + return ret;

IMHO you should drop the lock before you copy to userspace.

Regards
Oliver

2004-04-14 11:39:08

by Duncan Sands

Subject: Re: [linux-usb-devel] [PATCH 8/9] USB usbfs: missing lock in proc_getdriver

On Wednesday 14 April 2004 13:04, Oliver Neukum wrote:
> > + down_read(&usb_bus_type.subsys.rwsem);
> > + if (interface && interface->dev.driver) {
> > + strncpy(gd.driver, interface->dev.driver->name, sizeof(gd.driver));
> > + ret = copy_to_user(arg, &gd, sizeof(gd)) ? -EFAULT : 0;
> > + }
> > + up_read(&usb_bus_type.subsys.rwsem);
> > + return ret;
>
> IMHO you should drop the lock before you copy to userspace.

Hi Oliver, I wasn't particularly worried about it since it's a rwsem taken for
reading and writing is a rare event. Do you think it really matters? If so,
how about this instead (compiles but otherwise untested):

@@ -702,13 +708,15 @@
return -EFAULT;
if ((ret = findintfif(ps->dev, gd.interface)) < 0)
return ret;
+ down_read(&usb_bus_type.subsys.rwsem);
interface = ps->dev->actconfig->interface[ret];
- if (!interface->dev.driver)
+ if (!interface || !interface->dev.driver) {
+ up_read(&usb_bus_type.subsys.rwsem);
return -ENODATA;
+ }
strncpy(gd.driver, interface->dev.driver->name, sizeof(gd.driver));
- if (copy_to_user(arg, &gd, sizeof(gd)))
- return -EFAULT;
- return 0;
+ up_read(&usb_bus_type.subsys.rwsem);
+ return copy_to_user(arg, &gd, sizeof(gd)) ? -EFAULT : 0;
}

static int proc_connectinfo(struct dev_state *ps, void __user *arg)

2004-04-14 13:29:14

by Oliver Neukum

Subject: Re: [linux-usb-devel] [PATCH 8/9] USB usbfs: missing lock in proc_getdriver

Am Mittwoch, 14. April 2004 13:37 schrieb Duncan Sands:
> On Wednesday 14 April 2004 13:04, Oliver Neukum wrote:
> > > + down_read(&usb_bus_type.subsys.rwsem);
> > > + if (interface && interface->dev.driver) {
> > > + strncpy(gd.driver, interface->dev.driver->name, sizeof(gd.driver));
> > > + ret = copy_to_user(arg, &gd, sizeof(gd)) ? -EFAULT : 0;
> > > + }
> > > + up_read(&usb_bus_type.subsys.rwsem);
> > > + return ret;
> >
> > IMHO you should drop the lock before you copy to userspace.
>
> Hi Oliver, I wasn't particularly worried about it since it's a rwsem taken
> for reading and writing is a rare event. Do you think it really matters?
> If so, how about this instead (compiles but otherwise untested):

Hi,

I expect it to rarely matter, but it might matter now and then. It's
just a question of hygiene. If you are using a temporary buffer I'd
like to see it used to full advantage. So either drop the lock or do
a direct copy. I'd prefer the first option your patch implemented.

Regards
Oliver

2004-04-14 14:05:04

by Duncan Sands

Subject: Re: [linux-usb-devel] [PATCH 8/9] USB usbfs: missing lock in proc_getdriver

Hi Oliver,

> I expect it to rarely matter, but it might matter now and then. It's
> just a question of hygiene. If you are using a temporary buffer I'd
> like to see it used to full advantage. So either drop the lock or do
> a direct copy. I'd prefer the first option your patch implemented.

I agree. Greg, please consider applying the updated patch:

--- gregkh-2.6/drivers/usb/core/devio.c.orig 2004-04-14 16:02:44.000000000 +0200
+++ gregkh-2.6/drivers/usb/core/devio.c 2004-04-14 16:03:12.000000000 +0200
@@ -702,13 +708,15 @@
return -EFAULT;
if ((ret = findintfif(ps->dev, gd.interface)) < 0)
return ret;
+ down_read(&usb_bus_type.subsys.rwsem);
interface = ps->dev->actconfig->interface[ret];
- if (!interface->dev.driver)
+ if (!interface || !interface->dev.driver) {
+ up_read(&usb_bus_type.subsys.rwsem);
return -ENODATA;
+ }
strncpy(gd.driver, interface->dev.driver->name, sizeof(gd.driver));
- if (copy_to_user(arg, &gd, sizeof(gd)))
- return -EFAULT;
- return 0;
+ up_read(&usb_bus_type.subsys.rwsem);
+ return copy_to_user(arg, &gd, sizeof(gd)) ? -EFAULT : 0;
}

static int proc_connectinfo(struct dev_state *ps, void __user *arg)