Re: [PATCH 1/2] USB: core: add a way to revoke access to open USB devices

["Eric W. Biederman" <ebiederm@xmission.com>]

Bastien Nocera <hadess@hadess.net> writes:

> There is a need for userspace applications to open USB devices directly,
> for all the USB devices without a kernel-level class driver[1], and
> implemented in user-space.
>
> As not all devices are built equal, we want to be able to revoke
> access to those devices whether it's because the user isn't at the
> console anymore, or because the web browser, or sandbox doesn't want
> to allow access to that device.
>
> This commit implements the internal API used to revoke access to USB
> devices, given either bus and device numbers, or/and a user's
> effective UID.
>
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
>
> [1]:
> Non exhaustive list of devices and device types that need raw USB access:
> - all manners of single-board computers and programmable chips and
> devices (avrdude, STLink, sunxi bootloader, flashrom, etc.)
> - 3D printers
> - scanners
> - LCD "displays"
> - user-space webcam and still cameras
> - game controllers
> - video/audio capture devices
> - sensors
> - software-defined radios
> - DJ/music equipment
> - protocol analysers
> - Rio 500 music player
> ---
>  drivers/usb/core/devio.c | 79 ++++++++++++++++++++++++++++++++++++++--
>  drivers/usb/core/usb.h   |  2 +
>  2 files changed, 77 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index b5b85bf80329..d8d212ef581f 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -78,6 +78,7 @@ struct usb_dev_state {
>  	int not_yet_resumed;
>  	bool suspend_allowed;
>  	bool privileges_dropped;
> +	bool revoked;
>  };
>  
>  struct usb_memory {
> @@ -183,6 +184,13 @@ static int connected(struct usb_dev_state *ps)
>  			ps->dev->state != USB_STATE_NOTATTACHED);
>  }
>  
> +static int disconnected_or_revoked(struct usb_dev_state *ps)
> +{
> +	return (list_empty(&ps->list) ||
> +			ps->dev->state == USB_STATE_NOTATTACHED ||
> +			ps->revoked);
> +}
> +
>  static void dec_usb_memory_use_count(struct usb_memory *usbm, int *count)
>  {
>  	struct usb_dev_state *ps = usbm->ps;
> @@ -237,6 +245,9 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
>  	dma_addr_t dma_handle;
>  	int ret;
>  
> +	if (disconnected_or_revoked(ps))
> +		return -ENODEV;
> +
>  	ret = usbfs_increase_memory_usage(size + sizeof(struct usb_memory));
>  	if (ret)
>  		goto error;
> @@ -310,7 +321,7 @@ static ssize_t usbdev_read(struct file *file, char __user *buf, size_t nbytes,
>  
>  	pos = *ppos;
>  	usb_lock_device(dev);
> -	if (!connected(ps)) {
> +	if (disconnected_or_revoked(ps)) {
>  		ret = -ENODEV;
>  		goto err;
>  	} else if (pos < 0) {
> @@ -2315,7 +2326,7 @@ static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl)
>  	if (ps->privileges_dropped)
>  		return -EACCES;
>  
> -	if (!connected(ps))
> +	if (disconnected_or_revoked(ps))
>  		return -ENODEV;
>  
>  	/* alloc buffer */
> @@ -2555,7 +2566,7 @@ static int proc_forbid_suspend(struct usb_dev_state *ps)
>  
>  static int proc_allow_suspend(struct usb_dev_state *ps)
>  {
> -	if (!connected(ps))
> +	if (disconnected_or_revoked(ps))
>  		return -ENODEV;
>  
>  	WRITE_ONCE(ps->not_yet_resumed, 1);
> @@ -2580,6 +2591,66 @@ static int proc_wait_for_resume(struct usb_dev_state *ps)
>  	return proc_forbid_suspend(ps);
>  }
>  
> +static int usbdev_revoke(struct usb_dev_state *ps)
> +{
> +	struct usb_device *dev = ps->dev;
> +	unsigned int ifnum;
> +	struct async *as;
> +
> +	if (ps->revoked) {
> +		usb_unlock_device(dev);
> +		return -ENODEV;
> +	}
> +
> +	snoop(&dev->dev, "%s: REVOKE by PID %d: %s\n", __func__,
> +	      task_pid_nr(current), current->comm);
> +
> +	ps->revoked = true;
> +
> +	for (ifnum = 0; ps->ifclaimed && ifnum < 8*sizeof(ps->ifclaimed);
> +			ifnum++) {
> +		if (test_bit(ifnum, &ps->ifclaimed))
> +			releaseintf(ps, ifnum);
> +	}
> +	destroy_all_async(ps);
> +
> +	as = async_getcompleted(ps);
> +	while (as) {
> +		free_async(as);
> +		as = async_getcompleted(ps);
> +	}
> +
> +	wake_up(&ps->wait);
> +
> +	return 0;
> +}
> +
> +int usb_revoke_for_euid(struct usb_device *udev,
> +		int euid)
                ^^^

At a minimum that should be a kuid_t.

> +{
> +	struct usb_dev_state *ps;
> +
> +	usb_lock_device(udev);
> +
> +	list_for_each_entry(ps, &udev->filelist, list) {
> +		if (euid >= 0) {
                    ^^^^^^^^^
That test should be uid_valid.

> +			kuid_t kuid;
> +
> +			if (!ps || !ps->cred)
> +				continue;
> +			kuid = ps->cred->euid;
> +			if (kuid.val != euid)
                        ^^^^^^^^^^^^^^^^^^^^^
That test should be if (!uid_eq(ps->cred->euid, euid))


The point is that inside the kernel all uid data should be dealt with
in the kuid_t data type.  So as to avoid confusing uids with some other
kind of integer data.

> +				continue;
> +		}
> +
> +		usbdev_revoke(ps);
> +	}
> +
> +out:
> +	usb_unlock_device(udev);
> +	return 0;
> +}
> +
>  /*
>   * NOTE:  All requests here that have interface numbers as parameters
>   * are assuming that somehow the configuration has been prevented from
> @@ -2623,7 +2694,7 @@ static long usbdev_do_ioctl(struct file *file, unsigned int cmd,
>  #endif
>  	}
>  
> -	if (!connected(ps)) {
> +	if (disconnected_or_revoked(ps)) {
>  		usb_unlock_device(dev);
>  		return -ENODEV;
>  	}
> diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
> index 82538daac8b8..5b007530a1cf 100644
> --- a/drivers/usb/core/usb.h
> +++ b/drivers/usb/core/usb.h
> @@ -34,6 +34,8 @@ extern int usb_deauthorize_device(struct usb_device *);
>  extern int usb_authorize_device(struct usb_device *);
>  extern void usb_deauthorize_interface(struct usb_interface *);
>  extern void usb_authorize_interface(struct usb_interface *);
> +extern int usb_revoke_for_euid(struct usb_device *udev,
> +		int euid);
>  extern void usb_detect_quirks(struct usb_device *udev);
>  extern void usb_detect_interface_quirks(struct usb_device *udev);
>  extern void usb_release_quirk_list(void);

Eric