| Message ID | 20191009123829.07eacc7f@ingpc3.intern.lauterbach.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | USB: usbfs: Suppress emission of uevents for interfaces handled via usbfs | expand |
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 <ingo.rohloff@lauterbach.com> > 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 <ingo.rohloff@lauterbach.com> > --- > 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
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.
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) + dev_set_uevent_suppress(&intf->dev, 0); + } if (err == 0) set_bit(ifnum, &ps->ifclaimed); return err; diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c index 2b27d232d7a7..6a15bc5c2869 100644 --- a/drivers/usb/core/driver.c +++ b/drivers/usb/core/driver.c @@ -594,6 +594,8 @@ void usb_driver_release_interface(struct usb_driver *driver, */ if (device_is_registered(dev)) { device_release_driver(dev); + /* make sure we allow uevents again */ + dev_set_uevent_suppress(dev, 0); } else { device_lock(dev); usb_unbind_interface(dev);