Re: [PATCH] USB: usbfs: Suppress emission of uevents for interfaces handled via usbfs

[Greg KH <gregkh@linuxfoundation.org>]

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