[RFC v2] USB: core: add a way to revoke access to open USB devices

[RFC v2] USB: core: add a way to revoke access to open USB devices

[Bastien Nocera]

Hey,

This is a follow-up to "[RFC v1] USB: core: add USBDEVFS_REVOKE ioctl" from April.

The end-goal is being able to cut-off access to USB devices for
applications/programs that open raw USB devices using, in most cases,
libusb.

I've implemented this using BPF, so we don't need to add new ioctls.
The presence or absence of that feature can be evaluated at runtime,
and can be used to implement revoke support on session switching,
in logind for example:
https://github.com/hadess/usb-revoke-userspace/

I have some notes and questions on the API as it is exposed.

- I didn't see a point in having multiple kernel functions as entry
  points as I was going to use a single BPF program as an entry point,
  which can check the arguments. Can I rely on the BPF program checking
  those arguments, or should I re-check them again in the kernel
  implementation?

- Are my UID checks correct if I expect revoking to be called outside
  namespaces, on the effective UID of the user. Is there a way to assert
  that?

- Is there a good "errno" error for ENOSUCHUSER for using in the
  usb_revoke() loop if we couldn't find any USB device matching the
  requested user?

Cheers

Bastien Nocera (2):
  USB: core: add a way to revoke access to open USB devices
  usb: Implement usb_revoke() BPF function

 drivers/usb/core/devio.c | 105 ++++++++++++++++++++++++++++++++++++---
 drivers/usb/core/usb.c   |  51 +++++++++++++++++++
 drivers/usb/core/usb.h   |   8 +++
 3 files changed, 158 insertions(+), 6 deletions(-)

-- 
2.36.1

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

[Bastien Nocera]

There is a need for userspace applications to open USB devices directly,
for all the USB devices without a kernel-level class driver[1], and
implemented in user-space.

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

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

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

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

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index b5b85bf80329..a87fed12e307 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -78,6 +78,7 @@ struct usb_dev_state {
 	int not_yet_resumed;
 	bool suspend_allowed;
 	bool privileges_dropped;
+	bool revoked;
 };
 
 struct usb_memory {
@@ -237,6 +238,13 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
 	dma_addr_t dma_handle;
 	int ret;
 
+	usb_lock_device(ps->dev);
+	if (!connected(ps) || ps->revoked) {
+		usb_unlock_device(ps->dev);
+		return -ENODEV;
+	}
+	usb_unlock_device(ps->dev);
+
 	ret = usbfs_increase_memory_usage(size + sizeof(struct usb_memory));
 	if (ret)
 		goto error;
@@ -298,6 +306,15 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
 	return ret;
 }
 
+static loff_t usbdev_llseek(struct file *file, loff_t offset, int whence)
+{
+	struct usb_dev_state *ps = file->private_data;
+
+	if (!connected(ps) || ps->revoked)
+		return -ENODEV;
+	return no_seek_end_llseek(file, offset, whence);
+}
+
 static ssize_t usbdev_read(struct file *file, char __user *buf, size_t nbytes,
 			   loff_t *ppos)
 {
@@ -310,7 +327,7 @@ static ssize_t usbdev_read(struct file *file, char __user *buf, size_t nbytes,
 
 	pos = *ppos;
 	usb_lock_device(dev);
-	if (!connected(ps)) {
+	if (!connected(ps) || ps->revoked) {
 		ret = -ENODEV;
 		goto err;
 	} else if (pos < 0) {
@@ -2117,7 +2134,7 @@ static int proc_reapurbnonblock(struct usb_dev_state *ps, void __user *arg)
 		retval = processcompl(as, (void __user * __user *)arg);
 		free_async(as);
 	} else {
-		retval = (connected(ps) ? -EAGAIN : -ENODEV);
+		retval = (connected(ps) && !ps->revoked ? -EAGAIN : -ENODEV);
 	}
 	return retval;
 }
@@ -2262,7 +2279,7 @@ static int proc_reapurbnonblock_compat(struct usb_dev_state *ps, void __user *ar
 		retval = processcompl_compat(as, (void __user * __user *)arg);
 		free_async(as);
 	} else {
-		retval = (connected(ps) ? -EAGAIN : -ENODEV);
+		retval = (connected(ps) && !ps->revoked ? -EAGAIN : -ENODEV);
 	}
 	return retval;
 }
@@ -2580,6 +2597,82 @@ static int proc_wait_for_resume(struct usb_dev_state *ps)
 	return proc_forbid_suspend(ps);
 }
 
+static int usbdev_revoke(struct usb_dev_state *ps)
+{
+	struct usb_device *dev = ps->dev;
+	unsigned int ifnum;
+	struct async *as;
+
+	if (ps->revoked) {
+		usb_unlock_device(dev);
+		return -ENODEV;
+	}
+	ps->revoked = true;
+
+	for (ifnum = 0; ps->ifclaimed && ifnum < 8*sizeof(ps->ifclaimed);
+			ifnum++) {
+		if (test_bit(ifnum, &ps->ifclaimed))
+			releaseintf(ps, ifnum);
+	}
+	destroy_all_async(ps);
+
+	as = async_getcompleted(ps);
+	while (as) {
+		free_async(as);
+		as = async_getcompleted(ps);
+	}
+
+	wake_up(&ps->wait);
+
+	snoop(&dev->dev, "%s: REVOKE by PID %d: %s\n", __func__,
+	      task_pid_nr(current), current->comm);
+
+	return 0;
+}
+
+int usb_revoke(struct usb_device *udev,
+		struct usb_revoke_match *match)
+{
+	struct usb_dev_state *ps;
+	int ret = -ENOENT;
+
+	usb_lock_device(udev);
+
+	if (match->devnum >= 0 &&
+	    match->busnum >= 0) {
+		int devnum, busnum;
+
+		devnum = udev->devnum;
+		busnum = udev->bus->busnum;
+		if (match->busnum != busnum ||
+		    match->devnum != devnum) {
+			ret = -ENODEV;
+			goto out;
+		}
+	}
+
+	list_for_each_entry(ps, &udev->filelist, list) {
+		if (match->euid >= 0) {
+			kuid_t kuid;
+
+			if (!ps || !ps->cred)
+				continue;
+			kuid = ps->cred->euid;
+			if (kuid.val != match->euid)
+				continue;
+		}
+
+		if (ret == 0)
+			usbdev_revoke(ps);
+		else
+			ret = usbdev_revoke(ps);
+	}
+
+out:
+	usb_unlock_device(udev);
+	return ret;
+}
+
 /*
  * NOTE:  All requests here that have interface numbers as parameters
  * are assuming that somehow the configuration has been prevented from
@@ -2623,7 +2716,7 @@ static long usbdev_do_ioctl(struct file *file, unsigned int cmd,
 #endif
 	}
 
-	if (!connected(ps)) {
+	if (!connected(ps) || ps->revoked) {
 		usb_unlock_device(dev);
 		return -ENODEV;
 	}
@@ -2819,7 +2912,7 @@ static __poll_t usbdev_poll(struct file *file,
 	poll_wait(file, &ps->wait, wait);
 	if (file->f_mode & FMODE_WRITE && !list_empty(&ps->async_completed))
 		mask |= EPOLLOUT | EPOLLWRNORM;
-	if (!connected(ps))
+	if (!connected(ps) || ps->revoked)
 		mask |= EPOLLHUP;
 	if (list_empty(&ps->list))
 		mask |= EPOLLERR;
@@ -2828,7 +2921,7 @@ static __poll_t usbdev_poll(struct file *file,
 
 const struct file_operations usbdev_file_operations = {
 	.owner =	  THIS_MODULE,
-	.llseek =	  no_seek_end_llseek,
+	.llseek =	  usbdev_llseek,
 	.read =		  usbdev_read,
 	.poll =		  usbdev_poll,
 	.unlocked_ioctl = usbdev_ioctl,
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index 82538daac8b8..b12c352869f0 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -9,6 +9,11 @@
 struct usb_hub_descriptor;
 struct usb_dev_state;
 
+struct usb_revoke_match {
+	int busnum, devnum; /* -1 to match all devices */
+	int euid; /* -1 to match all users */
+};
+
 /* Functions local to drivers/usb/core/ */
 
 extern int usb_create_sysfs_dev_files(struct usb_device *dev);
