| Message ID | 20220804160306.134014-2-hadess@hadess.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC,v2,1/2] USB: core: add a way to revoke access to open USB devices | expand |
On Thu, Aug 04, 2022 at 06:03:05PM +0200, Bastien Nocera wrote: > 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 | 105 ++++++++++++++++++++++++++++++++++++--- > drivers/usb/core/usb.h | 8 +++ > 2 files changed, 107 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c > index b5b85bf80329..a87fed12e307 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; > }; Have you considered what should happen if two processes share the same file descriptor (and hence the same usb_dev_state structure) and you want to revoke access for one of the processes but not the other? > struct usb_memory { > @@ -237,6 +238,13 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma) > dma_addr_t dma_handle; > int ret; > > + usb_lock_device(ps->dev); > + if (!connected(ps) || ps->revoked) { > + usb_unlock_device(ps->dev); > + return -ENODEV; > + } > + usb_unlock_device(ps->dev); I'm not certain this check is needed at all. But if you want to add it, I don't see any reason for grab the lock. Also, here and in all the other places, instead of manually adding "|| ps_revoked" to all the "!connected(ps)" checks, why not just change the definition of connected(ps)? > + > ret = usbfs_increase_memory_usage(size + sizeof(struct usb_memory)); > if (ret) > goto error; > @@ -298,6 +306,15 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma) > return ret; > } > > +static loff_t usbdev_llseek(struct file *file, loff_t offset, int whence) > +{ > + struct usb_dev_state *ps = file->private_data; > + > + if (!connected(ps) || ps->revoked) > + return -ENODEV; Like the case above, this check is not present in the current code so it's not clear why it needs to be added now. > + return no_seek_end_llseek(file, offset, whence); > +} > + > static ssize_t usbdev_read(struct file *file, char __user *buf, size_t nbytes, > loff_t *ppos) > { > @@ -310,7 +327,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 (!connected(ps) || ps->revoked) { > ret = -ENODEV; > goto err; > } else if (pos < 0) { > @@ -2117,7 +2134,7 @@ static int proc_reapurbnonblock(struct usb_dev_state *ps, void __user *arg) > retval = processcompl(as, (void __user * __user *)arg); > free_async(as); > } else { > - retval = (connected(ps) ? -EAGAIN : -ENODEV); > + retval = (connected(ps) && !ps->revoked ? -EAGAIN : -ENODEV); > } > return retval; > } > @@ -2262,7 +2279,7 @@ static int proc_reapurbnonblock_compat(struct usb_dev_state *ps, void __user *ar > retval = processcompl_compat(as, (void __user * __user *)arg); > free_async(as); > } else { > - retval = (connected(ps) ? -EAGAIN : -ENODEV); > + retval = (connected(ps) && !ps->revoked ? -EAGAIN : -ENODEV); > } > return retval; > } > @@ -2580,6 +2597,82 @@ 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; > + } > + 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); > + > + snoop(&dev->dev, "%s: REVOKE by PID %d: %s\n", __func__, > + task_pid_nr(current), current->comm); I would put this snoop call before all the other activities, so that any debugging output they generate will come after the REVOKE entry in the kernel log. > + > + return 0; > +} > + > +int usb_revoke(struct usb_device *udev, > + struct usb_revoke_match *match) > +{ > + struct usb_dev_state *ps; > + int ret = -ENOENT; > + > + usb_lock_device(udev); > + > + if (match->devnum >= 0 && > + match->busnum >= 0) { > + int devnum, busnum; > + > + devnum = udev->devnum; > + busnum = udev->bus->busnum; > + if (match->busnum != busnum || > + match->devnum != devnum) { > + ret = -ENODEV; > + goto out; > + } > + } I have the feeling that this part of the function (matching the busnum and devnum values) doesn't belong here but rather with the iteration routines in your 2/2 patch. Filtering of devices generally is done as part of the iteration. As an added bonus, doing it that way means you don't need to pass around pointers to usb_revoke_match structures. > + > + list_for_each_entry(ps, &udev->filelist, list) { > + if (match->euid >= 0) { > + kuid_t kuid; > + > + if (!ps || !ps->cred) > + continue; > + kuid = ps->cred->euid; > + if (kuid.val != match->euid) > + continue; > + } > + > + if (ret == 0) > + usbdev_revoke(ps); > + else > + ret = usbdev_revoke(ps); You probably don't need this elaborate handling of return codes. In fact, you probably don't need either of these functions to return anything. Alan Stern > + } > + > +out: > + usb_unlock_device(udev); > + return ret; > +} > + > /* > * NOTE: All requests here that have interface numbers as parameters > * are assuming that somehow the configuration has been prevented from > @@ -2623,7 +2716,7 @@ static long usbdev_do_ioctl(struct file *file, unsigned int cmd, > #endif > } > > - if (!connected(ps)) { > + if (!connected(ps) || ps->revoked) { > usb_unlock_device(dev); > return -ENODEV; > } > @@ -2819,7 +2912,7 @@ static __poll_t usbdev_poll(struct file *file, > poll_wait(file, &ps->wait, wait); > if (file->f_mode & FMODE_WRITE && !list_empty(&ps->async_completed)) > mask |= EPOLLOUT | EPOLLWRNORM; > - if (!connected(ps)) > + if (!connected(ps) || ps->revoked) > mask |= EPOLLHUP; > if (list_empty(&ps->list)) > mask |= EPOLLERR; > @@ -2828,7 +2921,7 @@ static __poll_t usbdev_poll(struct file *file, > > const struct file_operations usbdev_file_operations = { > .owner = THIS_MODULE, > - .llseek = no_seek_end_llseek, > + .llseek = usbdev_llseek, > .read = usbdev_read, > .poll = usbdev_poll, > .unlocked_ioctl = usbdev_ioctl, > diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h > index 82538daac8b8..b12c352869f0 100644 > --- a/drivers/usb/core/usb.h > +++ b/drivers/usb/core/usb.h > @@ -9,6 +9,11 @@ > struct usb_hub_descriptor; > struct usb_dev_state; > > +struct usb_revoke_match { > + int busnum, devnum; /* -1 to match all devices */ > + int euid; /* -1 to match all users */ > +}; > + > /* Functions local to drivers/usb/core/ */ > > extern int usb_create_sysfs_dev_files(struct usb_device *dev); > @@ -34,6 +39,9 @@ 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(struct usb_device *udev, > + struct usb_revoke_match *match); > +extern int usbdev_get_uid(struct usb_dev_state *ps); > 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); > -- > 2.36.1 >
On Thu, 2022-08-04 at 15:25 -0400, Alan Stern wrote: > On Thu, Aug 04, 2022 at 06:03:05PM +0200, Bastien Nocera wrote: > > 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 | 105 > > ++++++++++++++++++++++++++++++++++++--- > > drivers/usb/core/usb.h | 8 +++ > > 2 files changed, 107 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c > > index b5b85bf80329..a87fed12e307 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; > > }; > > Have you considered what should happen if two processes share the > same > file descriptor (and hence the same usb_dev_state structure) and you > want > to revoke access for one of the processes but not the other? No, because this isn't something that happens in practice, as it's simpler for each programme to open their own file descriptor and claim the interfaces they need on their own. > > struct usb_memory { > > @@ -237,6 +238,13 @@ static int usbdev_mmap(struct file *file, > > struct vm_area_struct *vma) > > dma_addr_t dma_handle; > > int ret; > > > > + usb_lock_device(ps->dev); > > + if (!connected(ps) || ps->revoked) { > > + usb_unlock_device(ps->dev); > > + return -ENODEV; > > + } > > + usb_unlock_device(ps->dev); > > I'm not certain this check is needed at all. But if you want to add > it, > I don't see any reason for grab the lock. OK, removed. > Also, here and in all the other places, instead of manually adding > "|| > ps_revoked" to all the "!connected(ps)" checks, why not just change > the > definition of connected(ps)? I wasn't sure that all the entry points that used connected() would need to check revocation, but I'm simplified this now. > > > + > > ret = usbfs_increase_memory_usage(size + sizeof(struct > > usb_memory)); > > if (ret) > > goto error; > > @@ -298,6 +306,15 @@ static int usbdev_mmap(struct file *file, > > struct vm_area_struct *vma) > > return ret; > > } > > > > +static loff_t usbdev_llseek(struct file *file, loff_t offset, int > > whence) > > +{ > > + struct usb_dev_state *ps = file->private_data; > > + > > + if (!connected(ps) || ps->revoked) > > + return -ENODEV; > > Like the case above, this check is not present in the current code so > it's not clear why it needs to be added now. OK, removed. > > > + return no_seek_end_llseek(file, offset, whence); > > +} > > + > > static ssize_t usbdev_read(struct file *file, char __user *buf, > > size_t nbytes, > > loff_t *ppos) > > { > > @@ -310,7 +327,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 (!connected(ps) || ps->revoked) { > > ret = -ENODEV; > > goto err; > > } else if (pos < 0) { > > @@ -2117,7 +2134,7 @@ static int proc_reapurbnonblock(struct > > usb_dev_state *ps, void __user *arg) > > retval = processcompl(as, (void __user * __user > > *)arg); > > free_async(as); > > } else { > > - retval = (connected(ps) ? -EAGAIN : -ENODEV); > > + retval = (connected(ps) && !ps->revoked ? -EAGAIN : > > -ENODEV); > > } > > return retval; > > } > > @@ -2262,7 +2279,7 @@ static int proc_reapurbnonblock_compat(struct > > usb_dev_state *ps, void __user *ar > > retval = processcompl_compat(as, (void __user * > > __user *)arg); > > free_async(as); > > } else { > > - retval = (connected(ps) ? -EAGAIN : -ENODEV); > > + retval = (connected(ps) && !ps->revoked ? -EAGAIN : > > -ENODEV); > > } > > return retval; > > } > > @@ -2580,6 +2597,82 @@ 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; > > + } > > + 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); > > + > > + snoop(&dev->dev, "%s: REVOKE by PID %d: %s\n", __func__, > > + task_pid_nr(current), current->comm); > > I would put this snoop call before all the other activities, so that > any > debugging output they generate will come after the REVOKE entry in > the > kernel log. Done. > > > + > > + return 0; > > +} > > + > > +int usb_revoke(struct usb_device *udev, > > + struct usb_revoke_match *match) > > +{ > > + struct usb_dev_state *ps; > > + int ret = -ENOENT; > > + > > + usb_lock_device(udev); > > + > > + if (match->devnum >= 0 && > > + match->busnum >= 0) { > > + int devnum, busnum; > > + > > + devnum = udev->devnum; > > + busnum = udev->bus->busnum; > > + if (match->busnum != busnum || > > + match->devnum != devnum) { > > + ret = -ENODEV; > > + goto out; > > + } > > + } > > I have the feeling that this part of the function (matching the > busnum > and devnum values) doesn't belong here but rather with the iteration > routines in your 2/2 patch. Filtering of devices generally is done > as > part of the iteration. As an added bonus, doing it that way means > you > don't need to pass around pointers to usb_revoke_match structures. I felt it better to have the filtering done in one place, to avoid passing just a uid to check to that function. Should I rename the function something like usb_revoke_for_uid() ? > > > + > > + list_for_each_entry(ps, &udev->filelist, list) { > > + if (match->euid >= 0) { > > + kuid_t kuid; > > + > > + if (!ps || !ps->cred) > > + continue; > > + kuid = ps->cred->euid; > > + if (kuid.val != match->euid) > > + continue; > > + } > > + > > + if (ret == 0) > > + usbdev_revoke(ps); > > + else > > + ret = usbdev_revoke(ps); > > You probably don't need this elaborate handling of return codes. In > fact, you probably don't need either of these functions to return > anything. OK. > > Alan Stern > > > + } > > + > > +out: > > + usb_unlock_device(udev); > > + return ret; > > +} > > + > > /* > > * NOTE: All requests here that have interface numbers as > > parameters > > * are assuming that somehow the configuration has been prevented > > from > > @@ -2623,7 +2716,7 @@ static long usbdev_do_ioctl(struct file > > *file, unsigned int cmd, > > #endif > > } > > > > - if (!connected(ps)) { > > + if (!connected(ps) || ps->revoked) { > > usb_unlock_device(dev); > > return -ENODEV; > > } > > @@ -2819,7 +2912,7 @@ static __poll_t usbdev_poll(struct file > > *file, > > poll_wait(file, &ps->wait, wait); > > if (file->f_mode & FMODE_WRITE && !list_empty(&ps- > > >async_completed)) > > mask |= EPOLLOUT | EPOLLWRNORM; > > - if (!connected(ps)) > > + if (!connected(ps) || ps->revoked) > > mask |= EPOLLHUP; > > if (list_empty(&ps->list)) > > mask |= EPOLLERR; > > @@ -2828,7 +2921,7 @@ static __poll_t usbdev_poll(struct file > > *file, > > > > const struct file_operations usbdev_file_operations = { > > .owner = THIS_MODULE, > > - .llseek = no_seek_end_llseek, > > + .llseek = usbdev_llseek, > > .read = usbdev_read, > > .poll = usbdev_poll, > > .unlocked_ioctl = usbdev_ioctl, > > diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h > > index 82538daac8b8..b12c352869f0 100644 > > --- a/drivers/usb/core/usb.h > > +++ b/drivers/usb/core/usb.h > > @@ -9,6 +9,11 @@ > > struct usb_hub_descriptor; > > struct usb_dev_state; > > > > +struct usb_revoke_match { > > + int busnum, devnum; /* -1 to match all devices */ > > + int euid; /* -1 to match all users */ > > +}; > > + > > /* Functions local to drivers/usb/core/ */ > > > > extern int usb_create_sysfs_dev_files(struct usb_device *dev); > > @@ -34,6 +39,9 @@ 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(struct usb_device *udev, > > + struct usb_revoke_match *match); > > +extern int usbdev_get_uid(struct usb_dev_state *ps); > > 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); > > -- > > 2.36.1 > >
On Fri, Aug 05, 2022 at 02:38:13PM +0200, Bastien Nocera wrote: > On Thu, 2022-08-04 at 15:25 -0400, Alan Stern wrote: > > Have you considered what should happen if two processes share the > > same > > file descriptor (and hence the same usb_dev_state structure) and you > > want > > to revoke access for one of the processes but not the other? > > No, because this isn't something that happens in practice, as it's > simpler for each programme to open their own file descriptor and claim > the interfaces they need on their own. But it is possible for a program to open a USB device and then fork several children. The children would all share the same file descriptor. I have no idea how often this happens in practice. But kernel design is supposed to be based on correctness, not on handling only things that crop up "in practice". > > I have the feeling that this part of the function (matching the > > busnum > > and devnum values) doesn't belong here but rather with the iteration > > routines in your 2/2 patch. Filtering of devices generally is done > > as > > part of the iteration. As an added bonus, doing it that way means > > you > > don't need to pass around pointers to usb_revoke_match structures. > > I felt it better to have the filtering done in one place, to avoid > passing just a uid to check to that function. There's nothing wrong with passing just a uid. Especially since the same device might be open multiple times, for varying uids. > Should I rename the function something like usb_revoke_for_uid() ? Up to you. Alan Stern
On Fri, 2022-08-05 at 10:42 -0400, Alan Stern wrote: > > On Fri, Aug 05, 2022 at 02:38:13PM +0200, Bastien Nocera wrote: > > > > On Thu, 2022-08-04 at 15:25 -0400, Alan Stern wrote: > > > > > > Have you considered what should happen if two processes > > > > > > share the > > > > > > same > > > > > > file descriptor (and hence the same usb_dev_state > > > > > > structure) and > > > you > > > > > > want > > > > > > to revoke access for one of the processes but not the > > > > > > other? > > > > > > > > No, because this isn't something that happens in practice, as > > > > it's > > > > simpler for each programme to open their own file descriptor > > > > and > > claim > > > > the interfaces they need on their own. > > > > But it is possible for a program to open a USB device and then fork > > several children. The children would all share the same file > > > descriptor. > > > > I have no idea how often this happens in practice. But kernel > > design > is > > supposed to be based on correctness, not on handling only things > > that > > crop up "in practice". Given that the API makes no such promises, I think we're fine. The end goal is "user can't use device" or possibly "user in namespace can't use device" to cut sandboxes off from their USB devices. We don't need surgical precision revocation, but I would be happy to implement this API if we can come up with a good way to target specific programs. > > > > > > I have the feeling that this part of the function (matching > > > > > > the > > > > > > busnum > > > > > > and devnum values) doesn't belong here but rather with the > > > > > > > > > iteration > > > > > > routines in your 2/2 patch. Filtering of devices generally > > > > > > is > > > done > > > > > > as > > > > > > part of the iteration. As an added bonus, doing it that > > > > > > way > > > means > > > > > > you > > > > > > don't need to pass around pointers to usb_revoke_match > > > > > > > > > structures. > > > > > > > > I felt it better to have the filtering done in one place, to > > > > avoid > > > > passing just a uid to check to that function. > > > > There's nothing wrong with passing just a uid. Especially since > > the > same > > device might be open multiple times, for varying uids. > > > > > > Should I rename the function something like > > > > usb_revoke_for_uid() ? > > > > Up to you. I've done that now.
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c index b5b85bf80329..a87fed12e307 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 { @@ -237,6 +238,13 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma) dma_addr_t dma_handle; int ret; + usb_lock_device(ps->dev); + if (!connected(ps) || ps->revoked) { + usb_unlock_device(ps->dev); + return -ENODEV; + } + usb_unlock_device(ps->dev); + ret = usbfs_increase_memory_usage(size + sizeof(struct usb_memory)); if (ret) goto error; @@ -298,6 +306,15 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma) return ret; } +static loff_t usbdev_llseek(struct file *file, loff_t offset, int whence) +{ + struct usb_dev_state *ps = file->private_data; + + if (!connected(ps) || ps->revoked) + return -ENODEV; + return no_seek_end_llseek(file, offset, whence); +} + static ssize_t usbdev_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos) { @@ -310,7 +327,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 (!connected(ps) || ps->revoked) { ret = -ENODEV; goto err; } else if (pos < 0) { @@ -2117,7 +2134,7 @@ static int proc_reapurbnonblock(struct usb_dev_state *ps, void __user *arg) retval = processcompl(as, (void __user * __user *)arg); free_async(as); } else { - retval = (connected(ps) ? -EAGAIN : -ENODEV); + retval = (connected(ps) && !ps->revoked ? -EAGAIN : -ENODEV); } return retval; } @@ -2262,7 +2279,7 @@ static int proc_reapurbnonblock_compat(struct usb_dev_state *ps, void __user *ar retval = processcompl_compat(as, (void __user * __user *)arg); free_async(as); } else { - retval = (connected(ps) ? -EAGAIN : -ENODEV); + retval = (connected(ps) && !ps->revoked ? -EAGAIN : -ENODEV); } return retval; } @@ -2580,6 +2597,82 @@ 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; + } + 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); + + snoop(&dev->dev, "%s: REVOKE by PID %d: %s\n", __func__, + task_pid_nr(current), current->comm); + + return 0; +} + +int usb_revoke(struct usb_device *udev, + struct usb_revoke_match *match) +{ + struct usb_dev_state *ps; + int ret = -ENOENT; + + usb_lock_device(udev); + + if (match->devnum >= 0 && + match->busnum >= 0) { + int devnum, busnum; + + devnum = udev->devnum; + busnum = udev->bus->busnum; + if (match->busnum != busnum || + match->devnum != devnum) { + ret = -ENODEV; + goto out; + } + } + + list_for_each_entry(ps, &udev->filelist, list) { + if (match->euid >= 0) { + kuid_t kuid; + + if (!ps || !ps->cred) + continue; + kuid = ps->cred->euid; + if (kuid.val != match->euid) + continue; + } + + if (ret == 0) + usbdev_revoke(ps); + else + ret = usbdev_revoke(ps); + } + +out: + usb_unlock_device(udev); + return ret; +} + /* * NOTE: All requests here that have interface numbers as parameters * are assuming that somehow the configuration has been prevented from @@ -2623,7 +2716,7 @@ static long usbdev_do_ioctl(struct file *file, unsigned int cmd, #endif } - if (!connected(ps)) { + if (!connected(ps) || ps->revoked) { usb_unlock_device(dev); return -ENODEV; } @@ -2819,7 +2912,7 @@ static __poll_t usbdev_poll(struct file *file, poll_wait(file, &ps->wait, wait); if (file->f_mode & FMODE_WRITE && !list_empty(&ps->async_completed)) mask |= EPOLLOUT | EPOLLWRNORM; - if (!connected(ps)) + if (!connected(ps) || ps->revoked) mask |= EPOLLHUP; if (list_empty(&ps->list)) mask |= EPOLLERR; @@ -2828,7 +2921,7 @@ static __poll_t usbdev_poll(struct file *file, const struct file_operations usbdev_file_operations = { .owner = THIS_MODULE, - .llseek = no_seek_end_llseek, + .llseek = usbdev_llseek, .read = usbdev_read, .poll = usbdev_poll, .unlocked_ioctl = usbdev_ioctl, diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h index 82538daac8b8..b12c352869f0 100644 --- a/drivers/usb/core/usb.h +++ b/drivers/usb/core/usb.h @@ -9,6 +9,11 @@ struct usb_hub_descriptor; struct usb_dev_state; +struct usb_revoke_match { + int busnum, devnum; /* -1 to match all devices */ + int euid; /* -1 to match all users */ +}; + /* Functions local to drivers/usb/core/ */ extern int usb_create_sysfs_dev_files(struct usb_device *dev); @@ -34,6 +39,9 @@ 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(struct usb_device *udev, + struct usb_revoke_match *match); +extern int usbdev_get_uid(struct usb_dev_state *ps); 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);
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 | 105 ++++++++++++++++++++++++++++++++++++--- drivers/usb/core/usb.h | 8 +++ 2 files changed, 107 insertions(+), 6 deletions(-)