diff mbox series

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

Message ID 20220809094300.83116-2-hadess@hadess.net (mailing list archive)
State Not Applicable
Delegated to: BPF
Headers show
Series USB: core: add a way to revoke access to open USB devices | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch
bpf/vmtest-bpf-next-VM_Test-1 success Logs for Kernel LATEST on ubuntu-latest with gcc
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Kernel LATEST on ubuntu-latest with llvm-16
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Kernel LATEST on z15 with gcc

Commit Message

Bastien Nocera Aug. 9, 2022, 9:42 a.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[1], and
implemented in user-space.

As not all devices are built equal, we want to be able to revoke
access to those devices whether it's because the user isn't at the
console anymore, or because the web browser, or sandbox doesn't want
to allow access to that device.

This commit implements the internal API used to revoke access to USB
devices, given either bus and device numbers, or/and a user's
effective UID.

Signed-off-by: Bastien Nocera <hadess@hadess.net>

[1]:
Non exhaustive list of devices and device types that need raw USB access:
- all manners of single-board computers and programmable chips and
devices (avrdude, STLink, sunxi bootloader, flashrom, etc.)
- 3D printers
- scanners
- LCD "displays"
- user-space webcam and still cameras
- game controllers
- video/audio capture devices
- sensors
- software-defined radios
- DJ/music equipment
- protocol analysers
- Rio 500 music player
---
 drivers/usb/core/devio.c | 79 ++++++++++++++++++++++++++++++++++++++--
 drivers/usb/core/usb.h   |  2 +
 2 files changed, 77 insertions(+), 4 deletions(-)

Comments

Greg KH Aug. 9, 2022, 10:32 a.m. UTC | #1
On Tue, Aug 09, 2022 at 11:42:59AM +0200, Bastien Nocera wrote:
> There is a need for userspace applications to open USB devices directly,
> for all the USB devices without a kernel-level class driver[1], and
> implemented in user-space.
> 
> As not all devices are built equal, we want to be able to revoke
> access to those devices whether it's because the user isn't at the
> console anymore, or because the web browser, or sandbox doesn't want
> to allow access to that device.
> 
> This commit implements the internal API used to revoke access to USB
> devices, given either bus and device numbers, or/and a user's
> effective UID.
> 
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> 
> [1]:
> Non exhaustive list of devices and device types that need raw USB access:
> - all manners of single-board computers and programmable chips and
> devices (avrdude, STLink, sunxi bootloader, flashrom, etc.)
> - 3D printers
> - scanners
> - LCD "displays"
> - user-space webcam and still cameras
> - game controllers
> - video/audio capture devices
> - sensors
> - software-defined radios
> - DJ/music equipment
> - protocol analysers
> - Rio 500 music player

We can't take "footnotes" after a signed-off-by line, you know this :(
Greg KH Aug. 9, 2022, 10:35 a.m. UTC | #2
On Tue, Aug 09, 2022 at 11:42:59AM +0200, Bastien Nocera wrote:
> There is a need for userspace applications to open USB devices directly,
> for all the USB devices without a kernel-level class driver[1], and
> implemented in user-space.
> 
> As not all devices are built equal, we want to be able to revoke
> access to those devices whether it's because the user isn't at the
> console anymore, or because the web browser, or sandbox doesn't want
> to allow access to that device.
> 
> This commit implements the internal API used to revoke access to USB
> devices, given either bus and device numbers, or/and a user's
> effective UID.

Revoke what exactly?  You should be revoking the file descriptor that is
open, not anything else as those are all transient and not uniquely
identified and racy.