@@ -34,6 +39,9 @@ extern int usb_deauthorize_device(struct usb_device *);
 extern int usb_authorize_device(struct usb_device *);
 extern void usb_deauthorize_interface(struct usb_interface *);
 extern void usb_authorize_interface(struct usb_interface *);
+extern int usb_revoke(struct usb_device *udev,
+		struct usb_revoke_match *match);
+extern int usbdev_get_uid(struct usb_dev_state *ps);
 extern void usb_detect_quirks(struct usb_device *udev);
 extern void usb_detect_interface_quirks(struct usb_device *udev);
 extern void usb_release_quirk_list(void);
-- 
2.36.1

[RFC v2 2/2] usb: Implement usb_revoke() BPF function

[Bastien Nocera]

This functionality allows a sufficiently privileged user-space process
to upload a script that will call to usb_revoke_device() as if it were
a kernel API.

This functionality will be used by logind to revoke access to devices on
fast user-switching to start with.

Signed-off-by: Bastien Nocera <hadess@hadess.net>
---
 drivers/usb/core/usb.c | 51 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 2f71636af6e1..8cac72271a8a 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -38,6 +38,8 @@
 #include <linux/workqueue.h>
 #include <linux/debugfs.h>
 #include <linux/usb/of.h>
+#include <linux/btf.h>
+#include <linux/btf_ids.h>
 
 #include <asm/io.h>
 #include <linux/scatterlist.h>
@@ -438,6 +440,41 @@ static int usb_dev_uevent(struct device *dev, struct kobj_uevent_env *env)
 	return 0;
 }
 
+struct revoke_data {
+	struct usb_revoke_match match;
+	int ret;
+};
+
+static int
+__usb_revoke(struct usb_device *udev, void *data)
+{
+	struct revoke_data *revoke_data = data;
+	struct usb_revoke_match match = revoke_data->match;
+	int ret;
+
+	ret = usb_revoke(udev, &match);
+	if (ret == 0)
+		revoke_data->ret = 0;
+	else if (revoke_data->ret == 1)
+		revoke_data->ret = ret;
+	return 0;
+}
+
+noinline int
+usb_revoke_device(int busnum, int devnum, unsigned int euid)
+{
+	struct revoke_data revoke_data;
+
+	revoke_data.match.busnum = busnum;
+	revoke_data.match.devnum = devnum;
+	revoke_data.match.euid = euid;
+	revoke_data.ret = 1;
+
+	usb_for_each_dev(&revoke_data, __usb_revoke);
+
+	return revoke_data.ret;
+}
+
 #ifdef	CONFIG_PM
 
 /* USB device Power-Management thunks.
@@ -1004,6 +1041,15 @@ static void usb_debugfs_cleanup(void)
 /*
  * Init
  */
