| Message ID | 20220425132315.924477-1-hadess@hadess.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,v1] USB: core: add USBDEVFS_REVOKE ioctl | expand |
Hey, On Mon, 2022-04-25 at 15:23 +0200, Bastien Nocera wrote: > Note that this variant needs to do a few things that the evdev revoke > doesn't need to handle, particular: > - cancelling pending async transfers > - making sure to release claimed interfaces on revoke so they can be > opened by another process/user, as USB interfaces require being > exclusively claimed to be used. This is a first version of the patch, untested as yet (although I at least checked that it compiled...). I wanted to have comments on whether I was on the right path, in terms of coding style, but also that I had plugged all the entry points that would allow a user to communicate with a USB device after revocation. I also have a local patch that allows using BPF to revoke a USB device that's trivial so would need testing before posting. You can find links to Peter's hidraw revocation patches at: https://github.com/systemd/systemd/pull/23140#issue-1210571942 Cheers
On 25.04.22 15:23, Bastien Nocera wrote: > struct usb_memory { > @@ -237,6 +238,9 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma) > dma_addr_t dma_handle; > int ret; > > + if (!connected(ps) || ps->revoked) > + return -ENODEV; > + This lacks locking. > > +static int usbdev_revoke(struct usb_dev_state *ps) > +{ > + struct usb_device *dev = ps->dev; > + unsigned int ifnum; > + struct async *as; > + > + if (ps->revoked) > + return -ENODEV; > + ps->revoked = true; > + > + usb_lock_device(dev); And here you lock the device a second time. That is a bad idea. > + for (ifnum = 0; ps->ifclaimed && ifnum < 8*sizeof(ps->ifclaimed); > + ifnum++) { > + if (test_bit(ifnum, &ps->ifclaimed)) > + releaseintf(ps, ifnum); > + } > + destroy_all_async(ps); > + usb_unlock_device(dev); > + > + as = async_getcompleted(ps); > + while (as) { > + free_async(as); > + as = async_getcompleted(ps); > + } > + > + return 0; > +} Getting your file descriptor revoked should wake you up from poll(), shouldn't it? > + > /* > * NOTE: All requests here that have interface numbers as parameters > * are assuming that somehow the configuration has been prevented from > @@ -2619,7 +2660,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; > } > @@ -2779,6 +2820,11 @@ static long usbdev_do_ioctl(struct file *file, unsigned int cmd, > case USBDEVFS_WAIT_FOR_RESUME: > ret = proc_wait_for_resume(ps); > break; > + case USBDEVFS_REVOKE: You are still in usb_lock_device() Regards Oliver
On Mon, Apr 25, 2022 at 03:23:15PM +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, and > implemented in user-space. > > End-user access is usually handled by the uaccess tag in systemd, > shipping application-specific udev rules that implement this without too > much care for sandboxed applications, or overall security, or just sudo. > > A better approach is what we already have for evdev devices: give the > application a file descriptor and revoke it when it may no longer access > that device. Who is going to use this "better" approach? Is there support in libusb for it? Who talks raw usbfs other than libusb these days? > > This patch is the USB equivalent to the EVIOCREVOKE ioctl, see > commit c7dc65737c9a607d3e6f8478659876074ad129b8 for full details. c7dc65737c9a ("Input: evdev - add EVIOCREVOKE ioctl") is how I thought we were supposed to write out commits in changelogs these days :) > > Note that this variant needs to do a few things that the evdev revoke > doesn't need to handle, particular: > - cancelling pending async transfers > - making sure to release claimed interfaces on revoke so they can be > opened by another process/user, as USB interfaces require being > exclusively claimed to be used. I love the idea of a real revoke() someday, but can't you just do the "unbind/bind" hack instead if you really want to do this? Who wants to pass usbfs file descriptors around these days? thanks, greg k-h
On Mon, 2022-04-25 at 15:49 +0200, Oliver Neukum wrote: > > > On 25.04.22 15:23, Bastien Nocera wrote: > > struct usb_memory { > > @@ -237,6 +238,9 @@ static int usbdev_mmap(struct file *file, > > struct vm_area_struct *vma) > > dma_addr_t dma_handle; > > int ret; > > > > + if (!connected(ps) || ps->revoked) > > + return -ENODEV; > > + > This lacks locking. I'll look into straightening out the locking, thanks.
On Mon, 2022-04-25 at 16:10 +0200, Greg Kroah-Hartman wrote: > On Mon, Apr 25, 2022 at 03:23:15PM +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, and > > implemented in user-space. > > > > End-user access is usually handled by the uaccess tag in systemd, > > shipping application-specific udev rules that implement this > > without too > > much care for sandboxed applications, or overall security, or just > > sudo. > > > > A better approach is what we already have for evdev devices: give > > the > > application a file descriptor and revoke it when it may no longer > > access > > that device. > > Who is going to use this "better" approach? Is there support in > libusb > for it? Who talks raw usbfs other than libusb these days? Did you read the follow-up mail with the links to example code for the hid revoke support? > > > > > This patch is the USB equivalent to the EVIOCREVOKE ioctl, see > > commit c7dc65737c9a607d3e6f8478659876074ad129b8 for full details. > > c7dc65737c9a ("Input: evdev - add EVIOCREVOKE ioctl") is how I > thought > we were supposed to write out commits in changelogs these days :) > > > > > Note that this variant needs to do a few things that the evdev > > revoke > > doesn't need to handle, particular: > > - cancelling pending async transfers > > - making sure to release claimed interfaces on revoke so they can > > be > > opened by another process/user, as USB interfaces require being > > exclusively claimed to be used. > > I love the idea of a real revoke() someday, but can't you just do the > "unbind/bind" hack instead if you really want to do this? Who wants > to > pass usbfs file descriptors around these days? Again, please read the follow-up mail where I talk of the BPF support patch that would allow revoking USB fds without relying on a service in the middle to access devices (although that's eventually going to be the way to do things to allow elevating access to devices). Cheers
On Mon, 2022-04-25 at 15:49 +0200, Oliver Neukum wrote: > > > On 25.04.22 15:23, Bastien Nocera wrote: > > struct usb_memory { > > @@ -237,6 +238,9 @@ static int usbdev_mmap(struct file *file, > > struct vm_area_struct *vma) > > dma_addr_t dma_handle; > > int ret; > > > > + if (!connected(ps) || ps->revoked) > > + return -ENODEV; > > + > This lacks locking. Added locking. > > > > +static int usbdev_revoke(struct usb_dev_state *ps) > > +{ > > + struct usb_device *dev = ps->dev; > > + unsigned int ifnum; > > + struct async *as; > > + > > + if (ps->revoked) > > + return -ENODEV; > > + ps->revoked = true; > > + > > + usb_lock_device(dev); > And here you lock the device a second time. That is a bad idea. I've removed the locking in this function. > > + for (ifnum = 0; ps->ifclaimed && ifnum < 8*sizeof(ps- > > >ifclaimed); > > + ifnum++) { > > + if (test_bit(ifnum, &ps->ifclaimed)) > > + releaseintf(ps, ifnum); > > + } > > + destroy_all_async(ps); > > + usb_unlock_device(dev); > > + > > + as = async_getcompleted(ps); > > + while (as) { > > + free_async(as); > > + as = async_getcompleted(ps); > > + } > > + > > + return 0; > > +} > Getting your file descriptor revoked should wake you up > from poll(), shouldn't it? Added a wakeup. > > > + > > /* > > * NOTE: All requests here that have interface numbers as > > parameters > > * are assuming that somehow the configuration has been prevented > > from > > @@ -2619,7 +2660,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; > > } > > @@ -2779,6 +2820,11 @@ static long usbdev_do_ioctl(struct file > > *file, unsigned int cmd, > > case USBDEVFS_WAIT_FOR_RESUME: > > ret = proc_wait_for_resume(ps); > > break; > > + case USBDEVFS_REVOKE: > You are still in usb_lock_device() Noted. > > Regards > Oliver > I'll post those changes in v2, thanks.
On Mon, Apr 25, 2022 at 04:28:40PM +0200, Bastien Nocera wrote: > On Mon, 2022-04-25 at 16:10 +0200, Greg Kroah-Hartman wrote: > > On Mon, Apr 25, 2022 at 03:23:15PM +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, and > > > implemented in user-space. > > > > > > End-user access is usually handled by the uaccess tag in systemd, > > > shipping application-specific udev rules that implement this > > > without too > > > much care for sandboxed applications, or overall security, or just > > > sudo. > > > > > > A better approach is what we already have for evdev devices: give > > > the > > > application a file descriptor and revoke it when it may no longer > > > access > > > that device. > > > > Who is going to use this "better" approach? Is there support in > > libusb > > for it? Who talks raw usbfs other than libusb these days? > > Did you read the follow-up mail with the links to example code for the > hid revoke support? HID revoke does not mess with usbfs though. Or if it does, I don't understand the connection. And usually the 0/X email has the context, not follow-on messages that I didn't read yet :) > > > This patch is the USB equivalent to the EVIOCREVOKE ioctl, see > > > commit c7dc65737c9a607d3e6f8478659876074ad129b8 for full details. > > > > c7dc65737c9a ("Input: evdev - add EVIOCREVOKE ioctl") is how I > > thought > > we were supposed to write out commits in changelogs these days :) > > > > > > > > Note that this variant needs to do a few things that the evdev > > > revoke > > > doesn't need to handle, particular: > > > - cancelling pending async transfers > > > - making sure to release claimed interfaces on revoke so they can > > > be > > > opened by another process/user, as USB interfaces require being > > > exclusively claimed to be used. > > > > I love the idea of a real revoke() someday, but can't you just do the > > "unbind/bind" hack instead if you really want to do this? Who wants > > to > > pass usbfs file descriptors around these days? > > Again, please read the follow-up mail where I talk of the BPF support > patch that would allow revoking USB fds without relying on a service in > the middle to access devices (although that's eventually going to be > the way to do things to allow elevating access to devices). So would bpf be working at the usbfs level here? I still don't understand the connection... thanks, greg k-h
On Mon, 2022-04-25 at 17:00 +0200, Greg Kroah-Hartman wrote: > On Mon, Apr 25, 2022 at 04:28:40PM +0200, Bastien Nocera wrote: > > On Mon, 2022-04-25 at 16:10 +0200, Greg Kroah-Hartman wrote: > > > On Mon, Apr 25, 2022 at 03:23:15PM +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, > > > > and > > > > implemented in user-space. > > > > > > > > End-user access is usually handled by the uaccess tag in > > > > systemd, > > > > shipping application-specific udev rules that implement this > > > > without too > > > > much care for sandboxed applications, or overall security, or > > > > just > > > > sudo. > > > > > > > > A better approach is what we already have for evdev devices: > > > > give > > > > the > > > > application a file descriptor and revoke it when it may no > > > > longer > > > > access > > > > that device. > > > > > > Who is going to use this "better" approach? Is there support in > > > libusb > > > for it? Who talks raw usbfs other than libusb these days? > > > > Did you read the follow-up mail with the links to example code for > > the > > hid revoke support? > > HID revoke does not mess with usbfs though. Or if it does, I don't > understand the connection. evdev, HID and USB revoke are 3 separate implementations that are necessary for common device accesses to be revocable. The HID patch shows how device access is implemented in systemd, with the seat leader (usually the compositor) being able to request fds from logind if the user doesn't already have access. logind would then be responsible for closing the USB devices the user doesn't have access to anymore when logging out, or switching user. It could either close fds it passed out, or use BPF to revoke opened HID and USB devices without needing to act as an intermediary. In short: - libusb programme opens USB device, either directly, or after asking the compositor to pass a fd (and being authorised to do so) - programme does its thing - fast user switch to another user - logind revokes libusb access for the old user - new user can use the device without problems Note that user switching could also be a toggle to revoke USB device access for a sandbox. > And usually the 0/X email has the context, not follow-on messages > that I > didn't read yet :) Sorry, I'm not used to the horrendous workflow around email patches. In a "forge" that follow-up mail would have been separate from the commit messages. Let me know what you need to get up to speed after reading that follow- up mail (and this current one), so I know what to add to future cover letters and/or commit messages. > > > > > > This patch is the USB equivalent to the EVIOCREVOKE ioctl, see > > > > commit c7dc65737c9a607d3e6f8478659876074ad129b8 for full > > > > details. > > > > > > c7dc65737c9a ("Input: evdev - add EVIOCREVOKE ioctl") is how I > > > thought > > > we were supposed to write out commits in changelogs these days :) > > > > > > > > > > > Note that this variant needs to do a few things that the evdev > > > > revoke > > > > doesn't need to handle, particular: > > > > - cancelling pending async transfers > > > > - making sure to release claimed interfaces on revoke so they > > > > can > > > > be > > > > opened by another process/user, as USB interfaces require > > > > being > > > > exclusively claimed to be used. > > > > > > I love the idea of a real revoke() someday, but can't you just do > > > the > > > "unbind/bind" hack instead if you really want to do this? Who > > > wants > > > to > > > pass usbfs file descriptors around these days? > > > > Again, please read the follow-up mail where I talk of the BPF > > support > > patch that would allow revoking USB fds without relying on a > > service in > > the middle to access devices (although that's eventually going to > > be > > the way to do things to allow elevating access to devices). > > So would bpf be working at the usbfs level here? I still don't > understand the connection... The explanation is here (for hidraw): https://gitlab.freedesktop.org/bentiss/logind-hidraw/
On Mon, Apr 25, 2022 at 05:17:28PM +0200, Bastien Nocera wrote: > On Mon, 2022-04-25 at 17:00 +0200, Greg Kroah-Hartman wrote: > > On Mon, Apr 25, 2022 at 04:28:40PM +0200, Bastien Nocera wrote: > > > On Mon, 2022-04-25 at 16:10 +0200, Greg Kroah-Hartman wrote: > > > > On Mon, Apr 25, 2022 at 03:23:15PM +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, > > > > > and > > > > > implemented in user-space. > > > > > > > > > > End-user access is usually handled by the uaccess tag in > > > > > systemd, > > > > > shipping application-specific udev rules that implement this > > > > > without too > > > > > much care for sandboxed applications, or overall security, or > > > > > just > > > > > sudo. > > > > > > > > > > A better approach is what we already have for evdev devices: > > > > > give > > > > > the > > > > > application a file descriptor and revoke it when it may no > > > > > longer > > > > > access > > > > > that device. > > > > > > > > Who is going to use this "better" approach? Is there support in > > > > libusb > > > > for it? Who talks raw usbfs other than libusb these days? > > > > > > Did you read the follow-up mail with the links to example code for > > > the > > > hid revoke support? > > > > HID revoke does not mess with usbfs though. Or if it does, I don't > > understand the connection. > > evdev, HID and USB revoke are 3 separate implementations that are > necessary for common device accesses to be revocable. > > The HID patch shows how device access is implemented in systemd, with > the seat leader (usually the compositor) being able to request fds from > logind if the user doesn't already have access. > > logind would then be responsible for closing the USB devices the user > doesn't have access to anymore when logging out, or switching user. It > could either close fds it passed out, or use BPF to revoke opened HID > and USB devices without needing to act as an intermediary. > > In short: > - libusb programme opens USB device, either directly, or after asking > the compositor to pass a fd (and being authorised to do so) What libusb programs open usb devices today like this? And who is going to change them to use the compositor instead of just opening the file descriptor directly like they do today? > - programme does its thing > - fast user switch to another user > - logind revokes libusb access for the old user > - new user can use the device without problems > > Note that user switching could also be a toggle to revoke USB device > access for a sandbox. > > > And usually the 0/X email has the context, not follow-on messages > > that I > > didn't read yet :) > > Sorry, I'm not used to the horrendous workflow around email patches. In > a "forge" that follow-up mail would have been separate from the commit > messages. It'sn ot horrendous, it's much simpler, compose an email, send it off. Or use git send-email if you like. Much simpler than trying to log into a random web site and keep track of what is and is not happening. > Let me know what you need to get up to speed after reading that follow- > up mail (and this current one), so I know what to add to future cover > letters and/or commit messages. I still do not know who would use this. > > > > > This patch is the USB equivalent to the EVIOCREVOKE ioctl, see > > > > > commit c7dc65737c9a607d3e6f8478659876074ad129b8 for full > > > > > details. > > > > > > > > c7dc65737c9a ("Input: evdev - add EVIOCREVOKE ioctl") is how I > > > > thought > > > > we were supposed to write out commits in changelogs these days :) > > > > > > > > > > > > > > Note that this variant needs to do a few things that the evdev > > > > > revoke > > > > > doesn't need to handle, particular: > > > > > - cancelling pending async transfers > > > > > - making sure to release claimed interfaces on revoke so they > > > > > can > > > > > be > > > > > opened by another process/user, as USB interfaces require > > > > > being > > > > > exclusively claimed to be used. > > > > > > > > I love the idea of a real revoke() someday, but can't you just do > > > > the > > > > "unbind/bind" hack instead if you really want to do this? Who > > > > wants > > > > to > > > > pass usbfs file descriptors around these days? > > > > > > Again, please read the follow-up mail where I talk of the BPF > > > support > > > patch that would allow revoking USB fds without relying on a > > > service in > > > the middle to access devices (although that's eventually going to > > > be > > > the way to do things to allow elevating access to devices). > > > > So would bpf be working at the usbfs level here? I still don't > > understand the connection... > > The explanation is here (for hidraw): > https://gitlab.freedesktop.org/bentiss/logind-hidraw/ usbfs is not in that explanation at all. Will there be a logind-libusb process as well? But back to the original question, what programs would use this that today offer direct access to USB devices through libusb? I can maybe think of some fingerprint scanners and some flatbed scanners (printers?) But those are generally rare and the fingerprint scanners only have limited access to the device already. You're going to have to test this somehow with some program, what are you using today for this? thanks, greg k-h
On Mon, Apr 25, 2022 at 05:17:28PM +0200, Bastien Nocera wrote: > evdev, HID and USB revoke are 3 separate implementations that are > necessary for common device accesses to be revocable. > > The HID patch shows how device access is implemented in systemd, with > the seat leader (usually the compositor) being able to request fds from > logind if the user doesn't already have access. > > logind would then be responsible for closing the USB devices the user > doesn't have access to anymore when logging out, or switching user. It > could either close fds it passed out, or use BPF to revoke opened HID > and USB devices without needing to act as an intermediary. > > In short: > - libusb programme opens USB device, either directly, or after asking > the compositor to pass a fd (and being authorised to do so) > - programme does its thing > - fast user switch to another user > - logind revokes libusb access for the old user > - new user can use the device without problems What happens if there's another fast user switch back to the original user? Won't the original user then expect the old usbfs fds to continue working? Doesn't the whole idea of revoking file access permissions go against the Unix philosophy of checking access rights only once, when a file is opened, but not thereafter? I'm sure I've seen lots of emails by Linus complaining when people try to use a different approach. Alan Stern
On Mon, Apr 25, 2022 at 6:21 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Mon, Apr 25, 2022 at 05:17:28PM +0200, Bastien Nocera wrote: > > evdev, HID and USB revoke are 3 separate implementations that are > > necessary for common device accesses to be revocable. > > > > The HID patch shows how device access is implemented in systemd, with > > the seat leader (usually the compositor) being able to request fds from > > logind if the user doesn't already have access. > > > > logind would then be responsible for closing the USB devices the user > > doesn't have access to anymore when logging out, or switching user. It > > could either close fds it passed out, or use BPF to revoke opened HID > > and USB devices without needing to act as an intermediary. > > > > In short: > > - libusb programme opens USB device, either directly, or after asking > > the compositor to pass a fd (and being authorised to do so) > > - programme does its thing > > - fast user switch to another user > > - logind revokes libusb access for the old user > > - new user can use the device without problems > > What happens if there's another fast user switch back to the original > user? Won't the original user then expect the old usbfs fds to continue > working? > > Doesn't the whole idea of revoking file access permissions go against > the Unix philosophy of checking access rights only once, when a file is > opened, but not thereafter? I'm sure I've seen lots of emails by Linus > complaining when people try to use a different approach. Strictly speaking, it doesn't :) Basically, when you revoke an fd, it becomes unusable, and can not be reset to a usable state. It is as if the physical device has been unplugged. And when you think of it, it makes total sense to "unplug" the device when fast user switching, for the simple reason that you might (or the other user might) have set the device in a specific state, and when you come back, you have no idea what the current state is. So for end users: - they check the permissions once on open - fast user switching is happening, and they see the device as unplugged - then when the session comes back they need to re-open the device and re-initialize its state. Cheers, Benjamin
On Mon, Apr 25, 2022 at 05:45:11PM +0200, Greg Kroah-Hartman wrote: > On Mon, Apr 25, 2022 at 05:17:28PM +0200, Bastien Nocera wrote: > > > > Again, please read the follow-up mail where I talk of the BPF > > > > support > > > > patch that would allow revoking USB fds without relying on a > > > > service in > > > > the middle to access devices (although that's eventually going to > > > > be > > > > the way to do things to allow elevating access to devices). > > > > > > So would bpf be working at the usbfs level here? I still don't > > > understand the connection... > > > > The explanation is here (for hidraw): > > https://gitlab.freedesktop.org/bentiss/logind-hidraw/ > > usbfs is not in that explanation at all. Will there be a logind-libusb > process as well? chiming in here: the hidraw ioctl is independent (as already mentioned) but it's basically the same approach and/or intent. The hidraw revoke ioctl is "the evdev revoke, but for hidraw", this one is "the evdev revoke, but for usb". Not very creative, but at least we can point to prior art and say "this seems to be useful" :) The primary focus of all this are joystick devices (unless I missed some other grand plans Bastien had that I'm not aware of), that should put things in context a bit. Neither ioctl requires a new logind process - logind has TakeDevice(path) and ReleaseDevice() as equivalent to open()/close() and updating the logind *implementation* allows us to "take" a /dev/$whatever node. The (quite short) diff to logind for hidraw is here: https://github.com/systemd/systemd/pull/23140/files It's basically adding a new enum value and then the check for /dev/hidraw instead of /dev/input/event. Imagine the same thing for usb devices. With the ioctls, we need a chain which will require updating libusb etc. so a sandboxed application would do this: application → portal → compositor → logind → open() and the fd being passed back up from that to be used in the application. Ideally libusb can abstract this transparently. This chain is the ideal solution since it allows for tight controls of what the application can access, including the required "are you sure?" UI inserted where needed. Both logind and the portal would have a copy of the fd to call revoke on it when required (e.g. user switch or something interactive in the portal). Alas, this chain requires updates to how the application opens the fd. Benjamin's suggestion of using BPF (in addition to the ioctl) allows us to implement permission checks without that chain in place. With the right BPF program loaded, logind can effectively revoke an fd even where logind wasn't in the actual open() chain. For user-switching this immediately works, we need no application updates. For the "are you sure?" UI part it's more complicated but as a thought exercise: the portal could tell logind that $path was ok to be opened by any application, logind then allows that. If the portal tells logind that it's not longer ok, it can revoke any open fd. The portal does not need to know who actually has an fd open. There's some discussion on this "how's this actually going to work?" issue here: https://gitlab.freedesktop.org/bentiss/logind-hidraw/-/issues/1 just mentally substitute hidraw with usb. Notably, the application itself does not need updates. There are other efforts like that complement all this, like the proposal to tag "safe enough" joystick devices as such so they can be opened by an application: https://github.com/systemd/systemd/pull/22860 > But back to the original question, what programs would use this that > today offer direct access to USB devices through libusb? I can maybe > think of some fingerprint scanners and some flatbed scanners (printers?) > But those are generally rare and the fingerprint scanners only have > limited access to the device already. > > You're going to have to test this somehow with some program, what are > you using today for this? I have a very simple hidraw tester here: https://github.com/whot/hidiocrevoke-test/blob/master/revoke.c Updating that to take usb devices can't be that hard :) hth Cheers, Peter
On 26.04.22 04:27, Peter Hutterer wrote: > > chiming in here: the hidraw ioctl is independent (as already mentioned) > but it's basically the same approach and/or intent. The hidraw revoke ioctl is > "the evdev revoke, but for hidraw", this one is "the evdev revoke, but for > usb". Not very creative, but at least we can point to prior art and say "this > seems to be useful" :) > > The primary focus of all this are joystick devices (unless I missed some other > grand plans Bastien had that I'm not aware of), that should put things in > context a bit. > Hi, taking the advantages as a given, I must still ask, why this, if it is so useful, should be implemented for each subsystem separately. I cannot help but say that this should go into the VFS. Regards Oliver
On Tue, Apr 26, 2022 at 09:14:04AM +0200, Oliver Neukum wrote: > > > On 26.04.22 04:27, Peter Hutterer wrote: > > > > chiming in here: the hidraw ioctl is independent (as already mentioned) > > but it's basically the same approach and/or intent. The hidraw revoke ioctl is > > "the evdev revoke, but for hidraw", this one is "the evdev revoke, but for > > usb". Not very creative, but at least we can point to prior art and say "this > > seems to be useful" :) > > > > The primary focus of all this are joystick devices (unless I missed some other > > grand plans Bastien had that I'm not aware of), that should put things in > > context a bit. > > > Hi, > > taking the advantages as a given, I must still ask, why this, if it is > so useful, > should be implemented for each subsystem separately. I cannot help but > say that this should go into the VFS. Yes, but, it's not so simple. Many people have asked for revoke() to be added as a syscall like is in the BSDs, but the BSDs only allow that for a very small subset of file descriptor types, and doing it in a generic fashion seems very difficult (I tried a few years ago and gave up, but my knowledge of the vfs layer is minimal.) However doing it as a per-device/subsystem ioctl also seems crazy so perhaps we do just implement it at the vfs layer and let whatever device/filesystem type that wants to support it, hook up to it. That would make it much easier over time to implement in a way that works for everyone and is easier to understand from userspace. thanks, greg k-h
On 26.04.22 09:21, Greg Kroah-Hartman wrote: > Yes, but, it's not so simple. Many people have asked for revoke() to be > added as a syscall like is in the BSDs, but the BSDs only allow that for > a very small subset of file descriptor types, and doing it in a generic > fashion seems very difficult (I tried a few years ago and gave up, but > my knowledge of the vfs layer is minimal.) Well, then we should go for the minimalist approach and just add a hook to VFS. Multiple different ioctl()s are definitely a bad idea. An frevoke() looks much easier to do than one based on paths. If I understand the issue behind the proposal correctly the caller has opened the device. Regards Oliver
On Mon, 2022-04-25 at 17:45 +0200, Greg Kroah-Hartman wrote: > But back to the original question, what programs would use this that > today offer direct access to USB devices through libusb? I can maybe > think of some fingerprint scanners and some flatbed scanners > (printers?) > But those are generally rare and the fingerprint scanners only have > limited access to the device already. fingerprint readers are handled through a privileged daemon for the past 14 years: https://fprint.freedesktop.org/ Looking through libusb_open() users on the Debian repo[1], I could find those types of devices that could make use of sandboxing: - 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 There's also Rio500 support which I'm particularly attached to, and many many more device types, including one that should eventually get a kernel driver, because prototyping in user-space in Python or Javascript is probably easier than in C. [1]: https://codesearch.debian.net/search?q=libusb_open&literal=1&perpkg=1
On Tue, 2022-04-26 at 12:27 +1000, Peter Hutterer wrote: > The primary focus of all this are joystick devices (unless I missed > some other > grand plans Bastien had that I'm not aware of), that should put > things in > context a bit. It's for every USB device out there: https://github.com/flatpak/xdg-desktop-portal/issues/227 https://github.com/flatpak/xdg-desktop-portal/pull/559 including support for Chrome's WebUSB: https://wicg.github.io/webusb/ The end goal (with the portals and the revoke support) is sandboxed applications being able to enumerate USB devices, access them, and revoke access to them without full devices access, or overly broad user access. > I have a very simple hidraw tester here: > https://github.com/whot/hidiocrevoke-test/blob/master/revoke.c > Updating that to take usb devices can't be that hard :) It just requires me finding the right device to test with ;)
On Tue, 2022-04-26 at 10:46 +0200, Oliver Neukum wrote: > > > On 26.04.22 09:21, Greg Kroah-Hartman wrote: > > Yes, but, it's not so simple. Many people have asked for revoke() > > to be > > added as a syscall like is in the BSDs, but the BSDs only allow > > that for > > a very small subset of file descriptor types, and doing it in a > > generic > > fashion seems very difficult (I tried a few years ago and gave up, > > but > > my knowledge of the vfs layer is minimal.) > Well, then we should go for the minimalist approach and just > add a hook to VFS. Multiple different ioctl()s are definitely a bad > idea. > An frevoke() looks much easier to do than one based on paths. > If I understand the issue behind the proposal correctly the caller > has opened the device. Doesn't look like FreeBSD at least has an frevoke() syscall anymore, it had an FREVOKE flag, which is now a define for the O_VERIFY option which has quite different semantics: https://www.freebsd.org/cgi/man.cgi?sektion=2&query=open "O_VERIFY may be used to indicate to the kernel that the contents of the file should be verified before allowing the open to proceed. The details of what "verified" means is implementation specific. The run- time linker (rtld) uses this flag to ensure shared objects have been verified before operating on them." The AIX frevoke() also has different semantics: https://www.ibm.com/docs/en/aix/7.3?topic=f-frevoke-subroutine "All accesses to the file are revoked, except through the file descriptor specified by the FileDescriptor parameter to the frevoke subroutine." and: "Currently the frevoke subroutine works only on terminal devices." The point of USBDEVFS_REVOKE, and the other variants is to revoke access to the device, not to the file descriptor itself. If you're reticent to adding new ioctls, we could try and do that exclusively through BPF. The only thing that didn't look like the BPF codepath could do was wake up the fd so that fd could be poll()ed and error out immediately.
On Tue, Apr 26, 2022 at 12:07:32PM +0200, Bastien Nocera wrote: > On Tue, 2022-04-26 at 10:46 +0200, Oliver Neukum wrote: > > > > > > On 26.04.22 09:21, Greg Kroah-Hartman wrote: > > > Yes, but, it's not so simple. Many people have asked for revoke() > > > to be > > > added as a syscall like is in the BSDs, but the BSDs only allow > > > that for > > > a very small subset of file descriptor types, and doing it in a > > > generic > > > fashion seems very difficult (I tried a few years ago and gave up, > > > but > > > my knowledge of the vfs layer is minimal.) > > Well, then we should go for the minimalist approach and just > > add a hook to VFS. Multiple different ioctl()s are definitely a bad > > idea. > > An frevoke() looks much easier to do than one based on paths. > > If I understand the issue behind the proposal correctly the caller > > has opened the device. > > Doesn't look like FreeBSD at least has an frevoke() syscall anymore, it > had an FREVOKE flag, which is now a define for the O_VERIFY option > which has quite different semantics: > https://www.freebsd.org/cgi/man.cgi?sektion=2&query=open Take a look at this implementation: https://man.openbsd.org/revoke.2
On Tue, 2022-04-26 at 12:30 +0200, Greg Kroah-Hartman wrote: > On Tue, Apr 26, 2022 at 12:07:32PM +0200, Bastien Nocera wrote: > > On Tue, 2022-04-26 at 10:46 +0200, Oliver Neukum wrote: > > > > > > > > > On 26.04.22 09:21, Greg Kroah-Hartman wrote: > > > > Yes, but, it's not so simple. Many people have asked for > > > > revoke() > > > > to be > > > > added as a syscall like is in the BSDs, but the BSDs only allow > > > > that for > > > > a very small subset of file descriptor types, and doing it in a > > > > generic > > > > fashion seems very difficult (I tried a few years ago and gave > > > > up, > > > > but > > > > my knowledge of the vfs layer is minimal.) > > > Well, then we should go for the minimalist approach and just > > > add a hook to VFS. Multiple different ioctl()s are definitely a > > > bad > > > idea. > > > An frevoke() looks much easier to do than one based on paths. > > > If I understand the issue behind the proposal correctly the > > > caller > > > has opened the device. > > > > Doesn't look like FreeBSD at least has an frevoke() syscall > > anymore, it > > had an FREVOKE flag, which is now a define for the O_VERIFY option > > which has quite different semantics: > > https://www.freebsd.org/cgi/man.cgi?sektion=2&query=open > > Take a look at this implementation: > https://man.openbsd.org/revoke.2 > I don't think anyone wants to implement path based syscalls, and again, it's equivalent to remote closing the fd, not to disabling the access to the device: "If the file is a special file for a device which is open, the device close function is called as if all open references to the file had been closed." If there's an implementation done at the VFS level, it should probably try to steer away from those earlier disparate implementations.
On Tue, Apr 26, 2022 at 12:37:14PM +0200, Bastien Nocera wrote: > On Tue, 2022-04-26 at 12:30 +0200, Greg Kroah-Hartman wrote: > > On Tue, Apr 26, 2022 at 12:07:32PM +0200, Bastien Nocera wrote: > > > On Tue, 2022-04-26 at 10:46 +0200, Oliver Neukum wrote: > > > > > > > > > > > > On 26.04.22 09:21, Greg Kroah-Hartman wrote: > > > > > Yes, but, it's not so simple. Many people have asked for > > > > > revoke() > > > > > to be > > > > > added as a syscall like is in the BSDs, but the BSDs only allow > > > > > that for > > > > > a very small subset of file descriptor types, and doing it in a > > > > > generic > > > > > fashion seems very difficult (I tried a few years ago and gave > > > > > up, > > > > > but > > > > > my knowledge of the vfs layer is minimal.) > > > > Well, then we should go for the minimalist approach and just > > > > add a hook to VFS. Multiple different ioctl()s are definitely a > > > > bad > > > > idea. > > > > An frevoke() looks much easier to do than one based on paths. > > > > If I understand the issue behind the proposal correctly the > > > > caller > > > > has opened the device. > > > > > > Doesn't look like FreeBSD at least has an frevoke() syscall > > > anymore, it > > > had an FREVOKE flag, which is now a define for the O_VERIFY option > > > which has quite different semantics: > > > https://www.freebsd.org/cgi/man.cgi?sektion=2&query=open > > > > Take a look at this implementation: > > https://man.openbsd.org/revoke.2 > > > I don't think anyone wants to implement path based syscalls, and again, > it's equivalent to remote closing the fd, not to disabling the access > to the device: > "If the file is a special file for a device which is open, the device > close function is called as if all open references to the file had been > closed." You forgot the sentence before that which is the functionality we want here: Subsequent operations on any such descriptors fail, with the exceptions that a read() from a tty which has been revoked returns a count of zero (end of file), and a close() call will succeed. And don't do it only for a tty device, but for any device that implements it. And yes, we don't want to work off of a path, but a file descriptor: int revoke(int fd); > If there's an implementation done at the VFS level, it should probably > try to steer away from those earlier disparate implementations. Agreed. thanks, greg k-h
On 26.04.22 12:37, Bastien Nocera wrote: > > I don't think anyone wants to implement path based syscalls, and again, > it's equivalent to remote closing the fd, not to disabling the access > to the device: > "If the file is a special file for a device which is open, the device > close function is called as if all open references to the file had been > closed." OK, so do we still have a need to discuss this specifically to usbfs? I suppose if you put this into VFS, you will need a hook to map the syscall to drivers that need to handling ioctl() having effects beyond their syscall abstractly speaking. Regards Oliver
On Thu, 2022-04-28 at 12:28 +0200, Oliver Neukum wrote: > > > On 26.04.22 12:37, Bastien Nocera wrote: > > > > I don't think anyone wants to implement path based syscalls, > > and again, > > it's equivalent to remote closing the fd, not to disabling the > > access > > to the device: > > "If the file is a special file for a device which is open, the > > device > > close function is called as if all open references to the file had > > been > > closed." > OK, so do we still have a need to discuss this specifically to usbfs? > I suppose if you put this into VFS, you will need a hook to map the > syscall to drivers that need to handling ioctl() having effects > beyond > their syscall abstractly speaking. I'm afraid that I don't have any intentions on working on this feature at the vfs level. As I already mentioned, I think that the semantics are too different to make sense at that level. I'm instead investigating having an ioctl-less BPF interface, with the help of Benjamin and Peter.
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c index 6abb7294e919..959c5ce4ab37 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,9 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma) dma_addr_t dma_handle; int ret; + if (!connected(ps) || ps->revoked) + return -ENODEV; + ret = usbfs_increase_memory_usage(size + sizeof(struct usb_memory)); if (ret) goto error; @@ -298,6 +302,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 +323,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) { @@ -2576,6 +2589,34 @@ 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) + return -ENODEV; + ps->revoked = true; + + usb_lock_device(dev); + for (ifnum = 0; ps->ifclaimed && ifnum < 8*sizeof(ps->ifclaimed); + ifnum++) { + if (test_bit(ifnum, &ps->ifclaimed)) + releaseintf(ps, ifnum); + } + destroy_all_async(ps); + usb_unlock_device(dev); + + as = async_getcompleted(ps); + while (as) { + free_async(as); + as = async_getcompleted(ps); + } + + return 0; +} + /* * NOTE: All requests here that have interface numbers as parameters * are assuming that somehow the configuration has been prevented from @@ -2619,7 +2660,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; } @@ -2779,6 +2820,11 @@ static long usbdev_do_ioctl(struct file *file, unsigned int cmd, case USBDEVFS_WAIT_FOR_RESUME: ret = proc_wait_for_resume(ps); break; + case USBDEVFS_REVOKE: + ret = usbdev_revoke(ps); + snoop(&dev->dev, "%s: REVOKE by PID %d: %s\n", __func__, + task_pid_nr(current), current->comm); + break; } /* Handle variable-length commands */ @@ -2815,7 +2861,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; @@ -2824,7 +2870,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/include/uapi/linux/usbdevice_fs.h b/include/uapi/linux/usbdevice_fs.h index cf525cddeb94..2ad44328c337 100644 --- a/include/uapi/linux/usbdevice_fs.h +++ b/include/uapi/linux/usbdevice_fs.h @@ -227,5 +227,6 @@ struct usbdevfs_streams { #define USBDEVFS_FORBID_SUSPEND _IO('U', 33) #define USBDEVFS_ALLOW_SUSPEND _IO('U', 34) #define USBDEVFS_WAIT_FOR_RESUME _IO('U', 35) +#define USBDEVFS_REVOKE _IO('U', 36) #endif /* _UAPI_LINUX_USBDEVICE_FS_H */
There is a need for userspace applications to open USB devices directly, for all the USB devices without a kernel-level class driver, and implemented in user-space. End-user access is usually handled by the uaccess tag in systemd, shipping application-specific udev rules that implement this without too much care for sandboxed applications, or overall security, or just sudo. A better approach is what we already have for evdev devices: give the application a file descriptor and revoke it when it may no longer access that device. This patch is the USB equivalent to the EVIOCREVOKE ioctl, see commit c7dc65737c9a607d3e6f8478659876074ad129b8 for full details. Note that this variant needs to do a few things that the evdev revoke doesn't need to handle, particular: - cancelling pending async transfers - making sure to release claimed interfaces on revoke so they can be opened by another process/user, as USB interfaces require being exclusively claimed to be used. Signed-off-by: Bastien Nocera <hadess@hadess.net> --- drivers/usb/core/devio.c | 54 ++++++++++++++++++++++++++++--- include/uapi/linux/usbdevice_fs.h | 1 + 2 files changed, 51 insertions(+), 4 deletions(-)