Re: [PATCH v2] USB: legousbtower: fix a signedness bug in tower_probe()
On Fri, Oct 11, 2019 at 05:11:15PM +0300, Dan Carpenter wrote:
> The problem is that sizeof() is unsigned long so negative error codes
> are type promoted to high positive values and the condition becomes
> false.
>
> Fixes: 1d427be4a39d ("USB: legousbtower: fix slab info leak at probe")
> Signed-off-by: Dan Carpenter
Acked-by: Johan Hovold
> ---
> v2: style improvement suggested by Walter Harms.
>
> drivers/usb/misc/legousbtower.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
> index 9d4c52a7ebe0..9bd240df8f4c 100644
> --- a/drivers/usb/misc/legousbtower.c
> +++ b/drivers/usb/misc/legousbtower.c
> @@ -881,7 +881,7 @@ static int tower_probe (struct usb_interface *interface,
> const struct usb_device
> get_version_reply,
> sizeof(*get_version_reply),
> 1000);
> - if (result < sizeof(*get_version_reply)) {
> + if (result != sizeof(*get_version_reply)) {
> if (result >= 0)
> result = -EIO;
> dev_err(idev, "get version request failed: %d\n", result);
[PATCH v2] USB: legousbtower: fix a signedness bug in tower_probe()
The problem is that sizeof() is unsigned long so negative error codes
are type promoted to high positive values and the condition becomes
false.
Fixes: 1d427be4a39d ("USB: legousbtower: fix slab info leak at probe")
Signed-off-by: Dan Carpenter
---
v2: style improvement suggested by Walter Harms.
drivers/usb/misc/legousbtower.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 9d4c52a7ebe0..9bd240df8f4c 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -881,7 +881,7 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
get_version_reply,
sizeof(*get_version_reply),
1000);
- if (result < sizeof(*get_version_reply)) {
+ if (result != sizeof(*get_version_reply)) {
if (result >= 0)
result = -EIO;
dev_err(idev, "get version request failed: %d\n", result);
--
2.20.1
Re: [PATCH] USB: legousbtower: fix a signedness bug in tower_probe()
On Fri, Oct 11, 2019 at 04:58:56PM +0300, Dan Carpenter wrote:
> On Fri, Oct 11, 2019 at 03:51:26PM +0200, walter harms wrote:
> >
> >
> > Am 11.10.2019 15:35, schrieb Dan Carpenter:
> > > The problem is that sizeof() is unsigned long so negative error codes
> > > are type promoted to high positive values and the condition becomes
> > > false.
> > >
> > > Fixes: 1d427be4a39d ("USB: legousbtower: fix slab info leak at probe")
> > > Signed-off-by: Dan Carpenter
> > > ---
> > > drivers/usb/misc/legousbtower.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/usb/misc/legousbtower.c
> > > b/drivers/usb/misc/legousbtower.c
> > > index 9d4c52a7ebe0..835908fe1e65 100644
> > > --- a/drivers/usb/misc/legousbtower.c
> > > +++ b/drivers/usb/misc/legousbtower.c
> > > @@ -881,7 +881,7 @@ static int tower_probe (struct usb_interface
> > > *interface, const struct usb_device
> > > get_version_reply,
> > > sizeof(*get_version_reply),
> > > 1000);
> > > - if (result < sizeof(*get_version_reply)) {
> > > + if (result < 0 || result < sizeof(*get_version_reply)) {
> > > if (result >= 0)
> > > result = -EIO;
> > > dev_err(idev, "get version request failed: %d\n", result);
> >
> > i am not an USB expert but it seems that this is a complicated way
> > to check for result != sizeof(*get_version_reply).
>
> Yeah. You're right. That would look nicer. I will resend.
Your version, or adding an explicit cast to int, may be preferred as
they document that there's something to watch out for here.
Either way you have my ack.
Johan
Re: [PATCH] USB: legousbtower: fix a signedness bug in tower_probe()
On Fri, Oct 11, 2019 at 04:35:25PM +0300, Dan Carpenter wrote:
> The problem is that sizeof() is unsigned long so negative error codes
> are type promoted to high positive values and the condition becomes
> false.
>
> Fixes: 1d427be4a39d ("USB: legousbtower: fix slab info leak at probe")
> Signed-off-by: Dan Carpenter
> ---
> drivers/usb/misc/legousbtower.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
> index 9d4c52a7ebe0..835908fe1e65 100644
> --- a/drivers/usb/misc/legousbtower.c
> +++ b/drivers/usb/misc/legousbtower.c
> @@ -881,7 +881,7 @@ static int tower_probe (struct usb_interface *interface,
> const struct usb_device
> get_version_reply,
> sizeof(*get_version_reply),
> 1000);
> - if (result < sizeof(*get_version_reply)) {
> + if (result < 0 || result < sizeof(*get_version_reply)) {
> if (result >= 0)
> result = -EIO;
> dev_err(idev, "get version request failed: %d\n", result);
Bah, I should have noticed.
Thanks for fixing this!
Acked-by: Johan Hovold
Johan
Re: [PATCH] USB: legousbtower: fix a signedness bug in tower_probe()
On Fri, Oct 11, 2019 at 03:51:26PM +0200, walter harms wrote:
>
>
> Am 11.10.2019 15:35, schrieb Dan Carpenter:
> > The problem is that sizeof() is unsigned long so negative error codes
> > are type promoted to high positive values and the condition becomes
> > false.
> >
> > Fixes: 1d427be4a39d ("USB: legousbtower: fix slab info leak at probe")
> > Signed-off-by: Dan Carpenter
> > ---
> > drivers/usb/misc/legousbtower.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/misc/legousbtower.c
> > b/drivers/usb/misc/legousbtower.c
> > index 9d4c52a7ebe0..835908fe1e65 100644
> > --- a/drivers/usb/misc/legousbtower.c
> > +++ b/drivers/usb/misc/legousbtower.c
> > @@ -881,7 +881,7 @@ static int tower_probe (struct usb_interface
> > *interface, const struct usb_device
> > get_version_reply,
> > sizeof(*get_version_reply),
> > 1000);
> > - if (result < sizeof(*get_version_reply)) {
> > + if (result < 0 || result < sizeof(*get_version_reply)) {
> > if (result >= 0)
> > result = -EIO;
> > dev_err(idev, "get version request failed: %d\n", result);
>
> i am not an USB expert but it seems that this is a complicated way
> to check for result != sizeof(*get_version_reply).
Yeah. You're right. That would look nicer. I will resend.
regards,
dan carpenter
Re: [PATCH] USB: legousbtower: fix a signedness bug in tower_probe()
Am 11.10.2019 15:35, schrieb Dan Carpenter:
> The problem is that sizeof() is unsigned long so negative error codes
> are type promoted to high positive values and the condition becomes
> false.
>
> Fixes: 1d427be4a39d ("USB: legousbtower: fix slab info leak at probe")
> Signed-off-by: Dan Carpenter
> ---
> drivers/usb/misc/legousbtower.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
> index 9d4c52a7ebe0..835908fe1e65 100644
> --- a/drivers/usb/misc/legousbtower.c
> +++ b/drivers/usb/misc/legousbtower.c
> @@ -881,7 +881,7 @@ static int tower_probe (struct usb_interface *interface,
> const struct usb_device
> get_version_reply,
> sizeof(*get_version_reply),
> 1000);
> - if (result < sizeof(*get_version_reply)) {
> + if (result < 0 || result < sizeof(*get_version_reply)) {
> if (result >= 0)
> result = -EIO;
> dev_err(idev, "get version request failed: %d\n", result);
i am not an USB expert but it seems that this is a complicated way
to check for result != sizeof(*get_version_reply).
re,
wh
[PATCH] USB: legousbtower: fix a signedness bug in tower_probe()
The problem is that sizeof() is unsigned long so negative error codes
are type promoted to high positive values and the condition becomes
false.
Fixes: 1d427be4a39d ("USB: legousbtower: fix slab info leak at probe")
Signed-off-by: Dan Carpenter
---
drivers/usb/misc/legousbtower.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 9d4c52a7ebe0..835908fe1e65 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -881,7 +881,7 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
get_version_reply,
sizeof(*get_version_reply),
1000);
- if (result < sizeof(*get_version_reply)) {
+ if (result < 0 || result < sizeof(*get_version_reply)) {
if (result >= 0)
result = -EIO;
dev_err(idev, "get version request failed: %d\n", result);
--
2.20.1
[PATCH 4/5] USB: legousbtower: fix use-after-free on release
The driver was accessing its struct usb_device in its release()
callback without holding a reference. This would lead to a
use-after-free whenever the device was disconnected while the character
device was still open.
Fixes: fef526cae700 ("USB: legousbtower: remove custom debug macro")
Cc: stable # 3.12
Signed-off-by: Johan Hovold
---
drivers/usb/misc/legousbtower.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 44d6a3381804..9d4c52a7ebe0 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -296,6 +296,7 @@ static inline void tower_delete (struct lego_usb_tower *dev)
kfree (dev->read_buffer);
kfree (dev->interrupt_in_buffer);
kfree (dev->interrupt_out_buffer);
+ usb_put_dev(dev->udev);
kfree (dev);
}
@@ -810,7 +811,7 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
mutex_init(&dev->lock);
- dev->udev = udev;
+ dev->udev = usb_get_dev(udev);
dev->open_count = 0;
dev->disconnected = 0;
--
2.23.0
[PATCH RESEND 2/4] USB: legousbtower: fix deadlock on disconnect
Fix a potential deadlock if disconnect races with open.
Since commit d4ead16f50f9 ("USB: prevent char device open/deregister
race") core holds an rw-semaphore while open is called and when
releasing the minor number during deregistration. This can lead to an
ABBA deadlock if a driver takes a lock in open which it also holds
during deregistration.
This effectively reverts commit 78663ecc344b ("USB: disconnect open race
in legousbtower") which needlessly introduced this issue after a generic
fix for this race had been added to core by commit d4ead16f50f9 ("USB:
prevent char device open/deregister race").
Fixes: 78663ecc344b ("USB: disconnect open race in legousbtower")
Cc: stable # 2.6.24
Reported-by: syzbot+f9549f5ee8a5416f0...@syzkaller.appspotmail.com
Tested-by: syzbot+f9549f5ee8a5416f0...@syzkaller.appspotmail.com
Signed-off-by: Johan Hovold
---
drivers/usb/misc/legousbtower.c | 19 ++-
1 file changed, 2 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 1db07d4dc738..773e4188f336 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -179,7 +179,6 @@ static const struct usb_device_id tower_table[] = {
};
MODULE_DEVICE_TABLE (usb, tower_table);
-static DEFINE_MUTEX(open_disc_mutex);
#define LEGO_USB_TOWER_MINOR_BASE 160
@@ -332,18 +331,14 @@ static int tower_open (struct inode *inode, struct file
*file)
goto exit;
}
- mutex_lock(&open_disc_mutex);
dev = usb_get_intfdata(interface);
-
if (!dev) {
- mutex_unlock(&open_disc_mutex);
retval = -ENODEV;
goto exit;
}
/* lock this device */
if (mutex_lock_interruptible(&dev->lock)) {
- mutex_unlock(&open_disc_mutex);
retval = -ERESTARTSYS;
goto exit;
}
@@ -351,12 +346,10 @@ static int tower_open (struct inode *inode, struct file
*file)
/* allow opening only once */
if (dev->open_count) {
- mutex_unlock(&open_disc_mutex);
retval = -EBUSY;
goto unlock_exit;
}
dev->open_count = 1;
- mutex_unlock(&open_disc_mutex);
/* reset the tower */
result = usb_control_msg (dev->udev,
@@ -423,10 +416,9 @@ static int tower_release (struct inode *inode, struct file
*file)
if (dev == NULL) {
retval = -ENODEV;
- goto exit_nolock;
+ goto exit;
}
- mutex_lock(&open_disc_mutex);
if (mutex_lock_interruptible(&dev->lock)) {
retval = -ERESTARTSYS;
goto exit;
@@ -456,10 +448,7 @@ static int tower_release (struct inode *inode, struct file
*file)
unlock_exit:
mutex_unlock(&dev->lock);
-
exit:
- mutex_unlock(&open_disc_mutex);
-exit_nolock:
return retval;
}
@@ -912,7 +901,6 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
if (retval) {
/* something prevented us from registering this driver */
dev_err(idev, "Not able to get a minor for this device.\n");
- usb_set_intfdata (interface, NULL);
goto error;
}
dev->minor = interface->minor;
@@ -944,16 +932,13 @@ static void tower_disconnect (struct usb_interface
*interface)
int minor;
dev = usb_get_intfdata (interface);
- mutex_lock(&open_disc_mutex);
- usb_set_intfdata (interface, NULL);
minor = dev->minor;
- /* give back our minor */
+ /* give back our minor and prevent further open() */
usb_deregister_dev (interface, &tower_class);
mutex_lock(&dev->lock);
- mutex_unlock(&open_disc_mutex);
/* if the device is not opened, then we clean up right now */
if (!dev->open_count) {
--
2.23.0
[PATCH RESEND 3/4] USB: legousbtower: fix potential NULL-deref on disconnect
The driver is using its struct usb_device pointer as an inverted
disconnected flag, but was setting it to NULL before making sure all
completion handlers had run. This could lead to a NULL-pointer
dereference in a number of dev_dbg and dev_err statements in the
completion handlers which relies on said pointer.
Fix this by unconditionally stopping all I/O and preventing
resubmissions by poisoning the interrupt URBs at disconnect and using a
dedicated disconnected flag.
This also makes sure that all I/O has completed by the time the
disconnect callback returns.
Fixes: 9d974b2a06e3 ("USB: legousbtower.c: remove err() usage")
Fixes: fef526cae700 ("USB: legousbtower: remove custom debug macro")
Fixes: 4dae99638097 ("USB: legotower: remove custom debug macro and module
parameter")
Cc: stable # 3.5
Signed-off-by: Johan Hovold
---
drivers/usb/misc/legousbtower.c | 26 +++---
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 773e4188f336..4fa999882635 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -190,6 +190,7 @@ struct lego_usb_tower {
unsigned char minor; /* the starting minor number
for this device */
int open_count; /* number of times this port
has been opened */
+ unsigned long disconnected:1;
char* read_buffer;
size_t read_buffer_length; /* this much came in */
@@ -289,8 +290,6 @@ static inline void lego_usb_tower_debug_data(struct device
*dev,
*/
static inline void tower_delete (struct lego_usb_tower *dev)
{
- tower_abort_transfers (dev);
-
/* free data structures */
usb_free_urb(dev->interrupt_in_urb);
usb_free_urb(dev->interrupt_out_urb);
@@ -430,7 +429,8 @@ static int tower_release (struct inode *inode, struct file
*file)
retval = -ENODEV;
goto unlock_exit;
}
- if (dev->udev == NULL) {
+
+ if (dev->disconnected) {
/* the device was unplugged before the file was released */
/* unlock here as tower_delete frees dev */
@@ -466,10 +466,9 @@ static void tower_abort_transfers (struct lego_usb_tower
*dev)
if (dev->interrupt_in_running) {
dev->interrupt_in_running = 0;
mb();
- if (dev->udev)
- usb_kill_urb (dev->interrupt_in_urb);
+ usb_kill_urb(dev->interrupt_in_urb);
}
- if (dev->interrupt_out_busy && dev->udev)
+ if (dev->interrupt_out_busy)
usb_kill_urb(dev->interrupt_out_urb);
}
@@ -505,7 +504,7 @@ static __poll_t tower_poll (struct file *file, poll_table
*wait)
dev = file->private_data;
- if (!dev->udev)
+ if (dev->disconnected)
return EPOLLERR | EPOLLHUP;
poll_wait(file, &dev->read_wait, wait);
@@ -552,7 +551,7 @@ static ssize_t tower_read (struct file *file, char __user
*buffer, size_t count,
}
/* verify that the device wasn't unplugged */
- if (dev->udev == NULL) {
+ if (dev->disconnected) {
retval = -ENODEV;
pr_err("No device or device unplugged %d\n", retval);
goto unlock_exit;
@@ -638,7 +637,7 @@ static ssize_t tower_write (struct file *file, const char
__user *buffer, size_t
}
/* verify that the device wasn't unplugged */
- if (dev->udev == NULL) {
+ if (dev->disconnected) {
retval = -ENODEV;
pr_err("No device or device unplugged %d\n", retval);
goto unlock_exit;
@@ -748,7 +747,7 @@ static void tower_interrupt_in_callback (struct urb *urb)
resubmit:
/* resubmit if we're still running */
- if (dev->interrupt_in_running && dev->udev) {
+ if (dev->interrupt_in_running) {
retval = usb_submit_urb (dev->interrupt_in_urb, GFP_ATOMIC);
if (retval)
dev_err(&dev->udev->dev,
@@ -813,6 +812,7 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
dev->udev = udev;
dev->open_count = 0;
+ dev->disconnected = 0;
dev->read_buffer = NULL;
dev->read_buffer_length = 0;
@@ -938,6 +938,10 @@ static void tower_disconnect (struct usb_interface
*interface)
/* give back our minor and prevent further open() */
usb_deregister_dev (interface, &tower_class);
+ /* stop I/O */
+ usb_poison_urb(dev->interrupt_in_urb);
+ usb_poison_urb(dev->interrupt_out_urb);
+
mutex_lock(&dev->
[PATCH RESEND 4/4] USB: legousbtower: fix open after failed reset request
The driver would return with a nonzero open count in case the reset
control request failed. This would prevent any further attempts to open
the char dev until the device was disconnected.
Fix this by incrementing the open count only on successful open.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Johan Hovold
---
drivers/usb/misc/legousbtower.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 4fa999882635..44d6a3381804 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -348,7 +348,6 @@ static int tower_open (struct inode *inode, struct file
*file)
retval = -EBUSY;
goto unlock_exit;
}
- dev->open_count = 1;
/* reset the tower */
result = usb_control_msg (dev->udev,
@@ -388,13 +387,14 @@ static int tower_open (struct inode *inode, struct file
*file)
dev_err(&dev->udev->dev,
"Couldn't submit interrupt_in_urb %d\n", retval);
dev->interrupt_in_running = 0;
- dev->open_count = 0;
goto unlock_exit;
}
/* save device in the file's private structure */
file->private_data = dev;
+ dev->open_count = 1;
+
unlock_exit:
mutex_unlock(&dev->lock);
--
2.23.0
[PATCH RESEND 1/4] USB: legousbtower: fix slab info leak at probe
Make sure to check for short transfers when retrieving the version
information at probe to avoid leaking uninitialised slab data when
logging it.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable
Signed-off-by: Johan Hovold
---
drivers/usb/misc/legousbtower.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 006cf13b2199..1db07d4dc738 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -891,8 +891,10 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
get_version_reply,
sizeof(*get_version_reply),
1000);
- if (result < 0) {
- dev_err(idev, "LEGO USB Tower get version control request
failed\n");
+ if (result < sizeof(*get_version_reply)) {
+ if (result >= 0)
+ result = -EIO;
+ dev_err(idev, "get version request failed: %d\n", result);
retval = result;
goto error;
}
--
2.23.0
[PATCH RESEND 0/4] USB: legousbtower: misc fixes
[ Resend with Juergen on CC ] This series fixes a few issues found in the legousbtower driver. The potential deadlock issue was reported by syzbot, and the rest was found through inspection. I have bunch of clean ups for this driver as well that I'll post once these are in Linus's tree. Johan Johan Hovold (4): USB: legousbtower: fix slab info leak at probe USB: legousbtower: fix deadlock on disconnect USB: legousbtower: fix potential NULL-deref on disconnect USB: legousbtower: fix open after failed reset request drivers/usb/misc/legousbtower.c | 55 ++--- 1 file changed, 23 insertions(+), 32 deletions(-) -- 2.23.0
[PATCH 3/4] USB: legousbtower: fix potential NULL-deref on disconnect
The driver is using its struct usb_device pointer as an inverted
disconnected flag, but was setting it to NULL before making sure all
completion handlers had run. This could lead to a NULL-pointer
dereference in a number of dev_dbg and dev_err statements in the
completion handlers which relies on said pointer.
Fix this by unconditionally stopping all I/O and preventing
resubmissions by poisoning the interrupt URBs at disconnect and using a
dedicated disconnected flag.
This also makes sure that all I/O has completed by the time the
disconnect callback returns.
Fixes: 9d974b2a06e3 ("USB: legousbtower.c: remove err() usage")
Fixes: fef526cae700 ("USB: legousbtower: remove custom debug macro")
Fixes: 4dae99638097 ("USB: legotower: remove custom debug macro and module
parameter")
Cc: stable # 3.5
Signed-off-by: Johan Hovold
---
drivers/usb/misc/legousbtower.c | 26 +++---
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 773e4188f336..4fa999882635 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -190,6 +190,7 @@ struct lego_usb_tower {
unsigned char minor; /* the starting minor number
for this device */
int open_count; /* number of times this port
has been opened */
+ unsigned long disconnected:1;
char* read_buffer;
size_t read_buffer_length; /* this much came in */
@@ -289,8 +290,6 @@ static inline void lego_usb_tower_debug_data(struct device
*dev,
*/
static inline void tower_delete (struct lego_usb_tower *dev)
{
- tower_abort_transfers (dev);
-
/* free data structures */
usb_free_urb(dev->interrupt_in_urb);
usb_free_urb(dev->interrupt_out_urb);
@@ -430,7 +429,8 @@ static int tower_release (struct inode *inode, struct file
*file)
retval = -ENODEV;
goto unlock_exit;
}
- if (dev->udev == NULL) {
+
+ if (dev->disconnected) {
/* the device was unplugged before the file was released */
/* unlock here as tower_delete frees dev */
@@ -466,10 +466,9 @@ static void tower_abort_transfers (struct lego_usb_tower
*dev)
if (dev->interrupt_in_running) {
dev->interrupt_in_running = 0;
mb();
- if (dev->udev)
- usb_kill_urb (dev->interrupt_in_urb);
+ usb_kill_urb(dev->interrupt_in_urb);
}
- if (dev->interrupt_out_busy && dev->udev)
+ if (dev->interrupt_out_busy)
usb_kill_urb(dev->interrupt_out_urb);
}
@@ -505,7 +504,7 @@ static __poll_t tower_poll (struct file *file, poll_table
*wait)
dev = file->private_data;
- if (!dev->udev)
+ if (dev->disconnected)
return EPOLLERR | EPOLLHUP;
poll_wait(file, &dev->read_wait, wait);
@@ -552,7 +551,7 @@ static ssize_t tower_read (struct file *file, char __user
*buffer, size_t count,
}
/* verify that the device wasn't unplugged */
- if (dev->udev == NULL) {
+ if (dev->disconnected) {
retval = -ENODEV;
pr_err("No device or device unplugged %d\n", retval);
goto unlock_exit;
@@ -638,7 +637,7 @@ static ssize_t tower_write (struct file *file, const char
__user *buffer, size_t
}
/* verify that the device wasn't unplugged */
- if (dev->udev == NULL) {
+ if (dev->disconnected) {
retval = -ENODEV;
pr_err("No device or device unplugged %d\n", retval);
goto unlock_exit;
@@ -748,7 +747,7 @@ static void tower_interrupt_in_callback (struct urb *urb)
resubmit:
/* resubmit if we're still running */
- if (dev->interrupt_in_running && dev->udev) {
+ if (dev->interrupt_in_running) {
retval = usb_submit_urb (dev->interrupt_in_urb, GFP_ATOMIC);
if (retval)
dev_err(&dev->udev->dev,
@@ -813,6 +812,7 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
dev->udev = udev;
dev->open_count = 0;
+ dev->disconnected = 0;
dev->read_buffer = NULL;
dev->read_buffer_length = 0;
@@ -938,6 +938,10 @@ static void tower_disconnect (struct usb_interface
*interface)
/* give back our minor and prevent further open() */
usb_deregister_dev (interface, &tower_class);
+ /* stop I/O */
+ usb_poison_urb(dev->interrupt_in_urb);
+ usb_poison_urb(dev->interrupt_out_urb);
+
mutex_lock(&dev->
[PATCH 1/4] USB: legousbtower: fix slab info leak at probe
Make sure to check for short transfers when retrieving the version
information at probe to avoid leaking uninitialised slab data when
logging it.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable
Signed-off-by: Johan Hovold
---
drivers/usb/misc/legousbtower.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 006cf13b2199..1db07d4dc738 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -891,8 +891,10 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
get_version_reply,
sizeof(*get_version_reply),
1000);
- if (result < 0) {
- dev_err(idev, "LEGO USB Tower get version control request
failed\n");
+ if (result < sizeof(*get_version_reply)) {
+ if (result >= 0)
+ result = -EIO;
+ dev_err(idev, "get version request failed: %d\n", result);
retval = result;
goto error;
}
--
2.23.0
[PATCH 0/4] USB: legousbtower: misc fixes
This series fixes a few issues found in the legousbtower driver. The potential deadlock issue was reported by syzbot, and the rest was found through inspection. I have bunch of clean ups for this driver as well that I'll post once these are in Linus's tree. Johan Johan Hovold (4): USB: legousbtower: fix slab info leak at probe USB: legousbtower: fix deadlock on disconnect USB: legousbtower: fix potential NULL-deref on disconnect USB: legousbtower: fix open after failed reset request drivers/usb/misc/legousbtower.c | 55 ++--- 1 file changed, 23 insertions(+), 32 deletions(-) -- 2.23.0
[PATCH 2/4] USB: legousbtower: fix deadlock on disconnect
Fix a potential deadlock if disconnect races with open.
Since commit d4ead16f50f9 ("USB: prevent char device open/deregister
race") core holds an rw-semaphore while open is called and when
releasing the minor number during deregistration. This can lead to an
ABBA deadlock if a driver takes a lock in open which it also holds
during deregistration.
This effectively reverts commit 78663ecc344b ("USB: disconnect open race
in legousbtower") which needlessly introduced this issue after a generic
fix for this race had been added to core by commit d4ead16f50f9 ("USB:
prevent char device open/deregister race").
Fixes: 78663ecc344b ("USB: disconnect open race in legousbtower")
Cc: stable # 2.6.24
Reported-by: syzbot+f9549f5ee8a5416f0...@syzkaller.appspotmail.com
Tested-by: syzbot+f9549f5ee8a5416f0...@syzkaller.appspotmail.com
Signed-off-by: Johan Hovold
---
drivers/usb/misc/legousbtower.c | 19 ++-
1 file changed, 2 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 1db07d4dc738..773e4188f336 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -179,7 +179,6 @@ static const struct usb_device_id tower_table[] = {
};
MODULE_DEVICE_TABLE (usb, tower_table);
-static DEFINE_MUTEX(open_disc_mutex);
#define LEGO_USB_TOWER_MINOR_BASE 160
@@ -332,18 +331,14 @@ static int tower_open (struct inode *inode, struct file
*file)
goto exit;
}
- mutex_lock(&open_disc_mutex);
dev = usb_get_intfdata(interface);
-
if (!dev) {
- mutex_unlock(&open_disc_mutex);
retval = -ENODEV;
goto exit;
}
/* lock this device */
if (mutex_lock_interruptible(&dev->lock)) {
- mutex_unlock(&open_disc_mutex);
retval = -ERESTARTSYS;
goto exit;
}
@@ -351,12 +346,10 @@ static int tower_open (struct inode *inode, struct file
*file)
/* allow opening only once */
if (dev->open_count) {
- mutex_unlock(&open_disc_mutex);
retval = -EBUSY;
goto unlock_exit;
}
dev->open_count = 1;
- mutex_unlock(&open_disc_mutex);
/* reset the tower */
result = usb_control_msg (dev->udev,
@@ -423,10 +416,9 @@ static int tower_release (struct inode *inode, struct file
*file)
if (dev == NULL) {
retval = -ENODEV;
- goto exit_nolock;
+ goto exit;
}
- mutex_lock(&open_disc_mutex);
if (mutex_lock_interruptible(&dev->lock)) {
retval = -ERESTARTSYS;
goto exit;
@@ -456,10 +448,7 @@ static int tower_release (struct inode *inode, struct file
*file)
unlock_exit:
mutex_unlock(&dev->lock);
-
exit:
- mutex_unlock(&open_disc_mutex);
-exit_nolock:
return retval;
}
@@ -912,7 +901,6 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
if (retval) {
/* something prevented us from registering this driver */
dev_err(idev, "Not able to get a minor for this device.\n");
- usb_set_intfdata (interface, NULL);
goto error;
}
dev->minor = interface->minor;
@@ -944,16 +932,13 @@ static void tower_disconnect (struct usb_interface
*interface)
int minor;
dev = usb_get_intfdata (interface);
- mutex_lock(&open_disc_mutex);
- usb_set_intfdata (interface, NULL);
minor = dev->minor;
- /* give back our minor */
+ /* give back our minor and prevent further open() */
usb_deregister_dev (interface, &tower_class);
mutex_lock(&dev->lock);
- mutex_unlock(&open_disc_mutex);
/* if the device is not opened, then we clean up right now */
if (!dev->open_count) {
--
2.23.0
[PATCH 4/4] USB: legousbtower: fix open after failed reset request
The driver would return with a nonzero open count in case the reset
control request failed. This would prevent any further attempts to open
the char dev until the device was disconnected.
Fix this by incrementing the open count only on successful open.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Johan Hovold
---
drivers/usb/misc/legousbtower.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 4fa999882635..44d6a3381804 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -348,7 +348,6 @@ static int tower_open (struct inode *inode, struct file
*file)
retval = -EBUSY;
goto unlock_exit;
}
- dev->open_count = 1;
/* reset the tower */
result = usb_control_msg (dev->udev,
@@ -388,13 +387,14 @@ static int tower_open (struct inode *inode, struct file
*file)
dev_err(&dev->udev->dev,
"Couldn't submit interrupt_in_urb %d\n", retval);
dev->interrupt_in_running = 0;
- dev->open_count = 0;
goto unlock_exit;
}
/* save device in the file's private structure */
file->private_data = dev;
+ dev->open_count = 1;
+
unlock_exit:
mutex_unlock(&dev->lock);
--
2.23.0
[PATCH 12/12] usb: legousbtower: use irqsave() in USB's complete callback
The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.
Cc: Juergen Stuber
Cc: Greg Kroah-Hartman
Cc: legousb-de...@lists.sourceforge.net
Signed-off-by: Sebastian Andrzej Siewior
---
drivers/usb/misc/legousbtower.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index bf47bd8bc76f..006cf13b2199 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -722,6 +722,7 @@ static void tower_interrupt_in_callback (struct urb *urb)
struct lego_usb_tower *dev = urb->context;
int status = urb->status;
int retval;
+ unsigned long flags;
lego_usb_tower_debug_data(&dev->udev->dev, __func__,
urb->actual_length, urb->transfer_buffer);
@@ -740,7 +741,7 @@ static void tower_interrupt_in_callback (struct urb *urb)
}
if (urb->actual_length > 0) {
- spin_lock (&dev->read_buffer_lock);
+ spin_lock_irqsave(&dev->read_buffer_lock, flags);
if (dev->read_buffer_length + urb->actual_length <
read_buffer_size) {
memcpy (dev->read_buffer + dev->read_buffer_length,
dev->interrupt_in_buffer,
@@ -753,7 +754,7 @@ static void tower_interrupt_in_callback (struct urb *urb)
pr_warn("read_buffer overflow, %d bytes dropped\n",
urb->actual_length);
}
- spin_unlock (&dev->read_buffer_lock);
+ spin_unlock_irqrestore(&dev->read_buffer_lock, flags);
}
resubmit:
--
2.18.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] usb: misc: legousbtower: Fix memory leak
get_version_reply is not freed if function returns with success.
Fixes: 942a48730faf ("usb: misc: legousbtower: Fix buffers on stack")
Reported-by: Heikki Krogerus
Signed-off-by: Maksim Salau
---
v2:
Changed tags to match guidelines.
drivers/usb/misc/legousbtower.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index aa3c280fdf8d..0782ac6f5edf 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -926,6 +926,7 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
USB_MAJOR, dev->minor);
exit:
+ kfree(get_version_reply);
return retval;
error:
--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: misc: legousbtower: Fix memory leak
Hi,
On Thu, May 04, 2017 at 10:51:52PM +0300, Maksim Salau wrote:
> get_version_reply is not freed if function returns with success.
> Memory leak was introduced by commit 942a48730faf149ccbf3e12ac718aee120bb3529
Pointing the commit like that is probable fine, but you should also
use "Fixes" tag:
Fixes: 942a48730faf ("usb: misc: legousbtower: Fix buffers on stack")
Please check Documentation/process/submitting-patches.rst for more
information.
> Signed-off-by: Heikki Krogerus
You are signing the patch for me which you should not be doing in this
case. More appropriate tag would be for example "Suggested-by" or
something like that. There is more information about this too in
Documentation/process/submitting-patches.rst
> Signed-off-by: Maksim Salau
> ---
> drivers/usb/misc/legousbtower.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
> index aa3c280..0782ac6 100644
> --- a/drivers/usb/misc/legousbtower.c
> +++ b/drivers/usb/misc/legousbtower.c
> @@ -926,6 +926,7 @@ static int tower_probe (struct usb_interface *interface,
> const struct usb_device
>USB_MAJOR, dev->minor);
>
> exit:
> + kfree(get_version_reply);
> return retval;
>
> error:
> --
> 2.9.3
Thanks,
--
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: misc: legousbtower: Fix memory leak
get_version_reply is not freed if function returns with success. Memory leak was introduced by commit 942a48730faf149ccbf3e12ac718aee120bb3529 Signed-off-by: Heikki Krogerus Signed-off-by: Maksim Salau --- drivers/usb/misc/legousbtower.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c index aa3c280..0782ac6 100644 --- a/drivers/usb/misc/legousbtower.c +++ b/drivers/usb/misc/legousbtower.c @@ -926,6 +926,7 @@ static int tower_probe (struct usb_interface *interface, const struct usb_device USB_MAJOR, dev->minor); exit: + kfree(get_version_reply); return retval; error: -- 2.9.3 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] usb: misc: legousbtower: Fix buffers on stack
> > @@ -913,6 +929,7 @@ static int tower_probe (struct usb_interface > > *interface, const struct usb_device > > Don't you need to free get_version_reply here? > > > return retval; > > > > error: > > + kfree(get_version_reply); > > tower_delete(dev); > > return retval; > > } Thank you very much, Heikki! I was so focused on failure cases, that missed memory leak in the case when all calls succeeded. I'll prepare a patch shortly to fix this. Regards, Maksim. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] usb: misc: legousbtower: Fix buffers on stack
Hi Maksim,
Sorry for commenting this so late but..
On Tue, Apr 25, 2017 at 10:49:21PM +0300, Maksim Salau wrote:
> @@ -806,7 +814,7 @@ static int tower_probe (struct usb_interface *interface,
> const struct usb_device
> struct device *idev = &interface->dev;
> struct usb_device *udev = interface_to_usbdev(interface);
> struct lego_usb_tower *dev = NULL;
> - struct tower_get_version_reply get_version_reply;
> + struct tower_get_version_reply *get_version_reply = NULL;
> int retval = -ENOMEM;
> int result;
>
> @@ -871,6 +879,13 @@ static int tower_probe (struct usb_interface *interface,
> const struct usb_device
> dev->interrupt_in_interval = interrupt_in_interval ?
> interrupt_in_interval : dev->interrupt_in_endpoint->bInterval;
> dev->interrupt_out_interval = interrupt_out_interval ?
> interrupt_out_interval : dev->interrupt_out_endpoint->bInterval;
>
> + get_version_reply = kmalloc(sizeof(*get_version_reply), GFP_KERNEL);
> +
> + if (!get_version_reply) {
> + retval = -ENOMEM;
> + goto error;
> + }
> +
> /* get the firmware version and log it */
> result = usb_control_msg (udev,
> usb_rcvctrlpipe(udev, 0),
> @@ -878,18 +893,19 @@ static int tower_probe (struct usb_interface
> *interface, const struct usb_device
> USB_TYPE_VENDOR | USB_DIR_IN |
> USB_RECIP_DEVICE,
> 0,
> 0,
> - &get_version_reply,
> - sizeof(get_version_reply),
> + get_version_reply,
> + sizeof(*get_version_reply),
> 1000);
> if (result < 0) {
> dev_err(idev, "LEGO USB Tower get version control request
> failed\n");
> retval = result;
> goto error;
> }
> - dev_info(&interface->dev, "LEGO USB Tower firmware version is %d.%d "
> - "build %d\n", get_version_reply.major,
> - get_version_reply.minor,
> - le16_to_cpu(get_version_reply.build_no));
> + dev_info(&interface->dev,
> + "LEGO USB Tower firmware version is %d.%d build %d\n",
> + get_version_reply->major,
> + get_version_reply->minor,
> + le16_to_cpu(get_version_reply->build_no));
>
> /* we can register the device now, as it is ready */
> usb_set_intfdata (interface, dev);
> @@ -913,6 +929,7 @@ static int tower_probe (struct usb_interface *interface,
> const struct usb_device
Don't you need to free get_version_reply here?
> return retval;
>
> error:
> + kfree(get_version_reply);
> tower_delete(dev);
> return retval;
> }
Thanks,
--
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] usb: misc: legousbtower: Fix buffers on stack
> >* removed Tested-by: Alfredo Rafael Vicente Boix ; > > I added this back, as it matters, and your change from the previous > version was trivial. > > >* removed Cc: sta...@vger.kernel.org > > since this patch doesn't apply against v4.10.12 > > I added this back as well :) Thanks, Greg! I was not sure about how strict are the rules about these tags. Maksim. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] usb: misc: legousbtower: Fix buffers on stack
On Tue, Apr 25, 2017 at 10:49:21PM +0300, Maksim Salau wrote: > Allocate buffers on HEAP instead of STACK for local structures > that are to be received using usb_control_msg(). > > Signed-off-by: Maksim Salau > Tested-by: Alfredo Rafael Vicente Boix ; > Cc: stable > > --- > Changes in v3: >* rebased against usb-next; >* removed Tested-by: Alfredo Rafael Vicente Boix ; I added this back, as it matters, and your change from the previous version was trivial. >* removed Cc: sta...@vger.kernel.org > since this patch doesn't apply against v4.10.12 I added this back as well :) thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] usb: misc: legousbtower: Fix buffers on stack
On Tue, Apr 25, 2017 at 10:28:42PM +0300, Maksim Salau wrote: > On Tue, 25 Apr 2017 20:04:24 +0200 > Greg Kroah-Hartman wrote: > > > On Sat, Apr 22, 2017 at 07:24:37PM +0300, Maksim Salau wrote: > > > Allocate buffers on HEAP instead of STACK for local structures > > > that are to be received using usb_control_msg(). > > > > > > Signed-off-by: Maksim Salau > > > Tested-by: Alfredo Rafael Vicente Boix > > > Cc: sta...@vger.kernel.org > > > > This patch does not apply to usb-next, what did you make it against? > > > > Can you rebase it and resend? > > > > thanks, > > > > greg k-h > > Hi Greg, > > I've created the patch against linux-stable v4.10.12. I've checked history > of the driver and it turns out that the patch conflicts with > 9b181166f17534a82b4b628b13e524a893715dfc > > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/drivers/usb/misc/legousbtower.c?h=usb-next&id=9b181166f17534a82b4b628b13e524a893715dfc > > There are no conflicting changes, but context has changed. > I'll rebase and resend shortly. > > The same patch can't be used for stable and for usb-next anymore. Is there > any chance to backport the fix for stable? Yes, we can backport it, I'll mark it for stable and do the backport when it hits Linus's tree after 4.12-rc1 is out. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3] usb: misc: legousbtower: Fix buffers on stack
Allocate buffers on HEAP instead of STACK for local structures
that are to be received using usb_control_msg().
Signed-off-by: Maksim Salau
---
Changes in v3:
* rebased against usb-next;
* removed Tested-by: Alfredo Rafael Vicente Boix ;
* removed Cc: sta...@vger.kernel.org
since this patch doesn't apply against v4.10.12
Changes in v2:
* made checkpatch happy with the format string passed to dev_info
in tower_probe() (merged two parts into a single string literal);
* changed commit message to better reflect location of the module;
was: USB: legousbtower: Fix buffers on stack;
* added Tested-by: Alfredo Rafael Vicente Boix
and Cc: sta...@vger.kernel.org
drivers/usb/misc/legousbtower.c | 37 +++--
1 file changed, 27 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 201c9c3..aa3c280 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -317,9 +317,16 @@ static int tower_open (struct inode *inode, struct file
*file)
int subminor;
int retval = 0;
struct usb_interface *interface;
- struct tower_reset_reply reset_reply;
+ struct tower_reset_reply *reset_reply;
int result;
+ reset_reply = kmalloc(sizeof(*reset_reply), GFP_KERNEL);
+
+ if (!reset_reply) {
+ retval = -ENOMEM;
+ goto exit;
+ }
+
nonseekable_open(inode, file);
subminor = iminor(inode);
@@ -364,8 +371,8 @@ static int tower_open (struct inode *inode, struct file
*file)
USB_TYPE_VENDOR | USB_DIR_IN |
USB_RECIP_DEVICE,
0,
0,
- &reset_reply,
- sizeof(reset_reply),
+ reset_reply,
+ sizeof(*reset_reply),
1000);
if (result < 0) {
dev_err(&dev->udev->dev,
@@ -406,6 +413,7 @@ static int tower_open (struct inode *inode, struct file
*file)
mutex_unlock(&dev->lock);
exit:
+ kfree(reset_reply);
return retval;
}
@@ -806,7 +814,7 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
struct device *idev = &interface->dev;
struct usb_device *udev = interface_to_usbdev(interface);
struct lego_usb_tower *dev = NULL;
- struct tower_get_version_reply get_version_reply;
+ struct tower_get_version_reply *get_version_reply = NULL;
int retval = -ENOMEM;
int result;
@@ -871,6 +879,13 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
dev->interrupt_in_interval = interrupt_in_interval ?
interrupt_in_interval : dev->interrupt_in_endpoint->bInterval;
dev->interrupt_out_interval = interrupt_out_interval ?
interrupt_out_interval : dev->interrupt_out_endpoint->bInterval;
+ get_version_reply = kmalloc(sizeof(*get_version_reply), GFP_KERNEL);
+
+ if (!get_version_reply) {
+ retval = -ENOMEM;
+ goto error;
+ }
+
/* get the firmware version and log it */
result = usb_control_msg (udev,
usb_rcvctrlpipe(udev, 0),
@@ -878,18 +893,19 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
USB_TYPE_VENDOR | USB_DIR_IN |
USB_RECIP_DEVICE,
0,
0,
- &get_version_reply,
- sizeof(get_version_reply),
+ get_version_reply,
+ sizeof(*get_version_reply),
1000);
if (result < 0) {
dev_err(idev, "LEGO USB Tower get version control request
failed\n");
retval = result;
goto error;
}
- dev_info(&interface->dev, "LEGO USB Tower firmware version is %d.%d "
-"build %d\n", get_version_reply.major,
-get_version_reply.minor,
-le16_to_cpu(get_version_reply.build_no));
+ dev_info(&interface->dev,
+"LEGO USB Tower firmware version is %d.%d build %d\n",
+get_version_reply->major,
+get_version_reply->minor,
+le16_to_cpu(get_version_reply->build_no));
/* we can register the device now, as it is ready */
usb_set_intfdata (interface, dev);
@@ -913,6 +929,7 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
return retval;
error:
+
Re: [PATCH v2] usb: misc: legousbtower: Fix buffers on stack
On Tue, 25 Apr 2017 20:04:24 +0200 Greg Kroah-Hartman wrote: > On Sat, Apr 22, 2017 at 07:24:37PM +0300, Maksim Salau wrote: > > Allocate buffers on HEAP instead of STACK for local structures > > that are to be received using usb_control_msg(). > > > > Signed-off-by: Maksim Salau > > Tested-by: Alfredo Rafael Vicente Boix > > Cc: sta...@vger.kernel.org > > This patch does not apply to usb-next, what did you make it against? > > Can you rebase it and resend? > > thanks, > > greg k-h Hi Greg, I've created the patch against linux-stable v4.10.12. I've checked history of the driver and it turns out that the patch conflicts with 9b181166f17534a82b4b628b13e524a893715dfc https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/drivers/usb/misc/legousbtower.c?h=usb-next&id=9b181166f17534a82b4b628b13e524a893715dfc There are no conflicting changes, but context has changed. I'll rebase and resend shortly. The same patch can't be used for stable and for usb-next anymore. Is there any chance to backport the fix for stable? Regards, Maksim. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] usb: misc: legousbtower: Fix buffers on stack
On Sat, Apr 22, 2017 at 07:24:37PM +0300, Maksim Salau wrote: > Allocate buffers on HEAP instead of STACK for local structures > that are to be received using usb_control_msg(). > > Signed-off-by: Maksim Salau > Tested-by: Alfredo Rafael Vicente Boix > Cc: sta...@vger.kernel.org This patch does not apply to usb-next, what did you make it against? Can you rebase it and resend? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] usb: misc: legousbtower: Fix buffers on stack
Allocate buffers on HEAP instead of STACK for local structures
that are to be received using usb_control_msg().
Signed-off-by: Maksim Salau
Tested-by: Alfredo Rafael Vicente Boix
Cc: sta...@vger.kernel.org
---
Changes in v2:
* made checkpatch happy with the format string passed to dev_info
in tower_probe() (merged two parts into a single string literal);
* changed commit message to better reflect location of the module;
was: USB: legousbtower: Fix buffers on stack;
* added Tested-by: Alfredo Rafael Vicente Boix
and Cc: sta...@vger.kernel.org
drivers/usb/misc/legousbtower.c | 37 +++--
1 file changed, 27 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index b10e26c..ed1999e 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -317,9 +317,16 @@ static int tower_open (struct inode *inode, struct file
*file)
int subminor;
int retval = 0;
struct usb_interface *interface;
- struct tower_reset_reply reset_reply;
+ struct tower_reset_reply *reset_reply = NULL;
int result;
+ reset_reply = kmalloc(sizeof(*reset_reply), GFP_KERNEL);
+
+ if (!reset_reply) {
+ retval = -ENOMEM;
+ goto exit;
+ }
+
nonseekable_open(inode, file);
subminor = iminor(inode);
@@ -364,8 +371,8 @@ static int tower_open (struct inode *inode, struct file
*file)
USB_TYPE_VENDOR | USB_DIR_IN |
USB_RECIP_DEVICE,
0,
0,
- &reset_reply,
- sizeof(reset_reply),
+ reset_reply,
+ sizeof(*reset_reply),
1000);
if (result < 0) {
dev_err(&dev->udev->dev,
@@ -406,6 +413,7 @@ static int tower_open (struct inode *inode, struct file
*file)
mutex_unlock(&dev->lock);
exit:
+ kfree(reset_reply);
return retval;
}
@@ -808,7 +816,7 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
struct lego_usb_tower *dev = NULL;
struct usb_host_interface *iface_desc;
struct usb_endpoint_descriptor* endpoint;
- struct tower_get_version_reply get_version_reply;
+ struct tower_get_version_reply *get_version_reply = NULL;
int i;
int retval = -ENOMEM;
int result;
@@ -886,6 +894,13 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
dev->interrupt_in_interval = interrupt_in_interval ?
interrupt_in_interval : dev->interrupt_in_endpoint->bInterval;
dev->interrupt_out_interval = interrupt_out_interval ?
interrupt_out_interval : dev->interrupt_out_endpoint->bInterval;
+ get_version_reply = kmalloc(sizeof(*get_version_reply), GFP_KERNEL);
+
+ if (!get_version_reply) {
+ retval = -ENOMEM;
+ goto error;
+ }
+
/* get the firmware version and log it */
result = usb_control_msg (udev,
usb_rcvctrlpipe(udev, 0),
@@ -893,18 +908,19 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
USB_TYPE_VENDOR | USB_DIR_IN |
USB_RECIP_DEVICE,
0,
0,
- &get_version_reply,
- sizeof(get_version_reply),
+ get_version_reply,
+ sizeof(*get_version_reply),
1000);
if (result < 0) {
dev_err(idev, "LEGO USB Tower get version control request
failed\n");
retval = result;
goto error;
}
- dev_info(&interface->dev, "LEGO USB Tower firmware version is %d.%d "
-"build %d\n", get_version_reply.major,
-get_version_reply.minor,
-le16_to_cpu(get_version_reply.build_no));
+ dev_info(&interface->dev,
+"LEGO USB Tower firmware version is %d.%d build %d\n",
+get_version_reply->major,
+get_version_reply->minor,
+le16_to_cpu(get_version_reply->build_no));
/* we can register the device now, as it is ready */
usb_set_intfdata (interface, dev);
@@ -928,6 +944,7 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
return retval;
error:
+ kfree(get_version_reply);
tower_delete(dev);
return retval;
}
--
2.9.3
--
To unsubscribe from this list: send the line "unsubs
Re: [PATCH] USB: legousbtower: Fix buffers on stack
Hello, I have tested it and it works great!!! Thank you very much!! I use nqc to test it and it has uploaded the firmware to the brick perfectly. [ 164.835935] usb 4-5: new low-speed USB device number 3 using ohci-pci [ 165.050272] usb 4-5: New USB device found, idVendor=0694, idProduct=0001 [ 165.050276] usb 4-5: New USB device strings: Mfr=4, Product=26, SerialNumber=0 [ 165.050278] usb 4-5: Product: LEGO USB Tower [ 165.050280] usb 4-5: Manufacturer: LEGO Group [ 165.055275] legousbtower 4-5:1.0: LEGO USB Tower firmware version is 1.0 build 134 [ 165.055481] legousbtower 4-5:1.0: LEGO USB Tower #-160 now attached to major 180 minor 0 HOME Escritorio # nqc -Susb -firmware ./firm0332.lgo Downloading firmware:. Current Version: 00030001/00030302 HOME Escritorio # uname -a Linux HOME 4.10.12 #1 SMP Sat Apr 22 09:30:06 CEST 2017 x86_64 x86_64 x86_64 GNU/Linux Thank you very much 2017-04-22 7:03 GMT+02:00 Greg Kroah-Hartman : > On Fri, Apr 21, 2017 at 10:48:48PM +0300, Maksim Salau wrote: >> Allocate buffers on HEAP instead of STACK for local structures >> that are to be received using usb_control_msg(). >> >> Signed-off-by: Maksim Salau >> --- >> >> I took the liberty to fix the module if Greg don't mind. >> It is to be applied on vanilla v4.10.12 (without Greg's patch). > > Yeah! Thank you for this, I got distracted with stable kernel work for > the past few days. > >> Changes compared to Greg's version: >> * fixed tower_reset() which is used in the open callback; >> * better deallocation handling in case of failures. > > Alfredo, can you test this patch out? > > thanks again, > > greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: legousbtower: Fix buffers on stack
On Fri, Apr 21, 2017 at 10:48:48PM +0300, Maksim Salau wrote: > Allocate buffers on HEAP instead of STACK for local structures > that are to be received using usb_control_msg(). > > Signed-off-by: Maksim Salau > --- > > I took the liberty to fix the module if Greg don't mind. > It is to be applied on vanilla v4.10.12 (without Greg's patch). Yeah! Thank you for this, I got distracted with stable kernel work for the past few days. > Changes compared to Greg's version: > * fixed tower_reset() which is used in the open callback; > * better deallocation handling in case of failures. Alfredo, can you test this patch out? thanks again, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] USB: legousbtower: Fix buffers on stack
Allocate buffers on HEAP instead of STACK for local structures
that are to be received using usb_control_msg().
Signed-off-by: Maksim Salau
---
I took the liberty to fix the module if Greg don't mind.
It is to be applied on vanilla v4.10.12 (without Greg's patch).
Changes compared to Greg's version:
* fixed tower_reset() which is used in the open callback;
* better deallocation handling in case of failures.
BTW, using the following grep magic I've discovered 3 more
placed to be fixed:
$ grep --include=*.c -PIzor
"(?s)usb_control_msg\s*\([^;]+,\s*&?(\w+)\s*,\s*sizeof\(\1\)[^;]+\);[^\n]*\n"
video/fbdev/udlfb.c:usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
NR_USB_REQUEST_CHANNEL,
(USB_DIR_OUT | USB_TYPE_VENDOR), 0, 0,
set_def_chn, sizeof(set_def_chn), USB_CTRL_SET_TIMEOUT);
net/can/usb/gs_usb.c:usb_control_msg(interface_to_usbdev(dev->iface),
usb_sndctrlpipe(interface_to_usbdev(dev->iface),
0),
GS_USB_BREQ_IDENTIFY,
USB_DIR_OUT | USB_TYPE_VENDOR |
USB_RECIP_INTERFACE,
dev->channel,
0,
&imode,
sizeof(imode),
100);
net/wireless/intersil/orinoco/orinoco_usb.c:usb_control_msg(upriv->udev,
usb_sndctrlpipe(upriv->udev, 0),
EZUSB_REQUEST_FW_TRANS,
USB_TYPE_VENDOR | USB_RECIP_DEVICE |
USB_DIR_OUT, EZUSB_CPUCS_REG, 0, &res_val,
sizeof(res_val), DEF_TIMEOUT);
Here are 1 case with a local buffer and 2 cases with local variables
sent with usb_control_msg. I'll prepare fixes for those issues shortly.
Best regards,
Maksim.
drivers/usb/misc/legousbtower.c | 34 +-
1 file changed, 25 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 322a042..5747615c 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -317,9 +317,16 @@ static int tower_open (struct inode *inode, struct file
*file)
int subminor;
int retval = 0;
struct usb_interface *interface;
- struct tower_reset_reply reset_reply;
+ struct tower_reset_reply *reset_reply = NULL;
int result;
+ reset_reply = kmalloc(sizeof(*reset_reply), GFP_KERNEL);
+
+ if (!reset_reply) {
+ retval = -ENOMEM;
+ goto exit;
+ }
+
nonseekable_open(inode, file);
subminor = iminor(inode);
@@ -364,8 +371,8 @@ static int tower_open (struct inode *inode, struct file
*file)
USB_TYPE_VENDOR | USB_DIR_IN |
USB_RECIP_DEVICE,
0,
0,
- &reset_reply,
- sizeof(reset_reply),
+ reset_reply,
+ sizeof(*reset_reply),
1000);
if (result < 0) {
dev_err(&dev->udev->dev,
@@ -406,6 +413,7 @@ static int tower_open (struct inode *inode, struct file
*file)
mutex_unlock(&dev->lock);
exit:
+ kfree(reset_reply);
return retval;
}
@@ -808,7 +816,7 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
struct lego_usb_tower *dev = NULL;
struct usb_host_interface *iface_desc;
struct usb_endpoint_descriptor* endpoint;
- struct tower_get_version_reply get_version_reply;
+ struct tower_get_version_reply *get_version_reply = NULL;
int i;
int retval = -ENOMEM;
int result;
@@ -886,6 +894,13 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
dev->interrupt_in_interval = interrupt_in_interval ?
interrupt_in_interval : dev->interrupt_in_endpoint->bInterval;
dev->interrupt_out_interval = interrupt_out_interval ?
interrupt_out_interval : dev->interrupt_out_endpoint->bInterval;
+ get_version_reply = kmalloc(sizeof(*get_version_reply), GFP_KERNEL);
+
+ if (!get_version_reply) {
+ retval = -ENOMEM;
+ goto error;
+ }
+
/* get the firmware version and log it */
result = usb_control_msg (udev,
usb_rcvctrlpipe(udev, 0),
@@ -893,8 +908,8 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
USB_TYPE_VENDOR | USB_DIR_IN |
USB_RECIP_DEVICE,
0,
0,
- &get_v
Re: legousbtower
On Thu, Apr 20, 2017 at 05:21:22PM +0200, Greg KH wrote: > On Thu, Apr 20, 2017 at 04:43:49PM +0200, Alfredo Rafael Vicente Boix wrote: > > Hello, > > > > I have tried your patch and it works fine. It seems that it recognised > > the tower and also creates the path /dev/usb/legousbtower0. > > > > [ 784.308089] usb 8-2: new low-speed USB device number 4 using uhci_hcd > > [ 784.529202] usb 8-2: New USB device found, idVendor=0694, idProduct=0001 > > [ 784.529205] usb 8-2: New USB device strings: Mfr=4, Product=26, > > SerialNumber=0 > > [ 784.529208] usb 8-2: Product: LEGO USB Tower > > [ 784.529210] usb 8-2: Manufacturer: LEGO Group > > [ 784.537094] legousbtower 8-2:1.0: LEGO USB Tower firmware version > > is 1.0 build 134 > > [ 784.538146] legousbtower 8-2:1.0: LEGO USB Tower #-160 now attached > > to major 180 minor 0 > > > > But when I tried to access it with nqc (sudo nqc > > -S/dev/usb/legousbtower0 -firmware ./firm0332.lgo), for example, it > > gives me the following error: > > > > Could not open serial port or USB device. > > > > with the log: > > > > [ 784.538146] legousbtower 8-2:1.0: LEGO USB Tower #-160 now attached > > to major 180 minor 0 > > [ 994.013492] usb 8-2: LEGO USB Tower reset control request failed > > Oh good! That means you must have the VIRTUAL STACK configuration > option enabled. The driver is trying to send USB messages off of the > stack instead of dynamically allocating them. > > This request error is also a symptom of that. I'll work on a patch for > all of the parts of this driver that have that issue and send it to you > in a bit... Got sidetracked by real-life, it's late here now, will get to this in the morning, thanks. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: legousbtower
On Thu, Apr 20, 2017 at 04:43:49PM +0200, Alfredo Rafael Vicente Boix wrote: > Hello, > > I have tried your patch and it works fine. It seems that it recognised > the tower and also creates the path /dev/usb/legousbtower0. > > [ 784.308089] usb 8-2: new low-speed USB device number 4 using uhci_hcd > [ 784.529202] usb 8-2: New USB device found, idVendor=0694, idProduct=0001 > [ 784.529205] usb 8-2: New USB device strings: Mfr=4, Product=26, > SerialNumber=0 > [ 784.529208] usb 8-2: Product: LEGO USB Tower > [ 784.529210] usb 8-2: Manufacturer: LEGO Group > [ 784.537094] legousbtower 8-2:1.0: LEGO USB Tower firmware version > is 1.0 build 134 > [ 784.538146] legousbtower 8-2:1.0: LEGO USB Tower #-160 now attached > to major 180 minor 0 > > But when I tried to access it with nqc (sudo nqc > -S/dev/usb/legousbtower0 -firmware ./firm0332.lgo), for example, it > gives me the following error: > > Could not open serial port or USB device. > > with the log: > > [ 784.538146] legousbtower 8-2:1.0: LEGO USB Tower #-160 now attached > to major 180 minor 0 > [ 994.013492] usb 8-2: LEGO USB Tower reset control request failed Oh good! That means you must have the VIRTUAL STACK configuration option enabled. The driver is trying to send USB messages off of the stack instead of dynamically allocating them. This request error is also a symptom of that. I'll work on a patch for all of the parts of this driver that have that issue and send it to you in a bit... thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: legousbtower
Hello,
I have tried your patch and it works fine. It seems that it recognised
the tower and also creates the path /dev/usb/legousbtower0.
[ 784.308089] usb 8-2: new low-speed USB device number 4 using uhci_hcd
[ 784.529202] usb 8-2: New USB device found, idVendor=0694, idProduct=0001
[ 784.529205] usb 8-2: New USB device strings: Mfr=4, Product=26,
SerialNumber=0
[ 784.529208] usb 8-2: Product: LEGO USB Tower
[ 784.529210] usb 8-2: Manufacturer: LEGO Group
[ 784.537094] legousbtower 8-2:1.0: LEGO USB Tower firmware version
is 1.0 build 134
[ 784.538146] legousbtower 8-2:1.0: LEGO USB Tower #-160 now attached
to major 180 minor 0
But when I tried to access it with nqc (sudo nqc
-S/dev/usb/legousbtower0 -firmware ./firm0332.lgo), for example, it
gives me the following error:
Could not open serial port or USB device.
with the log:
[ 784.538146] legousbtower 8-2:1.0: LEGO USB Tower #-160 now attached
to major 180 minor 0
[ 994.013492] usb 8-2: LEGO USB Tower reset control request failed
I'm testing with kernel last stable kernel (4.10).
Alfredo
2017-04-20 14:10 GMT+02:00 Greg KH :
> On Thu, Apr 20, 2017 at 01:29:16PM +0200, Alfredo Rafael Vicente Boix wrote:
>> Hello,
>>
>> Thank you for your quick answer. I just have compiled the last stable
>> kernel and I'm having the same issue. This is Lliurex (based in
>> ubuntu)
>>
>> [ 2016.280112] usb 8-2: USB disconnect, device number 2
>> [ 2017.944091] usb 8-2: new low-speed USB device number 3 using uhci_hcd
>> [ 2018.165147] usb 8-2: New USB device found, idVendor=0694, idProduct=0001
>> [ 2018.165151] usb 8-2: New USB device strings: Mfr=4, Product=26,
>> SerialNumber=0
>> [ 2018.165153] usb 8-2: Product: LEGO USB Tower
>> [ 2018.165155] usb 8-2: Manufacturer: LEGO Group
>> [ 2018.168352] legousbtower 8-2:1.0: LEGO USB Tower get version
>> control request failed
>
> Ok, that is odd.
>
>> [ 2018.168361] legousbtower: probe of 8-2:1.0 failed with error -11
>
> And -11 is also probably not the right error to return here -EAGAIN
> doesn't make sense...
>
> But, this did find at least one bug in the driver, where data was being
> sent to the device off of the stack. Can you try the patch below to see
> if that fixes anything or not?
>
> It's not a "complete" patch, but should be good enough for testing.
>
> If that doesn't work, I don't see why asking for the firmware
> information is required, we should be able to just continue on if this
> fails. There was some reorginization done in this area of the code a
> few versions back, but I don't see how that could affect this device
> (famous last words...)
>
> thanks,
>
> greg k-h
>
> -
>
> diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
> index 322a042d6e59..aac51caa55b2 100644
> --- a/drivers/usb/misc/legousbtower.c
> +++ b/drivers/usb/misc/legousbtower.c
> @@ -808,7 +808,7 @@ static int tower_probe (struct usb_interface *interface,
> const struct usb_device
> struct lego_usb_tower *dev = NULL;
> struct usb_host_interface *iface_desc;
> struct usb_endpoint_descriptor* endpoint;
> - struct tower_get_version_reply get_version_reply;
> + struct tower_get_version_reply *get_version_reply;
> int i;
> int retval = -ENOMEM;
> int result;
> @@ -886,6 +886,7 @@ static int tower_probe (struct usb_interface *interface,
> const struct usb_device
> dev->interrupt_in_interval = interrupt_in_interval ?
> interrupt_in_interval : dev->interrupt_in_endpoint->bInterval;
> dev->interrupt_out_interval = interrupt_out_interval ?
> interrupt_out_interval : dev->interrupt_out_endpoint->bInterval;
>
> + get_version_reply = kmalloc(sizeof(*get_version_reply), GFP_KERNEL);
> /* get the firmware version and log it */
> result = usb_control_msg (udev,
> usb_rcvctrlpipe(udev, 0),
> @@ -893,8 +894,8 @@ static int tower_probe (struct usb_interface *interface,
> const struct usb_device
> USB_TYPE_VENDOR | USB_DIR_IN |
> USB_RECIP_DEVICE,
> 0,
> 0,
> - &get_version_reply,
> - sizeof(get_version_reply),
> + get_version_reply,
> + sizeof(*get_version_reply),
> 1000);
> if (result < 0) {
> dev_err(idev, "LEGO USB Tower get version control request
> failed\n");
> @@ -902,9 +9
Re: legousbtower
On Thu, Apr 20, 2017 at 01:29:16PM +0200, Alfredo Rafael Vicente Boix wrote:
> Hello,
>
> Thank you for your quick answer. I just have compiled the last stable
> kernel and I'm having the same issue. This is Lliurex (based in
> ubuntu)
>
> [ 2016.280112] usb 8-2: USB disconnect, device number 2
> [ 2017.944091] usb 8-2: new low-speed USB device number 3 using uhci_hcd
> [ 2018.165147] usb 8-2: New USB device found, idVendor=0694, idProduct=0001
> [ 2018.165151] usb 8-2: New USB device strings: Mfr=4, Product=26,
> SerialNumber=0
> [ 2018.165153] usb 8-2: Product: LEGO USB Tower
> [ 2018.165155] usb 8-2: Manufacturer: LEGO Group
> [ 2018.168352] legousbtower 8-2:1.0: LEGO USB Tower get version
> control request failed
Ok, that is odd.
> [ 2018.168361] legousbtower: probe of 8-2:1.0 failed with error -11
And -11 is also probably not the right error to return here -EAGAIN
doesn't make sense...
But, this did find at least one bug in the driver, where data was being
sent to the device off of the stack. Can you try the patch below to see
if that fixes anything or not?
It's not a "complete" patch, but should be good enough for testing.
If that doesn't work, I don't see why asking for the firmware
information is required, we should be able to just continue on if this
fails. There was some reorginization done in this area of the code a
few versions back, but I don't see how that could affect this device
(famous last words...)
thanks,
greg k-h
-
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 322a042d6e59..aac51caa55b2 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -808,7 +808,7 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
struct lego_usb_tower *dev = NULL;
struct usb_host_interface *iface_desc;
struct usb_endpoint_descriptor* endpoint;
- struct tower_get_version_reply get_version_reply;
+ struct tower_get_version_reply *get_version_reply;
int i;
int retval = -ENOMEM;
int result;
@@ -886,6 +886,7 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
dev->interrupt_in_interval = interrupt_in_interval ?
interrupt_in_interval : dev->interrupt_in_endpoint->bInterval;
dev->interrupt_out_interval = interrupt_out_interval ?
interrupt_out_interval : dev->interrupt_out_endpoint->bInterval;
+ get_version_reply = kmalloc(sizeof(*get_version_reply), GFP_KERNEL);
/* get the firmware version and log it */
result = usb_control_msg (udev,
usb_rcvctrlpipe(udev, 0),
@@ -893,8 +894,8 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
USB_TYPE_VENDOR | USB_DIR_IN |
USB_RECIP_DEVICE,
0,
0,
- &get_version_reply,
- sizeof(get_version_reply),
+ get_version_reply,
+ sizeof(*get_version_reply),
1000);
if (result < 0) {
dev_err(idev, "LEGO USB Tower get version control request
failed\n");
@@ -902,9 +903,10 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
goto error;
}
dev_info(&interface->dev, "LEGO USB Tower firmware version is %d.%d "
-"build %d\n", get_version_reply.major,
-get_version_reply.minor,
-le16_to_cpu(get_version_reply.build_no));
+"build %d\n", get_version_reply->major,
+get_version_reply->minor,
+le16_to_cpu(get_version_reply->build_no));
+ kfree(get_version_reply);
/* we can register the device now, as it is ready */
usb_set_intfdata (interface, dev);
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: legousbtower
Hello, Thank you for your quick answer. I just have compiled the last stable kernel and I'm having the same issue. This is Lliurex (based in ubuntu) [ 2016.280112] usb 8-2: USB disconnect, device number 2 [ 2017.944091] usb 8-2: new low-speed USB device number 3 using uhci_hcd [ 2018.165147] usb 8-2: New USB device found, idVendor=0694, idProduct=0001 [ 2018.165151] usb 8-2: New USB device strings: Mfr=4, Product=26, SerialNumber=0 [ 2018.165153] usb 8-2: Product: LEGO USB Tower [ 2018.165155] usb 8-2: Manufacturer: LEGO Group [ 2018.168352] legousbtower 8-2:1.0: LEGO USB Tower get version control request failed [ 2018.168361] legousbtower: probe of 8-2:1.0 failed with error -11 lliurex@MACROHPCompaq:~$ uname -a Linux MACROHPCompaq 4.10.11 #1 SMP Wed Apr 19 14:55:04 CEST 2017 x86_64 x86_64 x86_64 GNU/Linux I also tried in a different computer with manjaro (based in arch): [ 1613.925559] legousbtower 2-2:1.0: LEGO USB Tower get version control request failed [ 1613.925569] legousbtower: probe of 2-2:1.0 failed with error -11 [alviboi@portatil ~]$ uname -a Linux portatil 4.10.10-1-MANJARO #1 SMP PREEMPT Wed Apr 12 21:35:07 UTC 2017 x86_64 GNU/Linux 2017-04-20 10:17 GMT+02:00 Greg KH : > On Thu, Apr 20, 2017 at 09:42:50AM +0200, Alfredo Rafael Vicente Boix wrote: >> Hello, >> >> My name is Alfredo from Spain, I'm a secondary school teacher and I >> worked with my students with the lego usb tower, I write you in >> reference to the following commit >> (2fae9e5a7babada041e2e161699ade2447a01989): >> >> patch: 9363741 >> >> I have a little problem with the legousbtower module of kernel. I was >> working with kernel 4.4 and I had no problem with this module, each >> time that I connected the tower I have the following message in my >> log: >> >> [ 4927.822527] usb 3-1: new low-speed USB device number 2 using xhci_hcd >> [ 4928.022168] usb 3-1: New USB device found, idVendor=0694, idProduct=0001 >> [ 4928.022185] usb 3-1: New USB device strings: Mfr=4, Product=26, >> SerialNumber=0 >> [ 4928.022188] usb 3-1: Product: LEGO USB Tower >> [ 4928.022190] usb 3-1: Manufacturer: LEGO Group >> [ 4929.914818] legousbtower 3-1:1.0: LEGO USB Tower firmware version >> is 1.0 build 134 >> [ 4929.915157] legousbtower 3-1:1.0: LEGO USB Tower #-160 now attached >> to major 180 minor 0 >> [ 4929.915517] usbcore: registered new interface driver legousbtower >> >> >> And I can use nqc without any problems. >> >> Since kernel 4.9 (I've tested it with kernel 4.9 and 4.10), I'm having >> the same issue. Each time that I connect the legousbtower I have this >> error: >> >> [ 51.036609] usb 1-2: USB disconnect, device number 5 >> [ 53.025125] usb 1-2: new low-speed USB device number 6 using xhci_hcd >> [ 53.173087] legousbtower 1-2:1.0: LEGO USB Tower get version >> control request failed > > This looks like a xhci bug/issue, and not specifically a legousbtower > issue. > > What exact kernel release numbers did you try? Can you try the latest > 4.10.11 release? There have been a number of xhci bugfixes recently > (since 4.9.0) that hopefully should resolve issues like this. > > Or you can try 4.9.23, that too should have the issues fixed. > > Please let us know if that works or not. > > thanks, > > greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: legousbtower
On Thu, Apr 20, 2017 at 09:42:50AM +0200, Alfredo Rafael Vicente Boix wrote: > Hello, > > My name is Alfredo from Spain, I'm a secondary school teacher and I > worked with my students with the lego usb tower, I write you in > reference to the following commit > (2fae9e5a7babada041e2e161699ade2447a01989): > > patch: 9363741 > > I have a little problem with the legousbtower module of kernel. I was > working with kernel 4.4 and I had no problem with this module, each > time that I connected the tower I have the following message in my > log: > > [ 4927.822527] usb 3-1: new low-speed USB device number 2 using xhci_hcd > [ 4928.022168] usb 3-1: New USB device found, idVendor=0694, idProduct=0001 > [ 4928.022185] usb 3-1: New USB device strings: Mfr=4, Product=26, > SerialNumber=0 > [ 4928.022188] usb 3-1: Product: LEGO USB Tower > [ 4928.022190] usb 3-1: Manufacturer: LEGO Group > [ 4929.914818] legousbtower 3-1:1.0: LEGO USB Tower firmware version > is 1.0 build 134 > [ 4929.915157] legousbtower 3-1:1.0: LEGO USB Tower #-160 now attached > to major 180 minor 0 > [ 4929.915517] usbcore: registered new interface driver legousbtower > > > And I can use nqc without any problems. > > Since kernel 4.9 (I've tested it with kernel 4.9 and 4.10), I'm having > the same issue. Each time that I connect the legousbtower I have this > error: > > [ 51.036609] usb 1-2: USB disconnect, device number 5 > [ 53.025125] usb 1-2: new low-speed USB device number 6 using xhci_hcd > [ 53.173087] legousbtower 1-2:1.0: LEGO USB Tower get version > control request failed This looks like a xhci bug/issue, and not specifically a legousbtower issue. What exact kernel release numbers did you try? Can you try the latest 4.10.11 release? There have been a number of xhci bugfixes recently (since 4.9.0) that hopefully should resolve issues like this. Or you can try 4.9.23, that too should have the issues fixed. Please let us know if that works or not. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
legousbtower
Hello, My name is Alfredo from Spain, I'm a secondary school teacher and I worked with my students with the lego usb tower, I write you in reference to the following commit (2fae9e5a7babada041e2e161699ade2447a01989): patch: 9363741 I have a little problem with the legousbtower module of kernel. I was working with kernel 4.4 and I had no problem with this module, each time that I connected the tower I have the following message in my log: [ 4927.822527] usb 3-1: new low-speed USB device number 2 using xhci_hcd [ 4928.022168] usb 3-1: New USB device found, idVendor=0694, idProduct=0001 [ 4928.022185] usb 3-1: New USB device strings: Mfr=4, Product=26, SerialNumber=0 [ 4928.022188] usb 3-1: Product: LEGO USB Tower [ 4928.022190] usb 3-1: Manufacturer: LEGO Group [ 4929.914818] legousbtower 3-1:1.0: LEGO USB Tower firmware version is 1.0 build 134 [ 4929.915157] legousbtower 3-1:1.0: LEGO USB Tower #-160 now attached to major 180 minor 0 [ 4929.915517] usbcore: registered new interface driver legousbtower And I can use nqc without any problems. Since kernel 4.9 (I've tested it with kernel 4.9 and 4.10), I'm having the same issue. Each time that I connect the legousbtower I have this error: [ 51.036609] usb 1-2: USB disconnect, device number 5 [ 53.025125] usb 1-2: new low-speed USB device number 6 using xhci_hcd [ 53.173087] legousbtower 1-2:1.0: LEGO USB Tower get version control request failed [ 53.173102] legousbtower: probe of 1-2:1.0 failed with error -11 I have tested it in two different distros and in different computers (Lliurex that is based in Ubuntu and Manjaro that is based in Arch). I have also tried to download kernel 4.4 directly to see if it is a problem of the distro, and it works perfectly with the kernel. I have also tried to compile the module from kernel 4.4 in 4.9 but I'm having the same problem, probably at this time I'm groping because this is beyond my knowledge. I have never report a bug to the kernel and I don't really know if it could be a bug, before report it, I would like, if it isn't an inconvenience for you, if it is really a bug related with the last update of the legousbtower module or what it is, because I'm lost. If you need that I do some test I'm available. I know that lego RCX is quite old but I and plenty of my colleagues in Valencia use it and before update the kernel, this issue can be a handicap for them. Thank you for your attention. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 12/21] USB: legousbtower: refactor endpoint retrieval
Use the new endpoint helpers to lookup the required interrupt-in and
interrupt-out endpoints.
Note that the descriptors are searched in reverse order to avoid any
regressions.
Cc: Juergen Stuber
Cc: legousb-de...@lists.sourceforge.net
Signed-off-by: Johan Hovold
---
drivers/usb/misc/legousbtower.c | 29 +++--
1 file changed, 7 insertions(+), 22 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 322a042d6e59..201c9c3effbb 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -806,10 +806,7 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
struct device *idev = &interface->dev;
struct usb_device *udev = interface_to_usbdev(interface);
struct lego_usb_tower *dev = NULL;
- struct usb_host_interface *iface_desc;
- struct usb_endpoint_descriptor* endpoint;
struct tower_get_version_reply get_version_reply;
- int i;
int retval = -ENOMEM;
int result;
@@ -846,25 +843,13 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
dev->interrupt_out_urb = NULL;
dev->interrupt_out_busy = 0;
- iface_desc = interface->cur_altsetting;
-
- /* set up the endpoint information */
- for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
- endpoint = &iface_desc->endpoint[i].desc;
-
- if (usb_endpoint_xfer_int(endpoint)) {
- if (usb_endpoint_dir_in(endpoint))
- dev->interrupt_in_endpoint = endpoint;
- else
- dev->interrupt_out_endpoint = endpoint;
- }
- }
- if(dev->interrupt_in_endpoint == NULL) {
- dev_err(idev, "interrupt in endpoint not found\n");
- goto error;
- }
- if (dev->interrupt_out_endpoint == NULL) {
- dev_err(idev, "interrupt out endpoint not found\n");
+ result = usb_find_common_endpoints_reverse(interface->cur_altsetting,
+ NULL, NULL,
+ &dev->interrupt_in_endpoint,
+ &dev->interrupt_out_endpoint);
+ if (result) {
+ dev_err(idev, "interrupt endpoints not found\n");
+ retval = result;
goto error;
}
--
2.12.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 10/16] USB: legousbtower: refactor endpoint retrieval
Use the new endpoint helpers to lookup the required interrupt-in and
interrupt-out endpoints.
Note that the descriptors are searched in reverse order to avoid any
regressions.
Signed-off-by: Johan Hovold
---
drivers/usb/misc/legousbtower.c | 29 +++--
1 file changed, 7 insertions(+), 22 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 322a042d6e59..201c9c3effbb 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -806,10 +806,7 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
struct device *idev = &interface->dev;
struct usb_device *udev = interface_to_usbdev(interface);
struct lego_usb_tower *dev = NULL;
- struct usb_host_interface *iface_desc;
- struct usb_endpoint_descriptor* endpoint;
struct tower_get_version_reply get_version_reply;
- int i;
int retval = -ENOMEM;
int result;
@@ -846,25 +843,13 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
dev->interrupt_out_urb = NULL;
dev->interrupt_out_busy = 0;
- iface_desc = interface->cur_altsetting;
-
- /* set up the endpoint information */
- for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
- endpoint = &iface_desc->endpoint[i].desc;
-
- if (usb_endpoint_xfer_int(endpoint)) {
- if (usb_endpoint_dir_in(endpoint))
- dev->interrupt_in_endpoint = endpoint;
- else
- dev->interrupt_out_endpoint = endpoint;
- }
- }
- if(dev->interrupt_in_endpoint == NULL) {
- dev_err(idev, "interrupt in endpoint not found\n");
- goto error;
- }
- if (dev->interrupt_out_endpoint == NULL) {
- dev_err(idev, "interrupt out endpoint not found\n");
+ result = usb_find_common_endpoints_reverse(interface->cur_altsetting,
+ NULL, NULL,
+ &dev->interrupt_in_endpoint,
+ &dev->interrupt_out_endpoint);
+ if (result) {
+ dev_err(idev, "interrupt endpoints not found\n");
+ retval = result;
goto error;
}
--
2.12.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: misc: legousbtower: Fix NULL pointer deference
On Mon, Sep 19, 2016 at 07:09:51PM +0100, James wrote:
> From: Greg Kroah-Hartman
>
> This patch fixes a NULL pointer dereference caused by a race codition in the
> probe function of the legousbtower driver. It re-structures the probe
> function to only register the interface after successfully reading the
> board's firmware ID.
>
> The probe function does not deregister the usb interface after an error
> receiving the devices firmware ID. The device file registered
> (/dev/usb/legousbtower%d) may be read/written globally before the probe
> function returns. When tower_delete is called in the probe function
> (after an r/w has been initiated), core dev structures are deleted while
> the file operation functions are still running. If the 0 address is mappable
> on the machine, this vulnerability can be used to create a Local Priviege
> Escalation exploit via a write-what-where condition by remapping
> dev->interrupt_out_buffer in tower_write. A forged USB device and local
> program execution would be required for LPE. The USB
> device would have to delay the control message in tower_probe and accept
> the control urb in tower_open whilst guest code initiated a write to the
> device file as tower_delete is called from the error in tower_probe.
>
> This bug has existed since 2003. Patch tested by emulated device.
>
> Reported-by: James Patrick-Evans
> Tested-by: James Patrick-Evans
> Signed-off-by: James Patrick-Evans
> Signed-off-by: Greg Kroah-Hartman
> ---
> drivers/usb/misc/legousbtower.c | 35 +--
> 1 file changed, 17 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
> index 7771be3..4dd531a 100644
> --- a/drivers/usb/misc/legousbtower.c
> +++ b/drivers/usb/misc/legousbtower.c
> @@ -898,24 +898,6 @@ static int tower_probe (struct usb_interface *interface,
> const struct usb_device
>dev->interrupt_in_interval = interrupt_in_interval ?
> interrupt_in_interval : dev->interrupt_in_endpoint->bInterval;
>dev->interrupt_out_interval = interrupt_out_interval ?
> interrupt_out_interval : dev->interrupt_out_endpoint->bInterval;
>
> - /* we can register the device now, as it is ready */
> - usb_set_intfdata (interface, dev);
> -
> - retval = usb_register_dev (interface, &tower_class);
> -
> - if (retval) {
> - /* something prevented us from registering this driver */
> - dev_err(idev, "Not able to get a minor for this device.\n");
> - usb_set_intfdata (interface, NULL);
> - goto error;
> - }
> - dev->minor = interface->minor;
> -
> - /* let the user know what node this device is now attached to */
> - dev_info(&interface->dev, "LEGO USB Tower #%d now attached to major "
> -"%d minor %d\n", (dev->minor - LEGO_USB_TOWER_MINOR_BASE),
> -USB_MAJOR, dev->minor);
> -
>/* get the firmware version and log it */
>result = usb_control_msg (udev,
> usb_rcvctrlpipe(udev, 0),
> @@ -936,6 +918,23 @@ static int tower_probe (struct usb_interface *interface,
> const struct usb_device
> get_version_reply.minor,
> le16_to_cpu(get_version_reply.build_no));
>
> + /* we can register the device now, as it is ready */
> + usb_set_intfdata (interface, dev);
> +
> + retval = usb_register_dev (interface, &tower_class);
> +
> + if (retval) {
> + /* something prevented us from registering this driver */
> + dev_err(idev, "Not able to get a minor for this device.\n");
> + usb_set_intfdata (interface, NULL);
> + goto error;
> + }
> + dev->minor = interface->minor;
> +
> + /* let the user know what node this device is now attached to */
> + dev_info(&interface->dev, "LEGO USB Tower #%d now attached to major "
> +"%d minor %d\n", (dev->minor - LEGO_USB_TOWER_MINOR_BASE),
> +USB_MAJOR, dev->minor);
>
> exit:
>return retval;
> --
Ugh, all of the tabs got turned into spaces, next time be more careful
with your email client :(
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: misc: legousbtower: Fix NULL pointer deference
On Tue, Sep 20, 2016 at 10:09:56AM +0100, James wrote: > Hi greg, > > Many appologies, I did not see your patch attached/forgot it existed > when I > fixed this yesterday. I created this patch based on your > feedback from my original email on September 2nd. I have tested this patch > with an emulated device and it fixes the vulnerability. > Again, applogies for the long delay and patch confusion. Heh, my feedback contained the patch itself, so it might have been hard to miss it :) Anyway, I'll queue this up later today, thanks for the report, and testing, much appreciated. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: misc: legousbtower: Fix NULL pointer deference
Hi greg, Many appologies, I did not see your patch attached/forgot it existed when I fixed this yesterday. I created this patch based on your feedback from my original email on September 2nd. I have tested this patch with an emulated device and it fixes the vulnerability. Again, applogies for the long delay and patch confusion. Best regards, James On Tue, Sep 20, 2016 at 10:32:56AM +0200, Greg KH wrote: On Tue, Sep 20, 2016 at 10:18:21AM +0200, Oliver Neukum wrote: On Tue, 2016-09-20 at 08:22 +0200, Greg KH wrote: > When you sent this the last time (back on Sept 2), I sent you a > "simpler" patch, the same one below, and asked you if that worked > instead. I never heard back, could you test this patch and see if it > resolves the issue for you? As far as I can tell, your patches are identical. Hah, so they are. James's original patch was not this one, sorry for not looking at it closer. James, any reason why you are passing off my patch as yours here? Did you test mine and see that this did solve the problem? You did change your changelog text a bit, to reflect the change between mine and yours, so I'm guessing that you did? thanks, greg k-h signature.asc Description: Digital signature
Re: [PATCH] usb: misc: legousbtower: Fix NULL pointer deference
On Tue, Sep 20, 2016 at 10:18:21AM +0200, Oliver Neukum wrote: > On Tue, 2016-09-20 at 08:22 +0200, Greg KH wrote: > > When you sent this the last time (back on Sept 2), I sent you a > > "simpler" patch, the same one below, and asked you if that worked > > instead. I never heard back, could you test this patch and see if it > > resolves the issue for you? > > As far as I can tell, your patches are identical. Hah, so they are. James's original patch was not this one, sorry for not looking at it closer. James, any reason why you are passing off my patch as yours here? Did you test mine and see that this did solve the problem? You did change your changelog text a bit, to reflect the change between mine and yours, so I'm guessing that you did? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: misc: legousbtower: Fix NULL pointer deference
On Tue, 2016-09-20 at 08:22 +0200, Greg KH wrote: > When you sent this the last time (back on Sept 2), I sent you a > "simpler" patch, the same one below, and asked you if that worked > instead. I never heard back, could you test this patch and see if it > resolves the issue for you? As far as I can tell, your patches are identical. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: misc: legousbtower: Fix NULL pointer deference
On Mon, Sep 19, 2016 at 07:09:51PM +0100, James wrote:
> This patch fixes a NULL pointer dereference caused by a race codition in the
> probe function of the legousbtower driver. It re-structures the probe
> function to only register the interface after successfully reading the
> board's firmware ID.
>
> The probe function does not deregister the usb interface after an error
> receiving the devices firmware ID. The device file registered
> (/dev/usb/legousbtower%d) may be read/written globally before the probe
> function returns. When tower_delete is called in the probe function
> (after an r/w has been initiated), core dev structures are deleted while
> the file operation functions are still running. If the 0 address is mappable
> on the machine, this vulnerability can be used to create a Local Priviege
> Escalation exploit via a write-what-where condition by remapping
> dev->interrupt_out_buffer in tower_write. A forged USB device and local
> program execution would be required for LPE. The USB
> device would have to delay the control message in tower_probe and accept
> the control urb in tower_open whilst guest code initiated a write to the
> device file as tower_delete is called from the error in tower_probe.
>
> This bug has existed since 2003. Patch tested by emulated device.
>
> Signed-off-by: James Patrick-Evans
> ---
> drivers/usb/misc/legousbtower.c | 35 +--
> 1 file changed, 17 insertions(+), 18 deletions(-)
When you sent this the last time (back on Sept 2), I sent you a
"simpler" patch, the same one below, and asked you if that worked
instead. I never heard back, could you test this patch and see if it
resolves the issue for you?
thanks,
greg k-h
---
How about this simpler patch, that moves the registering for the minor
number as the last thing that happens in the probe function (which is
what it should be).
Can you test this to verify it prevents the above problem?
And this driver really needs an overhaul one of these days...
thanks,
greg k-h
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 7771be3ac178..4dd531ac5a7f 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -898,24 +898,6 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
dev->interrupt_in_interval = interrupt_in_interval ?
interrupt_in_interval : dev->interrupt_in_endpoint->bInterval;
dev->interrupt_out_interval = interrupt_out_interval ?
interrupt_out_interval : dev->interrupt_out_endpoint->bInterval;
- /* we can register the device now, as it is ready */
- usb_set_intfdata (interface, dev);
-
- retval = usb_register_dev (interface, &tower_class);
-
- if (retval) {
- /* something prevented us from registering this driver */
- dev_err(idev, "Not able to get a minor for this device.\n");
- usb_set_intfdata (interface, NULL);
- goto error;
- }
- dev->minor = interface->minor;
-
- /* let the user know what node this device is now attached to */
- dev_info(&interface->dev, "LEGO USB Tower #%d now attached to major "
-"%d minor %d\n", (dev->minor - LEGO_USB_TOWER_MINOR_BASE),
-USB_MAJOR, dev->minor);
-
/* get the firmware version and log it */
result = usb_control_msg (udev,
usb_rcvctrlpipe(udev, 0),
@@ -936,6 +918,23 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
get_version_reply.minor,
le16_to_cpu(get_version_reply.build_no));
+ /* we can register the device now, as it is ready */
+ usb_set_intfdata (interface, dev);
+
+ retval = usb_register_dev (interface, &tower_class);
+
+ if (retval) {
+ /* something prevented us from registering this driver */
+ dev_err(idev, "Not able to get a minor for this device.\n");
+ usb_set_intfdata (interface, NULL);
+ goto error;
+ }
+ dev->minor = interface->minor;
+
+ /* let the user know what node this device is now attached to */
+ dev_info(&interface->dev, "LEGO USB Tower #%d now attached to major "
+"%d minor %d\n", (dev->minor - LEGO_USB_TOWER_MINOR_BASE),
+USB_MAJOR, dev->minor);
exit:
return retval;
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: misc: legousbtower: Fix NULL pointer deference
This patch fixes a NULL pointer dereference caused by a race codition
in the probe function of the legousbtower driver. It re-structures the
probe function to only register the interface after successfully
reading the board's firmware ID.
The probe function does not deregister the usb interface after an error
receiving the devices firmware ID. The device file registered
(/dev/usb/legousbtower%d) may be read/written globally before the probe
function returns. When tower_delete is called in the probe function
(after an r/w has been initiated), core dev structures are deleted while
the file operation functions are still running. If the 0 address is
mappable on the machine, this vulnerability can be used to create a
Local Priviege Escalation exploit via a write-what-where condition by
remapping dev->interrupt_out_buffer in tower_write. A forged USB
device and local program execution would be required for LPE. The USB
device would have to delay the control message in tower_probe and accept
the control urb in tower_open whilst guest code initiated a write to the
device file as tower_delete is called from the error in tower_probe.
This bug has existed since 2003. Patch tested by emulated device.
Signed-off-by: James Patrick-Evans
---
drivers/usb/misc/legousbtower.c | 35 +--
1 file changed, 17 insertions(+), 18 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 7771be3..4dd531a 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -898,24 +898,6 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
dev->interrupt_in_interval = interrupt_in_interval ? interrupt_in_interval :
dev->interrupt_in_endpoint->bInterval;
dev->interrupt_out_interval = interrupt_out_interval ? interrupt_out_interval
: dev->interrupt_out_endpoint->bInterval;
- /* we can register the device now, as it is ready */
- usb_set_intfdata (interface, dev);
-
- retval = usb_register_dev (interface, &tower_class);
-
- if (retval) {
- /* something prevented us from registering this driver */
- dev_err(idev, "Not able to get a minor for this device.\n");
- usb_set_intfdata (interface, NULL);
- goto error;
- }
- dev->minor = interface->minor;
-
- /* let the user know what node this device is now attached to */
- dev_info(&interface->dev, "LEGO USB Tower #%d now attached to major "
-"%d minor %d\n", (dev->minor - LEGO_USB_TOWER_MINOR_BASE),
-USB_MAJOR, dev->minor);
-
/* get the firmware version and log it */
result = usb_control_msg (udev,
usb_rcvctrlpipe(udev, 0),
@@ -936,6 +918,23 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
get_version_reply.minor,
le16_to_cpu(get_version_reply.build_no));
+ /* we can register the device now, as it is ready */
+ usb_set_intfdata (interface, dev);
+
+ retval = usb_register_dev (interface, &tower_class);
+
+ if (retval) {
+ /* something prevented us from registering this driver */
+ dev_err(idev, "Not able to get a minor for this device.\n");
+ usb_set_intfdata (interface, NULL);
+ goto error;
+ }
+ dev->minor = interface->minor;
+
+ /* let the user know what node this device is now attached to */
+ dev_info(&interface->dev, "LEGO USB Tower #%d now attached to major "
+"%d minor %d\n", (dev->minor - LEGO_USB_TOWER_MINOR_BASE),
+USB_MAJOR, dev->minor);
exit:
return retval;
--
signature.asc
Description: Digital signature
[PATCH 27/44] usb: misc: legousbtower: don't print on ENOMEM
All kmalloc-based functions print enough information on failures.
Signed-off-by: Wolfram Sang
---
drivers/usb/misc/legousbtower.c | 16
1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 52b41fb66792eb..ece9b3c1eaac29 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -817,10 +817,8 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
dev = kmalloc (sizeof(struct lego_usb_tower), GFP_KERNEL);
- if (dev == NULL) {
- dev_err(idev, "Out of memory\n");
+ if (!dev)
goto exit;
- }
mutex_init(&dev->lock);
@@ -871,23 +869,17 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
}
dev->read_buffer = kmalloc (read_buffer_size, GFP_KERNEL);
- if (!dev->read_buffer) {
- dev_err(idev, "Couldn't allocate read_buffer\n");
+ if (!dev->read_buffer)
goto error;
- }
dev->interrupt_in_buffer = kmalloc
(usb_endpoint_maxp(dev->interrupt_in_endpoint), GFP_KERNEL);
- if (!dev->interrupt_in_buffer) {
- dev_err(idev, "Couldn't allocate interrupt_in_buffer\n");
+ if (!dev->interrupt_in_buffer)
goto error;
- }
dev->interrupt_in_urb = usb_alloc_urb(0, GFP_KERNEL);
if (!dev->interrupt_in_urb)
goto error;
dev->interrupt_out_buffer = kmalloc (write_buffer_size, GFP_KERNEL);
- if (!dev->interrupt_out_buffer) {
- dev_err(idev, "Couldn't allocate interrupt_out_buffer\n");
+ if (!dev->interrupt_out_buffer)
goto error;
- }
dev->interrupt_out_urb = usb_alloc_urb(0, GFP_KERNEL);
if (!dev->interrupt_out_urb)
goto error;
--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 10/19] usb: misc: legousbtower: don't print error when allocating urb fails
kmalloc will print enough information in case of failure.
Signed-off-by: Wolfram Sang
---
drivers/usb/misc/legousbtower.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 7771be3ac178ea..52b41fb66792eb 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -881,20 +881,16 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
goto error;
}
dev->interrupt_in_urb = usb_alloc_urb(0, GFP_KERNEL);
- if (!dev->interrupt_in_urb) {
- dev_err(idev, "Couldn't allocate interrupt_in_urb\n");
+ if (!dev->interrupt_in_urb)
goto error;
- }
dev->interrupt_out_buffer = kmalloc (write_buffer_size, GFP_KERNEL);
if (!dev->interrupt_out_buffer) {
dev_err(idev, "Couldn't allocate interrupt_out_buffer\n");
goto error;
}
dev->interrupt_out_urb = usb_alloc_urb(0, GFP_KERNEL);
- if (!dev->interrupt_out_urb) {
- dev_err(idev, "Couldn't allocate interrupt_out_urb\n");
+ if (!dev->interrupt_out_urb)
goto error;
- }
dev->interrupt_in_interval = interrupt_in_interval ?
interrupt_in_interval : dev->interrupt_in_endpoint->bInterval;
dev->interrupt_out_interval = interrupt_out_interval ?
interrupt_out_interval : dev->interrupt_out_endpoint->bInterval;
--
2.8.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 10/19] usb: misc: legousbtower: don't print error when allocating urb fails
kmalloc will print enough information in case of failure.
Signed-off-by: Wolfram Sang
---
drivers/usb/misc/legousbtower.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 7771be3ac178ea..52b41fb66792eb 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -881,20 +881,16 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
goto error;
}
dev->interrupt_in_urb = usb_alloc_urb(0, GFP_KERNEL);
- if (!dev->interrupt_in_urb) {
- dev_err(idev, "Couldn't allocate interrupt_in_urb\n");
+ if (!dev->interrupt_in_urb)
goto error;
- }
dev->interrupt_out_buffer = kmalloc (write_buffer_size, GFP_KERNEL);
if (!dev->interrupt_out_buffer) {
dev_err(idev, "Couldn't allocate interrupt_out_buffer\n");
goto error;
}
dev->interrupt_out_urb = usb_alloc_urb(0, GFP_KERNEL);
- if (!dev->interrupt_out_urb) {
- dev_err(idev, "Couldn't allocate interrupt_out_urb\n");
+ if (!dev->interrupt_out_urb)
goto error;
- }
dev->interrupt_in_interval = interrupt_in_interval ?
interrupt_in_interval : dev->interrupt_in_endpoint->bInterval;
dev->interrupt_out_interval = interrupt_out_interval ?
interrupt_out_interval : dev->interrupt_out_endpoint->bInterval;
--
2.8.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v1 10/49] USB: legousbtower: prepare for enabling irq in complete()
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Juergen Stuber
Signed-off-by: Ming Lei
---
drivers/usb/misc/legousbtower.c |6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index eb37c95..d1bc420 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -739,7 +739,9 @@ static void tower_interrupt_in_callback (struct urb *urb)
}
if (urb->actual_length > 0) {
- spin_lock (&dev->read_buffer_lock);
+ unsigned long flags;
+
+ spin_lock_irqsave(&dev->read_buffer_lock, flags);
if (dev->read_buffer_length + urb->actual_length <
read_buffer_size) {
memcpy (dev->read_buffer + dev->read_buffer_length,
dev->interrupt_in_buffer,
@@ -752,7 +754,7 @@ static void tower_interrupt_in_callback (struct urb *urb)
pr_warn("read_buffer overflow, %d bytes dropped\n",
urb->actual_length);
}
- spin_unlock (&dev->read_buffer_lock);
+ spin_unlock_irqrestore(&dev->read_buffer_lock, flags);
}
resubmit:
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/50] USB: legousbtower: spin_lock in complete() cleanup
Hello. On 11-07-2013 16:36, Oliver Neukum wrote: I don't think this patch passes checkpatch.pl. This series is a mechanical replacement in dozens of drivers. That mechanicity shows too much in some patches. We cannot demand nice formatting. If you want to do something productive, check the locking in the driver. I'm not paid for it and don't have time to do it for free. Regards Oliver WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/50] USB: legousbtower: spin_lock in complete() cleanup
On Thu, Jul 11, 2013 at 8:18 PM, Sergei Shtylyov
wrote:
> Hello.
>
>
> On 11-07-2013 13:05, Ming Lei wrote:
>
>> Complete() will be run with interrupt enabled, so change to
>> spin_lock_irqsave().
>
>
>> Cc: Juergen Stuber
>> Signed-off-by: Ming Lei
>> ---
>> drivers/usb/misc/legousbtower.c |5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>
>
>> diff --git a/drivers/usb/misc/legousbtower.c
>> b/drivers/usb/misc/legousbtower.c
>> index 8089479..4044989 100644
>> --- a/drivers/usb/misc/legousbtower.c
>> +++ b/drivers/usb/misc/legousbtower.c
>> @@ -771,6 +771,7 @@ static void tower_interrupt_in_callback (struct urb
>> *urb)
>> struct lego_usb_tower *dev = urb->context;
>> int status = urb->status;
>> int retval;
>> + unsigned long flags;
>>
>> dbg(4, "%s: enter, status %d", __func__, status);
>>
>> @@ -788,7 +789,7 @@ static void tower_interrupt_in_callback (struct urb
>> *urb)
>> }
>>
>> if (urb->actual_length > 0) {
>> - spin_lock (&dev->read_buffer_lock);
>> + spin_lock_irqsave (&dev->read_buffer_lock, flags);
>> if (dev->read_buffer_length + urb->actual_length <
>> read_buffer_size) {
>> memcpy (dev->read_buffer +
>> dev->read_buffer_length,
>> dev->interrupt_in_buffer,
>> @@ -799,7 +800,7 @@ static void tower_interrupt_in_callback (struct urb
>> *urb)
>> } else {
>> printk(KERN_WARNING "%s: read_buffer overflow, %d
>> bytes dropped", __func__, urb->actual_length);
>> }
>> - spin_unlock (&dev->read_buffer_lock);
>> + spin_unlock_irqrestore (&dev->read_buffer_lock, flags);
>> }
>
>
>I don't think this patch passes checkpatch.pl.
No errors reported from checkpatch.pl, only warnings which isn't introduced
by this patch.
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/50] USB: legousbtower: spin_lock in complete() cleanup
On Thursday 11 July 2013 16:18:17 Sergei Shtylyov wrote: > I don't think this patch passes checkpatch.pl. This series is a mechanical replacement in dozens of drivers. We cannot demand nice formatting. If you want to do something productive, check the locking in the driver. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/50] USB: legousbtower: spin_lock in complete() cleanup
Hello.
On 11-07-2013 13:05, Ming Lei wrote:
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Juergen Stuber
Signed-off-by: Ming Lei
---
drivers/usb/misc/legousbtower.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 8089479..4044989 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -771,6 +771,7 @@ static void tower_interrupt_in_callback (struct urb *urb)
struct lego_usb_tower *dev = urb->context;
int status = urb->status;
int retval;
+ unsigned long flags;
dbg(4, "%s: enter, status %d", __func__, status);
@@ -788,7 +789,7 @@ static void tower_interrupt_in_callback (struct urb *urb)
}
if (urb->actual_length > 0) {
- spin_lock (&dev->read_buffer_lock);
+ spin_lock_irqsave (&dev->read_buffer_lock, flags);
if (dev->read_buffer_length + urb->actual_length <
read_buffer_size) {
memcpy (dev->read_buffer + dev->read_buffer_length,
dev->interrupt_in_buffer,
@@ -799,7 +800,7 @@ static void tower_interrupt_in_callback (struct urb *urb)
} else {
printk(KERN_WARNING "%s: read_buffer overflow, %d bytes
dropped", __func__, urb->actual_length);
}
- spin_unlock (&dev->read_buffer_lock);
+ spin_unlock_irqrestore (&dev->read_buffer_lock, flags);
}
I don't think this patch passes checkpatch.pl.
WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 08/50] USB: legousbtower: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Juergen Stuber
Signed-off-by: Ming Lei
---
drivers/usb/misc/legousbtower.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 8089479..4044989 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -771,6 +771,7 @@ static void tower_interrupt_in_callback (struct urb *urb)
struct lego_usb_tower *dev = urb->context;
int status = urb->status;
int retval;
+ unsigned long flags;
dbg(4, "%s: enter, status %d", __func__, status);
@@ -788,7 +789,7 @@ static void tower_interrupt_in_callback (struct urb *urb)
}
if (urb->actual_length > 0) {
- spin_lock (&dev->read_buffer_lock);
+ spin_lock_irqsave (&dev->read_buffer_lock, flags);
if (dev->read_buffer_length + urb->actual_length <
read_buffer_size) {
memcpy (dev->read_buffer + dev->read_buffer_length,
dev->interrupt_in_buffer,
@@ -799,7 +800,7 @@ static void tower_interrupt_in_callback (struct urb *urb)
} else {
printk(KERN_WARNING "%s: read_buffer overflow, %d bytes
dropped", __func__, urb->actual_length);
}
- spin_unlock (&dev->read_buffer_lock);
+ spin_unlock_irqrestore (&dev->read_buffer_lock, flags);
}
resubmit:
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 03/10] USB: legousbtower: remove custom debug macro
Don't use a custom debug macro for just one driver, instead rely on the
in-kernel dynamic debugging logic, which can handle this much better.
Cc: Juergen Stuber
Signed-off-by: Greg Kroah-Hartman
---
drivers/usb/misc/legousbtower.c | 27 +++
1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 93c95d0..4a5a8b1 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -443,7 +443,6 @@ static int tower_release (struct inode *inode, struct file
*file)
dev = file->private_data;
if (dev == NULL) {
- dbg(1, "%s: object is NULL", __func__);
retval = -ENODEV;
goto exit_nolock;
}
@@ -455,7 +454,8 @@ static int tower_release (struct inode *inode, struct file
*file)
}
if (dev->open_count != 1) {
- dbg(1, "%s: device not opened exactly once", __func__);
+ dev_dbg(&dev->udev->dev, "%s: device not opened exactly once\n",
+ __func__);
retval = -ENODEV;
goto unlock_exit;
}
@@ -491,10 +491,8 @@ exit_nolock:
*/
static void tower_abort_transfers (struct lego_usb_tower *dev)
{
- if (dev == NULL) {
- dbg(1, "%s: dev is null", __func__);
+ if (dev == NULL)
return;
- }
/* shutdown transfer */
if (dev->interrupt_in_running) {
@@ -594,7 +592,7 @@ static ssize_t tower_read (struct file *file, char __user
*buffer, size_t count,
/* verify that we actually have some data to read */
if (count == 0) {
- dbg(1, "%s: read request of 0 bytes", __func__);
+ dev_dbg(&dev->udev->dev, "read request of 0 bytes\n");
goto unlock_exit;
}
@@ -680,7 +678,7 @@ static ssize_t tower_write (struct file *file, const char
__user *buffer, size_t
/* verify that we actually have some data to write */
if (count == 0) {
- dbg(1, "%s: write request of 0 bytes", __func__);
+ dev_dbg(&dev->udev->dev, "write request of 0 bytes\n");
goto unlock_exit;
}
@@ -698,7 +696,8 @@ static ssize_t tower_write (struct file *file, const char
__user *buffer, size_t
/* write the data into interrupt_out_buffer from userspace */
bytes_to_write = min_t(int, count, write_buffer_size);
- dbg(4, "%s: count = %Zd, bytes_to_write = %Zd", __func__, count,
bytes_to_write);
+ dev_dbg(&dev->udev->dev, "%s: count = %Zd, bytes_to_write = %Zd\n",
+ __func__, count, bytes_to_write);
if (copy_from_user (dev->interrupt_out_buffer, buffer, bytes_to_write))
{
retval = -EFAULT;
@@ -753,7 +752,9 @@ static void tower_interrupt_in_callback (struct urb *urb)
status == -ESHUTDOWN) {
goto exit;
} else {
- dbg(1, "%s: nonzero status received: %d", __func__,
status);
+ dev_dbg(&dev->udev->dev,
+ "%s: nonzero status received: %d\n", __func__,
+ status);
goto resubmit; /* maybe we can recover */
}
}
@@ -766,7 +767,8 @@ static void tower_interrupt_in_callback (struct urb *urb)
urb->actual_length);
dev->read_buffer_length += urb->actual_length;
dev->read_last_arrival = jiffies;
- dbg(3, "%s: received %d bytes", __func__,
urb->actual_length);
+ dev_dbg(&dev->udev->dev, "%s: received %d bytes\n",
+ __func__, urb->actual_length);
} else {
printk(KERN_WARNING "%s: read_buffer overflow, %d bytes
dropped", __func__, urb->actual_length);
}
@@ -805,8 +807,9 @@ static void tower_interrupt_out_callback (struct urb *urb)
if (status && !(status == -ENOENT ||
status == -ECONNRESET ||
status == -ESHUTDOWN)) {
- dbg(1, "%s - nonzero write bulk status received: %d",
- __func__, status);
+ dev_dbg(&dev->udev->dev,
+ "%s: nonzero write bulk status received: %d\n",
__func__,
+ status);
}
dev->interrupt_out_busy = 0;
--
1.8.3.rc0.20.gb99dd2e
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 092/128] USB: misc: legousbtower: semaphore to mutex
From: Daniel Walker <[EMAIL PROTECTED]>
The dev->sem conforms to mutex style usage. This patch converts it to use
the struct mutex type, and new API.
There is also a small style fix around this comment,
/* unlock here as tower_delete frees dev */
Where I broke the line up to meet the 80 char limit.
Signed-off-by: Daniel Walker <[EMAIL PROTECTED]>
Signed-off-by: Greg Kroah-Hartman <[EMAIL PROTECTED]>
---
drivers/usb/misc/legousbtower.c | 30 --
1 files changed, 16 insertions(+), 14 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index aab3200..6664043 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -205,7 +205,7 @@ static DEFINE_MUTEX(open_disc_mutex);
/* Structure to hold all of our device specific stuff */
struct lego_usb_tower {
- struct semaphoresem;/* locks this structure */
+ struct mutexlock; /* locks this structure */
struct usb_device* udev; /* save off the usb device
pointer */
unsigned char minor; /* the starting minor number
for this device */
@@ -361,7 +361,7 @@ static int tower_open (struct inode *inode, struct file
*file)
}
/* lock this device */
- if (down_interruptible (&dev->sem)) {
+ if (mutex_lock_interruptible(&dev->lock)) {
mutex_unlock(&open_disc_mutex);
retval = -ERESTARTSYS;
goto exit;
@@ -421,7 +421,7 @@ static int tower_open (struct inode *inode, struct file
*file)
file->private_data = dev;
unlock_exit:
- up (&dev->sem);
+ mutex_unlock(&dev->lock);
exit:
dbg(2, "%s: leave, return value %d ", __FUNCTION__, retval);
@@ -448,7 +448,7 @@ static int tower_release (struct inode *inode, struct file
*file)
}
mutex_lock(&open_disc_mutex);
- if (down_interruptible (&dev->sem)) {
+ if (mutex_lock_interruptible(&dev->lock)) {
retval = -ERESTARTSYS;
goto exit;
}
@@ -460,7 +460,9 @@ static int tower_release (struct inode *inode, struct file
*file)
}
if (dev->udev == NULL) {
/* the device was unplugged before the file was released */
- up (&dev->sem); /* unlock here as tower_delete frees dev */
+
+ /* unlock here as tower_delete frees dev */
+ mutex_unlock(&dev->lock);
tower_delete (dev);
goto exit;
}
@@ -473,7 +475,7 @@ static int tower_release (struct inode *inode, struct file
*file)
dev->open_count = 0;
unlock_exit:
- up (&dev->sem);
+ mutex_unlock(&dev->lock);
exit:
mutex_unlock(&open_disc_mutex);
@@ -586,7 +588,7 @@ static ssize_t tower_read (struct file *file, char __user
*buffer, size_t count,
dev = (struct lego_usb_tower *)file->private_data;
/* lock this object */
- if (down_interruptible (&dev->sem)) {
+ if (mutex_lock_interruptible(&dev->lock)) {
retval = -ERESTARTSYS;
goto exit;
}
@@ -653,7 +655,7 @@ static ssize_t tower_read (struct file *file, char __user
*buffer, size_t count,
unlock_exit:
/* unlock the device */
- up (&dev->sem);
+ mutex_unlock(&dev->lock);
exit:
dbg(2, "%s: leave, return value %d", __FUNCTION__, retval);
@@ -675,7 +677,7 @@ static ssize_t tower_write (struct file *file, const char
__user *buffer, size_t
dev = (struct lego_usb_tower *)file->private_data;
/* lock this object */
- if (down_interruptible (&dev->sem)) {
+ if (mutex_lock_interruptible(&dev->lock)) {
retval = -ERESTARTSYS;
goto exit;
}
@@ -737,7 +739,7 @@ static ssize_t tower_write (struct file *file, const char
__user *buffer, size_t
unlock_exit:
/* unlock the device */
- up (&dev->sem);
+ mutex_unlock(&dev->lock);
exit:
dbg(2, "%s: leave, return value %d", __FUNCTION__, retval);
@@ -862,7 +864,7 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
goto exit;
}
- init_MUTEX (&dev->sem);
+ mutex_init(&dev->lock);
dev->udev = udev;
dev->open_count = 0;
@@ -1007,16 +1009,16 @@ static void tower_disconnect (struct usb_interface
*interface)
/* give back our minor */
usb_deregister_dev (interface, &tower_class);
- down (&dev->sem);
+ mutex_lock(&dev->lock);
mutex_unlock(&open_disc_mutex);
/* if the device is not opened, then we clean up right now */
if (!dev->open_count) {
- up (&dev->sem);
+ mutex_unlock(&dev->lock);
tower_delete (dev);
} else {
dev->udev = NULL;
- up (&dev->sem);
+ mutex_unlock(&dev->lock)
[PATCH 1/1] usb: misc: legousbtower: semaphore to mutex
The dev->sem conforms to mutex style usage. This patch converts it to use
the struct mutex type, and new API.
There is also a small style fix around this comment,
/* unlock here as tower_delete frees dev */
Where I broke the line up to meet the 80 char limit.
Signed-off-by: Daniel Walker <[EMAIL PROTECTED]>
---
drivers/usb/misc/legousbtower.c | 30 --
1 file changed, 16 insertions(+), 14 deletions(-)
Index: linux-2.6.23/drivers/usb/misc/legousbtower.c
===
--- linux-2.6.23.orig/drivers/usb/misc/legousbtower.c
+++ linux-2.6.23/drivers/usb/misc/legousbtower.c
@@ -205,7 +205,7 @@ static DEFINE_MUTEX(open_disc_mutex);
/* Structure to hold all of our device specific stuff */
struct lego_usb_tower {
- struct semaphoresem;/* locks this structure */
+ struct mutexlock; /* locks this structure */
struct usb_device* udev; /* save off the usb device
pointer */
unsigned char minor; /* the starting minor number
for this device */
@@ -361,7 +361,7 @@ static int tower_open (struct inode *ino
}
/* lock this device */
- if (down_interruptible (&dev->sem)) {
+ if (mutex_lock_interruptible(&dev->lock)) {
mutex_unlock(&open_disc_mutex);
retval = -ERESTARTSYS;
goto exit;
@@ -421,7 +421,7 @@ static int tower_open (struct inode *ino
file->private_data = dev;
unlock_exit:
- up (&dev->sem);
+ mutex_unlock(&dev->lock);
exit:
dbg(2, "%s: leave, return value %d ", __FUNCTION__, retval);
@@ -448,7 +448,7 @@ static int tower_release (struct inode *
}
mutex_lock(&open_disc_mutex);
- if (down_interruptible (&dev->sem)) {
+ if (mutex_lock_interruptible(&dev->lock)) {
retval = -ERESTARTSYS;
goto exit;
}
@@ -460,7 +460,9 @@ static int tower_release (struct inode *
}
if (dev->udev == NULL) {
/* the device was unplugged before the file was released */
- up (&dev->sem); /* unlock here as tower_delete frees dev */
+
+ /* unlock here as tower_delete frees dev */
+ mutex_unlock(&dev->lock);
tower_delete (dev);
goto exit;
}
@@ -473,7 +475,7 @@ static int tower_release (struct inode *
dev->open_count = 0;
unlock_exit:
- up (&dev->sem);
+ mutex_unlock(&dev->lock);
exit:
mutex_unlock(&open_disc_mutex);
@@ -586,7 +588,7 @@ static ssize_t tower_read (struct file *
dev = (struct lego_usb_tower *)file->private_data;
/* lock this object */
- if (down_interruptible (&dev->sem)) {
+ if (mutex_lock_interruptible(&dev->lock)) {
retval = -ERESTARTSYS;
goto exit;
}
@@ -653,7 +655,7 @@ static ssize_t tower_read (struct file *
unlock_exit:
/* unlock the device */
- up (&dev->sem);
+ mutex_unlock(&dev->lock);
exit:
dbg(2, "%s: leave, return value %d", __FUNCTION__, retval);
@@ -675,7 +677,7 @@ static ssize_t tower_write (struct file
dev = (struct lego_usb_tower *)file->private_data;
/* lock this object */
- if (down_interruptible (&dev->sem)) {
+ if (mutex_lock_interruptible(&dev->lock)) {
retval = -ERESTARTSYS;
goto exit;
}
@@ -737,7 +739,7 @@ static ssize_t tower_write (struct file
unlock_exit:
/* unlock the device */
- up (&dev->sem);
+ mutex_unlock(&dev->lock);
exit:
dbg(2, "%s: leave, return value %d", __FUNCTION__, retval);
@@ -862,7 +864,7 @@ static int tower_probe (struct usb_inter
goto exit;
}
- init_MUTEX (&dev->sem);
+ mutex_init(&dev->lock);
dev->udev = udev;
dev->open_count = 0;
@@ -1007,16 +1009,16 @@ static void tower_disconnect (struct usb
/* give back our minor */
usb_deregister_dev (interface, &tower_class);
- down (&dev->sem);
+ mutex_lock(&dev->lock);
mutex_unlock(&open_disc_mutex);
/* if the device is not opened, then we clean up right now */
if (!dev->open_count) {
- up (&dev->sem);
+ mutex_unlock(&dev->lock);
tower_delete (dev);
} else {
dev->udev = NULL;
- up (&dev->sem);
+ mutex_unlock(&dev->lock);
}
info("LEGO USB Tower #%d now disconnected", (minor -
LEGO_USB_TOWER_MINOR_BASE));
--
--
-
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