+BTF_SET_START(usbdev_kfunc_ids)
+BTF_ID(func, usb_revoke_device)
+BTF_SET_END(usbdev_kfunc_ids)
+
+static const struct btf_kfunc_id_set usbdev_kfunc_set = {
+	.owner     = THIS_MODULE,
+	.check_set = &usbdev_kfunc_ids,
+};
+
 static int __init usb_init(void)
 {
 	int retval;
@@ -1035,9 +1081,14 @@ static int __init usb_init(void)
 	if (retval)
 		goto hub_init_failed;
 	retval = usb_register_device_driver(&usb_generic_driver, THIS_MODULE);
+	if (retval)
+		goto register_failed;
+	retval = register_btf_kfunc_id_set(BPF_PROG_TYPE_SYSCALL, &usbdev_kfunc_set);
 	if (!retval)
 		goto out;
+	usb_deregister_device_driver(&usb_generic_driver);
 
+register_failed:
 	usb_hub_cleanup();
 hub_init_failed:
 	usb_devio_cleanup();
-- 
2.36.1

Re: [RFC v2] USB: core: add a way to revoke access to open USB devices

[Alan Stern]

On Thu, Aug 04, 2022 at 06:03:04PM +0200, Bastien Nocera wrote:
> Hey,
> 
> This is a follow-up to "[RFC v1] USB: core: add USBDEVFS_REVOKE ioctl" from April.
> 
> The end-goal is being able to cut-off access to USB devices for
> applications/programs that open raw USB devices using, in most cases,
> libusb.
> 
> I've implemented this using BPF, so we don't need to add new ioctls.

I'm not sure that's a good reason for using BPF.

> The presence or absence of that feature can be evaluated at runtime,
> and can be used to implement revoke support on session switching,
> in logind for example:
> https://github.com/hadess/usb-revoke-userspace/

This isn't clear.  Are you talking about having the kernel automatically 
do this when certain conditions are met?  Or do you mean that logind 
could invoke the BPF program at the appropriate times?

Left unsaid here is that logind generally won't have a file descriptor 
for any of the open USB devices that it wants to revoke access to.  
That's a good reason to use BPF instead of ioctl.

If you're going to revoke access to devices upon session switching, 
shouldn't the mechanism be more general?  I mean, shouldn't it apply to 
all sorts of devices, not just those that happen to be USB?

Also, if you're going to use session switching as your criterion for 
revoking access to USB devices then what will you do to restore access 
when the session switches back?

> I have some notes and questions on the API as it is exposed.
> 
> - I didn't see a point in having multiple kernel functions as entry
>   points as I was going to use a single BPF program as an entry point,
>   which can check the arguments. Can I rely on the BPF program checking
>   those arguments, or should I re-check them again in the kernel
>   implementation?

What is there to check?

> - Are my UID checks correct if I expect revoking to be called outside
>   namespaces, on the effective UID of the user. Is there a way to assert
>   that?

We're probably not the right people to ask.  You could try Eric 
Biederman.

> - Is there a good "errno" error for ENOSUCHUSER for using in the
>   usb_revoke() loop if we couldn't find any USB device matching the
>   requested user?

Why do you want that to be an error?  If you tell the kernel "Remove all 
access to USB devices for user X", why should it be an error if X doesn't 
have any open file descriptors for USB devices?

For that matter, are you certain that basing this on the UID is the right 
way to go?  What if there are two different login sessions both using the 
same UID?

Alan Stern

> 
> Cheers
> 
> Bastien Nocera (2):
>   USB: core: add a way to revoke access to open USB devices
>   usb: Implement usb_revoke() BPF function
> 
>  drivers/usb/core/devio.c | 105 ++++++++++++++++++++++++++++++++++++---
>  drivers/usb/core/usb.c   |  51 +++++++++++++++++++
>  drivers/usb/core/usb.h   |   8 +++
>  3 files changed, 158 insertions(+), 6 deletions(-)
> 
> -- 
> 2.36.1
> 

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

[Alan Stern]

On Thu, Aug 04, 2022 at 06:03:05PM +0200, Bastien Nocera wrote:
> There is a need for userspace applications to open USB devices directly,
> for all the USB devices without a kernel-level class driver[1], and
> implemented in user-space.
> 
> As not all devices are built equal, we want to be able to revoke
> access to those devices whether it's because the user isn't at the
> console anymore, or because the web browser, or sandbox doesn't want
> to allow access to that device.
> 
> This commit implements the internal API used to revoke access to USB
> devices, given either bus and device numbers, or/and a user's
> effective UID.
> 
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> 
> [1]:
> Non exhaustive list of devices and device types that need raw USB access:
> - all manners of single-board computers and programmable chips and
> devices (avrdude, STLink, sunxi bootloader, flashrom, etc.)
> - 3D printers
> - scanners
> - LCD "displays"
> - user-space webcam and still cameras
> - game controllers
> - video/audio capture devices
> - sensors
> - software-defined radios
> - DJ/music equipment
> - protocol analysers
> - Rio 500 music player
> ---
>  drivers/usb/core/devio.c | 105 ++++++++++++++++++++++++++++++++++++---
>  drivers/usb/core/usb.h   |   8 +++
>  2 files changed, 107 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index b5b85bf80329..a87fed12e307 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -78,6 +78,7 @@ struct usb_dev_state {
>  	int not_yet_resumed;
>  	bool suspend_allowed;
>  	bool privileges_dropped;
> +	bool revoked;
>  };

Have you considered what should happen if two processes share the same 
file descriptor (and hence the same usb_dev_state structure) and you want 
to revoke access for one of the processes but not the other?

>  struct usb_memory {
> @@ -237,6 +238,13 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
>  	dma_addr_t dma_handle;
>  	int ret;
>  
> +	usb_lock_device(ps->dev);
> +	if (!connected(ps) || ps->revoked) {
> +		usb_unlock_device(ps->dev);
> +		return -ENODEV;
> +	}
> +	usb_unlock_device(ps->dev);

I'm not certain this check is needed at all.  But if you want to add it, 
I don't see any reason for grab the lock.

Also, here and in all the other places, instead of manually adding "|| 
ps_revoked" to all the "!connected(ps)" checks, why not just change the 
definition of connected(ps)?

> +
>  	ret = usbfs_increase_memory_usage(size + sizeof(struct usb_memory));
>  	if (ret)
>  		goto error;
> @@ -298,6 +306,15 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
>  	return ret;
>  }
>  
> +static loff_t usbdev_llseek(struct file *file, loff_t offset, int whence)
> +{
> +	struct usb_dev_state *ps = file->private_data;
> +
> +	if (!connected(ps) || ps->revoked)
> +		return -ENODEV;

Like the case above, this check is not present in the current code so 
it's not clear why it needs to be added now.

> +	return no_seek_end_llseek(file, offset, whence);
> +}
> +
>  static ssize_t usbdev_read(struct file *file, char __user *buf, size_t nbytes,
>  			   loff_t *ppos)
>  {
> @@ -310,7 +327,7 @@ static ssize_t usbdev_read(struct file *file, char __user *buf, size_t nbytes,
>  
>  	pos = *ppos;
>  	usb_lock_device(dev);
> -	if (!connected(ps)) {
> +	if (!connected(ps) || ps->revoked) {
>  		ret = -ENODEV;
>  		goto err;
>  	} else if (pos < 0) {
> @@ -2117,7 +2134,7 @@ static int proc_reapurbnonblock(struct usb_dev_state *ps, void __user *arg)
>  		retval = processcompl(as, (void __user * __user *)arg);
>  		free_async(as);
>  	} else {
> -		retval = (connected(ps) ? -EAGAIN : -ENODEV);
> +		retval = (connected(ps) && !ps->revoked ? -EAGAIN : -ENODEV);
>  	}
>  	return retval;
>  }
> @@ -2262,7 +2279,7 @@ static int proc_reapurbnonblock_compat(struct usb_dev_state *ps, void __user *ar
>  		retval = processcompl_compat(as, (void __user * __user *)arg);
>  		free_async(as);
>  	} else {
> -		retval = (connected(ps) ? -EAGAIN : -ENODEV);
> +		retval = (connected(ps) && !ps->revoked ? -EAGAIN : -ENODEV);
>  	}
>  	return retval;
>  }
> @@ -2580,6 +2597,82 @@ static int proc_wait_for_resume(struct usb_dev_state *ps)
>  	return proc_forbid_suspend(ps);
>  }
>  
> +static int usbdev_revoke(struct usb_dev_state *ps)
> +{
> +	struct usb_device *dev = ps->dev;
> +	unsigned int ifnum;
> +	struct async *as;
> +
> +	if (ps->revoked) {
> +		usb_unlock_device(dev);
> +		return -ENODEV;
> +	}
> +	ps->revoked = true;
> +
> +	for (ifnum = 0; ps->ifclaimed && ifnum < 8*sizeof(ps->ifclaimed);
> +			ifnum++) {
> +		if (test_bit(ifnum, &ps->ifclaimed))
> +			releaseintf(ps, ifnum);
> +	}
> +	destroy_all_async(ps);
> +
> +	as = async_getcompleted(ps);
> +	while (as) {
> +		free_async(as);
> +		as = async_getcompleted(ps);
> +	}
> +
> +	wake_up(&ps->wait);
> +
> +	snoop(&dev->dev, "%s: REVOKE by PID %d: %s\n", __func__,
> +	      task_pid_nr(current), current->comm);

I would put this snoop call before all the other activities, so that any 
debugging output they generate will come after the REVOKE entry in the 
kernel log.

> +
> +	return 0;
> +}
> +
> +int usb_revoke(struct usb_device *udev,
> +		struct usb_revoke_match *match)
> +{
> +	struct usb_dev_state *ps;
> +	int ret = -ENOENT;
> +
> +	usb_lock_device(udev);
> +
> +	if (match->devnum >= 0 &&
> +	    match->busnum >= 0) {
> +		int devnum, busnum;
> +
> +		devnum = udev->devnum;
> +		busnum = udev->bus->busnum;
> +		if (match->busnum != busnum ||
> +		    match->devnum != devnum) {
> +			ret = -ENODEV;
> +			goto out;
> +		}
> +	}

I have the feeling that this part of the function (matching the busnum 
and devnum values) doesn't belong here but rather with the iteration 
routines in your 2/2 patch.  Filtering of devices generally is done as 
part of the iteration.  As an added bonus, doing it that way means you 
don't need to pass around pointers to usb_revoke_match structures.

> +
> +	list_for_each_entry(ps, &udev->filelist, list) {
> +		if (match->euid >= 0) {
> +			kuid_t kuid;
> +
> +			if (!ps || !ps->cred)
> +				continue;
> +			kuid = ps->cred->euid;
> +			if (kuid.val != match->euid)
> +				continue;
> +		}
> +
> +		if (ret == 0)
> +			usbdev_revoke(ps);
> +		else
> +			ret = usbdev_revoke(ps);

You probably don't need this elaborate handling of return codes.  In 
fact, you probably don't need either of these functions to return 
anything.

Alan Stern

> +	}
> +
> +out:
> +	usb_unlock_device(udev);
> +	return ret;
> +}
> +
>  /*
>   * NOTE:  All requests here that have interface numbers as parameters
>   * are assuming that somehow the configuration has been prevented from
> @@ -2623,7 +2716,7 @@ static long usbdev_do_ioctl(struct file *file, unsigned int cmd,
>  #endif
>  	}
>  
> -	if (!connected(ps)) {
> +	if (!connected(ps) || ps->revoked) {
>  		usb_unlock_device(dev);
>  		return -ENODEV;
>  	}
> @@ -2819,7 +2912,7 @@ static __poll_t usbdev_poll(struct file *file,
>  	poll_wait(file, &ps->wait, wait);
>  	if (file->f_mode & FMODE_WRITE && !list_empty(&ps->async_completed))
>  		mask |= EPOLLOUT | EPOLLWRNORM;
> -	if (!connected(ps))
> +	if (!connected(ps) || ps->revoked)
>  		mask |= EPOLLHUP;
>  	if (list_empty(&ps->list))
>  		mask |= EPOLLERR;
> @@ -2828,7 +2921,7 @@ static __poll_t usbdev_poll(struct file *file,
>  
>  const struct file_operations usbdev_file_operations = {
>  	.owner =	  THIS_MODULE,
> -	.llseek =	  no_seek_end_llseek,
> +	.llseek =	  usbdev_llseek,
>  	.read =		  usbdev_read,
>  	.poll =		  usbdev_poll,
>  	.unlocked_ioctl = usbdev_ioctl,
> diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
> index 82538daac8b8..b12c352869f0 100644
> --- a/drivers/usb/core/usb.h
> +++ b/drivers/usb/core/usb.h
> @@ -9,6 +9,11 @@
>  struct usb_hub_descriptor;
>  struct usb_dev_state;
>  
> +struct usb_revoke_match {
> +	int busnum, devnum; /* -1 to match all devices */
> +	int euid; /* -1 to match all users */
> +};
> +
>  /* Functions local to drivers/usb/core/ */
>  
>  extern int usb_create_sysfs_dev_files(struct usb_device *dev);
> @@ -34,6 +39,9 @@ extern int usb_deauthorize_device(struct usb_device *);
>  extern int usb_authorize_device(struct usb_device *);
>  extern void usb_deauthorize_interface(struct usb_interface *);
>  extern void usb_authorize_interface(struct usb_interface *);
> +extern int usb_revoke(struct usb_device *udev,
> +		struct usb_revoke_match *match);
> +extern int usbdev_get_uid(struct usb_dev_state *ps);
>  extern void usb_detect_quirks(struct usb_device *udev);
>  extern void usb_detect_interface_quirks(struct usb_device *udev);
>  extern void usb_release_quirk_list(void);
> -- 
> 2.36.1
> 

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

