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

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

[Bastien Nocera]

BPF list, first CC: here, I hope the commit messages are clear enough to
understand the purpose of the patchset. If not, your comments would be
greatly appreciated so I can make the commit messages self-explanatory.

Eric, what would be the right identifier to use for a specific user
namespace that userspace could find out? I know the PIDs of the
bubblewrap processes that created those user namespaces, would those be
good enough?

Changes since v2:
- Changed the internal API to pass a struct usb_device
- Fixed potential busy loop in user-space when revoking access to a
  device

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 | 79 ++++++++++++++++++++++++++++++++++++++--
 drivers/usb/core/usb.c   | 51 ++++++++++++++++++++++++++
 drivers/usb/core/usb.h   |  2 +
 3 files changed, 128 insertions(+), 4 deletions(-)

-- 
2.37.1

[PATCH 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 | 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;
+			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);
-- 
2.37.1

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

[Bastien Nocera]

This functionality allows a sufficiently privileged user-space process
to upload a BPF programme 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.

logind, and other session management software, does not have access to
the file descriptor used by the application so other identifiers
are used.

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..ca394848a51e 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 usb_revoke_match {
+	int busnum, devnum; /* -1 to match all devices */
+	int euid; /* -1 to match all users */
+};
+
+static int
+__usb_revoke(struct usb_device *udev, void *data)
+{
+	struct usb_revoke_match *match = data;
+
+	if (match->devnum >= 0 && match->busnum >= 0) {
+		if (match->busnum != udev->bus->busnum ||
+		    match->devnum != udev->devnum) {
+			return 0;
+		}
+	}
+
+	usb_revoke_for_euid(udev, match->euid);
+	return 0;
+}
+
+noinline int
+usb_revoke_device(int busnum, int devnum, unsigned int euid)
+{
+	struct usb_revoke_match match;
+
+	match.busnum = busnum;
+	match.devnum = devnum;
+	match.euid = euid;
+
+	usb_for_each_dev(&match, __usb_revoke);
+
+	return 0;
+}
+
 #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.37.1

Re: [PATCH 0/2] USB: core: add a way to revoke access to open USB devices

[Greg Kroah-Hartman]

On Tue, Aug 09, 2022 at 11:42:58AM +0200, Bastien Nocera wrote:
> BPF list, first CC: here, I hope the commit messages are clear enough to
> understand the purpose of the patchset. If not, your comments would be
> greatly appreciated so I can make the commit messages self-explanatory.
> 
> Eric, what would be the right identifier to use for a specific user
> namespace that userspace could find out? I know the PIDs of the
> bubblewrap processes that created those user namespaces, would those be
> good enough?
> 
> Changes since v2:
> - Changed the internal API to pass a struct usb_device
> - Fixed potential busy loop in user-space when revoking access to a
>   device
> 
> 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 | 79 ++++++++++++++++++++++++++++++++++++++--
>  drivers/usb/core/usb.c   | 51 ++++++++++++++++++++++++++
>  drivers/usb/core/usb.h   |  2 +
>  3 files changed, 128 insertions(+), 4 deletions(-)
> 
> -- 
> 2.37.1
> 

