Re: [PATCH 2/5] USB: Make it possible to "subclass" usb_device_driver
More replies inline (which I always miss)
On Wed, 2019-10-09 at 10:34 -0400, Alan Stern wrote:
> On Wed, 9 Oct 2019, Bastien Nocera wrote:
>
> > The kernel currenly has only 2 usb_device_drivers, one generic one,
> one
> > that completely replaces the generic one to make USB devices usable
> over
> > a network.
>
> Presumably your first driver is in generic.c. Where is the second
> one?
>
> > Use the newly exported generic driver functions when a driver
> declares
> > to want them run, in addition to its own code. This makes it
> possible to
> > write drivers that extend the generic USB driver.
> >
> > Signed-off-by: Bastien Nocera
>
> This has a few problems. The biggest one is that the device core
> does
> not guarantee any order of driver probing. If generic.c is probed
> first, the subclass driver will never get probed -- which is a
> pretty
> fatal flaw.
>
> > ---
> > drivers/usb/core/driver.c | 36 ++-
> -
> > include/linux/usb.h | 1 +
> > 2 files changed, 31 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> > index 2b27d232d7a7..863e380a272b 100644
> > --- a/drivers/usb/core/driver.c
> > +++ b/drivers/usb/core/driver.c
> > @@ -261,10 +261,17 @@ static int usb_probe_device(struct device
> *dev)
> >*/
> > if (!udriver->supports_autosuspend)
> > error = usb_autoresume_device(udev);
> > + if (error)
> > + return error;
> >
> > - if (!error)
> > - error = udriver->probe(udev);
> > - return error;
> > + if (udriver->generic_init)
> > + error = usb_generic_driver_probe(udev);
> > + if (error)
> > + return error;
> > +
> > + if (udriver->probe)
> > + return udriver->probe(udev);
> > + return 0;
> > }
> >
> > /* called from driver core with dev locked */
> > @@ -273,7 +280,10 @@ static int usb_unbind_device(struct device
> *dev)
> > struct usb_device *udev = to_usb_device(dev);
> > struct usb_device_driver *udriver = to_usb_device_driver(dev-
> >driver);
> >
> > - udriver->disconnect(udev);
> > + if (udriver->generic_init)
> > + usb_generic_driver_disconnect(udev);
> > + if (udriver->disconnect)
> > + udriver->disconnect(udev);
>
> The order is wrong. The disconnects should always be done in
> reverse
> order of probing. This is true whenever you have a destructor for a
> subclass; the subclasses destructor runs before the superclass's
> destructor.
Fixed. Fixed in the suspend function as well.
> > if (!udriver->supports_autosuspend)
> > usb_autosuspend_device(udev);
> > return 0;
> > @@ -886,6 +896,14 @@ int usb_register_device_driver(struct
> usb_device_driver *new_udriver,
> > if (usb_disabled())
> > return -ENODEV;
> >
> > + if (new_udriver->probe == NULL &&
> > + !new_udriver->generic_init) {
>
> There's no point adding this extra test. Even subclass drivers
> should
> have a probe function.
Removed.
> > + printk(KERN_ERR "%s: error %d registering device "
> > +"driver %s, no probe() function\n",
>
> Don't split character strings. They are an exception to the 80-
> column
> limit.
I was using the error message just below in the function as an example.
A bad one apparently. This is gone in any case.
> > +usbcore_name, retval, new_udriver->name);
> > + return -EINVAL;
> > + }
> > +
> > new_udriver->drvwrap.for_devices = 1;
> > new_udriver->drvwrap.driver.name = new_udriver->name;
> > new_udriver->drvwrap.driver.bus = &usb_bus_type;
> > @@ -1149,7 +1167,10 @@ static int usb_suspend_device(struct
> usb_device *udev, pm_message_t msg)
> > udev->do_remote_wakeup = 0;
> > udriver = &usb_generic_driver;
> > }
> > - status = udriver->suspend(udev, msg);
> > + if (udriver->generic_init)
> > + status = usb_generic_driver_suspend (udev, msg);
> > + if (status == 0 && udriver->suspend)
> > + status = udriver->suspend(udev, msg);
>
> Again, the order is wrong. Suspend the subclass driver first.
Done, as mentioned above.
> > done:
> > dev_vdbg(&udev->dev, "%s: status %d\n", __func__, status);
> > @@ -1181,7 +1202,10 @@ static int usb_resume_device(struct
> usb_device *udev, pm_message_t msg)
> > udev->reset_resume = 1;
> >
> > udriver = to_usb_device_driver(udev->dev.driver);
> > - status = udriver->resume(udev, msg);
> > + if (udriver->generic_init)
> > + status = usb_generic_driver_resume (udev, msg);
> > + if (status == 0 && udriver->resume)
> > + status = udriver->resume(udev, msg);
> >
> > done:
> > dev_vdbg(&udev->dev, "%s: status %d\n", __func__, status);
> > diff --git a/include/linux/usb.h b/include/linux/usb.h
>
Re: [PATCH 4/5] USB: Select better matching USB drivers when available
On Wed, 2019-10-09 at 14:45 -0400, Alan Stern wrote: > On Wed, 9 Oct 2019, Bastien Nocera wrote: > > > > + return > device_driver_attach(usb_generic_driver.drvwrap.driver, dev); > > + return error; > > I think that's right. A little testing wouldn't hurt. device_driver_attach() isn't available to this part of the code. I think the only way to do things here might be to set status bit for the usb_device and launch device_reprobe(). The second time around, we wouldn't match or probe the specific driver.
Re: [PATCH 2/7] usb: musb: omap2430: Wait on enable to avoid babble
Hello! On 10.10.2019 0:21, Tony Lindgren wrote: We can babble interrupt if we attempt to switch to USB host mode too ^ verb missing? soon after enabling musb. Let's fix the issue by waiting a bit in runtime_resume. Cc: Jacopo Mondi Cc: Marcel Partap Cc: Merlijn Wajer Cc: Michael Scott Cc: NeKit Cc: Pavel Machek Cc: Sebastian Reichel Signed-off-by: Tony Lindgren [...] MBR, Sergei
Re: [PATCH 2/5] USB: Make it possible to "subclass" usb_device_driver
Hi Bastien, I love your patch! Perhaps something to improve: [auto build test WARNING on usb/usb-testing] [cannot apply to v5.4-rc2 next-20191010] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Bastien-Nocera/Add-Apple-MFi-fastcharge-USB-device-driver/20191010-155853 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing reproduce: make htmldocs If you fix the issue, kindly add following tag Reported-by: kbuild test robot All warnings (new ones prefixed by >>): Warning: The Sphinx 'sphinx_rtd_theme' HTML theme was not found. Make sure you have the theme installed to produce pretty HTML output. Falling back to the default theme. WARNING: dot(1) not found, for better output quality install graphviz from http://www.graphviz.org WARNING: convert(1) not found, for SVG to PDF conversion install ImageMagick (https://www.imagemagick.org) include/linux/regulator/machine.h:196: warning: Function parameter or member 'max_uV_step' not described in 'regulation_constraints' include/linux/regulator/driver.h:223: warning: Function parameter or member 'resume' not described in 'regulator_ops' Error: Cannot open file drivers/dma-buf/reservation.c Error: Cannot open file drivers/dma-buf/reservation.c Error: Cannot open file drivers/dma-buf/reservation.c Error: Cannot open file include/linux/reservation.h Error: Cannot open file include/linux/reservation.h include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'quotactl' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'quota_on' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'sb_free_mnt_opts' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'sb_eat_lsm_opts' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'sb_kern_mount' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'sb_show_options' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'sb_add_mnt_opt' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'd_instantiate' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'getprocattr' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'setprocattr' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'locked_down' not described in 'security_list_options' include/linux/spi/spi.h:190: warning: Function parameter or member 'driver_override' not described in 'spi_device' lib/genalloc.c:1: warning: 'gen_pool_add_virt' not found lib/genalloc.c:1: warning: 'gen_pool_alloc' not found lib/genalloc.c:1: warning: 'gen_pool_free' not found lib/genalloc.c:1: warning: 'gen_pool_alloc_algo' not found drivers/gpio/gpiolib-of.c:92: warning: Excess function parameter 'dev' description in 'of_gpio_need_valid_mask' include/linux/i2c.h:337: warning: Function parameter or member 'init_irq' not described in 'i2c_client' drivers/usb/typec/bus.c:1: warning: 'typec_altmode_unregister_driver' not found drivers/usb/typec/bus.c:1: warning: 'typec_altmode_register_driver' not found drivers/usb/typec/class.c:1: warning: 'typec_altmode_unregister_notifier' not found drivers/usb/typec/class.c:1: warning: 'typec_altmode_register_notifier' not found >> include/linux/usb.h:1247: warning: Function parameter or member >> 'generic_init' not described in 'usb_device_driver' include/linux/w1.h:277: warning: Function parameter or member 'of_match_table' not described in 'w1_family' fs/fs-writeback.c:913: warning: Excess function parameter 'nr_pages' description in 'cgroup_writeback_by_id' fs/direct-io.c:258: warning: Excess function parameter 'offset' description in 'dio_complete' fs/libf
Re: [PATCH] USB: usbfs: Suppress emission of uevents for interfaces handled via usbfs
On Wed, Oct 09, 2019 at 12:38:35PM +0200, Ingo Rohloff wrote:
> >From 17d1e75543e26cfe702e7f5b0d4e07e0e45e5250 Mon Sep 17 00:00:00 2001
> From: Ingo Rohloff
> Date: Tue, 8 Oct 2019 20:27:57 +0200
> Subject: [PATCH] USB: usbfs: Suppress emission of uevents for interfaces
> handled via usbfs.
No need for this in the changelog body :)
> commit 1455cf8dbfd0
> ("driver core: emit uevents when device is bound to a driver")
> added bind/unbind uevents when a driver is bound/unbound
> to a physical device.
You can wrap the line a bit nicer:
commit 1455cf8dbfd0 ("driver core: emit uevents when device is bound to
a driver") added bind/unbind uevents when a driver is bound/unbound to a
physical device.
> For USB devices which are handled via the generic usbfs layer
> (via libusb for example), this is problematic:
> Each time a user space program calls
>ioctl(usb_fd, USBDEVFS_CLAIMINTERFACE, &usb_intf_nr);
> and then later
>ioctl(usb_fd, USBDEVFS_RELEASEINTERFACE, &usb_intf_nr);
> The kernel will now produce a bind/unbind event,
> which does not really contain any useful information.
>
> This allows a user space program to run a DoS attack against
> programs which listen to uevents (in particular systemd/eudev/upowerd):
> A malicious user space program just has to call in a tight loop
>
>ioctl(usb_fd, USBDEVFS_CLAIMINTERFACE, &usb_intf_nr);
>ioctl(usb_fd, USBDEVFS_RELEASEINTERFACE, &usb_intf_nr);
>
> With this loop the malicious user space program floods
> the kernel and all programs listening to uevents with
> tons of bind/unbind events.
>
> This patch suppresses uevents for interfaces claimed via usbfs.
>
> Signed-off-by: Ingo Rohloff
> ---
> drivers/usb/core/devio.c | 7 ++-
> drivers/usb/core/driver.c | 2 ++
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index 3f899552f6e3..a1af1d9b2ae7 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -764,8 +764,13 @@ static int claimintf(struct usb_dev_state *ps, unsigned
> int ifnum)
> intf = usb_ifnum_to_if(dev, ifnum);
> if (!intf)
> err = -ENOENT;
> - else
> + else {
> + /* suppress uevents for devices handled by usbfs */
> + dev_set_uevent_suppress(&intf->dev, 1);
> err = usb_driver_claim_interface(&usbfs_driver, intf, ps);
> + if (err != 0)
Did checkpatch let this go through? Shouldn't that be:
if (err)
And did you send this patch twice?
Anyway, if you fix those minor things up, it looks good to me.
thanks,
greg k-h
Re: [PATCH 4/5] USB: Select better matching USB drivers when available
Hi Bastien, I love your patch! Perhaps something to improve: [auto build test WARNING on usb/usb-testing] [cannot apply to v5.4-rc2 next-20191010] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Bastien-Nocera/Add-Apple-MFi-fastcharge-USB-device-driver/20191010-155853 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing reproduce: make htmldocs If you fix the issue, kindly add following tag Reported-by: kbuild test robot All warnings (new ones prefixed by >>): Warning: The Sphinx 'sphinx_rtd_theme' HTML theme was not found. Make sure you have the theme installed to produce pretty HTML output. Falling back to the default theme. WARNING: dot(1) not found, for better output quality install graphviz from http://www.graphviz.org WARNING: convert(1) not found, for SVG to PDF conversion install ImageMagick (https://www.imagemagick.org) include/linux/regulator/machine.h:196: warning: Function parameter or member 'max_uV_step' not described in 'regulation_constraints' include/linux/regulator/driver.h:223: warning: Function parameter or member 'resume' not described in 'regulator_ops' Error: Cannot open file drivers/dma-buf/reservation.c Error: Cannot open file drivers/dma-buf/reservation.c Error: Cannot open file drivers/dma-buf/reservation.c Error: Cannot open file include/linux/reservation.h Error: Cannot open file include/linux/reservation.h include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'quotactl' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'quota_on' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'sb_free_mnt_opts' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'sb_eat_lsm_opts' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'sb_kern_mount' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'sb_show_options' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'sb_add_mnt_opt' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'd_instantiate' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'getprocattr' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'setprocattr' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'locked_down' not described in 'security_list_options' include/linux/spi/spi.h:190: warning: Function parameter or member 'driver_override' not described in 'spi_device' lib/genalloc.c:1: warning: 'gen_pool_add_virt' not found lib/genalloc.c:1: warning: 'gen_pool_alloc' not found lib/genalloc.c:1: warning: 'gen_pool_free' not found lib/genalloc.c:1: warning: 'gen_pool_alloc_algo' not found drivers/gpio/gpiolib-of.c:92: warning: Excess function parameter 'dev' description in 'of_gpio_need_valid_mask' include/linux/i2c.h:337: warning: Function parameter or member 'init_irq' not described in 'i2c_client' drivers/usb/typec/bus.c:1: warning: 'typec_altmode_unregister_driver' not found drivers/usb/typec/bus.c:1: warning: 'typec_altmode_register_driver' not found drivers/usb/typec/class.c:1: warning: 'typec_altmode_unregister_notifier' not found drivers/usb/typec/class.c:1: warning: 'typec_altmode_register_notifier' not found >> include/linux/usb.h:1251: warning: Function parameter or member 'match' not >> described in 'usb_device_driver' >> include/linux/usb.h:1251: warning: Function parameter or member 'id_table' >> not described in 'usb_device_driver' include/linux/usb.h:1251: warning: Function parameter or member 'generic_init' not described in 'usb_device_driver' include/linux/w1.h:277: warning: Function parameter or member 'of_match_table' not described in
Re: [PATCH 5/5] USB: yurex: fix NULL-derefs on disconnect
On Wed, Oct 09, 2019 at 05:38:48PM +0200, Johan Hovold wrote:
> The driver was using its struct usb_interface pointer as an inverted
> disconnected flag, but was setting it to NULL without making sure all
> code paths that used it were done with it.
>
> Before commit ef61eb43ada6 ("USB: yurex: Fix protection fault after
> device removal") this included the interrupt-in completion handler, but
> there are further accesses in dev_err and dev_dbg statements in
> yurex_write() and the driver-data destructor (sic!).
>
> Fix this by unconditionally stopping also the control URB at disconnect
> and by using a dedicated disconnected flag.
>
> Note that we need to take a reference to the struct usb_interface to
> avoid a use-after-free in the destructor whenever the device was
> disconnected while the character device was still open.
>
> Fixes: aadd6472d904 ("USB: yurex.c: remove dbg() usage")
> Fixes: 45714104b9e8 ("USB: yurex.c: remove err() usage")
> Cc: stable # 3.5: ef61eb43ada6
> Signed-off-by: Johan Hovold
Greg, I noticed that you picked up all patches in this series except
this last one.
Was that one purpose or by mistake?
Johan
Re: [PATCH 5/5] USB: yurex: fix NULL-derefs on disconnect
On Thu, Oct 10, 2019 at 01:05:32PM +0200, Johan Hovold wrote:
> On Wed, Oct 09, 2019 at 05:38:48PM +0200, Johan Hovold wrote:
> > The driver was using its struct usb_interface pointer as an inverted
> > disconnected flag, but was setting it to NULL without making sure all
> > code paths that used it were done with it.
> >
> > Before commit ef61eb43ada6 ("USB: yurex: Fix protection fault after
> > device removal") this included the interrupt-in completion handler, but
> > there are further accesses in dev_err and dev_dbg statements in
> > yurex_write() and the driver-data destructor (sic!).
> >
> > Fix this by unconditionally stopping also the control URB at disconnect
> > and by using a dedicated disconnected flag.
> >
> > Note that we need to take a reference to the struct usb_interface to
> > avoid a use-after-free in the destructor whenever the device was
> > disconnected while the character device was still open.
> >
> > Fixes: aadd6472d904 ("USB: yurex.c: remove dbg() usage")
> > Fixes: 45714104b9e8 ("USB: yurex.c: remove err() usage")
> > Cc: stable # 3.5: ef61eb43ada6
> > Signed-off-by: Johan Hovold
>
> Greg, I noticed that you picked up all patches in this series except
> this last one.
>
> Was that one purpose or by mistake?
Mistake, thanks for catching that. Now queued up.
greg k-h
Re: [PATCH] USB: usbfs: Suppress emission of uevents for interfaces handled via usbfs
Hello Greg
> > + else {
> > + /* suppress uevents for devices handled by usbfs */
> > + dev_set_uevent_suppress(&intf->dev, 1);
> > err = usb_driver_claim_interface(&usbfs_driver, intf, ps);
> > + if (err != 0)
> > + dev_set_uevent_suppress(&intf->dev, 0);
> Did checkpatch let this go through? Shouldn't that be:
> if (err)
I actually wanted it the way it is, but it really might not be the best option.
Let me explain:
The main goal was to suppress bind/unbind uevents produced by libusb
or any other user space program which calls
ioctl USBDEVFS_CLAIMINTERFACE/USBDEVFS_RELEASEINTERFACE .
Now I can suppress uevents produced by usb_driver_claim_interface
with the code above.
But I was not sure how to handle the call to usb_driver_release_interface
from devio.c/releaseintf()
The strategy I used was:
1) Set suppression of uevents when user space program tries to claim interface
2) If claiming the interface works, then KEEP uevents suppressed,
otherwise undo suppression.
That's why its "if err !=0"; error happened => undo suppression.
3) When interface is released make sure suppression is undone AFTER unbinding
the driver.
Thinking about your comment: It might be better + simpler to just use
1) Suppress uevents when calling usb_driver_claim_interface. Undo suppression
right after the call.
2) Suppress uevents when calling usb_driver_release_interface. Undo suppression
right after the call.
The main semantic problem I do not know about:
Is it correct to modify uevent suppression of an USB interface device
even if it CANNOT be claimed by usbfs ?
I grepped the source code for usage of dev_set_uevent_suppress, but it seems
not to be 100%
clear how that should be used (sometimes uevents are only suppressed
temporarily to implement
a delay, sometimes they are actually kept suppressed).
I will prepare/send an alternative.
with best regards
Ingo
PS:
> ...
> No need for this in the changelog body :)
I should have read the documentation about how to send correct E-Mails for
patches more intensively.
I just found out about "git send-email" and had not set it up (did now...). I
am sorry.
> And did you send this patch twice?
Unfortunately yes: I was struggling how to format this correctly.
Apply For Financial investment at a lower rate 2%
-- Hello, We are private lenders based in UK. Do you need a loan (credit) as soon as possible. Are you in search of money to solve your personal needs or finance your business venture, then get Your desired loan today! Consult us at Sunrise Funding Ltd. * We offer personal loan & huge capital loan at 2% interest rate to the general public both locally and internationally. * Credit amount range from $5,000.00 -- $500,000.00 and above. * Special $10,000,000.00 Loan offer for huge project also available. * Loan period of 6 months -- 10 years. * Loan is granted 24 hours after approval and accredited, directly in hand or bank account. Please note that you are advised to contact us for more details via the following e-mail address below; EMAIL : sunrisefundinglt...@gmail.com FIRM : Sunrise Funding Ltd UK.
Re: [PATCH 4/5] USB: Select better matching USB drivers when available
On Thu, 10 Oct 2019, Bastien Nocera wrote: > On Wed, 2019-10-09 at 14:45 -0400, Alan Stern wrote: > > > On Wed, 9 Oct 2019, Bastien Nocera wrote: > > > > > > > + return > > device_driver_attach(usb_generic_driver.drvwrap.driver, dev); > > > + return error; > > > > I think that's right. A little testing wouldn't hurt. > > device_driver_attach() isn't available to this part of the code. > > I think the only way to do things here might be to set status bit for > the usb_device and launch device_reprobe(). The second time around, we > wouldn't match or probe the specific driver. That would mean probing generic_driver twice, right? You probably should call its disconnect routine in between. That sounds pretty awkward, but if there's no other way then go ahead and do it. Ala Stern
[PATCH] usb: usbfs: Suppress problematic bind and unbind uevents.
commit 1455cf8dbfd0 ("driver core: emit uevents when device is bound
to a driver") added bind and unbind uevents when a driver is bound or
unbound to a physical device.
For USB devices which are handled via the generic usbfs layer (via
libusb for example), this is problematic:
Each time a user space program calls
ioctl(usb_fd, USBDEVFS_CLAIMINTERFACE, &usb_intf_nr);
and then later
ioctl(usb_fd, USBDEVFS_RELEASEINTERFACE, &usb_intf_nr);
The kernel will now produce a bind or unbind event, which does not
really contain any useful information.
This allows a user space program to run a DoS attack against programs
which listen to uevents (in particular systemd/eudev/upowerd):
A malicious user space program just has to call in a tight loop
ioctl(usb_fd, USBDEVFS_CLAIMINTERFACE, &usb_intf_nr);
ioctl(usb_fd, USBDEVFS_RELEASEINTERFACE, &usb_intf_nr);
With this loop the malicious user space program floods the kernel and
all programs listening to uevents with tons of bind and unbind
events.
This patch suppresses uevents for ioctls USBDEVFS_CLAIMINTERFACE and
USBDEVFS_RELEASEINTERFACE.
Signed-off-by: Ingo Rohloff
---
drivers/usb/core/devio.c | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 3f899552f6e3..6ca40d135430 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -764,8 +764,15 @@ static int claimintf(struct usb_dev_state *ps, unsigned
int ifnum)
intf = usb_ifnum_to_if(dev, ifnum);
if (!intf)
err = -ENOENT;
- else
+ else {
+ unsigned int old_suppress;
+
+ /* suppress uevents while claiming interface */
+ old_suppress = dev_get_uevent_suppress(&intf->dev);
+ dev_set_uevent_suppress(&intf->dev, 1);
err = usb_driver_claim_interface(&usbfs_driver, intf, ps);
+ dev_set_uevent_suppress(&intf->dev, old_suppress);
+ }
if (err == 0)
set_bit(ifnum, &ps->ifclaimed);
return err;
@@ -785,7 +792,13 @@ static int releaseintf(struct usb_dev_state *ps, unsigned
int ifnum)
if (!intf)
err = -ENOENT;
else if (test_and_clear_bit(ifnum, &ps->ifclaimed)) {
+ unsigned int old_suppress;
+
+ /* suppress uevents while releasing interface */
+ old_suppress = dev_get_uevent_suppress(&intf->dev);
+ dev_set_uevent_suppress(&intf->dev, 1);
usb_driver_release_interface(&usbfs_driver, intf);
+ dev_set_uevent_suppress(&intf->dev, old_suppress);
err = 0;
}
return err;
--
2.17.1
RE: [PATCH 00/14] usb: typec: UCSI driver overhaul
Hi Heikki, > > > > Hi Ajay, > > > > > > > > Here's the pretty much complete rewrite of the I/O handling that I > > > > was talking about. The first seven patches are not actually > > > > related to this stuff, but I'm including them here because the > > > > rest of the series is made on top of them. I'm including also that > > > > fix patch I send you earlier. > > > > > > > > After this it should be easier to handle quirks. My idea how to > > > > handle the multi-instance connector alt modes is that we "emulate" > > > > the PPM in ucsi_ccg.c in order to handle them, so ucsi.c is not > > > > touched at > > all. > > > > > > > > We can now get the connector alternate modes that the actual > > > > controller supplies during probe - before registering the ucsi > > > > interface > > > How can ucsi_ccg.c get the connector alternate modes before > > > registering ucsi interface? PPM reset, notification enable, etc. > > > is done during ucsi registration. UCSI spec says: > > > " The only commands the PPM is required to process in the "PPM Idle > > > (Notifications Disabled)" state are SET_NOTIFICATION_ENABLE and > > > PPM_RESE" > > > > > > Also, it doesn't look correct if ucsi_ccg.c has to replicate most of > > > the stuff done in ucsi_init() of ucsi.c. > > > > How about if we split ucsi_init() into a function that first simply > > constructs the struct ucsi and struct ucsi_connector instances without > > registering anything, and into separate functions that then register > > the ports, altmodes and what have you. I don't think that should be a > > huge problem. It will make ucsi.c even more like a library, which is > > probable > a good thing. > Do you mean the solution to follow steps (a->b->c->d1) or (a->b->c->d2) ? > a) ucsi_ccg.c calls first part of split ucsi_init() > b) ucsi_ccg.c uses ucsi_send_command() to collect actual alternate modes. > c) ucsi_ccg.c looks into actual alternate modes and squashes if duplicate > altmode found. > d1) ucsi_ccg.c calls new method to register connector alternate modes. > This method issues GET_ALTERNATE_MODES command again and ucsi_ccg.c is > expected to send squashed alternate mode. This will require changes in > .read(), .sync_write() and > .async_write() to make it appear as if the squashed data coming from the ppm. > OR > d2) ucsi_ccg.c calls new method to register squashed connector alternate > modes. > This method doesn't issue GET_ALTERNATE_MODES commands to PPM but > simply registers the alternate mode values passed to this function. > > If you mean the (a->b->c->d2) solution then it looks fine to me and would wait > for patches from you. This solution would mean that GET_ALTERNATE_MODES > for connector is done only by each ucsi_xxx.c and not by ucsi.c I am waiting for your comments on this. Thanks > nvpublic > > I can prepare patches for that too if you like? > > After that you should be able to get the struct ucsi instance that > > represents the "real" PPM without registering anything by calling a > > single function, most likely ucsi_init(). And after getting that you > > can construct the connector alternate modes that we actually register. > > Finally you register the final interface which does not use > > ucsi_ccg_ops, but instead something like ucsi_nvidia_ops. > I didn't understand this part. ucsi_ccg_ops has .read(), .sync_write() and > .async_write() interface and they remain same for all ucsi_ccg controllers. > > Thanks > > How would this sound to you? > > > > Br, > > > > -- > > heikki
Re: Regression: USB/xhci issues on some systems with newer kernel versions
I've just noticed that this problem also occurs when unplugging an affected device. When unplugging the device the error "WARN Set TR Deq Ptr cmd failed due to incorrect slot or ep state" gets shown, even though I don't get this error when plugging the device in. Here is a link to the dmesg and trace logs: https://gist.github.com/Brn9hrd7/011405276fdf7a699dcc5cb83c67d276 maybe there is something useful in there that was missing in the previous logs. - Bernhard Am 03.10.19 um 17:13 schrieb Bernhard Gebetsberger: > I sent the instructions to one of the users in the bug tracker. > Here is the download link for his logs: https://www.sendspace.com/file/413hlj > > - Bernhard > > Am 03.10.19 um 12:23 schrieb Mathias Nyman: >> On 2.10.2019 15.28, Bernhard Gebetsberger wrote: >>> Hi, >>> >>> There has been a regression in the xhci driver since kernel version 4.20, >>> on some systems some usb devices won't work until the system gets rebooted. >>> The error message in dmesg is "WARN Set TR Deq Ptr cmd failed due to >>> incorrect slot or ep state", although for some reason there are some usb >>> devices that are affected by this issue but don't throw the error >>> message(including the device I'm using, I got the error in previous kernel >>> versions though). >>> It seems like this bug can also lead to system instability, one user >>> reported in the bug >>> tracker(https://bugzilla.kernel.org/show_bug.cgi?id=202541#c58) that he got >>> a system freeze because of this when using kernel 5.3.1. >>> >> Ok, lets take a look at this. >> Some of the symptoms vary a bit in the report, so lets focus on ones that >> show: "WARN Set TR Deq Ptr cmd failed due to incorrect slot or ep state" >> >>> When looking at the responses in the bug tracker, it looks like it mostly >>> affects Ryzen based systems with 300 series motherboards, although there >>> are some other affected systems as well. It doesn't only affect >>> wifi/bluetooth sticks, some users even got this issue when connecting their >>> smartphone or their external hard drive to their PC. >>> I have uploaded the whole dmesg file and the tracing file to transfer.sh: >>> https://transfer.sh/zYohl/dmesg and https://transfer.sh/KNbFL/xhci-trace >> Hmm, trying to download these just shows "Not Found" >> >> Could someone with a affected system enable tracing and dynamic debug on a >> recent kernel, take logs and traces of one failing instance where the message >> "WARN Set TR Deq Ptr cmd failed due to incorrect slot or ep state" is seen. >> >> mount -t debugfs none /sys/kernel/debug >> echo 'module xhci_hcd =p' >/sys/kernel/debug/dynamic_debug/control >> echo 'module usbcore =p' >/sys/kernel/debug/dynamic_debug/control >> echo 81920 > /sys/kernel/debug/tracing/buffer_size_kb >> echo 1 > /sys/kernel/debug/tracing/events/xhci-hcd/enable >> >> < Trigger the issue > >> >> Send output of dmesg >> Send content of /sys/kernel/debug/tracing/trace >> >>> The issues occur since commit f8f80be501aa2f10669585c3e328fad079d8cb3a >>> "xhci: Use soft retry to recover faster from transaction errors". I think >>> this commit should be reverted at least until a workaround has been found, >>> especially since the next two kernel versions will be used by a lot of >>> distributions(5.4 because it's a LTS kernel and 5.5 will probably be used >>> in Ubuntu 20.04) so more users would be affected by this. >>> >> There some time left before 5.4 is out, lets see if we can find the root >> cause first. >> >> -Mathias >>
Re: [PATCH] usb: usbfs: Suppress problematic bind and unbind uevents.
On Thu, Oct 10, 2019 at 06:48:00PM +0200, Ingo Rohloff wrote:
> commit 1455cf8dbfd0 ("driver core: emit uevents when device is bound
> to a driver") added bind and unbind uevents when a driver is bound or
> unbound to a physical device.
>
> For USB devices which are handled via the generic usbfs layer (via
> libusb for example), this is problematic:
> Each time a user space program calls
>ioctl(usb_fd, USBDEVFS_CLAIMINTERFACE, &usb_intf_nr);
> and then later
>ioctl(usb_fd, USBDEVFS_RELEASEINTERFACE, &usb_intf_nr);
> The kernel will now produce a bind or unbind event, which does not
> really contain any useful information.
>
> This allows a user space program to run a DoS attack against programs
> which listen to uevents (in particular systemd/eudev/upowerd):
> A malicious user space program just has to call in a tight loop
>
>ioctl(usb_fd, USBDEVFS_CLAIMINTERFACE, &usb_intf_nr);
>ioctl(usb_fd, USBDEVFS_RELEASEINTERFACE, &usb_intf_nr);
>
> With this loop the malicious user space program floods the kernel and
> all programs listening to uevents with tons of bind and unbind
> events.
>
> This patch suppresses uevents for ioctls USBDEVFS_CLAIMINTERFACE and
> USBDEVFS_RELEASEINTERFACE.
>
> Signed-off-by: Ingo Rohloff
> ---
> drivers/usb/core/devio.c | 15 ++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
I am guessing this is a new version of a previously-submitted patch? If
so, you need to include a "version" number on it, and put what you
changed below the --- line. The kernel documentation should explain how
to do this, if not, please let us know.
Please fix this up and resend.
thanks,
greg k-h
[PATCH 1/3] usb: chipidea: imx: change hsic power regulator as optional
Not every platform needs this regulator.
Signed-off-by: Peter Chen
---
drivers/usb/chipidea/ci_hdrc_imx.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
b/drivers/usb/chipidea/ci_hdrc_imx.c
index 16700170bc34..25a38ed27aa8 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -359,7 +359,8 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
return PTR_ERR(data->pinctrl_hsic_active);
}
- data->hsic_pad_regulator = devm_regulator_get(dev, "hsic");
+ data->hsic_pad_regulator =
+ devm_regulator_get_optional(dev, "hsic");
if (PTR_ERR(data->hsic_pad_regulator) == -EPROBE_DEFER) {
return -EPROBE_DEFER;
} else if (PTR_ERR(data->hsic_pad_regulator) == -ENODEV) {
--
2.17.1
[PATCH 2/3] usb: chipidea: imx: refine the error handling for hsic
- -EPROBE_DEFER is an error, but without need show error message
- If pintrol is not existed, as pintrol is NULL
Signed-off-by: Peter Chen
---
drivers/usb/chipidea/ci_hdrc_imx.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
b/drivers/usb/chipidea/ci_hdrc_imx.c
index 25a38ed27aa8..c34fcc079cd4 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -330,8 +330,11 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
pdata.flags |= CI_HDRC_IMX_IS_HSIC;
data->usbmisc_data->hsic = 1;
data->pinctrl = devm_pinctrl_get(dev);
- if (IS_ERR(data->pinctrl)) {
- dev_err(dev, "pinctrl get failed, err=%ld\n",
+ if (PTR_ERR(data->pinctrl) == -ENODEV)
+ data->pinctrl = NULL;
+ else if (IS_ERR(data->pinctrl)) {
+ if (PTR_ERR(data->pinctrl) != -EPROBE_DEFER)
+ dev_err(dev, "pinctrl get failed, err=%ld\n",
PTR_ERR(data->pinctrl));
return PTR_ERR(data->pinctrl);
}
@@ -361,13 +364,13 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
data->hsic_pad_regulator =
devm_regulator_get_optional(dev, "hsic");
- if (PTR_ERR(data->hsic_pad_regulator) == -EPROBE_DEFER) {
- return -EPROBE_DEFER;
- } else if (PTR_ERR(data->hsic_pad_regulator) == -ENODEV) {
+ if (PTR_ERR(data->hsic_pad_regulator) == -ENODEV) {
/* no pad regualator is needed */
data->hsic_pad_regulator = NULL;
} else if (IS_ERR(data->hsic_pad_regulator)) {
- dev_err(dev, "Get HSIC pad regulator error: %ld\n",
+ if (PTR_ERR(data->hsic_pad_regulator) != -EPROBE_DEFER)
+ dev_err(dev,
+ "Get HSIC pad regulator error: %ld\n",
PTR_ERR(data->hsic_pad_regulator));
return PTR_ERR(data->hsic_pad_regulator);
}
--
2.17.1
[PATCH 3/3] usb: chipidea: imx: pinctrl for HSIC is optional
For imx chipidea controllers, if they use mxs PHY, they need pinctrl
for HSIC. Otherwise, it doesn't need pinctrl and usbmisc control. Like
imx7d and imx8mm.
Reported-by: AndréDraszik
Signed-off-by: Peter Chen
---
drivers/usb/chipidea/ci_hdrc_imx.c | 63 +-
1 file changed, 35 insertions(+), 28 deletions(-)
diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
b/drivers/usb/chipidea/ci_hdrc_imx.c
index c34fcc079cd4..d8e7eb2f97b9 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -274,11 +274,14 @@ static int ci_hdrc_imx_notify_event(struct ci_hdrc *ci,
unsigned int event)
switch (event) {
case CI_HDRC_IMX_HSIC_ACTIVE_EVENT:
- ret = pinctrl_select_state(data->pinctrl,
- data->pinctrl_hsic_active);
- if (ret)
- dev_err(dev, "hsic_active select failed, err=%d\n",
- ret);
+ if (data->pinctrl) {
+ ret = pinctrl_select_state(data->pinctrl,
+ data->pinctrl_hsic_active);
+ if (ret)
+ dev_err(dev,
+ "hsic_active select failed, err=%d\n",
+ ret);
+ }
break;
case CI_HDRC_IMX_HSIC_SUSPEND_EVENT:
ret = imx_usbmisc_hsic_set_connect(data->usbmisc_data);
@@ -306,7 +309,6 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
const struct ci_hdrc_imx_platform_flag *imx_platform_flag;
struct device_node *np = pdev->dev.of_node;
struct device *dev = &pdev->dev;
- struct pinctrl_state *pinctrl_hsic_idle;
of_id = of_match_device(ci_hdrc_imx_dt_ids, dev);
if (!of_id)
@@ -339,6 +341,33 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
return PTR_ERR(data->pinctrl);
}
+ data->hsic_pad_regulator =
+ devm_regulator_get_optional(dev, "hsic");
+ if (PTR_ERR(data->hsic_pad_regulator) == -ENODEV) {
+ /* no pad regualator is needed */
+ data->hsic_pad_regulator = NULL;
+ } else if (IS_ERR(data->hsic_pad_regulator)) {
+ if (PTR_ERR(data->hsic_pad_regulator) != -EPROBE_DEFER)
+ dev_err(dev,
+ "Get HSIC pad regulator error: %ld\n",
+ PTR_ERR(data->hsic_pad_regulator));
+ return PTR_ERR(data->hsic_pad_regulator);
+ }
+
+ if (data->hsic_pad_regulator) {
+ ret = regulator_enable(data->hsic_pad_regulator);
+ if (ret) {
+ dev_err(dev,
+ "Failed to enable HSIC pad
regulator\n");
+ return ret;
+ }
+ }
+ }
+
+ /* HSIC pinctrl handling */
+ if (data->pinctrl) {
+ struct pinctrl_state *pinctrl_hsic_idle;
+
pinctrl_hsic_idle = pinctrl_lookup_state(data->pinctrl, "idle");
if (IS_ERR(pinctrl_hsic_idle)) {
dev_err(dev,
@@ -361,28 +390,6 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
PTR_ERR(data->pinctrl_hsic_active));
return PTR_ERR(data->pinctrl_hsic_active);
}
-
- data->hsic_pad_regulator =
- devm_regulator_get_optional(dev, "hsic");
- if (PTR_ERR(data->hsic_pad_regulator) == -ENODEV) {
- /* no pad regualator is needed */
- data->hsic_pad_regulator = NULL;
- } else if (IS_ERR(data->hsic_pad_regulator)) {
- if (PTR_ERR(data->hsic_pad_regulator) != -EPROBE_DEFER)
- dev_err(dev,
- "Get HSIC pad regulator error: %ld\n",
- PTR_ERR(data->hsic_pad_regulator));
- return PTR_ERR(data->hsic_pad_regulator);
- }
-
- if (data->hsic_pad_regulator) {
- ret = regulator_enable(data->hsic_pad_regulator);
- if (ret) {
- dev_err(dev,
- "Failed to enable HSIC pad
regulator\n");
- return ret;
- }
- }
}
if (pdata.flags & CI_HDRC_PMQOS)
--
2.17.1
usb: dwc2: Re: Maximum packet size in dwc2 gadget HS mode < 1024
Hi Pavel, On 10/10/2019 8:36 PM, Pavel Hofman wrote: > I forgot version information - branch 5.3 > https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_raspberrypi_linux_tree_rpi-2D5.3.y_drivers_usb_dwc2&d=DwICBA&c=DPL6_X_6JkXFx7AXWqB0tg&r=cQBKt4q-qzNVC53rNAwuwplH23V61rHQhhULvdLA0U8&m=roVTrb8UpajEEelBpJicBEnDNjj4_Ee8-BPoSHmYa-8&s=6Ijfw31Oy-ep8P8YzooBpOpciF5CLCBeGj7iYuXzQMQ&e= > > with latest version of > https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_torvalds_linux_blob_master_drivers_usb_dwc2_gadget.c&d=DwICBA&c=DPL6_X_6JkXFx7AXWqB0tg&r=cQBKt4q-qzNVC53rNAwuwplH23V61rHQhhULvdLA0U8&m=roVTrb8UpajEEelBpJicBEnDNjj4_Ee8-BPoSHmYa-8&s=eEEdIx-XNzSx7PzLmt5XYWeoRfeqyI1W5aZLt2GNSok&e= > > (i.e. only a few patches behind upstream master). > > Dne 10. 10. 19 v 18:33 Pavel Hofman napsal(a): >> Dear Mr. Harutyunyan, >> >> I am turning to you as the core developer of the dwc2 driver in linux. >> I would like to ask you for help with debugging the following issue: >> >> I would like to improve the g_audio gadget for RaspberryPi4 (the >> gadget alsa device stalls when no USB communication occurs + a few >> other issues but this is not important now). >> >> EP IN works perfectly, I could pass audio 768kHz/3 bytes/3 channels >> which is the maximum for one endpoint (768/8 * 3 * 3 = 864 bytes every >> 125us frame) >> >> But for EP OUT, every g_audio configuration creating USB packet size > >> 900 bytes (approx) results in dwc2/gadget.c not passing any data to >> the audio function (req->actual is always zero). >> >> For interval=8 (bInterval = 4, i.e. data every 1ms): >> >> * 32kHz/1byte/28 channels = 896 bytes packet size -> working perfect, >> bitperfect transmission >> >> * 32kHz/1byte/29 channels = 928 bytes packet size -> no data passed to >> g_audio.c (dwc2_hsotg_handle_outdone() is never called in >> dwc2_hsotg_epint() >> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_torvalds_linux_blob_master_drivers_usb_dwc2_gadget.c-23L3039&d=DwICBA&c=DPL6_X_6JkXFx7AXWqB0tg&r=cQBKt4q-qzNVC53rNAwuwplH23V61rHQhhULvdLA0U8&m=roVTrb8UpajEEelBpJicBEnDNjj4_Ee8-BPoSHmYa-8&s=wMRtUtO5zFAK062HmVWxGAVxbpX9h9wlpzJaQU_URkg&e= >> >> ) >> >> I tried for different packet sizes, every setup below 900 bytes works, >> every setup above 900 bytes does not work. >> >> Tested with host x86 linux PC and USB loopback on the same RPi (from >> the other USB onboard controller), exactly same behavior for both hosts. >> >> It looks as if the DWC2 considers all isochronous packets with size > >> 900 bytes as incomplete. I do not have a hardware USB analyzer, but >> wireshark on the host PC shows the packets are properly passed to the >> USB controller in linux. >> >> >> I found a DWC2 datasheet at >> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.intel.com_content_www_us_en_programmable_documentation_sfo1410144425160.html&d=DwICBA&c=DPL6_X_6JkXFx7AXWqB0tg&r=cQBKt4q-qzNVC53rNAwuwplH23V61rHQhhULvdLA0U8&m=roVTrb8UpajEEelBpJicBEnDNjj4_Ee8-BPoSHmYa-8&s=bHXds7A-CT3K5jg2C8JX8Gvcl2Yy-ojtuLM5Z4AkrFc&e= >> >> . >> >> I can provide any debugging information, debugfs of the dwc2 module, >> kernel dynamic debug traces etc. >> >> RPi4 would make a great audio device if the gadget mode worked >> reliably. I can fix/code the alsa device issues, but USB core is above >> my skills. >> >> Thank you very much in advance. >> >> Best regards, >> >> Pavel Hofman. First of all I'll prefer communicate via Linux-usb mailing list . Let's keep this mailing list address in CC. And one more thing, keep on the beginning of subject follow string: "usb: dwc2: " Thank you for your email. I'll dedicate time on the next week to debug your case. Thanks, Minas