[Bastien Nocera]

On Thu, 2022-08-04 at 15:25 -0400, Alan Stern wrote:
> On Thu, Aug 04, 2022 at 06:03:05PM +0200, Bastien Nocera wrote:
> > There is a need for userspace applications to open USB devices
> > directly,
> > for all the USB devices without a kernel-level class driver[1], and
> > implemented in user-space.
> > 
> > As not all devices are built equal, we want to be able to revoke
> > access to those devices whether it's because the user isn't at the
> > console anymore, or because the web browser, or sandbox doesn't
> > want
> > to allow access to that device.
> > 
> > This commit implements the internal API used to revoke access to
> > USB
> > devices, given either bus and device numbers, or/and a user's
> > effective UID.
> > 
> > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > 
> > [1]:
> > Non exhaustive list of devices and device types that need raw USB
> > access:
> > - all manners of single-board computers and programmable chips and
> > devices (avrdude, STLink, sunxi bootloader, flashrom, etc.)
> > - 3D printers
> > - scanners
> > - LCD "displays"
> > - user-space webcam and still cameras
> > - game controllers
> > - video/audio capture devices
> > - sensors
> > - software-defined radios
> > - DJ/music equipment
> > - protocol analysers
> > - Rio 500 music player
> > ---
> >  drivers/usb/core/devio.c | 105
> > ++++++++++++++++++++++++++++++++++++---
> >  drivers/usb/core/usb.h   |   8 +++
> >  2 files changed, 107 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> > index b5b85bf80329..a87fed12e307 100644
> > --- a/drivers/usb/core/devio.c
> > +++ b/drivers/usb/core/devio.c
> > @@ -78,6 +78,7 @@ struct usb_dev_state {
> >         int not_yet_resumed;
> >         bool suspend_allowed;
> >         bool privileges_dropped;
> > +       bool revoked;
> >  };
> 
> Have you considered what should happen if two processes share the
> same 
> file descriptor (and hence the same usb_dev_state structure) and you
> want 
> to revoke access for one of the processes but not the other?

No, because this isn't something that happens in practice, as it's
simpler for each programme to open their own file descriptor and claim
the interfaces they need on their own.