> 
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> 
> [1]:
> Non exhaustive list of devices and device types that need raw USB access:
> - all manners of single-board computers and programmable chips and
> devices (avrdude, STLink, sunxi bootloader, flashrom, etc.)
> - 3D printers
> - scanners
> - LCD "displays"
> - user-space webcam and still cameras
> - game controllers
> - video/audio capture devices
> - sensors
> - software-defined radios
> - DJ/music equipment
> - protocol analysers
> - Rio 500 music player
> ---
>  drivers/usb/core/devio.c | 79 ++++++++++++++++++++++++++++++++++++++--
>  drivers/usb/core/usb.h   |  2 +
>  2 files changed, 77 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index b5b85bf80329..d8d212ef581f 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -78,6 +78,7 @@ struct usb_dev_state {
>  	int not_yet_resumed;
>  	bool suspend_allowed;
>  	bool privileges_dropped;
> +	bool revoked;
>  };
>  
>  struct usb_memory {
> @@ -183,6 +184,13 @@ static int connected(struct usb_dev_state *ps)
>  			ps->dev->state != USB_STATE_NOTATTACHED);
>  }
>  
> +static int disconnected_or_revoked(struct usb_dev_state *ps)
> +{
> +	return (list_empty(&ps->list) ||
> +			ps->dev->state == USB_STATE_NOTATTACHED ||
> +			ps->revoked);
> +}
> +
>  static void dec_usb_memory_use_count(struct usb_memory *usbm, int *count)
>  {
>  	struct usb_dev_state *ps = usbm->ps;
> @@ -237,6 +245,9 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
>  	dma_addr_t dma_handle;
>  	int ret;
>  
> +	if (disconnected_or_revoked(ps))
> +		return -ENODEV;
> +
>  	ret = usbfs_increase_memory_usage(size + sizeof(struct usb_memory));
>  	if (ret)
>  		goto error;
> @@ -310,7 +321,7 @@ static ssize_t usbdev_read(struct file *file, char __user *buf, size_t nbytes,
>  
>  	pos = *ppos;
>  	usb_lock_device(dev);
> -	if (!connected(ps)) {
> +	if (disconnected_or_revoked(ps)) {
>  		ret = -ENODEV;
>  		goto err;
>  	} else if (pos < 0) {
> @@ -2315,7 +2326,7 @@ static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl)
>  	if (ps->privileges_dropped)
>  		return -EACCES;
>  
> -	if (!connected(ps))
> +	if (disconnected_or_revoked(ps))
>  		return -ENODEV;
>  
>  	/* alloc buffer */
> @@ -2555,7 +2566,7 @@ static int proc_forbid_suspend(struct usb_dev_state *ps)
>  
>  static int proc_allow_suspend(struct usb_dev_state *ps)
>  {
> -	if (!connected(ps))
> +	if (disconnected_or_revoked(ps))
>  		return -ENODEV;
>  
>  	WRITE_ONCE(ps->not_yet_resumed, 1);
> @@ -2580,6 +2591,66 @@ static int proc_wait_for_resume(struct usb_dev_state *ps)
>  	return proc_forbid_suspend(ps);
>  }
>  
> +static int usbdev_revoke(struct usb_dev_state *ps)
> +{
> +	struct usb_device *dev = ps->dev;
> +	unsigned int ifnum;
> +	struct async *as;
> +
> +	if (ps->revoked) {
> +		usb_unlock_device(dev);
> +		return -ENODEV;
> +	}
> +
> +	snoop(&dev->dev, "%s: REVOKE by PID %d: %s\n", __func__,
> +	      task_pid_nr(current), current->comm);
> +
> +	ps->revoked = true;
> +
> +	for (ifnum = 0; ps->ifclaimed && ifnum < 8*sizeof(ps->ifclaimed);
> +			ifnum++) {
> +		if (test_bit(ifnum, &ps->ifclaimed))
> +			releaseintf(ps, ifnum);
> +	}
> +	destroy_all_async(ps);
> +
> +	as = async_getcompleted(ps);
> +	while (as) {
> +		free_async(as);
> +		as = async_getcompleted(ps);
> +	}
> +
> +	wake_up(&ps->wait);
> +
> +	return 0;
> +}
> +
> +int usb_revoke_for_euid(struct usb_device *udev,
> +		int euid)
> +{
> +	struct usb_dev_state *ps;
> +
> +	usb_lock_device(udev);
> +
> +	list_for_each_entry(ps, &udev->filelist, list) {
> +		if (euid >= 0) {
> +			kuid_t kuid;
> +
> +			if (!ps || !ps->cred)
> +				continue;
> +			kuid = ps->cred->euid;

How is this handling uid namespaces?

Again, just revoke the file descriptor, like the BSDs do for a tiny
subset of device drivers.

This comes up ever so often, why does someone not just add real
revoke(2) support to Linux to handle it if they really really want it (I
tried a long time ago, but didn't have it in me as I had no real users
for it...)

thanks,

greg k-h
Bastien Nocera Aug. 9, 2022, 11:15 a.m. UTC | #3
On Tue, 2022-08-09 at 12:32 +0200, Greg Kroah-Hartman wrote:
> On Tue, Aug 09, 2022 at 11:42:59AM +0200, Bastien Nocera wrote:
> > There is a need for userspace applications to open USB devices
> > directly,
> > for all the USB devices without a kernel-level class driver[1], and
> > implemented in user-space.
> > 
> > As not all devices are built equal, we want to be able to revoke
> > access to those devices whether it's because the user isn't at the
> > console anymore, or because the web browser, or sandbox doesn't
> > want
> > to allow access to that device.
> > 
> > This commit implements the internal API used to revoke access to
> > USB
> > devices, given either bus and device numbers, or/and a user's
> > effective UID.
> > 
> > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > 
> > [1]:
> > Non exhaustive list of devices and device types that need raw USB
> > access:
> > - all manners of single-board computers and programmable chips and
> > devices (avrdude, STLink, sunxi bootloader, flashrom, etc.)
> > - 3D printers
> > - scanners
> > - LCD "displays"
> > - user-space webcam and still cameras
> > - game controllers
> > - video/audio capture devices
> > - sensors
> > - software-defined radios
> > - DJ/music equipment
> > - protocol analysers
> > - Rio 500 music player
> 
> We can't take "footnotes" after a signed-off-by line, you know this
> :(

Where would I know this from?

checkpatch.pl doesn't warn about it.
Bastien Nocera Aug. 9, 2022, 11:18 a.m. UTC | #4
On Tue, 2022-08-09 at 12:35 +0200, Greg Kroah-Hartman wrote:
> On Tue, Aug 09, 2022 at 11:42:59AM +0200, Bastien Nocera wrote:
> > There is a need for userspace applications to open USB devices
> > directly,
> > for all the USB devices without a kernel-level class driver[1], and
> > implemented in user-space.
> > 
> > As not all devices are built equal, we want to be able to revoke
> > access to those devices whether it's because the user isn't at the
> > console anymore, or because the web browser, or sandbox doesn't
> > want
> > to allow access to that device.
> > 
> > This commit implements the internal API used to revoke access to
> > USB
> > devices, given either bus and device numbers, or/and a user's
> > effective UID.
> 
> Revoke what exactly?

Access.

>   You should be revoking the file descriptor that is
> open, not anything else as those are all transient and not uniquely
> identified and racy.

They're "unique enough" (tm). The 

> 
> 
> 
> > 
> > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > 
> > [1]:
> > Non exhaustive list of devices and device types that need raw USB
> > access:
> > - all manners of single-board computers and programmable chips and
> > devices (avrdude, STLink, sunxi bootloader, flashrom, etc.)
> > - 3D printers
> > - scanners
> > - LCD "displays"
> > - user-space webcam and still cameras
> > - game controllers
> > - video/audio capture devices
> > - sensors
> > - software-defined radios
> > - DJ/music equipment
> > - protocol analysers
> > - Rio 500 music player
> > ---
> >  drivers/usb/core/devio.c | 79
> > ++++++++++++++++++++++++++++++++++++++--
> >  drivers/usb/core/usb.h   |  2 +
> >  2 files changed, 77 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> > index b5b85bf80329..d8d212ef581f 100644
> > --- a/drivers/usb/core/devio.c
> > +++ b/drivers/usb/core/devio.c
> > @@ -78,6 +78,7 @@ struct usb_dev_state {
> >         int not_yet_resumed;
> >         bool suspend_allowed;
> >         bool privileges_dropped;
> > +       bool revoked;
> >  };
> >  
> >  struct usb_memory {
> > @@ -183,6 +184,13 @@ static int connected(struct usb_dev_state *ps)
> >                         ps->dev->state != USB_STATE_NOTATTACHED);
> >  }
> >  
> > +static int disconnected_or_revoked(struct usb_dev_state *ps)
> > +{
> > +       return (list_empty(&ps->list) ||
> > +                       ps->dev->state == USB_STATE_NOTATTACHED ||
> > +                       ps->revoked);
> > +}
> > +
> >  static void dec_usb_memory_use_count(struct usb_memory *usbm, int
> > *count)
> >  {
> >         struct usb_dev_state *ps = usbm->ps;
> > @@ -237,6 +245,9 @@ static int usbdev_mmap(struct file *file,
> > struct vm_area_struct *vma)
> >         dma_addr_t dma_handle;
> >         int ret;
> >  
> > +       if (disconnected_or_revoked(ps))
> > +               return -ENODEV;
> > +
> >         ret = usbfs_increase_memory_usage(size + sizeof(struct
> > usb_memory));
> >         if (ret)
> >                 goto error;
> > @@ -310,7 +321,7 @@ static ssize_t usbdev_read(struct file *file,
> > char __user *buf, size_t nbytes,
> >  
> >         pos = *ppos;
> >         usb_lock_device(dev);
> > -       if (!connected(ps)) {
> > +       if (disconnected_or_revoked(ps)) {
> >                 ret = -ENODEV;
> >                 goto err;
> >         } else if (pos < 0) {
> > @@ -2315,7 +2326,7 @@ static int proc_ioctl(struct usb_dev_state
> > *ps, struct usbdevfs_ioctl *ctl)
> >         if (ps->privileges_dropped)
> >                 return -EACCES;
> >  
> > -       if (!connected(ps))
> > +       if (disconnected_or_revoked(ps))
> >                 return -ENODEV;
> >  
> >         /* alloc buffer */
> > @@ -2555,7 +2566,7 @@ static int proc_forbid_suspend(struct
> > usb_dev_state *ps)
> >  
> >  static int proc_allow_suspend(struct usb_dev_state *ps)
> >  {
> > -       if (!connected(ps))
> > +       if (disconnected_or_revoked(ps))
> >                 return -ENODEV;
> >  
> >         WRITE_ONCE(ps->not_yet_resumed, 1);
> > @@ -2580,6 +2591,66 @@ static int proc_wait_for_resume(struct
> > usb_dev_state *ps)
> >         return proc_forbid_suspend(ps);
> >  }
> >  
> > +static int usbdev_revoke(struct usb_dev_state *ps)
> > +{
> > +       struct usb_device *dev = ps->dev;
> > +       unsigned int ifnum;
> > +       struct async *as;
> > +
> > +       if (ps->revoked) {
> > +               usb_unlock_device(dev);
> > +               return -ENODEV;
> > +       }
> > +
> > +       snoop(&dev->dev, "%s: REVOKE by PID %d: %s\n", __func__,
> > +             task_pid_nr(current), current->comm);
> > +
> > +       ps->revoked = true;
> > +
> > +       for (ifnum = 0; ps->ifclaimed && ifnum < 8*sizeof(ps-
> > >ifclaimed);
> > +                       ifnum++) {
> > +               if (test_bit(ifnum, &ps->ifclaimed))
> > +                       releaseintf(ps, ifnum);
> > +       }
> > +       destroy_all_async(ps);
> > +
> > +       as = async_getcompleted(ps);
> > +       while (as) {
> > +               free_async(as);
> > +               as = async_getcompleted(ps);
> > +       }
> > +
> > +       wake_up(&ps->wait);
> > +
> > +       return 0;
> > +}
> > +
> > +int usb_revoke_for_euid(struct usb_device *udev,
> > +               int euid)
> > +{
> > +       struct usb_dev_state *ps;
> > +
> > +       usb_lock_device(udev);
> > +
> > +       list_for_each_entry(ps, &udev->filelist, list) {
> > +               if (euid >= 0) {
> > +                       kuid_t kuid;
> > +
> > +                       if (!ps || !ps->cred)
> > +                               continue;
> > +                       kuid = ps->cred->euid;
> 
> How is this handling uid namespaces?

The caller is supposed to be outside namespaces. It's a BPF programme,
so must be loaded by a privileged caller.

> 
> Again, just revoke the file descriptor, like the BSDs do for a tiny
> subset of device drivers.
> 
> This comes up ever so often, why does someone not just add real
> revoke(2) support to Linux to handle it if they really really want it
> (I
> tried a long time ago, but didn't have it in me as I had no real
> users
> for it...)

This was already explained twice, there's nothing to "hand out" those
file descriptors, so no one but the kernel and the programme itself
have that file descriptor, so it can't be used as an identifier.
Greg KH Aug. 9, 2022, 11:30 a.m. UTC | #5
On Tue, Aug 09, 2022 at 01:15:50PM +0200, Bastien Nocera wrote:
> On Tue, 2022-08-09 at 12:32 +0200, Greg Kroah-Hartman wrote:
> > On Tue, Aug 09, 2022 at 11:42:59AM +0200, Bastien Nocera wrote:
> > > There is a need for userspace applications to open USB devices
> > > directly,
> > > for all the USB devices without a kernel-level class driver[1], and
> > > implemented in user-space.
> > > 
> > > As not all devices are built equal, we want to be able to revoke
> > > access to those devices whether it's because the user isn't at the
> > > console anymore, or because the web browser, or sandbox doesn't
> > > want
> > > to allow access to that device.
> > > 
> > > This commit implements the internal API used to revoke access to
> > > USB
> > > devices, given either bus and device numbers, or/and a user's
> > > effective UID.
> > > 
> > > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > > 
> > > [1]:
> > > Non exhaustive list of devices and device types that need raw USB
> > > access:
> > > - all manners of single-board computers and programmable chips and
> > > devices (avrdude, STLink, sunxi bootloader, flashrom, etc.)
> > > - 3D printers
> > > - scanners
> > > - LCD "displays"
> > > - user-space webcam and still cameras
> > > - game controllers
> > > - video/audio capture devices
> > > - sensors
> > > - software-defined radios
> > > - DJ/music equipment
> > > - protocol analysers
> > > - Rio 500 music player
> > 
> > We can't take "footnotes" after a signed-off-by line, you know this
> > :(
> 
> Where would I know this from?

To quote from our documentation:
	The sign-off is a simple line at the end of the explanation for the
	patch,

> checkpatch.pl doesn't warn about it.

Odd, it should.  Send a patch for that?  :)

thanks,

greg k-h
Bastien Nocera Aug. 9, 2022, 11:53 a.m. UTC | #6
On Tue, 2022-08-09 at 13:30 +0200, Greg Kroah-Hartman wrote:
> > checkpatch.pl doesn't warn about it.
> 
> Odd, it should.  Send a patch for that?  :)

I haven't written any Perl in 20-odd years, and sold my 1999 Perl
Cookbook years ago, so probably not.
Greg KH Aug. 9, 2022, 12:52 p.m. UTC | #7
On Tue, Aug 09, 2022 at 01:18:43PM +0200, Bastien Nocera wrote:
> On Tue, 2022-08-09 at 12:35 +0200, Greg Kroah-Hartman wrote:
> > On Tue, Aug 09, 2022 at 11:42:59AM +0200, Bastien Nocera wrote:
> > > There is a need for userspace applications to open USB devices
> > > directly,
> > > for all the USB devices without a kernel-level class driver[1], and
> > > implemented in user-space.
> > > 
> > > As not all devices are built equal, we want to be able to revoke
> > > access to those devices whether it's because the user isn't at the
> > > console anymore, or because the web browser, or sandbox doesn't
> > > want
> > > to allow access to that device.
> > > 
> > > This commit implements the internal API used to revoke access to
> > > USB
> > > devices, given either bus and device numbers, or/and a user's
> > > effective UID.
> > 
> > Revoke what exactly?
> 
> Access.

Access from what?

> >   You should be revoking the file descriptor that is
> > open, not anything else as those are all transient and not uniquely
> > identified and racy.
> 
> They're "unique enough" (tm). The 

Are you sure?

They are racy from what I can tell, especially as you have no locking in
the patch that I noticed.

> > > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > > 
> > > [1]:
> > > Non exhaustive list of devices and device types that need raw USB
> > > access:
> > > - all manners of single-board computers and programmable chips and
> > > devices (avrdude, STLink, sunxi bootloader, flashrom, etc.)
> > > - 3D printers
> > > - scanners
> > > - LCD "displays"
> > > - user-space webcam and still cameras
> > > - game controllers
> > > - video/audio capture devices
> > > - sensors
> > > - software-defined radios
> > > - DJ/music equipment
> > > - protocol analysers
> > > - Rio 500 music player
> > > ---
> > >  drivers/usb/core/devio.c | 79
> > > ++++++++++++++++++++++++++++++++++++++--
> > >  drivers/usb/core/usb.h   |  2 +
> > >  2 files changed, 77 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> > > index b5b85bf80329..d8d212ef581f 100644
> > > --- a/drivers/usb/core/devio.c
> > > +++ b/drivers/usb/core/devio.c
> > > @@ -78,6 +78,7 @@ struct usb_dev_state {
> > >         int not_yet_resumed;
> > >         bool suspend_allowed;
> > >         bool privileges_dropped;
> > > +       bool revoked;
> > >  };
> > >  
> > >  struct usb_memory {
> > > @@ -183,6 +184,13 @@ static int connected(struct usb_dev_state *ps)
> > >                         ps->dev->state != USB_STATE_NOTATTACHED);
> > >  }
> > >  
> > > +static int disconnected_or_revoked(struct usb_dev_state *ps)
> > > +{
> > > +       return (list_empty(&ps->list) ||
> > > +                       ps->dev->state == USB_STATE_NOTATTACHED ||
> > > +                       ps->revoked);
> > > +}
> > > +
> > >  static void dec_usb_memory_use_count(struct usb_memory *usbm, int
> > > *count)
> > >  {
> > >         struct usb_dev_state *ps = usbm->ps;
> > > @@ -237,6 +245,9 @@ static int usbdev_mmap(struct file *file,
> > > struct vm_area_struct *vma)
> > >         dma_addr_t dma_handle;
> > >         int ret;
> > >  
> > > +       if (disconnected_or_revoked(ps))
> > > +               return -ENODEV;
> > > +
> > >         ret = usbfs_increase_memory_usage(size + sizeof(struct
> > > usb_memory));
> > >         if (ret)
> > >                 goto error;
> > > @@ -310,7 +321,7 @@ static ssize_t usbdev_read(struct file *file,
> > > char __user *buf, size_t nbytes,
> > >  
> > >         pos = *ppos;
> > >         usb_lock_device(dev);
> > > -       if (!connected(ps)) {
> > > +       if (disconnected_or_revoked(ps)) {
> > >                 ret = -ENODEV;
> > >                 goto err;
> > >         } else if (pos < 0) {
> > > @@ -2315,7 +2326,7 @@ static int proc_ioctl(struct usb_dev_state
> > > *ps, struct usbdevfs_ioctl *ctl)
> > >         if (ps->privileges_dropped)
> > >                 return -EACCES;
> > >  
> > > -       if (!connected(ps))
> > > +       if (disconnected_or_revoked(ps))
> > >                 return -ENODEV;
> > >  
> > >         /* alloc buffer */
> > > @@ -2555,7 +2566,7 @@ static int proc_forbid_suspend(struct
> > > usb_dev_state *ps)
> > >  
> > >  static int proc_allow_suspend(struct usb_dev_state *ps)
> > >  {
> > > -       if (!connected(ps))
> > > +       if (disconnected_or_revoked(ps))
> > >                 return -ENODEV;
> > >  
> > >         WRITE_ONCE(ps->not_yet_resumed, 1);
> > > @@ -2580,6 +2591,66 @@ static int proc_wait_for_resume(struct
> > > usb_dev_state *ps)
> > >         return proc_forbid_suspend(ps);
> > >  }
> > >  
> > > +static int usbdev_revoke(struct usb_dev_state *ps)
> > > +{
> > > +       struct usb_device *dev = ps->dev;
> > > +       unsigned int ifnum;
> > > +       struct async *as;
> > > +
> > > +       if (ps->revoked) {
> > > +               usb_unlock_device(dev);
> > > +               return -ENODEV;
> > > +       }
> > > +
> > > +       snoop(&dev->dev, "%s: REVOKE by PID %d: %s\n", __func__,
> > > +             task_pid_nr(current), current->comm);
> > > +
> > > +       ps->revoked = true;
> > > +
> > > +       for (ifnum = 0; ps->ifclaimed && ifnum < 8*sizeof(ps-
> > > >ifclaimed);
> > > +                       ifnum++) {
> > > +               if (test_bit(ifnum, &ps->ifclaimed))
> > > +                       releaseintf(ps, ifnum);
> > > +       }
> > > +       destroy_all_async(ps);
> > > +
> > > +       as = async_getcompleted(ps);
> > > +       while (as) {
> > > +               free_async(as);
> > > +               as = async_getcompleted(ps);
> > > +       }
> > > +
> > > +       wake_up(&ps->wait);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +int usb_revoke_for_euid(struct usb_device *udev,
> > > +               int euid)
> > > +{
> > > +       struct usb_dev_state *ps;
> > > +
> > > +       usb_lock_device(udev);
> > > +
> > > +       list_for_each_entry(ps, &udev->filelist, list) {
> > > +               if (euid >= 0) {
> > > +                       kuid_t kuid;
> > > +
> > > +                       if (!ps || !ps->cred)
> > > +                               continue;
> > > +                       kuid = ps->cred->euid;
> > 
> > How is this handling uid namespaces?
> 
> The caller is supposed to be outside namespaces. It's a BPF programme,
> so must be loaded by a privileged caller.

Where is "outside namespaces" defined anywhere?

And how is this tied to any bpf program?  I don't see that interface
anywhere in this series, where did I miss that?

> > Again, just revoke the file descriptor, like the BSDs do for a tiny
> > subset of device drivers.
> > 
> > This comes up ever so often, why does someone not just add real
> > revoke(2) support to Linux to handle it if they really really want it
> > (I
> > tried a long time ago, but didn't have it in me as I had no real
> > users
> > for it...)
> 
> This was already explained twice,

Explained where?

> there's nothing to "hand out" those file descriptors,

The kernel already handed out one when the device was opened using
usbfs.

> so no one but
> the kernel and the programme itself
> have that file descriptor, so it can't be used as an identifier.

That's the only unique identifier that you want to revoke and "know" it
is the device you are referring to.

That's why I recommend creating revoke(2), not just an odd "pause the
data to this device" api like this seems.  That's just going to confuse
lots of people as to "why did my device stop working?"

thanks,

greg k-h
Bastien Nocera Aug. 9, 2022, 1:27 p.m. UTC | #8
On Tue, 2022-08-09 at 14:52 +0200, Greg Kroah-Hartman wrote:
> On Tue, Aug 09, 2022 at 01:18:43PM +0200, Bastien Nocera wrote:
> > On Tue, 2022-08-09 at 12:35 +0200, Greg Kroah-Hartman wrote:
> > > On Tue, Aug 09, 2022 at 11:42:59AM +0200, Bastien Nocera wrote:
> > > > There is a need for userspace applications to open USB devices
> > > > directly,
> > > > for all the USB devices without a kernel-level class driver[1],
> > > > and
> > > > implemented in user-space.
> > > > 
> > > > As not all devices are built equal, we want to be able to
> > > > revoke
> > > > access to those devices whether it's because the user isn't at
> > > > the
> > > > console anymore, or because the web browser, or sandbox doesn't
> > > > want
> > > > to allow access to that device.
> > > > 
> > > > This commit implements the internal API used to revoke access
> > > > to
> > > > USB
> > > > devices, given either bus and device numbers, or/and a user's
> > > > effective UID.
> > > 
> > > Revoke what exactly?
> > 
> > Access.
> 
> Access from what?

revoke access to the USB device by one or all users.

> > >   You should be revoking the file descriptor that is
> > > open, not anything else as those are all transient and not
> > > uniquely
> > > identified and racy.
> > 
> > They're "unique enough" (tm). The 
> 
> Are you sure?
> 
> They are racy from what I can tell, especially as you have no locking
> in
> the patch that I noticed.

It does. It's not good locking because I rewrote that API at least 4
different times.

> > > > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > > > 
> > > > [1]:
> > > > Non exhaustive list of devices and device types that need raw
> > > > USB
> > > > access:
> > > > - all manners of single-board computers and programmable chips
> > > > and
> > > > devices (avrdude, STLink, sunxi bootloader, flashrom, etc.)
> > > > - 3D printers
> > > > - scanners
> > > > - LCD "displays"
> > > > - user-space webcam and still cameras
> > > > - game controllers
> > > > - video/audio capture devices
> > > > - sensors
> > > > - software-defined radios
> > > > - DJ/music equipment
> > > > - protocol analysers
> > > > - Rio 500 music player
> > > > ---
> > > >  drivers/usb/core/devio.c | 79
> > > > ++++++++++++++++++++++++++++++++++++++--
> > > >  drivers/usb/core/usb.h   |  2 +
> > > >  2 files changed, 77 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/usb/core/devio.c
> > > > b/drivers/usb/core/devio.c
> > > > index b5b85bf80329..d8d212ef581f 100644
> > > > --- a/drivers/usb/core/devio.c
> > > > +++ b/drivers/usb/core/devio.c
> > > > @@ -78,6 +78,7 @@ struct usb_dev_state {
> > > >         int not_yet_resumed;
> > > >         bool suspend_allowed;
> > > >         bool privileges_dropped;
> > > > +       bool revoked;
> > > >  };
> > > >  
> > > >  struct usb_memory {
> > > > @@ -183,6 +184,13 @@ static int connected(struct usb_dev_state
> > > > *ps)
> > > >                         ps->dev->state !=
> > > > USB_STATE_NOTATTACHED);
> > > >  }
> > > >  
> > > > +static int disconnected_or_revoked(struct usb_dev_state *ps)
> > > > +{
> > > > +       return (list_empty(&ps->list) ||
> > > > +                       ps->dev->state == USB_STATE_NOTATTACHED
> > > > ||
> > > > +                       ps->revoked);
> > > > +}
> > > > +
> > > >  static void dec_usb_memory_use_count(struct usb_memory *usbm,
> > > > int
> > > > *count)
> > > >  {
> > > >         struct usb_dev_state *ps = usbm->ps;
> > > > @@ -237,6 +245,9 @@ static int usbdev_mmap(struct file *file,
> > > > struct vm_area_struct *vma)
> > > >         dma_addr_t dma_handle;
> > > >         int ret;
> > > >  
> > > > +       if (disconnected_or_revoked(ps))
> > > > +               return -ENODEV;
> > > > +
> > > >         ret = usbfs_increase_memory_usage(size + sizeof(struct
> > > > usb_memory));
> > > >         if (ret)
> > > >                 goto error;
> > > > @@ -310,7 +321,7 @@ static ssize_t usbdev_read(struct file
> > > > *file,
> > > > char __user *buf, size_t nbytes,
> > > >  
> > > >         pos = *ppos;
> > > >         usb_lock_device(dev);
> > > > -       if (!connected(ps)) {
> > > > +       if (disconnected_or_revoked(ps)) {
> > > >                 ret = -ENODEV;
> > > >                 goto err;
> > > >         } else if (pos < 0) {
> > > > @@ -2315,7 +2326,7 @@ static int proc_ioctl(struct
> > > > usb_dev_state
> > > > *ps, struct usbdevfs_ioctl *ctl)
> > > >         if (ps->privileges_dropped)
> > > >                 return -EACCES;
> > > >  
> > > > -       if (!connected(ps))
> > > > +       if (disconnected_or_revoked(ps))
> > > >                 return -ENODEV;
> > > >  
> > > >         /* alloc buffer */
> > > > @@ -2555,7 +2566,7 @@ static int proc_forbid_suspend(struct
> > > > usb_dev_state *ps)
> > > >  
> > > >  static int proc_allow_suspend(struct usb_dev_state *ps)
> > > >  {
> > > > -       if (!connected(ps))
> > > > +       if (disconnected_or_revoked(ps))
> > > >                 return -ENODEV;
> > > >  
> > > >         WRITE_ONCE(ps->not_yet_resumed, 1);
> > > > @@ -2580,6 +2591,66 @@ static int proc_wait_for_resume(struct
> > > > usb_dev_state *ps)
> > > >         return proc_forbid_suspend(ps);
> > > >  }
> > > >  
> > > > +static int usbdev_revoke(struct usb_dev_state *ps)
> > > > +{
> > > > +       struct usb_device *dev = ps->dev;
> > > > +       unsigned int ifnum;
> > > > +       struct async *as;
> > > > +
> > > > +       if (ps->revoked) {
> > > > +               usb_unlock_device(dev);
> > > > +               return -ENODEV;
> > > > +       }
> > > > +
> > > > +       snoop(&dev->dev, "%s: REVOKE by PID %d: %s\n",
> > > > __func__,
> > > > +             task_pid_nr(current), current->comm);
> > > > +
> > > > +       ps->revoked = true;
> > > > +
> > > > +       for (ifnum = 0; ps->ifclaimed && ifnum < 8*sizeof(ps-
> > > > > ifclaimed);
> > > > +                       ifnum++) {
> > > > +               if (test_bit(ifnum, &ps->ifclaimed))
> > > > +                       releaseintf(ps, ifnum);
> > > > +       }
> > > > +       destroy_all_async(ps);
> > > > +
> > > > +       as = async_getcompleted(ps);
> > > > +       while (as) {
> > > > +               free_async(as);
> > > > +               as = async_getcompleted(ps);
> > > > +       }
> > > > +
> > > > +       wake_up(&ps->wait);
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +int usb_revoke_for_euid(struct usb_device *udev,
> > > > +               int euid)
> > > > +{
> > > > +       struct usb_dev_state *ps;
> > > > +
> > > > +       usb_lock_device(udev);
> > > > +
> > > > +       list_for_each_entry(ps, &udev->filelist, list) {
> > > > +               if (euid >= 0) {
> > > > +                       kuid_t kuid;
> > > > +
> > > > +                       if (!ps || !ps->cred)
> > > > +                               continue;
> > > > +                       kuid = ps->cred->euid;
> > > 
> > > How is this handling uid namespaces?
> > 
> > The caller is supposed to be outside namespaces. It's a BPF
> > programme,
> > so must be loaded by a privileged caller.
> 
> Where is "outside namespaces" defined anywhere?
> 
> And how is this tied to any bpf program?  I don't see that interface
> anywhere in this series, where did I miss that?

It's in 2/2.

The link to the user-space programme is in the "RFC v2" version of the
patch from last week. It calls into the kernel through that function
which is exported through BPF.

> 
> > > Again, just revoke the file descriptor, like the BSDs do for a
> > > tiny
> > > subset of device drivers.
> > > 
> > > This comes up ever so often, why does someone not just add real
> > > revoke(2) support to Linux to handle it if they really really
> > > want it
> > > (I
> > > tried a long time ago, but didn't have it in me as I had no real
> > > users
> > > for it...)
> > 
> > This was already explained twice,
> 
> Explained where?

https://www.spinics.net/lists/linux-usb/msg225448.html
https://www.spinics.net/lists/linux-usb/msg229753.html

> > there's nothing to "hand out" those file descriptors,
> 
> The kernel already handed out one when the device was opened using
> usbfs.
> 
> > so no one but
> > the kernel and the programme itself
> > have that file descriptor, so it can't be used as an identifier.
> 
> That's the only unique identifier that you want to revoke and "know"
> it
> is the device you are referring to.
> 
> That's why I recommend creating revoke(2), not just an odd "pause the
> data to this device" api like this seems.  That's just going to
> confuse
> lots of people as to "why did my device stop working?"

"Why did my app stop working. Because you switched users and your app
needs to be updated to take that into account."

I mean, ENODEV is returned on ioctls, it's just that the poll() doesn't
HUP.
Greg KH Aug. 9, 2022, 4:31 p.m. UTC | #9
On Tue, Aug 09, 2022 at 03:27:16PM +0200, Bastien Nocera wrote:
> The link to the user-space programme is in the "RFC v2" version of the
> patch from last week. It calls into the kernel through that function
> which is exported through BPF.
> 
> > 
> > > > Again, just revoke the file descriptor, like the BSDs do for a
> > > > tiny
> > > > subset of device drivers.
> > > > 
> > > > This comes up ever so often, why does someone not just add real
> > > > revoke(2) support to Linux to handle it if they really really
> > > > want it
> > > > (I
> > > > tried a long time ago, but didn't have it in me as I had no real
> > > > users
> > > > for it...)
> > > 
> > > This was already explained twice,
> > 
> > Explained where?
> 
> https://www.spinics.net/lists/linux-usb/msg225448.html
> https://www.spinics.net/lists/linux-usb/msg229753.html

Please use lore.kernel.org.

Anyway, pointing to random old submissions of an RFC series does not
mean that you do not have to document and justify this design decision
in this patch submission.

Assume that reviewers have NO knowlege of previous submissions of your
patch series.  Because we usually do not, given how many changes we
review all the time.

Please resend this, as a v4, and update the changelog descriptions based
on the comments so far on this series and I will be glad to review it
sometime after -rc1 is out, as there's nothing I can do with it right
now.

thanks,

greg k-h
Eric W. Biederman Aug. 9, 2022, 4:46 p.m. UTC | #10
Bastien Nocera <hadess@hadess.net> writes:

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

At a minimum that should be a kuid_t.

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

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


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

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

Eric
Bastien Nocera Aug. 9, 2022, 5:08 p.m. UTC | #11
On Tue, 2022-08-09 at 11:46 -0500, Eric W. Biederman wrote:
> Bastien Nocera <hadess@hadess.net> writes:
> 
> > +                       kuid_t kuid;
> > +
> > +                       if (!ps || !ps->cred)
> > +                               continue;
> > +                       kuid = ps->cred->euid;
> > +                       if (kuid.val != euid)
>                         ^^^^^^^^^^^^^^^^^^^^^
> That test should be if (!uid_eq(ps->cred->euid, euid))
> 
> 
> The point is that inside the kernel all uid data should be dealt with
> in the kuid_t data type.  So as to avoid confusing uids with some
> other
> kind of integer data.

That uid comes from user-space, see patch 2/2.

Do you have examples of accepting euids from user-space and stashing
them into kuid_t?

If you also have any idea about user namespaces as mentioned in the
cover letter for this patch set, I would appreciate.
Bastien Nocera Aug. 9, 2022, 5:16 p.m. UTC | #12
On Tue, 2022-08-09 at 18:31 +0200, Greg Kroah-Hartman wrote:
> On Tue, Aug 09, 2022 at 03:27:16PM +0200, Bastien Nocera wrote:
> > The link to the user-space programme is in the "RFC v2" version of
> > the
> > patch from last week. It calls into the kernel through that
> > function
> > which is exported through BPF.
> > 
> > > 
> > > > > Again, just revoke the file descriptor, like the BSDs do for
> > > > > a
> > > > > tiny
> > > > > subset of device drivers.
> > > > > 
> > > > > This comes up ever so often, why does someone not just add
> > > > > real
> > > > > revoke(2) support to Linux to handle it if they really really
> > > > > want it
> > > > > (I
> > > > > tried a long time ago, but didn't have it in me as I had no
> > > > > real
> > > > > users
> > > > > for it...)
> > > > 
> > > > This was already explained twice,
> > > 
> > > Explained where?
> > 
> > https://www.spinics.net/lists/linux-usb/msg225448.html
> > https://www.spinics.net/lists/linux-usb/msg229753.html
> 
> Please use lore.kernel.org.

Would be great if it showed up when somebody searches for "linux-usb
mailing-list".

> Anyway, pointing to random old submissions of an RFC series does not
> mean that you do not have to document and justify this design
> decision
> in this patch submission.

I guess me repeatedly asking for guidance as to what information I
should add to the commit message while I was being yelled at didn't get
through.

> Assume that reviewers have NO knowlege of previous submissions of
> your
> patch series.  Because we usually do not, given how many changes we
> review all the time.
> 
> Please resend this, as a v4, and update the changelog descriptions
> based
> on the comments so far on this series and I will be glad to review it
> sometime after -rc1 is out, as there's nothing I can do with it right
> now.

Sure.
Alan Stern Aug. 9, 2022, 7:43 p.m. UTC | #13
On Tue, Aug 09, 2022 at 03:27:16PM +0200, Bastien Nocera wrote:
> On Tue, 2022-08-09 at 14:52 +0200, Greg Kroah-Hartman wrote:
> > On Tue, Aug 09, 2022 at 01:18:43PM +0200, Bastien Nocera wrote:
> > > > Revoke what exactly?
> > > 
> > > Access.
> > 
> > Access from what?
> 
> revoke access to the USB device by one or all users.
> 
> > > >   You should be revoking the file descriptor that is
> > > > open, not anything else as those are all transient and not
> > > > uniquely
> > > > identified and racy.
> > > 
> > > They're "unique enough" (tm). The 
> > 
> > Are you sure?
> > 
> > They are racy from what I can tell, especially as you have no locking
> > in
> > the patch that I noticed.
> 
> It does. It's not good locking because I rewrote that API at least 4
> different times.

> > > > How is this handling uid namespaces?
> > > 
> > > The caller is supposed to be outside namespaces. It's a BPF
> > > programme,
> > > so must be loaded by a privileged caller.
> > 
> > Where is "outside namespaces" defined anywhere?
> > 
> > And how is this tied to any bpf program?  I don't see that interface
> > anywhere in this series, where did I miss that?
> 
> It's in 2/2.
> 
> The link to the user-space programme is in the "RFC v2" version of the
> patch from last week. It calls into the kernel through that function
> which is exported through BPF.
> 
> > 
> > > > Again, just revoke the file descriptor, like the BSDs do for a
> > > > tiny
> > > > subset of device drivers.
> > > > 
> > > > This comes up ever so often, why does someone not just add real
> > > > revoke(2) support to Linux to handle it if they really really
> > > > want it
> > > > (I
> > > > tried a long time ago, but didn't have it in me as I had no real
> > > > users
> > > > for it...)
> > > 
> > > This was already explained twice,
> > 
> > Explained where?
> 
> https://www.spinics.net/lists/linux-usb/msg225448.html
> https://www.spinics.net/lists/linux-usb/msg229753.html
> 
> > > there's nothing to "hand out" those file descriptors,
> > 
> > The kernel already handed out one when the device was opened using
> > usbfs.
> > 
> > > so no one but
> > > the kernel and the programme itself
> > > have that file descriptor, so it can't be used as an identifier.
> > 
> > That's the only unique identifier that you want to revoke and "know"
> > it
> > is the device you are referring to.
> > 
> > That's why I recommend creating revoke(2), not just an odd "pause the
> > data to this device" api like this seems.  That's just going to
> > confuse
> > lots of people as to "why did my device stop working?"
> 
> "Why did my app stop working. Because you switched users and your app
> needs to be updated to take that into account."
> 
> I mean, ENODEV is returned on ioctls, it's just that the poll() doesn't
> HUP.

Bastien, maybe it would help if you explained to Greg and the mailing 
list exactly what you want to accomplish, both in more detail and from a 
higher-level viewpoint than you already have provided.  Also, give an 
example or two showing when this mechanism would be invoked, how it would 
work, and what it would do.

(Also, usbfs doesn't bind to USB devices but it does bind to USB 
interfaces; see claimintf() in devio.c.)

Greg, unbinding usbfs would do only part of what Bastien wants.  It would 
prevent a process from sending or receiving data over bulk, interrupt, or 
isochronous endpoints connected to interfaces that were already bound, 
but it would not prevent the process from accessing endpoint 0 or from 
re-binding to interfaces.  What Bastien wants is a lot more like the 
kernel pretending to the process that the USB device has been 
disconnected and therefore is totally inaccessible.

As for whether this is the best (or even the right) thing to do, or 
whether there are other ways to accomplish the ultimate goal of not 
allowing user programs to access certain local devices unless those 
programs belong to an active (console?) login session...  That's beyond 
my scope.  I'm also not clear on how namespaces figure into the whole 
discussion.

Alan Stern
kernel test robot Aug. 10, 2022, 5:18 p.m. UTC | #14
Hi Bastien,

I love your patch! Perhaps something to improve:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on balbi-usb/testing/next peter-chen-usb/for-usb-next linus/master v5.19 next-20220810]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Bastien-Nocera/USB-core-add-a-way-to-revoke-access-to-open-USB-devices/20220809-174609
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: arm64-randconfig-r001-20220810 (https://download.01.org/0day-ci/archive/20220811/202208110108.ilG9Ea14-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 5f1c7e2cc5a3c07cbc2412e851a7283c1841f520)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/6bd6f04e6d463be82fbf45585e4af84925bf1ab9
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Bastien-Nocera/USB-core-add-a-way-to-revoke-access-to-open-USB-devices/20220809-174609
        git checkout 6bd6f04e6d463be82fbf45585e4af84925bf1ab9
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/usb/core/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/usb/core/devio.c:2649:1: warning: unused label 'out' [-Wunused-label]
   out:
   ^~~~
   1 warning generated.


vim +/out +2649 drivers/usb/core/devio.c

  2627	
  2628	int usb_revoke_for_euid(struct usb_device *udev,
  2629			int euid)
  2630	{
  2631		struct usb_dev_state *ps;
  2632	
  2633		usb_lock_device(udev);
  2634	
  2635		list_for_each_entry(ps, &udev->filelist, list) {
  2636			if (euid >= 0) {
  2637				kuid_t kuid;
  2638	
  2639				if (!ps || !ps->cred)
  2640					continue;
  2641				kuid = ps->cred->euid;
  2642				if (kuid.val != euid)
  2643					continue;
  2644			}
  2645	
  2646			usbdev_revoke(ps);
  2647		}
  2648	
> 2649	out:
  2650		usb_unlock_device(udev);
  2651		return 0;
  2652	}
  2653
kernel test robot Aug. 10, 2022, 5:28 p.m. UTC | #15
Hi Bastien,

I love your patch! Perhaps something to improve:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on balbi-usb/testing/next peter-chen-usb/for-usb-next linus/master v5.19 next-20220810]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Bastien-Nocera/USB-core-add-a-way-to-revoke-access-to-open-USB-devices/20220809-174609
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: i386-defconfig (https://download.01.org/0day-ci/archive/20220811/202208110130.PZkeHYmL-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/6bd6f04e6d463be82fbf45585e4af84925bf1ab9
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Bastien-Nocera/USB-core-add-a-way-to-revoke-access-to-open-USB-devices/20220809-174609
        git checkout 6bd6f04e6d463be82fbf45585e4af84925bf1ab9
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/usb/core/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/usb/core/devio.c: In function 'usb_revoke_for_euid':
>> drivers/usb/core/devio.c:2649:1: warning: label 'out' defined but not used [-Wunused-label]
    2649 | out:
         | ^~~


vim +/out +2649 drivers/usb/core/devio.c

  2627	
  2628	int usb_revoke_for_euid(struct usb_device *udev,
  2629			int euid)
  2630	{
  2631		struct usb_dev_state *ps;
  2632	
  2633		usb_lock_device(udev);
  2634	
  2635		list_for_each_entry(ps, &udev->filelist, list) {
  2636			if (euid >= 0) {
  2637				kuid_t kuid;
  2638	
  2639				if (!ps || !ps->cred)
  2640					continue;
  2641				kuid = ps->cred->euid;
  2642				if (kuid.val != euid)
  2643					continue;
  2644			}
  2645	
  2646			usbdev_revoke(ps);
  2647		}
  2648	
> 2649	out:
  2650		usb_unlock_device(udev);
  2651		return 0;
  2652	}
  2653
diff mbox series

Patch

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index b5b85bf80329..d8d212ef581f 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -78,6 +78,7 @@  struct usb_dev_state {
 	int not_yet_resumed;
 	bool suspend_allowed;
 	bool privileges_dropped;
+	bool revoked;
 };
 
 struct usb_memory {
@@ -183,6 +184,13 @@  static int connected(struct usb_dev_state *ps)
 			ps->dev->state != USB_STATE_NOTATTACHED);
 }
 
+static int disconnected_or_revoked(struct usb_dev_state *ps)
+{
+	return (list_empty(&ps->list) ||
+			ps->dev->state == USB_STATE_NOTATTACHED ||
+			ps->revoked);
+}
+
 static void dec_usb_memory_use_count(struct usb_memory *usbm, int *count)
 {
 	struct usb_dev_state *ps = usbm->ps;
@@ -237,6 +245,9 @@  static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
 	dma_addr_t dma_handle;
 	int ret;
 
+	if (disconnected_or_revoked(ps))
+		return -ENODEV;
+
 	ret = usbfs_increase_memory_usage(size + sizeof(struct usb_memory));
 	if (ret)
 		goto error;
@@ -310,7 +321,7 @@  static ssize_t usbdev_read(struct file *file, char __user *buf, size_t nbytes,
 
 	pos = *ppos;
 	usb_lock_device(dev);
-	if (!connected(ps)) {
+	if (disconnected_or_revoked(ps)) {
 		ret = -ENODEV;
 		goto err;
 	} else if (pos < 0) {
@@ -2315,7 +2326,7 @@  static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl)
 	if (ps->privileges_dropped)
 		return -EACCES;
 
-	if (!connected(ps))
+	if (disconnected_or_revoked(ps))
 		return -ENODEV;
 
 	/* alloc buffer */
@@ -2555,7 +2566,7 @@  static int proc_forbid_suspend(struct usb_dev_state *ps)
 
 static int proc_allow_suspend(struct usb_dev_state *ps)
 {
-	if (!connected(ps))
+	if (disconnected_or_revoked(ps))
 		return -ENODEV;
 
 	WRITE_ONCE(ps->not_yet_resumed, 1);
@@ -2580,6 +2591,66 @@  static int proc_wait_for_resume(struct usb_dev_state *ps)
 	return proc_forbid_suspend(ps);
 }
 
+static int usbdev_revoke(struct usb_dev_state *ps)
+{
+	struct usb_device *dev = ps->dev;
+	unsigned int ifnum;
+	struct async *as;
+
+	if (ps->revoked) {
+		usb_unlock_device(dev);
+		return -ENODEV;
+	}
+
+	snoop(&dev->dev, "%s: REVOKE by PID %d: %s\n", __func__,
+	      task_pid_nr(current), current->comm);
+
+	ps->revoked = true;
+
+	for (ifnum = 0; ps->ifclaimed && ifnum < 8*sizeof(ps->ifclaimed);
+			ifnum++) {
+		if (test_bit(ifnum, &ps->ifclaimed))
+			releaseintf(ps, ifnum);
+	}
+	destroy_all_async(ps);
+
+	as = async_getcompleted(ps);
+	while (as) {
+		free_async(as);
+		as = async_getcompleted(ps);
+	}
+
+	wake_up(&ps->wait);
+
+	return 0;
+}
+
+int usb_revoke_for_euid(struct usb_device *udev,
+		int euid)
+{
+	struct usb_dev_state *ps;
+
+	usb_lock_device(udev);
+
+	list_for_each_entry(ps, &udev->filelist, list) {
+		if (euid >= 0) {
+			kuid_t kuid;
+
+			if (!ps || !ps->cred)
+				continue;
+			kuid = ps->cred->euid;
+			if (kuid.val != euid)
+				continue;
+		}
+
+		usbdev_revoke(ps);
+	}
+
+out:
+	usb_unlock_device(udev);
+	return 0;
+}
+
 /*
  * NOTE:  All requests here that have interface numbers as parameters
  * are assuming that somehow the configuration has been prevented from
@@ -2623,7 +2694,7 @@  static long usbdev_do_ioctl(struct file *file, unsigned int cmd,
 #endif
 	}
 
-	if (!connected(ps)) {
+	if (disconnected_or_revoked(ps)) {
 		usb_unlock_device(dev);
 		return -ENODEV;
 	}
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index 82538daac8b8..5b007530a1cf 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -34,6 +34,8 @@  extern int usb_deauthorize_device(struct usb_device *);
 extern int usb_authorize_device(struct usb_device *);
 extern void usb_deauthorize_interface(struct usb_interface *);
 extern void usb_authorize_interface(struct usb_interface *);
+extern int usb_revoke_for_euid(struct usb_device *udev,
+		int euid);
 extern void usb_detect_quirks(struct usb_device *udev);
 extern void usb_detect_interface_quirks(struct usb_device *udev);
 extern void usb_release_quirk_list(void);