| Message ID | 20220809094300.83116-3-hadess@hadess.net (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | BPF |
| Headers | show |
| Series | USB: core: add a way to revoke access to open USB devices | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for Kernel LATEST on ubuntu-latest with gcc |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for Kernel LATEST on ubuntu-latest with llvm-16 |
| bpf/vmtest-bpf-next-PR | success | PR summary |
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for Kernel LATEST on z15 with gcc |
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
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.
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
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
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.
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
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();
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.
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
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
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.
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
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.
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?
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
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();
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(+)