> >  struct usb_memory {
> > @@ -237,6 +238,13 @@ static int usbdev_mmap(struct file *file,
> > struct vm_area_struct *vma)
> >         dma_addr_t dma_handle;
> >         int ret;
> >  
> > +       usb_lock_device(ps->dev);
> > +       if (!connected(ps) || ps->revoked) {
> > +               usb_unlock_device(ps->dev);
> > +               return -ENODEV;
> > +       }
> > +       usb_unlock_device(ps->dev);
> 
> I'm not certain this check is needed at all.  But if you want to add
> it, 
> I don't see any reason for grab the lock.

OK, removed.

> Also, here and in all the other places, instead of manually adding
> "|| 
> ps_revoked" to all the "!connected(ps)" checks, why not just change
> the 
> definition of connected(ps)?

I wasn't sure that all the entry points that used connected() would
need to check revocation, but I'm simplified this now.

> 
> > +
> >         ret = usbfs_increase_memory_usage(size + sizeof(struct
> > usb_memory));
> >         if (ret)
> >                 goto error;
> > @@ -298,6 +306,15 @@ static int usbdev_mmap(struct file *file,
> > struct vm_area_struct *vma)
> >         return ret;
> >  }
> >  
> > +static loff_t usbdev_llseek(struct file *file, loff_t offset, int
> > whence)
> > +{
> > +       struct usb_dev_state *ps = file->private_data;
> > +
> > +       if (!connected(ps) || ps->revoked)
> > +               return -ENODEV;
> 
> Like the case above, this check is not present in the current code so
> it's not clear why it needs to be added now.

OK, removed.

> 
> > +       return no_seek_end_llseek(file, offset, whence);
> > +}
> > +
> >  static ssize_t usbdev_read(struct file *file, char __user *buf,
> > size_t nbytes,
> >                            loff_t *ppos)
> >  {
> > @@ -310,7 +327,7 @@ static ssize_t usbdev_read(struct file *file,
> > char __user *buf, size_t nbytes,
> >  
> >         pos = *ppos;
> >         usb_lock_device(dev);
> > -       if (!connected(ps)) {
> > +       if (!connected(ps) || ps->revoked) {
> >                 ret = -ENODEV;
> >                 goto err;
> >         } else if (pos < 0) {
> > @@ -2117,7 +2134,7 @@ static int proc_reapurbnonblock(struct
> > usb_dev_state *ps, void __user *arg)
> >                 retval = processcompl(as, (void __user * __user
> > *)arg);
> >                 free_async(as);
> >         } else {
> > -               retval = (connected(ps) ? -EAGAIN : -ENODEV);
> > +               retval = (connected(ps) && !ps->revoked ? -EAGAIN :
> > -ENODEV);
> >         }
> >         return retval;
> >  }
> > @@ -2262,7 +2279,7 @@ static int proc_reapurbnonblock_compat(struct
> > usb_dev_state *ps, void __user *ar
> >                 retval = processcompl_compat(as, (void __user *
> > __user *)arg);
> >                 free_async(as);
> >         } else {
> > -               retval = (connected(ps) ? -EAGAIN : -ENODEV);
> > +               retval = (connected(ps) && !ps->revoked ? -EAGAIN :
> > -ENODEV);
> >         }
> >         return retval;
> >  }
> > @@ -2580,6 +2597,82 @@ static int proc_wait_for_resume(struct
> > usb_dev_state *ps)
> >         return proc_forbid_suspend(ps);
> >  }
> >  
> > +static int usbdev_revoke(struct usb_dev_state *ps)
> > +{
> > +       struct usb_device *dev = ps->dev;
> > +       unsigned int ifnum;
> > +       struct async *as;
> > +
> > +       if (ps->revoked) {
> > +               usb_unlock_device(dev);
> > +               return -ENODEV;
> > +       }
> > +       ps->revoked = true;
> > +
> > +       for (ifnum = 0; ps->ifclaimed && ifnum < 8*sizeof(ps-
> > >ifclaimed);
> > +                       ifnum++) {
> > +               if (test_bit(ifnum, &ps->ifclaimed))
> > +                       releaseintf(ps, ifnum);
> > +       }
> > +       destroy_all_async(ps);
> > +
> > +       as = async_getcompleted(ps);
> > +       while (as) {
> > +               free_async(as);
> > +               as = async_getcompleted(ps);
> > +       }
> > +
> > +       wake_up(&ps->wait);
> > +
> > +       snoop(&dev->dev, "%s: REVOKE by PID %d: %s\n", __func__,
> > +             task_pid_nr(current), current->comm);
> 
> I would put this snoop call before all the other activities, so that
> any 
> debugging output they generate will come after the REVOKE entry in
> the 
> kernel log.

Done.

> 
> > +
> > +       return 0;
> > +}
> > +
> > +int usb_revoke(struct usb_device *udev,
> > +               struct usb_revoke_match *match)
> > +{
> > +       struct usb_dev_state *ps;
> > +       int ret = -ENOENT;
> > +
> > +       usb_lock_device(udev);
> > +
> > +       if (match->devnum >= 0 &&
> > +           match->busnum >= 0) {
> > +               int devnum, busnum;
> > +
> > +               devnum = udev->devnum;
> > +               busnum = udev->bus->busnum;
> > +               if (match->busnum != busnum ||
> > +                   match->devnum != devnum) {
> > +                       ret = -ENODEV;
> > +                       goto out;
> > +               }
> > +       }
> 
> I have the feeling that this part of the function (matching the
> busnum 
> and devnum values) doesn't belong here but rather with the iteration 
> routines in your 2/2 patch.  Filtering of devices generally is done
> as 
> part of the iteration.  As an added bonus, doing it that way means
> you 
> don't need to pass around pointers to usb_revoke_match structures.

I felt it better to have the filtering done in one place, to avoid
passing just a uid to check to that function.

Should I rename the function something like usb_revoke_for_uid() ?

> 
> > +
> > +       list_for_each_entry(ps, &udev->filelist, list) {
> > +               if (match->euid >= 0) {
> > +                       kuid_t kuid;
> > +
> > +                       if (!ps || !ps->cred)
> > +                               continue;
> > +                       kuid = ps->cred->euid;
> > +                       if (kuid.val != match->euid)
> > +                               continue;
> > +               }
> > +
> > +               if (ret == 0)
> > +                       usbdev_revoke(ps);
> > +               else
> > +                       ret = usbdev_revoke(ps);
> 
> You probably don't need this elaborate handling of return codes.  In 
> fact, you probably don't need either of these functions to return 
> anything.

OK.

> 
> Alan Stern
> 
> > +       }
> > +
> > +out:
> > +       usb_unlock_device(udev);
> > +       return ret;
> > +}
> > +
> >  /*
> >   * NOTE:  All requests here that have interface numbers as
> > parameters
> >   * are assuming that somehow the configuration has been prevented
> > from
> > @@ -2623,7 +2716,7 @@ static long usbdev_do_ioctl(struct file
> > *file, unsigned int cmd,
> >  #endif
> >         }
> >  
> > -       if (!connected(ps)) {
> > +       if (!connected(ps) || ps->revoked) {
> >                 usb_unlock_device(dev);
> >                 return -ENODEV;
> >         }
> > @@ -2819,7 +2912,7 @@ static __poll_t usbdev_poll(struct file
> > *file,
> >         poll_wait(file, &ps->wait, wait);
> >         if (file->f_mode & FMODE_WRITE && !list_empty(&ps-
> > >async_completed))
> >                 mask |= EPOLLOUT | EPOLLWRNORM;
> > -       if (!connected(ps))
> > +       if (!connected(ps) || ps->revoked)
> >                 mask |= EPOLLHUP;
> >         if (list_empty(&ps->list))
> >                 mask |= EPOLLERR;
> > @@ -2828,7 +2921,7 @@ static __poll_t usbdev_poll(struct file
> > *file,
> >  
> >  const struct file_operations usbdev_file_operations = {
> >         .owner =          THIS_MODULE,
> > -       .llseek =         no_seek_end_llseek,
> > +       .llseek =         usbdev_llseek,
> >         .read =           usbdev_read,
> >         .poll =           usbdev_poll,
> >         .unlocked_ioctl = usbdev_ioctl,
> > diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
> > index 82538daac8b8..b12c352869f0 100644
> > --- a/drivers/usb/core/usb.h
> > +++ b/drivers/usb/core/usb.h
> > @@ -9,6 +9,11 @@
> >  struct usb_hub_descriptor;
> >  struct usb_dev_state;
> >  
> > +struct usb_revoke_match {
> > +       int busnum, devnum; /* -1 to match all devices */
> > +       int euid; /* -1 to match all users */
> > +};
> > +
> >  /* Functions local to drivers/usb/core/ */
> >  
> >  extern int usb_create_sysfs_dev_files(struct usb_device *dev);
> > @@ -34,6 +39,9 @@ extern int usb_deauthorize_device(struct
> > usb_device *);
> >  extern int usb_authorize_device(struct usb_device *);
> >  extern void usb_deauthorize_interface(struct usb_interface *);
> >  extern void usb_authorize_interface(struct usb_interface *);
> > +extern int usb_revoke(struct usb_device *udev,
> > +               struct usb_revoke_match *match);
> > +extern int usbdev_get_uid(struct usb_dev_state *ps);
> >  extern void usb_detect_quirks(struct usb_device *udev);
> >  extern void usb_detect_interface_quirks(struct usb_device *udev);
> >  extern void usb_release_quirk_list(void);
> > -- 
> > 2.36.1
> > 

