diff mbox series

[RFC,v1] USB: core: add USBDEVFS_REVOKE ioctl

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

Commit Message

Bastien Nocera April 25, 2022, 1:23 p.m. UTC
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(-)

Comments

Bastien Nocera April 25, 2022, 1:28 p.m. UTC | #1
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
Oliver Neukum April 25, 2022, 1:49 p.m. UTC | #2
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
Greg KH April 25, 2022, 2:10 p.m. UTC | #3
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
Bastien Nocera April 25, 2022, 2:25 p.m. UTC | #4
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.
Bastien Nocera April 25, 2022, 2:28 p.m. UTC | #5
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
Bastien Nocera April 25, 2022, 2:45 p.m. UTC | #6
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.
Greg KH April 25, 2022, 3 p.m. UTC | #7
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
Bastien Nocera April 25, 2022, 3:17 p.m. UTC | #8
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/
Greg KH April 25, 2022, 3:45 p.m. UTC | #9
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
Alan Stern April 25, 2022, 4:14 p.m. UTC | #10
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
Benjamin Tissoires April 25, 2022, 5:09 p.m. UTC | #11
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
Peter Hutterer April 26, 2022, 2:27 a.m. UTC | #12
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
Oliver Neukum April 26, 2022, 7:14 a.m. UTC | #13
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
Greg KH April 26, 2022, 7:21 a.m. UTC | #14
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
Oliver Neukum April 26, 2022, 8:46 a.m. UTC | #15
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
Bastien Nocera April 26, 2022, 10:07 a.m. UTC | #16
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
Bastien Nocera April 26, 2022, 10:07 a.m. UTC | #17
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 ;)
Bastien Nocera April 26, 2022, 10:07 a.m. UTC | #18
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.
Greg KH April 26, 2022, 10:30 a.m. UTC | #19
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
Bastien Nocera April 26, 2022, 10:37 a.m. UTC | #20
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.
Greg KH April 26, 2022, 11:10 a.m. UTC | #21
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
Oliver Neukum April 28, 2022, 10:28 a.m. UTC | #22
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
Bastien Nocera April 28, 2022, 11:21 a.m. UTC | #23
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 mbox series

Patch

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 */