| Message ID | 20220809094300.83116-2-hadess@hadess.net (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | BPF |
| Headers | show |
| Series | USB: core: add a way to revoke access to open USB devices | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for Kernel LATEST on ubuntu-latest with gcc |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for Kernel LATEST on ubuntu-latest with llvm-16 |
| bpf/vmtest-bpf-next-PR | success | PR summary |
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for Kernel LATEST on z15 with gcc |
On Tue, Aug 09, 2022 at 11:42:59AM +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 We can't take "footnotes" after a signed-off-by line, you know this :(
On Tue, Aug 09, 2022 at 11:42:59AM +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. Revoke what exactly? You should be revoking the file descriptor that is open, not anything else as those are all transient and not uniquely identified and racy. > > 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) > +{ > + struct usb_dev_state *ps; > + > + usb_lock_device(udev); > + > + list_for_each_entry(ps, &udev->filelist, list) { > + if (euid >= 0) { > + kuid_t kuid; > + > + if (!ps || !ps->cred) > + continue; > + kuid = ps->cred->euid; How is this handling uid namespaces? Again, just revoke the file descriptor, like the BSDs do for a tiny subset of device drivers. This comes up ever so often, why does someone not just add real revoke(2) support to Linux to handle it if they really really want it (I tried a long time ago, but didn't have it in me as I had no real users for it...) thanks, greg k-h
On Tue, 2022-08-09 at 12:32 +0200, Greg Kroah-Hartman wrote: > On Tue, Aug 09, 2022 at 11:42:59AM +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 > > We can't take "footnotes" after a signed-off-by line, you know this > :( Where would I know this from? checkpatch.pl doesn't warn about it.
On Tue, 2022-08-09 at 12:35 +0200, Greg Kroah-Hartman wrote: > On Tue, Aug 09, 2022 at 11:42:59AM +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. > > Revoke what exactly? Access. > You should be revoking the file descriptor that is > open, not anything else as those are all transient and not uniquely > identified and racy. They're "unique enough" (tm). The > > > > > > > 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) > > +{ > > + struct usb_dev_state *ps; > > + > > + usb_lock_device(udev); > > + > > + list_for_each_entry(ps, &udev->filelist, list) { > > + if (euid >= 0) { > > + kuid_t kuid; > > + > > + if (!ps || !ps->cred) > > + continue; > > + kuid = ps->cred->euid; > > How is this handling uid namespaces? The caller is supposed to be outside namespaces. It's a BPF programme, so must be loaded by a privileged caller. > > Again, just revoke the file descriptor, like the BSDs do for a tiny > subset of device drivers. > > This comes up ever so often, why does someone not just add real > revoke(2) support to Linux to handle it if they really really want it > (I > tried a long time ago, but didn't have it in me as I had no real > users > for it...) This was already explained twice, there's nothing to "hand out" those file descriptors, so no one but the kernel and the programme itself have that file descriptor, so it can't be used as an identifier.
On Tue, Aug 09, 2022 at 01:15:50PM +0200, Bastien Nocera wrote: > On Tue, 2022-08-09 at 12:32 +0200, Greg Kroah-Hartman wrote: > > On Tue, Aug 09, 2022 at 11:42:59AM +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 > > > > We can't take "footnotes" after a signed-off-by line, you know this > > :( > > Where would I know this from? To quote from our documentation: The sign-off is a simple line at the end of the explanation for the patch, > checkpatch.pl doesn't warn about it. Odd, it should. Send a patch for that? :) thanks, greg k-h
On Tue, 2022-08-09 at 13:30 +0200, Greg Kroah-Hartman wrote: > > checkpatch.pl doesn't warn about it. > > Odd, it should. Send a patch for that? :) I haven't written any Perl in 20-odd years, and sold my 1999 Perl Cookbook years ago, so probably not.
On Tue, Aug 09, 2022 at 01:18:43PM +0200, Bastien Nocera wrote: > On Tue, 2022-08-09 at 12:35 +0200, Greg Kroah-Hartman wrote: > > On Tue, Aug 09, 2022 at 11:42:59AM +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. > > > > Revoke what exactly? > > Access. Access from what? > > You should be revoking the file descriptor that is > > open, not anything else as those are all transient and not uniquely > > identified and racy. > > They're "unique enough" (tm). The Are you sure? They are racy from what I can tell, especially as you have no locking in the patch that I noticed. > > > 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) > > > +{ > > > + struct usb_dev_state *ps; > > > + > > > + usb_lock_device(udev); > > > + > > > + list_for_each_entry(ps, &udev->filelist, list) { > > > + if (euid >= 0) { > > > + kuid_t kuid; > > > + > > > + if (!ps || !ps->cred) > > > + continue; > > > + kuid = ps->cred->euid; > > > > How is this handling uid namespaces? > > The caller is supposed to be outside namespaces. It's a BPF programme, > so must be loaded by a privileged caller. Where is "outside namespaces" defined anywhere? And how is this tied to any bpf program? I don't see that interface anywhere in this series, where did I miss that? > > Again, just revoke the file descriptor, like the BSDs do for a tiny > > subset of device drivers. > > > > This comes up ever so often, why does someone not just add real > > revoke(2) support to Linux to handle it if they really really want it > > (I > > tried a long time ago, but didn't have it in me as I had no real > > users > > for it...) > > This was already explained twice, Explained where? > there's nothing to "hand out" those file descriptors, The kernel already handed out one when the device was opened using usbfs. > so no one but > the kernel and the programme itself > have that file descriptor, so it can't be used as an identifier. That's the only unique identifier that you want to revoke and "know" it is the device you are referring to. That's why I recommend creating revoke(2), not just an odd "pause the data to this device" api like this seems. That's just going to confuse lots of people as to "why did my device stop working?" thanks, greg k-h
On Tue, 2022-08-09 at 14:52 +0200, Greg Kroah-Hartman wrote: > On Tue, Aug 09, 2022 at 01:18:43PM +0200, Bastien Nocera wrote: > > On Tue, 2022-08-09 at 12:35 +0200, Greg Kroah-Hartman wrote: > > > On Tue, Aug 09, 2022 at 11:42:59AM +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. > > > > > > Revoke what exactly? > > > > Access. > > Access from what? revoke access to the USB device by one or all users. > > > You should be revoking the file descriptor that is > > > open, not anything else as those are all transient and not > > > uniquely > > > identified and racy. > > > > They're "unique enough" (tm). The > > Are you sure? > > They are racy from what I can tell, especially as you have no locking > in > the patch that I noticed. It does. It's not good locking because I rewrote that API at least 4 different times. > > > > 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) > > > > +{ > > > > + struct usb_dev_state *ps; > > > > + > > > > + usb_lock_device(udev); > > > > + > > > > + list_for_each_entry(ps, &udev->filelist, list) { > > > > + if (euid >= 0) { > > > > + kuid_t kuid; > > > > + > > > > + if (!ps || !ps->cred) > > > > + continue; > > > > + kuid = ps->cred->euid; > > > > > > How is this handling uid namespaces? > > > > The caller is supposed to be outside namespaces. It's a BPF > > programme, > > so must be loaded by a privileged caller. > > Where is "outside namespaces" defined anywhere? > > And how is this tied to any bpf program? I don't see that interface > anywhere in this series, where did I miss that? It's in 2/2. The link to the user-space programme is in the "RFC v2" version of the patch from last week. It calls into the kernel through that function which is exported through BPF. > > > > Again, just revoke the file descriptor, like the BSDs do for a > > > tiny > > > subset of device drivers. > > > > > > This comes up ever so often, why does someone not just add real > > > revoke(2) support to Linux to handle it if they really really > > > want it > > > (I > > > tried a long time ago, but didn't have it in me as I had no real > > > users > > > for it...) > > > > This was already explained twice, > > Explained where? https://www.spinics.net/lists/linux-usb/msg225448.html https://www.spinics.net/lists/linux-usb/msg229753.html > > there's nothing to "hand out" those file descriptors, > > The kernel already handed out one when the device was opened using > usbfs. > > > so no one but > > the kernel and the programme itself > > have that file descriptor, so it can't be used as an identifier. > > That's the only unique identifier that you want to revoke and "know" > it > is the device you are referring to. > > That's why I recommend creating revoke(2), not just an odd "pause the > data to this device" api like this seems. That's just going to > confuse > lots of people as to "why did my device stop working?" "Why did my app stop working. Because you switched users and your app needs to be updated to take that into account." I mean, ENODEV is returned on ioctls, it's just that the poll() doesn't HUP.
On Tue, Aug 09, 2022 at 03:27:16PM +0200, Bastien Nocera wrote: > The link to the user-space programme is in the "RFC v2" version of the > patch from last week. It calls into the kernel through that function > which is exported through BPF. > > > > > > > Again, just revoke the file descriptor, like the BSDs do for a > > > > tiny > > > > subset of device drivers. > > > > > > > > This comes up ever so often, why does someone not just add real > > > > revoke(2) support to Linux to handle it if they really really > > > > want it > > > > (I > > > > tried a long time ago, but didn't have it in me as I had no real > > > > users > > > > for it...) > > > > > > This was already explained twice, > > > > Explained where? > > https://www.spinics.net/lists/linux-usb/msg225448.html > https://www.spinics.net/lists/linux-usb/msg229753.html Please use lore.kernel.org. Anyway, pointing to random old submissions of an RFC series does not mean that you do not have to document and justify this design decision in this patch submission. Assume that reviewers have NO knowlege of previous submissions of your patch series. Because we usually do not, given how many changes we review all the time. Please resend this, as a v4, and update the changelog descriptions based on the comments so far on this series and I will be glad to review it sometime after -rc1 is out, as there's nothing I can do with it right now. thanks, greg k-h
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
On Tue, 2022-08-09 at 11:46 -0500, Eric W. Biederman wrote: > Bastien Nocera <hadess@hadess.net> writes: > > > + 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. That uid comes from user-space, see patch 2/2. Do you have examples of accepting euids from user-space and stashing them into kuid_t? If you also have any idea about user namespaces as mentioned in the cover letter for this patch set, I would appreciate.
On Tue, 2022-08-09 at 18:31 +0200, Greg Kroah-Hartman wrote: > On Tue, Aug 09, 2022 at 03:27:16PM +0200, Bastien Nocera wrote: > > The link to the user-space programme is in the "RFC v2" version of > > the > > patch from last week. It calls into the kernel through that > > function > > which is exported through BPF. > > > > > > > > > > Again, just revoke the file descriptor, like the BSDs do for > > > > > a > > > > > tiny > > > > > subset of device drivers. > > > > > > > > > > This comes up ever so often, why does someone not just add > > > > > real > > > > > revoke(2) support to Linux to handle it if they really really > > > > > want it > > > > > (I > > > > > tried a long time ago, but didn't have it in me as I had no > > > > > real > > > > > users > > > > > for it...) > > > > > > > > This was already explained twice, > > > > > > Explained where? > > > > https://www.spinics.net/lists/linux-usb/msg225448.html > > https://www.spinics.net/lists/linux-usb/msg229753.html > > Please use lore.kernel.org. Would be great if it showed up when somebody searches for "linux-usb mailing-list". > Anyway, pointing to random old submissions of an RFC series does not > mean that you do not have to document and justify this design > decision > in this patch submission. I guess me repeatedly asking for guidance as to what information I should add to the commit message while I was being yelled at didn't get through. > Assume that reviewers have NO knowlege of previous submissions of > your > patch series. Because we usually do not, given how many changes we > review all the time. > > Please resend this, as a v4, and update the changelog descriptions > based > on the comments so far on this series and I will be glad to review it > sometime after -rc1 is out, as there's nothing I can do with it right > now. Sure.
On Tue, Aug 09, 2022 at 03:27:16PM +0200, Bastien Nocera wrote: > On Tue, 2022-08-09 at 14:52 +0200, Greg Kroah-Hartman wrote: > > On Tue, Aug 09, 2022 at 01:18:43PM +0200, Bastien Nocera wrote: > > > > Revoke what exactly? > > > > > > Access. > > > > Access from what? > > revoke access to the USB device by one or all users. > > > > > You should be revoking the file descriptor that is > > > > open, not anything else as those are all transient and not > > > > uniquely > > > > identified and racy. > > > > > > They're "unique enough" (tm). The > > > > Are you sure? > > > > They are racy from what I can tell, especially as you have no locking > > in > > the patch that I noticed. > > It does. It's not good locking because I rewrote that API at least 4 > different times. > > > > How is this handling uid namespaces? > > > > > > The caller is supposed to be outside namespaces. It's a BPF > > > programme, > > > so must be loaded by a privileged caller. > > > > Where is "outside namespaces" defined anywhere? > > > > And how is this tied to any bpf program? I don't see that interface > > anywhere in this series, where did I miss that? > > It's in 2/2. > > The link to the user-space programme is in the "RFC v2" version of the > patch from last week. It calls into the kernel through that function > which is exported through BPF. > > > > > > > Again, just revoke the file descriptor, like the BSDs do for a > > > > tiny > > > > subset of device drivers. > > > > > > > > This comes up ever so often, why does someone not just add real > > > > revoke(2) support to Linux to handle it if they really really > > > > want it > > > > (I > > > > tried a long time ago, but didn't have it in me as I had no real > > > > users > > > > for it...) > > > > > > This was already explained twice, > > > > Explained where? > > https://www.spinics.net/lists/linux-usb/msg225448.html > https://www.spinics.net/lists/linux-usb/msg229753.html > > > > there's nothing to "hand out" those file descriptors, > > > > The kernel already handed out one when the device was opened using > > usbfs. > > > > > so no one but > > > the kernel and the programme itself > > > have that file descriptor, so it can't be used as an identifier. > > > > That's the only unique identifier that you want to revoke and "know" > > it > > is the device you are referring to. > > > > That's why I recommend creating revoke(2), not just an odd "pause the > > data to this device" api like this seems. That's just going to > > confuse > > lots of people as to "why did my device stop working?" > > "Why did my app stop working. Because you switched users and your app > needs to be updated to take that into account." > > I mean, ENODEV is returned on ioctls, it's just that the poll() doesn't > HUP. Bastien, maybe it would help if you explained to Greg and the mailing list exactly what you want to accomplish, both in more detail and from a higher-level viewpoint than you already have provided. Also, give an example or two showing when this mechanism would be invoked, how it would work, and what it would do. (Also, usbfs doesn't bind to USB devices but it does bind to USB interfaces; see claimintf() in devio.c.) Greg, unbinding usbfs would do only part of what Bastien wants. It would prevent a process from sending or receiving data over bulk, interrupt, or isochronous endpoints connected to interfaces that were already bound, but it would not prevent the process from accessing endpoint 0 or from re-binding to interfaces. What Bastien wants is a lot more like the kernel pretending to the process that the USB device has been disconnected and therefore is totally inaccessible. As for whether this is the best (or even the right) thing to do, or whether there are other ways to accomplish the ultimate goal of not allowing user programs to access certain local devices unless those programs belong to an active (console?) login session... That's beyond my scope. I'm also not clear on how namespaces figure into the whole discussion. Alan Stern
Hi Bastien,
I love your patch! Perhaps something to improve:
[auto build test WARNING on usb/usb-testing]
[also build test WARNING on balbi-usb/testing/next peter-chen-usb/for-usb-next linus/master v5.19 next-20220810]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Bastien-Nocera/USB-core-add-a-way-to-revoke-access-to-open-USB-devices/20220809-174609
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: arm64-randconfig-r001-20220810 (https://download.01.org/0day-ci/archive/20220811/202208110108.ilG9Ea14-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 5f1c7e2cc5a3c07cbc2412e851a7283c1841f520)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/6bd6f04e6d463be82fbf45585e4af84925bf1ab9
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Bastien-Nocera/USB-core-add-a-way-to-revoke-access-to-open-USB-devices/20220809-174609
git checkout 6bd6f04e6d463be82fbf45585e4af84925bf1ab9
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/usb/core/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/usb/core/devio.c:2649:1: warning: unused label 'out' [-Wunused-label]
out:
^~~~
1 warning generated.
vim +/out +2649 drivers/usb/core/devio.c
2627
2628 int usb_revoke_for_euid(struct usb_device *udev,
2629 int euid)
2630 {
2631 struct usb_dev_state *ps;
2632
2633 usb_lock_device(udev);
2634
2635 list_for_each_entry(ps, &udev->filelist, list) {
2636 if (euid >= 0) {
2637 kuid_t kuid;
2638
2639 if (!ps || !ps->cred)
2640 continue;
2641 kuid = ps->cred->euid;
2642 if (kuid.val != euid)
2643 continue;
2644 }
2645
2646 usbdev_revoke(ps);
2647 }
2648
> 2649 out:
2650 usb_unlock_device(udev);
2651 return 0;
2652 }
2653
Hi Bastien,
I love your patch! Perhaps something to improve:
[auto build test WARNING on usb/usb-testing]
[also build test WARNING on balbi-usb/testing/next peter-chen-usb/for-usb-next linus/master v5.19 next-20220810]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Bastien-Nocera/USB-core-add-a-way-to-revoke-access-to-open-USB-devices/20220809-174609
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: i386-defconfig (https://download.01.org/0day-ci/archive/20220811/202208110130.PZkeHYmL-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/6bd6f04e6d463be82fbf45585e4af84925bf1ab9
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Bastien-Nocera/USB-core-add-a-way-to-revoke-access-to-open-USB-devices/20220809-174609
git checkout 6bd6f04e6d463be82fbf45585e4af84925bf1ab9
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/usb/core/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/usb/core/devio.c: In function 'usb_revoke_for_euid':
>> drivers/usb/core/devio.c:2649:1: warning: label 'out' defined but not used [-Wunused-label]
2649 | out:
| ^~~
vim +/out +2649 drivers/usb/core/devio.c
2627
2628 int usb_revoke_for_euid(struct usb_device *udev,
2629 int euid)
2630 {
2631 struct usb_dev_state *ps;
2632
2633 usb_lock_device(udev);
2634
2635 list_for_each_entry(ps, &udev->filelist, list) {
2636 if (euid >= 0) {
2637 kuid_t kuid;
2638
2639 if (!ps || !ps->cred)
2640 continue;
2641 kuid = ps->cred->euid;
2642 if (kuid.val != euid)
2643 continue;
2644 }
2645
2646 usbdev_revoke(ps);
2647 }
2648
> 2649 out:
2650 usb_unlock_device(udev);
2651 return 0;
2652 }
2653
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) +{ + struct usb_dev_state *ps; + + usb_lock_device(udev); + + list_for_each_entry(ps, &udev->filelist, list) { + if (euid >= 0) { + kuid_t kuid; + + if (!ps || !ps->cred) + continue; + kuid = ps->cred->euid; + if (kuid.val != euid) + 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);
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(-)