Re: [RFC v2] USB: core: add a way to revoke access to open USB devices

[Bastien Nocera]

On Thu, 2022-08-04 at 15:12 -0400, Alan Stern wrote:
> On Thu, Aug 04, 2022 at 06:03:04PM +0200, Bastien Nocera wrote:
> > Hey,
> > 
> > This is a follow-up to "[RFC v1] USB: core: add USBDEVFS_REVOKE
> > ioctl" from April.
> > 
> > The end-goal is being able to cut-off access to USB devices for
> > applications/programs that open raw USB devices using, in most
> > cases,
> > libusb.
> > 
> > I've implemented this using BPF, so we don't need to add new
> > ioctls.
> 
> I'm not sure that's a good reason for using BPF.
> 
> > The presence or absence of that feature can be evaluated at
> > runtime,
> > and can be used to implement revoke support on session switching,
> > in logind for example:
> > https://github.com/hadess/usb-revoke-userspace/
> 
> This isn't clear.  Are you talking about having the kernel
> automatically 
> do this when certain conditions are met?  Or do you mean that logind 
> could invoke the BPF program at the appropriate times?

logind would be the one loading and executing the BPF program. I've
slightly reworded the first paragraph in the 2nd patch, but it's pretty
clear there that the functionality is implemented in user-space, and
the BPF acts as a bridge.

> Left unsaid here is that logind generally won't have a file
> descriptor 
> for any of the open USB devices that it wants to revoke access to.  
> That's a good reason to use BPF instead of ioctl.

That's right. I've make a note of this in the BPF patch.

> If you're going to revoke access to devices upon session switching, 
> shouldn't the mechanism be more general?  I mean, shouldn't it apply
> to 
> all sorts of devices, not just those that happen to be USB?

I don't see how that could be done in a generic way without causing
problems for devices accessed by multiple different parts of the OS.

Revoking doesn't invalidate the file descriptor (whether in the
existing evdev revoke ioctl, or this USB function), and as you can see
in the patch, there are still things that can be done on the device
even when revoked.

> Also, if you're going to use session switching as your criterion for 
> revoking access to USB devices then what will you do to restore
> access 
> when the session switches back?

It's up to the application to do that. This is, for example, what
compositors do when accessing input devices which already have their
own revoke ioctl.

> > I have some notes and questions on the API as it is exposed.
> > 
> > - I didn't see a point in having multiple kernel functions as entry
> >   points as I was going to use a single BPF program as an entry
> > point,
> >   which can check the arguments. Can I rely on the BPF program
> > checking
> >   those arguments, or should I re-check them again in the kernel
> >   implementation?
> 
> What is there to check?

Whether either busnum and devnum were passed, or a valid UID.

> > - Are my UID checks correct if I expect revoking to be called
> > outside
> >   namespaces, on the effective UID of the user. Is there a way to
> > assert
> >   that?
> 
> We're probably not the right people to ask.  You could try Eric 
> Biederman.
> 

I'll CC: Eric on the next patch then.

> > - Is there a good "errno" error for ENOSUCHUSER for using in the
> >   usb_revoke() loop if we couldn't find any USB device matching the
> >   requested user?
> 
> Why do you want that to be an error?  If you tell the kernel "Remove
> all 
> access to USB devices for user X", why should it be an error if X
> doesn't 
> have any open file descriptors for USB devices?

I imagined that it would be useful to know whether any devices were
found, or any users matched, but I guess this isn't strictly necessary.
I'll remove it.

> 
> For that matter, are you certain that basing this on the UID is the
> right 
> way to go?  What if there are two different login sessions both using
> the 
> same UID?

Do you know another identifier that's not kernel internal, or relying
on logind knowing the fd that we could use to differentiate those 2
users?

Is there a use case for a USB device being opened, two separate
interfaces being claimed, by two different user sessions of the same
user?

> 
> Alan Stern
> 
> > 
> > Cheers
> > 
> > Bastien Nocera (2):
> >   USB: core: add a way to revoke access to open USB devices
> >   usb: Implement usb_revoke() BPF function
> > 
> >  drivers/usb/core/devio.c | 105
> > ++++++++++++++++++++++++++++++++++++---
> >  drivers/usb/core/usb.c   |  51 +++++++++++++++++++
> >  drivers/usb/core/usb.h   |   8 +++
> >  3 files changed, 158 insertions(+), 6 deletions(-)
> > 
> > -- 
> > 2.36.1
> > 

