Re: [PATCH] usb: core: Abort deauthorization if unsetting configuration fails

2013-12-04 Thread Alan Stern 
On Tue, 3 Dec 2013, Julius Werner wrote:

 usb_deauthorize_device() tries to unset the configuration of a USB
 device and then unconditionally blows away the configuration descriptors
 with usb_destroy_configuration(). This is bad if the
 usb_set_configuration() call failed before the configuration could be
 correctly unset, since pointers like udev-actconfig can keep pointing
 to the now invalid memory. We have encountered hard to reproduce crashes
 from devices disconnected during suspend due to this, where khubd's
 disconnect handling races with userspace tools that change authorization
 on resume.
 
 This patch ensures that usb_deauthorize_device() aborts when
 usb_set_configuration() fails, and the underlying status code (such as
 ENODEV) is returned to userspace.

I don't like this approach at all.  It should always be possible to
deauthorize a device, no matter what.

Instead, how about changing usb_set_configuration() so that it will
never fail when the new config is -1?  Except perhaps for -ENODEV 
errors (the device has been disconnected), which 
usb_deauthorize_device() could check for.

Alan Stern

--
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: core: Abort deauthorization if unsetting configuration fails

2013-12-04 Thread Julius Werner 
 Instead, how about changing usb_set_configuration() so that it will
 never fail when the new config is -1?  Except perhaps for -ENODEV
 errors (the device has been disconnected), which
 usb_deauthorize_device() could check for.

Yes, that should work as well. It's really just one autoresume and one
disable_lpm that can fail in that case so it shouldn't be too
intrusive. I would prefer not to special-case ENODEV though, no need
to add more complexity than necessary.

I will write up a new version for that tomorrow.
--
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: core: Abort deauthorization if unsetting configuration fails

2013-12-03 Thread Julius Werner 
usb_deauthorize_device() tries to unset the configuration of a USB
device and then unconditionally blows away the configuration descriptors
with usb_destroy_configuration(). This is bad if the
usb_set_configuration() call failed before the configuration could be
correctly unset, since pointers like udev-actconfig can keep pointing
to the now invalid memory. We have encountered hard to reproduce crashes
from devices disconnected during suspend due to this, where khubd's
disconnect handling races with userspace tools that change authorization
on resume.

This patch ensures that usb_deauthorize_device() aborts when
usb_set_configuration() fails, and the underlying status code (such as
ENODEV) is returned to userspace.

Signed-off-by: Julius Werner jwer...@chromium.org
---
 drivers/usb/core/hub.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index a7c04e2..ade315f 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2420,13 +2420,17 @@ fail:
  */
 int usb_deauthorize_device(struct usb_device *usb_dev)
 {
+   int result = 0;
+
usb_lock_device(usb_dev);
if (usb_dev-authorized == 0)
goto out_unauthorized;
 
-   usb_dev-authorized = 0;
-   usb_set_configuration(usb_dev, -1);
+   result = usb_set_configuration(usb_dev, -1);
+   if (result)
+   goto error_deconfigure;
 
+   usb_dev-authorized = 0;
kfree(usb_dev-product);
usb_dev-product = kstrdup(n/a (unauthorized), GFP_KERNEL);
kfree(usb_dev-manufacturer);
@@ -2437,9 +2441,10 @@ int usb_deauthorize_device(struct usb_device *usb_dev)
usb_destroy_configuration(usb_dev);
usb_dev-descriptor.bNumConfigurations = 0;
 
+error_deconfigure:
 out_unauthorized:
usb_unlock_device(usb_dev);
-   return 0;
+   return result;
 }
 
 
-- 
1.7.12.4

--
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