You say "changes since v2", but have no version identifier on this
series at all :(

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

[Greg Kroah-Hartman]

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 :(

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

[Greg Kroah-Hartman]

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

Re: [PATCH 2/2] usb: Implement usb_revoke() BPF function

[Greg Kroah-Hartman]

On Tue, Aug 09, 2022 at 11:43:00AM +0200, Bastien Nocera wrote:
> This functionality allows a sufficiently privileged user-space process
> to upload a BPF programme 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.
> 
> logind, and other session management software, does not have access to
> the file descriptor used by the application so other identifiers
> are used.
> 
> 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..ca394848a51e 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 usb_revoke_match {
> +	int busnum, devnum; /* -1 to match all devices */
> +	int euid; /* -1 to match all users */
> +};
> +
> +static int
> +__usb_revoke(struct usb_device *udev, void *data)
> +{
> +	struct usb_revoke_match *match = data;
> +
> +	if (match->devnum >= 0 && match->busnum >= 0) {
> +		if (match->busnum != udev->bus->busnum ||
> +		    match->devnum != udev->devnum) {
> +			return 0;
> +		}
> +	}
> +
> +	usb_revoke_for_euid(udev, match->euid);

How are you not racing with other devices being added and removed at the
same time?

Again, please stick with the file descriptor, that's the unique thing
you know you have that you want to revoke.

Now if you really really want to disable a device from under a user,
without the file handle present, you can do that today, as root, by
doing the 'unbind' hack through userspace and sysfs.  It's so common
that this seems to be how virtual device managers handle virtual
machines, so it should be well tested by now.

or does usbfs not bind to the device it opens?

thanks,

greg k-h

Re: [PATCH 0/2] USB: core: add a way to revoke access to open USB devices

[Bastien Nocera]

On Tue, 2022-08-09 at 12:31 +0200, Greg Kroah-Hartman wrote:
> On Tue, Aug 09, 2022 at 11:42:58AM +0200, Bastien Nocera wrote:
> > BPF list, first CC: here, I hope the commit messages are clear
> > enough to
> > understand the purpose of the patchset. If not, your comments would
> > be
> > greatly appreciated so I can make the commit messages self-
> > explanatory.
> > 
> > Eric, what would be the right identifier to use for a specific user
> > namespace that userspace could find out? I know the PIDs of the
> > bubblewrap processes that created those user namespaces, would
> > those be
> > good enough?
> > 
> > Changes since v2:
> > - Changed the internal API to pass a struct usb_device
> > - Fixed potential busy loop in user-space when revoking access to a
> >   device
> > 
> > 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 | 79
> > ++++++++++++++++++++++++++++++++++++++--
> >  drivers/usb/core/usb.c   | 51 ++++++++++++++++++++++++++
> >  drivers/usb/core/usb.h   |  2 +
> >  3 files changed, 128 insertions(+), 4 deletions(-)
> > 
> > -- 
> > 2.37.1
> > 
> 
> You say "changes since v2", but have no version identifier on this
> series at all :(

It was sent as "RFC v2" under the same name. This is v3.

Sorry, but this will probably keep happening until the tools folks have
to use for kernel development aren't as clunky as they are now...

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

[Bastien Nocera]

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.

Re: [PATCH 2/2] usb: Implement usb_revoke() BPF function

[Bastien Nocera]

On Tue, 2022-08-09 at 12:38 +0200, Greg Kroah-Hartman wrote:
> On Tue, Aug 09, 2022 at 11:43:00AM +0200, Bastien Nocera wrote:
> > This functionality allows a sufficiently privileged user-space
> > process
> > to upload a BPF programme 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.
> > 
> > logind, and other session management software, does not have access
> > to
> > the file descriptor used by the application so other identifiers
> > are used.
> > 
> > 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..ca394848a51e 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 usb_revoke_match {
> > +       int busnum, devnum; /* -1 to match all devices */
> > +       int euid; /* -1 to match all users */
> > +};
> > +
> > +static int
> > +__usb_revoke(struct usb_device *udev, void *data)
> > +{
> > +       struct usb_revoke_match *match = data;
> > +
> > +       if (match->devnum >= 0 && match->busnum >= 0) {
> > +               if (match->busnum != udev->bus->busnum ||
> > +                   match->devnum != udev->devnum) {
> > +                       return 0;
> > +               }
> > +       }
> > +
> > +       usb_revoke_for_euid(udev, match->euid);
> 
> How are you not racing with other devices being added and removed at
> the
> same time?
> 
> Again, please stick with the file descriptor, that's the unique thing
> you know you have that you want to revoke.
> 
> Now if you really really want to disable a device from under a user,
> without the file handle present, you can do that today, as root, by
> doing the 'unbind' hack through userspace and sysfs.  It's so common
> that this seems to be how virtual device managers handle virtual
> machines, so it should be well tested by now.
> 
> or does usbfs not bind to the device it opens?

And how is this not racy, and clunky and slow?

It would lose all the PM setup the drive might have done, reset and
reprobe the device, and forcibly close the file descriptor the
programme had opened.

This revocation just "mutes" (with no possibility to unmute) the file
descriptor the programme has, so it can't talk to physical device
anymore.

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

[Bastien Nocera]

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.

Re: [PATCH 0/2] USB: core: add a way to revoke access to open USB devices

[Greg Kroah-Hartman]

On Tue, Aug 09, 2022 at 01:15:38PM +0200, Bastien Nocera wrote:
> Sorry, but this will probably keep happening until the tools folks have
> to use for kernel development aren't as clunky as they are now...

It's an editor, you can get them to do anything you want to do, they are
not clunky...

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

[Greg Kroah-Hartman]

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

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

[Bastien Nocera]

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.

Re: [PATCH 2/2] usb: Implement usb_revoke() BPF function

[Greg Kroah-Hartman]

On Tue, Aug 09, 2022 at 01:18:27PM +0200, Bastien Nocera wrote:
> On Tue, 2022-08-09 at 12:38 +0200, Greg Kroah-Hartman wrote:
> > On Tue, Aug 09, 2022 at 11:43:00AM +0200, Bastien Nocera wrote:
> > > This functionality allows a sufficiently privileged user-space
> > > process
> > > to upload a BPF programme 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.
> > > 
> > > logind, and other session management software, does not have access
> > > to
> > > the file descriptor used by the application so other identifiers
> > > are used.
> > > 
> > > 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..ca394848a51e 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 usb_revoke_match {
> > > +       int busnum, devnum; /* -1 to match all devices */
> > > +       int euid; /* -1 to match all users */
> > > +};
> > > +
> > > +static int
> > > +__usb_revoke(struct usb_device *udev, void *data)
> > > +{
> > > +       struct usb_revoke_match *match = data;
> > > +
> > > +       if (match->devnum >= 0 && match->busnum >= 0) {
> > > +               if (match->busnum != udev->bus->busnum ||
> > > +                   match->devnum != udev->devnum) {
> > > +                       return 0;
> > > +               }
> > > +       }
> > > +
> > > +       usb_revoke_for_euid(udev, match->euid);
> > 
> > How are you not racing with other devices being added and removed at
> > the
> > same time?
> > 
> > Again, please stick with the file descriptor, that's the unique thing
> > you know you have that you want to revoke.
> > 
> > Now if you really really want to disable a device from under a user,
> > without the file handle present, you can do that today, as root, by
> > doing the 'unbind' hack through userspace and sysfs.  It's so common
> > that this seems to be how virtual device managers handle virtual
> > machines, so it should be well tested by now.
> > 
> > or does usbfs not bind to the device it opens?
> 
> And how is this not racy, and clunky and slow?

Is speed an issue here?

And how is that any less racy than your proposal?

And yes, clunky, but well defined and has been around for well over a
decade.

> It would lose all the PM setup the drive might have done, reset and
> reprobe the device, and forcibly close the file descriptor the
> programme had opened.

It will not reset the device, try it and see!

And forcibly closing the file descriptor is the goal here I thought.  If
not, I have no idea what this is for at all, sorry.

greg k-h

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

[Greg Kroah-Hartman]

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

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

[Bastien Nocera]

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.

Re: [PATCH 2/2] usb: Implement usb_revoke() BPF function

[Bastien Nocera]

On Tue, 2022-08-09 at 14:49 +0200, Greg Kroah-Hartman wrote:

<snip>
> And forcibly closing the file descriptor is the goal here I thought. 
> If
> not, I have no idea what this is for at all, sorry.

See c7dc65737c9a607d3e6f8478659876074ad129b8

It was mentioned in the first email I sent about this feature I wanted
to work on:
https://www.spinics.net/lists/linux-usb/msg225448.html

Which I did mention on the v2 I sent a couple of days ago:
https://www.spinics.net/lists/linux-usb/msg229753.html

Re: [PATCH 2/2] usb: Implement usb_revoke() BPF function

[Bastien Nocera]

On Tue, 2022-08-09 at 12:38 +0200, Greg Kroah-Hartman wrote:
> Now if you really really want to disable a device from under a user,
> without the file handle present, you can do that today, as root, by
> doing the 'unbind' hack through userspace and sysfs.  It's so common
> that this seems to be how virtual device managers handle virtual
> machines, so it should be well tested by now.

The only thing I know that works that way is usbip, and it requires
unbinding each of the interfaces:

https://sourceforge.net/p/usbip/git-windows/ci/master/tree/trunk/userspace/src/bind-driver.c#l157

That means that, for example, revoking access to the raw USB device
that OpenRGB used to blink colours across a keyboard would disconnect
the keyboard from the HID device.

Can you show me any other users of that "trick" that would keep the
"hid" keyboard driver working while access to the /dev/bus/usb/* device
node is revoked/closed/yanked/unbound?

And if you can't, I would appreciate some efforts being made trying to
understand the use case, along with the limitations we're working
against, so we can find a good solution to the problem, instead of
retreading discussion points.

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

[Greg Kroah-Hartman]

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

Re: [PATCH 2/2] usb: Implement usb_revoke() BPF function

[Greg Kroah-Hartman]

On Tue, Aug 09, 2022 at 04:31:04PM +0200, Bastien Nocera wrote:
> On Tue, 2022-08-09 at 12:38 +0200, Greg Kroah-Hartman wrote:
> > Now if you really really want to disable a device from under a user,
> > without the file handle present, you can do that today, as root, by
> > doing the 'unbind' hack through userspace and sysfs.  It's so common
> > that this seems to be how virtual device managers handle virtual
> > machines, so it should be well tested by now.
> 
> The only thing I know that works that way is usbip, and it requires
> unbinding each of the interfaces:
> 
> https://sourceforge.net/p/usbip/git-windows/ci/master/tree/trunk/userspace/src/bind-driver.c#l157

virtio devices also use the api from what I recall.

> That means that, for example, revoking access to the raw USB device
> that OpenRGB used to blink colours across a keyboard would disconnect
> the keyboard from the HID device.

No, you unbind the usbfs driver, not the hid driver.

> Can you show me any other users of that "trick" that would keep the
> "hid" keyboard driver working while access to the /dev/bus/usb/* device
> node is revoked/closed/yanked/unbound?

Try unbinding usbfs from the device instead.

> And if you can't, I would appreciate some efforts being made trying to
> understand the use case, along with the limitations we're working
> against, so we can find a good solution to the problem, instead of
> retreading discussion points.

As you have not documented the use case well enough in these changelog
entries for me to understand it, the fact that I brought up things you
previously discussed seems to mean you didn't document it well enough
here for it not to come up again :)

thanks,

greg k-h

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

[Eric W. Biederman]

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

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

[Bastien Nocera]

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.

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

[Bastien Nocera]

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.

Re: [PATCH 2/2] usb: Implement usb_revoke() BPF function

[Eric W. Biederman]

Bastien Nocera <hadess@hadess.net> writes:

> This functionality allows a sufficiently privileged user-space process
> to upload a BPF programme 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.
>
> logind, and other session management software, does not have access to
> the file descriptor used by the application so other identifiers
> are used.

Revoking access to hardware devices when switching users is entirely
reasonable.

I really think a system call or possibly and ioctl would be more
appropriate than having to load a bpf program to perform the work of a
system call.

Making it so that logind can't run in any kind of container seems like a
very unfortunate design choice.

The tty subsystem has something like this with the vhangup system
call, and the bsd have revoke(2).  So there is definitely precedent for
doing something like this.

I suspect what you want to pass is an O_PATH file descriptor to the
appropriate device node.   That should allow races to be eliminated,
or at the very least seriously reduced.

Eric



>
> 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..ca394848a51e 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 usb_revoke_match {
> +	int busnum, devnum; /* -1 to match all devices */
> +	int euid; /* -1 to match all users */
> +};
> +
> +static int
> +__usb_revoke(struct usb_device *udev, void *data)
> +{
> +	struct usb_revoke_match *match = data;
> +
> +	if (match->devnum >= 0 && match->busnum >= 0) {
> +		if (match->busnum != udev->bus->busnum ||
> +		    match->devnum != udev->devnum) {
> +			return 0;
> +		}
> +	}
> +
> +	usb_revoke_for_euid(udev, match->euid);
> +	return 0;
> +}
> +
> +noinline int
> +usb_revoke_device(int busnum, int devnum, unsigned int euid)
> +{
> +	struct usb_revoke_match match;
> +
> +	match.busnum = busnum;
> +	match.devnum = devnum;
> +	match.euid = euid;
> +
> +	usb_for_each_dev(&match, __usb_revoke);
> +
> +	return 0;
> +}
> +
>  #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();

Re: [PATCH 0/2] USB: core: add a way to revoke access to open USB devices

[Eric W. Biederman]

Bastien Nocera <hadess@hadess.net> writes:

> BPF list, first CC: here, I hope the commit messages are clear enough to
> understand the purpose of the patchset. If not, your comments would be
> greatly appreciated so I can make the commit messages self-explanatory.
>
> Eric, what would be the right identifier to use for a specific user
> namespace that userspace could find out? I know the PIDs of the
> bubblewrap processes that created those user namespaces, would those be
> good enough?

A namespace file descriptor would work. AKA The result of
opening /proc/<pid>/ns/user.

I assume you are asking so that you can filter the set of file
descriptors to revoked not by user but by user namespace.

Eric



> Changes since v2:
> - Changed the internal API to pass a struct usb_device
> - Fixed potential busy loop in user-space when revoking access to a
>   device
>
> 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 | 79 ++++++++++++++++++++++++++++++++++++++--
>  drivers/usb/core/usb.c   | 51 ++++++++++++++++++++++++++
>  drivers/usb/core/usb.h   |  2 +
>  3 files changed, 128 insertions(+), 4 deletions(-)

Re: [PATCH 2/2] usb: Implement usb_revoke() BPF function

[Bastien Nocera]

On Tue, 2022-08-09 at 18:33 +0200, Greg Kroah-Hartman wrote:
> On Tue, Aug 09, 2022 at 04:31:04PM +0200, Bastien Nocera wrote:
> > On Tue, 2022-08-09 at 12:38 +0200, Greg Kroah-Hartman wrote:
> > > Now if you really really want to disable a device from under a
> > > user,
> > > without the file handle present, you can do that today, as root,
> > > by
> > > doing the 'unbind' hack through userspace and sysfs.  It's so
> > > common
> > > that this seems to be how virtual device managers handle virtual
> > > machines, so it should be well tested by now.
> > 
> > The only thing I know that works that way is usbip, and it requires
> > unbinding each of the interfaces:
> > 
> > https://sourceforge.net/p/usbip/git-windows/ci/master/tree/trunk/userspace/src/bind-driver.c#l157
> 
> virtio devices also use the api from what I recall.

I can't find any code that would reference
/sys/bus/usb/drivers/usbfs/unbind or /sys/bus/usb/drivers/usbfs wrt
virtio. Where's the host side code for that?

> > That means that, for example, revoking access to the raw USB device
> > that OpenRGB used to blink colours across a keyboard would
> > disconnect
> > the keyboard from the HID device.
> 
> No, you unbind the usbfs driver, not the hid driver.

Honestly, I don't understand how this is supposed to work. The USB
device is bound to the usb_generic driver, usbfs doesn't have a link to
the devices it's supposed to handle.

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

[Alan Stern]

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

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

[kernel test robot]

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	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

[kernel test robot]

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	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 2/2] usb: Implement usb_revoke() BPF function

[kernel test robot]

Hi Bastien,

I love your patch! Yet something to improve:

[auto build test ERROR on usb/usb-testing]
[also build test ERROR 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/202208110101.rbONyPek-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/b8d37bab24eee13dfbbb947c6a44f5f363c6bb7a
        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 b8d37bab24eee13dfbbb947c6a44f5f363c6bb7a
        # 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 error/warnings (new ones prefixed by >>):

>> drivers/usb/core/usb.c:465:1: warning: no previous prototype for function 'usb_revoke_device' [-Wmissing-prototypes]
   usb_revoke_device(int busnum, int devnum, unsigned int euid)
   ^
   drivers/usb/core/usb.c:464:10: note: declare 'static' if the function is not intended to be used outside of this translation unit
   noinline int
            ^
            static 
>> drivers/usb/core/usb.c:1050:3: error: field designator 'check_set' does not refer to any field in type 'const struct btf_kfunc_id_set'
           .check_set = &usbdev_kfunc_ids,
            ^
   1 warning and 1 error generated.


vim +1050 drivers/usb/core/usb.c

  1047	
  1048	static const struct btf_kfunc_id_set usbdev_kfunc_set = {
  1049		.owner     = THIS_MODULE,
> 1050		.check_set = &usbdev_kfunc_ids,
  1051	};
  1052	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 2/2] usb: Implement usb_revoke() BPF function

[Greg Kroah-Hartman]

On Tue, Aug 09, 2022 at 07:27:11PM +0200, Bastien Nocera wrote:
> On Tue, 2022-08-09 at 18:33 +0200, Greg Kroah-Hartman wrote:
> > On Tue, Aug 09, 2022 at 04:31:04PM +0200, Bastien Nocera wrote:
> > > On Tue, 2022-08-09 at 12:38 +0200, Greg Kroah-Hartman wrote:
> > > > Now if you really really want to disable a device from under a
> > > > user,
> > > > without the file handle present, you can do that today, as root,
> > > > by
> > > > doing the 'unbind' hack through userspace and sysfs.  It's so
> > > > common
> > > > that this seems to be how virtual device managers handle virtual
> > > > machines, so it should be well tested by now.
> > > 
> > > The only thing I know that works that way is usbip, and it requires
> > > unbinding each of the interfaces:
> > > 
> > > https://sourceforge.net/p/usbip/git-windows/ci/master/tree/trunk/userspace/src/bind-driver.c#l157
> > 
> > virtio devices also use the api from what I recall.
> 
> I can't find any code that would reference
> /sys/bus/usb/drivers/usbfs/unbind or /sys/bus/usb/drivers/usbfs wrt
> virtio. Where's the host side code for that?

I mean the virtio code uses bind/unbind for it's devices, nothing to do
with USB other than the userspace interface involved.

thanks,

greg k-h

Re: [PATCH 2/2] usb: Implement usb_revoke() BPF function

[Bastien Nocera]

On Thu, 2022-08-18 at 17:08 +0200, Greg Kroah-Hartman wrote:
> On Tue, Aug 09, 2022 at 07:27:11PM +0200, Bastien Nocera wrote:
> > On Tue, 2022-08-09 at 18:33 +0200, Greg Kroah-Hartman wrote:
> > > On Tue, Aug 09, 2022 at 04:31:04PM +0200, Bastien Nocera wrote:
> > > > On Tue, 2022-08-09 at 12:38 +0200, Greg Kroah-Hartman wrote:
> > > > > Now if you really really want to disable a device from under
> > > > > a
> > > > > user,
> > > > > without the file handle present, you can do that today, as
> > > > > root,
> > > > > by
> > > > > doing the 'unbind' hack through userspace and sysfs.  It's so
> > > > > common
> > > > > that this seems to be how virtual device managers handle
> > > > > virtual
> > > > > machines, so it should be well tested by now.
> > > > 
> > > > The only thing I know that works that way is usbip, and it
> > > > requires
> > > > unbinding each of the interfaces:
> > > > 
> > > > https://sourceforge.net/p/usbip/git-windows/ci/master/tree/trunk/userspace/src/bind-driver.c#l157
> > > 
> > > virtio devices also use the api from what I recall.
> > 
> > I can't find any code that would reference
> > /sys/bus/usb/drivers/usbfs/unbind or /sys/bus/usb/drivers/usbfs wrt
> > virtio. Where's the host side code for that?
> 
> I mean the virtio code uses bind/unbind for it's devices, nothing to
> do
> with USB other than the userspace interface involved.

This is one big hammer that is really counterproductive in some fairly
common use cases. It's fine for assigning a full USB device to a VM, it
really isn't for gently removing "just that bit of interface" the user
is using while leaving the rest running.

If a USB device has 2 interfaces, and one of those interfaces is used
by a kernel driver, or a system-wide application, then the whole USB
device is made unavailable.

For example:
- a wireless headset, sound is handled by ALSA at the kernel-level,
battery status is monitored through another interface by the user
directly, unbinding the USB driver will disable both the sound driver
and the battery monitor
- a keyboard with RGB backlight, key presses are handled by the input
subsystem in the kernel, RGB backlight through another interface as a
normal user, unbinding the USB driver disconnects the keyboard
completely making it lose lock keys status, amongst other things
- a phone used for both network access and file access through MTP,
unbinding the USB driver will boot the computer off the network as well
as disconnecting the file access when we only wanted the latter to
happen

I'll explain those use cases in the commit message.

For those who want to reproduce the problem, tested with USB wireless
headset:
$ lsusb
Bus 001 Device 011: ID 1038:12b6 SteelSeries ApS SteelSeries Arctis 1 Wireless

# Using the bus and device IDs
$ grep -l 001/011 /sys/bus/usb/devices/*/uevent | sed 's,/sys/bus/usb/devices/,,' | sed 's,/uevent,,'
1-10

$ echo 1-10 > /sys/bus/usb/drivers/usb/unbind

Both the ALSA device:
Aug 30 16:19:07 classic pipewire[2061]: spa.alsa: hw:2: snd_pcm_drop No such device
Aug 30 16:19:07 classic pipewire[2061]: spa.alsa: hw:2: close failed: No such device
Aug 30 16:19:07 classic pipewire[2061]: spa.alsa: front:2: snd_pcm_drop No such device
Aug 30 16:19:07 classic pipewire[2061]: spa.alsa: front:2: close failed: No such device


and the battery script
(https://github.com/Sapd/HeadsetControl/pull/230) are gone.

Re: [PATCH 2/2] usb: Implement usb_revoke() BPF function

[Greg Kroah-Hartman]

On Tue, Aug 30, 2022 at 04:44:52PM +0200, Bastien Nocera wrote:
> On Thu, 2022-08-18 at 17:08 +0200, Greg Kroah-Hartman wrote:
> > On Tue, Aug 09, 2022 at 07:27:11PM +0200, Bastien Nocera wrote:
> > > On Tue, 2022-08-09 at 18:33 +0200, Greg Kroah-Hartman wrote:
> > > > On Tue, Aug 09, 2022 at 04:31:04PM +0200, Bastien Nocera wrote:
> > > > > On Tue, 2022-08-09 at 12:38 +0200, Greg Kroah-Hartman wrote:
> > > > > > Now if you really really want to disable a device from under
> > > > > > a
> > > > > > user,
> > > > > > without the file handle present, you can do that today, as
> > > > > > root,
> > > > > > by
> > > > > > doing the 'unbind' hack through userspace and sysfs.  It's so
> > > > > > common
> > > > > > that this seems to be how virtual device managers handle
> > > > > > virtual
> > > > > > machines, so it should be well tested by now.
> > > > > 
> > > > > The only thing I know that works that way is usbip, and it
> > > > > requires
> > > > > unbinding each of the interfaces:
> > > > > 
> > > > > https://sourceforge.net/p/usbip/git-windows/ci/master/tree/trunk/userspace/src/bind-driver.c#l157
> > > > 
> > > > virtio devices also use the api from what I recall.
> > > 
> > > I can't find any code that would reference
> > > /sys/bus/usb/drivers/usbfs/unbind or /sys/bus/usb/drivers/usbfs wrt
> > > virtio. Where's the host side code for that?
> > 
> > I mean the virtio code uses bind/unbind for it's devices, nothing to
> > do
> > with USB other than the userspace interface involved.
> 
> This is one big hammer that is really counterproductive in some fairly
> common use cases. It's fine for assigning a full USB device to a VM, it
> really isn't for gently removing "just that bit of interface" the user
> is using while leaving the rest running.

In USB, drivers are bound to interfaces, not to the device.

But as Alan pointed out, we don't ever really "bind" the usbfs code to
the interface, so that will not work all that well :(

greg k-h

Re: [PATCH 2/2] usb: Implement usb_revoke() BPF function

[Bastien Nocera]

On Tue, 2022-08-30 at 17:10 +0200, Greg Kroah-Hartman wrote:
> On Tue, Aug 30, 2022 at 04:44:52PM +0200, Bastien Nocera wrote:
> > On Thu, 2022-08-18 at 17:08 +0200, Greg Kroah-Hartman wrote:
> > > On Tue, Aug 09, 2022 at 07:27:11PM +0200, Bastien Nocera wrote:
> > > > On Tue, 2022-08-09 at 18:33 +0200, Greg Kroah-Hartman wrote:
> > > > > On Tue, Aug 09, 2022 at 04:31:04PM +0200, Bastien Nocera
> > > > > wrote:
> > > > > > On Tue, 2022-08-09 at 12:38 +0200, Greg Kroah-Hartman
> > > > > > wrote:
> > > > > > > Now if you really really want to disable a device from
> > > > > > > under
> > > > > > > a
> > > > > > > user,
> > > > > > > without the file handle present, you can do that today,
> > > > > > > as
> > > > > > > root,
> > > > > > > by
> > > > > > > doing the 'unbind' hack through userspace and sysfs. 
> > > > > > > It's so
> > > > > > > common
> > > > > > > that this seems to be how virtual device managers handle
> > > > > > > virtual
> > > > > > > machines, so it should be well tested by now.
> > > > > > 
> > > > > > The only thing I know that works that way is usbip, and it
> > > > > > requires
> > > > > > unbinding each of the interfaces:
> > > > > > 
> > > > > > https://sourceforge.net/p/usbip/git-windows/ci/master/tree/trunk/userspace/src/bind-driver.c#l157
> > > > > 
> > > > > virtio devices also use the api from what I recall.
> > > > 
> > > > I can't find any code that would reference
> > > > /sys/bus/usb/drivers/usbfs/unbind or /sys/bus/usb/drivers/usbfs
> > > > wrt
> > > > virtio. Where's the host side code for that?
> > > 
> > > I mean the virtio code uses bind/unbind for it's devices, nothing
> > > to
> > > do
> > > with USB other than the userspace interface involved.
> > 
> > This is one big hammer that is really counterproductive in some
> > fairly
> > common use cases. It's fine for assigning a full USB device to a
> > VM, it
> > really isn't for gently removing "just that bit of interface" the
> > user
> > is using while leaving the rest running.
> 
> In USB, drivers are bound to interfaces, not to the device.

I did implement kernel drivers for devices all the way back in 2020, if
you remember.

> But as Alan pointed out, we don't ever really "bind" the usbfs code
> to
> the interface, so that will not work all that well :(

Right.

Re: [PATCH 2/2] usb: Implement usb_revoke() BPF function

[Bastien Nocera]

Hey,

On Tue, 2022-08-09 at 11:43 +0200, Bastien Nocera wrote:
> This functionality allows a sufficiently privileged user-space
> process
> to upload a BPF programme 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.
> 
> logind, and other session management software, does not have access
> to
> the file descriptor used by the application so other identifiers
> are used.

Locally, I have a newer version of the code that I've been able to test
successfully on some hardware, but I haven't been able to cover all of
its branches.

So I've started writing some test application that would create devices
with multiple interfaces using dummy_hcd, and client software that
talks to those fake devices. I also have a version of the revoke tool.

My question is about all the dependencies that those test tools could
use, and where to host it.

- Can I use libusb?
- Can I use libusbgx and raw-gadget?
- Can I use the GLib versions of those libraries?
- Do I need to have those tests as part of the kernel?
- Does it need to integrate with the kernel's compilation?
- Can I use a Makefile? meson?

Ultimately, only the revoke tool might have a use as a general purpose
debugging application, with the functionality being integrated in
systemd and co.

Opinions?

Re: [PATCH 2/2] usb: Implement usb_revoke() BPF function

[Greg Kroah-Hartman]

On Wed, Oct 26, 2022 at 05:00:35PM +0200, Bastien Nocera wrote:
> Hey,
> 
> On Tue, 2022-08-09 at 11:43 +0200, Bastien Nocera wrote:
> > This functionality allows a sufficiently privileged user-space
> > process
> > to upload a BPF programme 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.
> > 
> > logind, and other session management software, does not have access
> > to
> > the file descriptor used by the application so other identifiers
> > are used.
> 
> Locally, I have a newer version of the code that I've been able to test
> successfully on some hardware, but I haven't been able to cover all of
> its branches.
> 
> So I've started writing some test application that would create devices
> with multiple interfaces using dummy_hcd, and client software that
> talks to those fake devices. I also have a version of the revoke tool.
> 
> My question is about all the dependencies that those test tools could
> use, and where to host it.
> 
> - Can I use libusb?

You could, but do you need to just to create a device?

> - Can I use libusbgx and raw-gadget?

raw-gadget is good, libusbgx I have found is pretty "thin", is it really
needed?

> - Can I use the GLib versions of those libraries?

That might be pushing it.

> - Do I need to have those tests as part of the kernel?

Ideally yes, why not?

> - Does it need to integrate with the kernel's compilation?

All kernel tests are in the tree, yes.

> - Can I use a Makefile? meson?

Makefile should be fine, look at all of the other examples we have.
Also tie into the other kernel test framework to provide the proper
results in the correct format so that tools can parse them correctly.

thanks,

greg k-h