Re: [RFC v2] USB: core: add a way to revoke access to open USB devices

[Alan Stern]

On Fri, Aug 05, 2022 at 02:38:16PM +0200, Bastien Nocera wrote:
> On Thu, 2022-08-04 at 15:12 -0400, Alan Stern wrote:
> > If you're going to revoke access to devices upon session switching, 
> > shouldn't the mechanism be more general?  I mean, shouldn't it apply
> > to 
> > all sorts of devices, not just those that happen to be USB?
> 
> I don't see how that could be done in a generic way without causing
> problems for devices accessed by multiple different parts of the OS.

I don't know enough about the issues involved to help much.  Still, 
doesn't it make more sense to offer logind a single API for revoking a 
session's access to all appropriate devices, rather than one API for 
revoking access to USB devices, another API for revoking access to HID 
devices, another API for revoking access to serial devices, another API 
for... etc.?

This sounds a lot like the old BSD concept of "foreground" and 
"background" ttys.  It didn't rely on revoking access to anything; maybe 
you should try to follow that example instead.

Why would multiple different parts of the OS cause problems?

> Revoking doesn't invalidate the file descriptor (whether in the
> existing evdev revoke ioctl, or this USB function), and as you can see
> in the patch, there are still things that can be done on the device
> even when revoked.

I didn't notice anything that can still be done.  You might be able, for 
instance, to do an lseek on the device file descriptor, but that doesn't 
affect the device.

> > Also, if you're going to use session switching as your criterion for 
> > revoking access to USB devices then what will you do to restore
> > access 
> > when the session switches back?
> 
> It's up to the application to do that. This is, for example, what
> compositors do when accessing input devices which already have their
> own revoke ioctl.

Okay, I'll take your word for it.  I'm not sure it's a good idea.

> > > I have some notes and questions on the API as it is exposed.
> > > 
> > > - I didn't see a point in having multiple kernel functions as entry
> > >   points as I was going to use a single BPF program as an entry
> > > point,
> > >   which can check the arguments. Can I rely on the BPF program
> > > checking
> > >   those arguments, or should I re-check them again in the kernel
> > >   implementation?
> > 
> > What is there to check?
> 
> Whether either busnum and devnum were passed, or a valid UID.

Does it really matter if none of them are valid values?  The call will 
end up doing nothing, but that's not really an error.

For that matter, how do you define whether a UID is valid?  As far as I 
know, the kernel doesn't keep track of valid UIDs; userspace does (in the 
/etc/passwd file or equivalent).

> > For that matter, are you certain that basing this on the UID is the
> > right 
> > way to go?  What if there are two different login sessions both using
> > the 
> > same UID?
> 
> Do you know another identifier that's not kernel internal, or relying
> on logind knowing the fd that we could use to differentiate those 2
> users?

Process or session ID, perhaps?  But I don't have a clear idea of how all 
this is meant to fit together.

> Is there a use case for a USB device being opened, two separate
> interfaces being claimed, by two different user sessions of the same
> user?

Well, you can ssh into the same machine from two different windows at the 
same time, and each of those ssh sessions could claim a different 
interface in the same USB device.  (In theory, that is; but maybe you 
don't want to allow ssh sessions to access USB devices at all.)

Alan Stern

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

[Alan Stern]

On Fri, Aug 05, 2022 at 02:38:13PM +0200, Bastien Nocera wrote:
> On Thu, 2022-08-04 at 15:25 -0400, Alan Stern wrote:
> > Have you considered what should happen if two processes share the
> > same 
> > file descriptor (and hence the same usb_dev_state structure) and you
> > want 
> > to revoke access for one of the processes but not the other?
> 
> No, because this isn't something that happens in practice, as it's
> simpler for each programme to open their own file descriptor and claim
> the interfaces they need on their own.

But it is possible for a program to open a USB device and then fork 
several children.  The children would all share the same file descriptor.

I have no idea how often this happens in practice.  But kernel design is 
supposed to be based on correctness, not on handling only things that 
crop up "in practice".

> > I have the feeling that this part of the function (matching the
> > busnum 
> > and devnum values) doesn't belong here but rather with the iteration 
> > routines in your 2/2 patch.  Filtering of devices generally is done
> > as 
> > part of the iteration.  As an added bonus, doing it that way means
> > you 
> > don't need to pass around pointers to usb_revoke_match structures.
> 
> I felt it better to have the filtering done in one place, to avoid
> passing just a uid to check to that function.

There's nothing wrong with passing just a uid.  Especially since the same 
device might be open multiple times, for varying uids.

> Should I rename the function something like usb_revoke_for_uid() ?

Up to you.

Alan Stern

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

[Bastien Nocera]

On Fri, 2022-08-05 at 10:42 -0400, Alan Stern wrote:
> > On Fri, Aug 05, 2022 at 02:38:13PM +0200, Bastien Nocera wrote:
> > > > On Thu, 2022-08-04 at 15:25 -0400, Alan Stern wrote:
> > > > > > Have you considered what should happen if two processes
> > > > > > share the
> > > > > > same 
> > > > > > file descriptor (and hence the same usb_dev_state
> > > > > > structure) and > > > you
> > > > > > want 
> > > > > > to revoke access for one of the processes but not the
> > > > > > other?
> > > > 
> > > > No, because this isn't something that happens in practice, as
> > > > it's
> > > > simpler for each programme to open their own file descriptor
> > > > and > > claim
> > > > the interfaces they need on their own.
> > 
> > But it is possible for a program to open a USB device and then fork
> > several children.  The children would all share the same file >
> > descriptor.
> > 
> > I have no idea how often this happens in practice.  But kernel
> > design > is 
> > supposed to be based on correctness, not on handling only things
> > that
> > crop up "in practice".

Given that the API makes no such promises, I think we're fine. The end
goal is "user can't use device" or possibly "user in namespace can't
use device" to cut sandboxes off from their USB devices.

We don't need surgical precision revocation, but I would be happy to
implement this API if we can come up with a good way to target specific
programs.

> > > > > > I have the feeling that this part of the function (matching
> > > > > > the
> > > > > > busnum 
> > > > > > and devnum values) doesn't belong here but rather with the
> > > > > > > > > iteration 
> > > > > > routines in your 2/2 patch.  Filtering of devices generally
> > > > > > is > > > done
> > > > > > as 
> > > > > > part of the iteration.  As an added bonus, doing it that
> > > > > > way > > > means
> > > > > > you 
> > > > > > don't need to pass around pointers to usb_revoke_match > >
> > > > > > > structures.
> > > > 
> > > > I felt it better to have the filtering done in one place, to
> > > > avoid
> > > > passing just a uid to check to that function.
> > 
> > There's nothing wrong with passing just a uid.  Especially since
> > the > same 
> > device might be open multiple times, for varying uids.
> > 
> > > > Should I rename the function something like
> > > > usb_revoke_for_uid() ?
> > 
> > Up to you.

I've done that now.

Re: [RFC v2] USB: core: add a way to revoke access to open USB devices

[Bastien Nocera]

On Fri, 2022-08-05 at 10:31 -0400, Alan Stern wrote:
> > On Fri, Aug 05, 2022 at 02:38:16PM +0200, Bastien Nocera wrote:
> > > > On Thu, 2022-08-04 at 15:12 -0400, Alan Stern wrote:
> > > > > > If you're going to revoke access to devices upon session >
> > > > > > > > switching, 
> > > > > > shouldn't the mechanism be more general?  I mean, shouldn't
> > > > > > it > > > apply
> > > > > > to 
> > > > > > all sorts of devices, not just those that happen to be USB?
> > > > 
> > > > I don't see how that could be done in a generic way without
> > > > causing
> > > > problems for devices accessed by multiple different parts of
> > > > the > > OS.
> > 
> > I don't know enough about the issues involved to help much.  Still,
> > doesn't it make more sense to offer logind a single API for
> > revoking > a 
> > session's access to all appropriate devices, rather than one API
> > for 
> > revoking access to USB devices, another API for revoking access to
> > > HID 
> > devices, another API for revoking access to serial devices, another
> > > API 
> > for... etc.?
> > 
> > This sounds a lot like the old BSD concept of "foreground" and 
> > "background" ttys.  It didn't rely on revoking access to anything;
> > > maybe 
> > you should try to follow that example instead.

Would be happy to read up about the BSD concept of background and
foreground ttys, although I doubt that they dealt with the same sort of
devices we're dealing with here, but I couldn't find any useful
information about it (loads of info on changing the tty's background
colour or putting processes in the background using bash...).

About having a single revoking API, others have already mentioned
revoking "all types of devices" when this was first brought up in
April. There's a couple of problems though.

For USB at least, we already have a long-tail of existing applications
and APIs, and we don't have file descriptors to manipulate as we don't
have a proxy/broker for those apps. So a file descriptor solution like
revoke() wouldn't work for our use.

The other problem is that because we don't have a file descriptor, we
need another way to identify devices, and that can only be
hardware/bus/type specific.

Finally we don't want or need to revoke access to *all* the devices, or
even to all the real devices, when fast-user switching. There will be
cases where a particular device can used through 2 different interfaces
where it will be useful for the user to hang on to. This sounds like a
policy decision which is best taken in user-space.
> 

> > > > Revoking doesn't invalidate the file descriptor (whether in the
> > > > existing evdev revoke ioctl, or this USB function), and as you
> > > > can > > see
> > > > in the patch, there are still things that can be done on the
> > > > device
> > > > even when revoked.
> > 
> > I didn't notice anything that can still be done.  You might be
> > able, > for 
> > instance, to do an lseek on the device file descriptor, but that >
> > doesn't 
> > affect the device.

You can still reap the in-flight URBs, and the device is inert, not
closed.

If the app doesn't know anything, then USB traffic just stopped, and
any other contact will fail. If the app does know, it knows that it
doesn't have to let go of the file descriptor, and when the user's
session is back active and presumably the user gains back access to the
device node, it can reopen the fd.

> > 
> > > > > > Also, if you're going to use session switching as your
> > > > > > criterion > > > for 
> > > > > > revoking access to USB devices then what will you do to
> > > > > > restore
> > > > > > access 
> > > > > > when the session switches back?
> > > > 
> > > > It's up to the application to do that. This is, for example,
> > > > what
> > > > compositors do when accessing input devices which already have
> > > > > > their
> > > > own revoke ioctl.
> > 
> > Okay, I'll take your word for it.  I'm not sure it's a good idea.

The process goes something like:
- change permissions on the device node so user A can't access it
- revoke access so that the device is effectively "muted"
- change permissions on the device node so user B can access it

It's already what exists for input devices.

> > 
> > > > > > > > I have some notes and questions on the API as it is
> > > > > > > > exposed.
> > > > > > > > 
> > > > > > > > - I didn't see a point in having multiple kernel
> > > > > > > > functions as > > > > entry
> > > > > > > >   points as I was going to use a single BPF program as
> > > > > > > > an entry
> > > > > > > > point,
> > > > > > > >   which can check the arguments. Can I rely on the BPF
> > > > > > > > program
> > > > > > > > checking
> > > > > > > >   those arguments, or should I re-check them again in
> > > > > > > > the > > > > kernel
> > > > > > > >   implementation?
> > > > > > 
> > > > > > What is there to check?
> > > > 
> > > > Whether either busnum and devnum were passed, or a valid UID.
> > 
> > Does it really matter if none of them are valid values?  The call >
> > will 
> > end up doing nothing, but that's not really an error.
> > 
> > For that matter, how do you define whether a UID is valid?  As far
> > as > I 
> > know, the kernel doesn't keep track of valid UIDs; userspace does
> > (in > the 
> > /etc/passwd file or equivalent).

I meant valid as in "being a positive integer".

> > 
> > > > > > For that matter, are you certain that basing this on the
> > > > > > UID is > > > the
> > > > > > right 
> > > > > > way to go?  What if there are two different login sessions
> > > > > > both > > > using
> > > > > > the 
> > > > > > same UID?
> > > > 
> > > > Do you know another identifier that's not kernel internal, or >
> > > > > relying
> > > > on logind knowing the fd that we could use to differentiate
> > > > those 2
> > > > users?
> > 
> > Process or session ID, perhaps?  But I don't have a clear idea of
> > how > all 
> > this is meant to fit together.
> > 
> > > > Is there a use case for a USB device being opened, two separate
> > > > interfaces being claimed, by two different user sessions of the
> > > > > > same
> > > > user?
> > 
> > Well, you can ssh into the same machine from two different windows
> > at > the 
> > same time, and each of those ssh sessions could claim a different 
> > interface in the same USB device.  (In theory, that is; but maybe
> > you
> > don't want to allow ssh sessions to access USB devices at all.)

It's a user-space policy whether to do that or not. I don't believe
that any permissions get changed/device access would get revoked until
the user's last session is closed.

Cheers

Re: [RFC v2] USB: core: add a way to revoke access to open USB devices

[Bastien Nocera]

On Tue, 2022-08-09 at 11:10 +0200, Bastien Nocera wrote:
> The process goes something like:
> - change permissions on the device node so user A can't access it
> - revoke access so that the device is effectively "muted"
> - change permissions on the device node so user B can access it
> 
> It's already what exists for input devices.

Correction, it does:
- change ACL on device from user A to user B:
https://github.com/systemd/systemd/blob/main/src/shared/devnode-acl.c#L139
- revoke access for user A:
https://github.com/systemd/systemd/blob/main/src/login/logind-session-device.c#L116