[PATCH] Allow userspace control of runtime disabling/enabling of driver probing

[PATCH] Allow userspace control of runtime disabling/enabling of driver probing

[Kees Cook]

From: Matthew Garrett <mjg59@coreos.com>

Various attacks are made possible due to the large attack surface of
kernel drivers and the easy availability of hotpluggable hardware that can
be programmed to mimic arbitrary devices. This allows attackers to find a
single vulnerable driver and then produce a device that can exploit it by
plugging into a hotpluggable bus (such as PCI or USB). This violates user
assumptions about unattended systems being secure as long as the screen
is locked.

The kernel already has support for deferring driver binding in order
to avoid problems over suspend/resume. By exposing this to userspace we
can disable probing when the screen is locked and simply reenable it on
unlock.

This is not a complete solution - since this still permits device
creation and simply blocks driver binding, it won't stop userspace
drivers from attaching to devices and it won't protect against any kernel
vulnerabilities in the core bus code. However, it should be sufficient to
block attacks like Poisontap (https://samy.pl/poisontap/).

Signed-off-by: Matthew Garrett <mjg59@coreos.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 .../ABI/testing/sysfs-kernel-disable-device-probe  | 10 ++++++++
 drivers/base/base.h                                |  2 --
 drivers/base/dd.c                                  | 10 ++++++++
 drivers/base/power/main.c                          | 16 ++++++++++---
 include/linux/device.h                             |  4 ++++
 kernel/ksysfs.c                                    | 28 ++++++++++++++++++++++
 6 files changed, 65 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-kernel-disable-device-probe

diff --git a/Documentation/ABI/testing/sysfs-kernel-disable-device-probe b/Documentation/ABI/testing/sysfs-kernel-disable-device-probe
new file mode 100644
index 000000000000..1ca6c2d11d8b
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-kernel-disable-device-probe
@@ -0,0 +1,10 @@
+What:		/sys/kernel/disable_device_probe
+Date:		December 2016
+KernelVersion:	4.11
+Contact:	Matthew Garrett <mjg59@srcf.ucam.org>
+Description
+		Disables automatic driver probing of any newly added devices.
+		If "1", driver probing is disabled - any newly added devices
+		will not have a driver bound to them. If "0", newly added
+		devices will be probed, along with any devices connected while
+		"1" was set.
diff --git a/drivers/base/base.h b/drivers/base/base.h
index ada9dce34e6d..7bee2e4e38ce 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -134,8 +134,6 @@ extern void device_remove_groups(struct device *dev,
 extern char *make_class_name(const char *name, struct kobject *kobj);
 
 extern int devres_release_all(struct device *dev);
-extern void device_block_probing(void);
-extern void device_unblock_probing(void);
 
 /* /sys/devices directory */
 extern struct kset *devices_kset;
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index a8b258e5407b..4d70fa41132c 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -191,6 +191,16 @@ static void driver_deferred_probe_trigger(void)
 }
 
 /**
+ * device_probing_deferred() - Get the current state of device probing
+ *
+ *	Returns whether or not device probing is currently deferred
+ */
+bool device_probing_deferred(void)
+{
+	return defer_all_probes;
+}
+
+/**
  * device_block_probing() - Block/defere device's probes
  *
  *	It will disable probing of devices and defer their probes instead.
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 249e0304597f..b566e7a6140c 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -59,6 +59,8 @@ struct suspend_stats suspend_stats;
 static DEFINE_MUTEX(dpm_list_mtx);
 static pm_message_t pm_transition;
 
+static bool probing_deferred;
+
 static int async_error;
 
 static char *pm_verb(int event)
@@ -1024,8 +1026,12 @@ void dpm_complete(pm_message_t state)
 	list_splice(&list, &dpm_list);
 	mutex_unlock(&dpm_list_mtx);
 
-	/* Allow device probing and trigger re-probing of deferred devices */
-	device_unblock_probing();
+	/*
+	 * Allow device probing and trigger re-probing of deferred devices
+	 * unless userspace has explicitly disabled probing
+	 */
+	if (!probing_deferred)
+		device_unblock_probing();
 	trace_suspend_resume(TPS("dpm_complete"), state.event, false);
 }
 
@@ -1714,8 +1720,12 @@ int dpm_prepare(pm_message_t state)
 	 * hibernation and system behavior will be unpredictable in this case.
 	 * So, let's prohibit device's probing here and defer their probes
 	 * instead. The normal behavior will be restored in dpm_complete().
+	 * Skip this if probing is already deferred, otherwise we'll override
+	 * explicitly configured state.
 	 */
-	device_block_probing();
+	probing_deferred = device_probing_deferred();
+	if (!probing_deferred)
+		device_block_probing();
 
 	mutex_lock(&dpm_list_mtx);
 	while (!list_empty(&dpm_list)) {
diff --git a/include/linux/device.h b/include/linux/device.h
index 491b4c0ca633..e7ca593605f6 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -43,6 +43,10 @@ struct iommu_ops;
 struct iommu_group;
 struct iommu_fwspec;
 
+extern bool device_probing_deferred(void);
+extern void device_block_probing(void);
+extern void device_unblock_probing(void);
+
 struct bus_attribute {
 	struct attribute	attr;
 	ssize_t (*show)(struct bus_type *bus, char *buf);
diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c
index ee1bc1bb8feb..22276140b47c 100644
--- a/kernel/ksysfs.c
+++ b/kernel/ksysfs.c
@@ -19,6 +19,7 @@
 #include <linux/sched.h>
 #include <linux/capability.h>
 #include <linux/compiler.h>
+#include <linux/device.h>
 
 #include <linux/rcupdate.h>	/* rcu_expedited and rcu_normal */
 
@@ -144,6 +145,32 @@ static ssize_t fscaps_show(struct kobject *kobj,
 }
 KERNEL_ATTR_RO(fscaps);
 
+/* Disable/reenable device probing*/
+static ssize_t disable_device_probe_show(struct kobject *kobj,
+					 struct kobj_attribute *attr,
+					 char *buf)
+{
+	return sprintf(buf, "%d\n", device_probing_deferred());
+}
+
+static ssize_t disable_device_probe_store(struct kobject *kobj,
+					  struct kobj_attribute *attr,
+					  const char *buf, size_t count)
+{
+	bool disable_probing;
+
+	if (kstrtobool(buf, &disable_probing))
+		return -EINVAL;
+
+	if (disable_probing)
+		device_block_probing();
+	else
+		device_unblock_probing();
+
+	return count;
+}
+KERNEL_ATTR_RW(disable_device_probe);
+
 #ifndef CONFIG_TINY_RCU
 int rcu_expedited;
 static ssize_t rcu_expedited_show(struct kobject *kobj,
@@ -225,6 +252,7 @@ static struct attribute * kernel_attrs[] = {
 	&rcu_expedited_attr.attr,
 	&rcu_normal_attr.attr,
 #endif
+	&disable_device_probe_attr.attr,
 	NULL
 };
 
-- 
2.7.4


-- 
Kees Cook
Nexus Security

Re: [PATCH] Allow userspace control of runtime disabling/enabling of driver probing

[Rafael J. Wysocki]

On Tue, Jan 3, 2017 at 11:58 PM, Kees Cook <keescook@chromium.org> wrote:
> From: Matthew Garrett <mjg59@coreos.com>
>
> Various attacks are made possible due to the large attack surface of
> kernel drivers and the easy availability of hotpluggable hardware that can
> be programmed to mimic arbitrary devices. This allows attackers to find a
> single vulnerable driver and then produce a device that can exploit it by
> plugging into a hotpluggable bus (such as PCI or USB). This violates user
> assumptions about unattended systems being secure as long as the screen
> is locked.
>
> The kernel already has support for deferring driver binding in order
> to avoid problems over suspend/resume. By exposing this to userspace we
> can disable probing when the screen is locked and simply reenable it on
> unlock.
>
> This is not a complete solution - since this still permits device
> creation and simply blocks driver binding, it won't stop userspace
> drivers from attaching to devices and it won't protect against any kernel
> vulnerabilities in the core bus code. However, it should be sufficient to
> block attacks like Poisontap (https://samy.pl/poisontap/).

It also looks like this may be worked around by tricking the user to
unlock the screen while the malicious device is still attached to the
system.

If that really is the case, I wonder if it's worth the extra complexity.

Thanks,
Rafael

Re: [PATCH] Allow userspace control of runtime disabling/enabling of driver probing

[Kees Cook]

On Tue, Jan 3, 2017 at 3:34 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Tue, Jan 3, 2017 at 11:58 PM, Kees Cook <keescook@chromium.org> wrote:
>> From: Matthew Garrett <mjg59@coreos.com>
>>
>> Various attacks are made possible due to the large attack surface of
>> kernel drivers and the easy availability of hotpluggable hardware that can
>> be programmed to mimic arbitrary devices. This allows attackers to find a
>> single vulnerable driver and then produce a device that can exploit it by
>> plugging into a hotpluggable bus (such as PCI or USB). This violates user
>> assumptions about unattended systems being secure as long as the screen
>> is locked.
>>
>> The kernel already has support for deferring driver binding in order
>> to avoid problems over suspend/resume. By exposing this to userspace we
>> can disable probing when the screen is locked and simply reenable it on
>> unlock.
>>
>> This is not a complete solution - since this still permits device
>> creation and simply blocks driver binding, it won't stop userspace
>> drivers from attaching to devices and it won't protect against any kernel
>> vulnerabilities in the core bus code. However, it should be sufficient to
>> block attacks like Poisontap (https://samy.pl/poisontap/).
>
> It also looks like this may be worked around by tricking the user to
> unlock the screen while the malicious device is still attached to the
> system.

It certainly changes the temporal aspect of the attack (i.e. there is
a delay and must be "silent" in that the local user cannot notice it).

> If that really is the case, I wonder if it's worth the extra complexity.

I think so, since it's not that much more complexity (it uses the
existing deferral mechanism).

-Kees

-- 
Kees Cook
Nexus Security

Re: [PATCH] Allow userspace control of runtime disabling/enabling of driver probing

[Rafael J. Wysocki]

On Wed, Jan 4, 2017 at 12:38 AM, Kees Cook <keescook@chromium.org> wrote:
> On Tue, Jan 3, 2017 at 3:34 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Tue, Jan 3, 2017 at 11:58 PM, Kees Cook <keescook@chromium.org> wrote:
>>> From: Matthew Garrett <mjg59@coreos.com>
>>>
>>> Various attacks are made possible due to the large attack surface of
>>> kernel drivers and the easy availability of hotpluggable hardware that can
>>> be programmed to mimic arbitrary devices. This allows attackers to find a
>>> single vulnerable driver and then produce a device that can exploit it by
>>> plugging into a hotpluggable bus (such as PCI or USB). This violates user
>>> assumptions about unattended systems being secure as long as the screen
>>> is locked.
>>>
>>> The kernel already has support for deferring driver binding in order
>>> to avoid problems over suspend/resume. By exposing this to userspace we
>>> can disable probing when the screen is locked and simply reenable it on
>>> unlock.
>>>
>>> This is not a complete solution - since this still permits device
>>> creation and simply blocks driver binding, it won't stop userspace
>>> drivers from attaching to devices and it won't protect against any kernel
>>> vulnerabilities in the core bus code. However, it should be sufficient to
>>> block attacks like Poisontap (https://samy.pl/poisontap/).
>>
>> It also looks like this may be worked around by tricking the user to
>> unlock the screen while the malicious device is still attached to the
>> system.
>
> It certainly changes the temporal aspect of the attack (i.e. there is
> a delay and must be "silent" in that the local user cannot notice it).

It will be silent until a driver binds to it anyway, won't it?

>> If that really is the case, I wonder if it's worth the extra complexity.
>
> I think so, since it's not that much more complexity (it uses the
> existing deferral mechanism).

But that existing mechanism certainly wasn't designed to be turned on
and off at random and concurrently etc.  The way it is going to be
used now is far more complex IMO.

Thanks,
Rafael

Re: [PATCH] Allow userspace control of runtime disabling/enabling of driver probing

[Greg Kroah-Hartman]

On Tue, Jan 03, 2017 at 02:58:31PM -0800, Kees Cook wrote:
> From: Matthew Garrett <mjg59@coreos.com>
> 
> Various attacks are made possible due to the large attack surface of
> kernel drivers and the easy availability of hotpluggable hardware that can
> be programmed to mimic arbitrary devices. This allows attackers to find a
> single vulnerable driver and then produce a device that can exploit it by
> plugging into a hotpluggable bus (such as PCI or USB). This violates user
> assumptions about unattended systems being secure as long as the screen
> is locked.
> 
> The kernel already has support for deferring driver binding in order
> to avoid problems over suspend/resume. By exposing this to userspace we
> can disable probing when the screen is locked and simply reenable it on
> unlock.
> 
> This is not a complete solution - since this still permits device
> creation and simply blocks driver binding, it won't stop userspace
> drivers from attaching to devices and it won't protect against any kernel
> vulnerabilities in the core bus code. However, it should be sufficient to
> block attacks like Poisontap (https://samy.pl/poisontap/).
> 
> Signed-off-by: Matthew Garrett <mjg59@coreos.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  .../ABI/testing/sysfs-kernel-disable-device-probe  | 10 ++++++++
>  drivers/base/base.h                                |  2 --
>  drivers/base/dd.c                                  | 10 ++++++++
>  drivers/base/power/main.c                          | 16 ++++++++++---
>  include/linux/device.h                             |  4 ++++
>  kernel/ksysfs.c                                    | 28 ++++++++++++++++++++++
>  6 files changed, 65 insertions(+), 5 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-kernel-disable-device-probe
> 
> diff --git a/Documentation/ABI/testing/sysfs-kernel-disable-device-probe b/Documentation/ABI/testing/sysfs-kernel-disable-device-probe
> new file mode 100644
> index 000000000000..1ca6c2d11d8b
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-kernel-disable-device-probe
> @@ -0,0 +1,10 @@
> +What:		/sys/kernel/disable_device_probe
> +Date:		December 2016
> +KernelVersion:	4.11
> +Contact:	Matthew Garrett <mjg59@srcf.ucam.org>
> +Description
> +		Disables automatic driver probing of any newly added devices.
> +		If "1", driver probing is disabled - any newly added devices
> +		will not have a driver bound to them. If "0", newly added
> +		devices will be probed, along with any devices connected while
> +		"1" was set.
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index ada9dce34e6d..7bee2e4e38ce 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -134,8 +134,6 @@ extern void device_remove_groups(struct device *dev,
>  extern char *make_class_name(const char *name, struct kobject *kobj);
>  
>  extern int devres_release_all(struct device *dev);
> -extern void device_block_probing(void);
> -extern void device_unblock_probing(void);
>  
>  /* /sys/devices directory */
>  extern struct kset *devices_kset;
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index a8b258e5407b..4d70fa41132c 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -191,6 +191,16 @@ static void driver_deferred_probe_trigger(void)
>  }
>  
>  /**
> + * device_probing_deferred() - Get the current state of device probing
> + *
> + *	Returns whether or not device probing is currently deferred
> + */
> +bool device_probing_deferred(void)
> +{
> +	return defer_all_probes;
> +}
> +
> +/**
>   * device_block_probing() - Block/defere device's probes
>   *
>   *	It will disable probing of devices and defer their probes instead.
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 249e0304597f..b566e7a6140c 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -59,6 +59,8 @@ struct suspend_stats suspend_stats;
>  static DEFINE_MUTEX(dpm_list_mtx);
>  static pm_message_t pm_transition;
>  
> +static bool probing_deferred;
> +
>  static int async_error;
>  
>  static char *pm_verb(int event)
> @@ -1024,8 +1026,12 @@ void dpm_complete(pm_message_t state)
>  	list_splice(&list, &dpm_list);
>  	mutex_unlock(&dpm_list_mtx);
>  
> -	/* Allow device probing and trigger re-probing of deferred devices */
> -	device_unblock_probing();
> +	/*
> +	 * Allow device probing and trigger re-probing of deferred devices
> +	 * unless userspace has explicitly disabled probing
> +	 */
> +	if (!probing_deferred)
> +		device_unblock_probing();

Ick, hiding this in the power management code?  That's messy, and
complex, as Rafael pointed out.

Turning on and off at random times "new devices can not be bound, wait,
now they can!" is ripe for loads of issues and is going to be a pain to
properly maintain over time.

What's wrong with the facility that the USB layer provides today to
allow only "authenticated" devices to be enabled?  That has been used
for a few years now to prevent these "don't allow random devices that
are plugged into the computer to be enabled" type attacks.  Doing much
the same thing, in a different manner, is ripe for problems (how do the
two interact now?)

If you are worried about PCI devices, why not just implement what USB
did for PCI?  Or better yet, move the USB functionality into the driver
core, adding that type of authentication ability to any bus that wishes
to have it (and not break existing working systems that are using the
USB solution today.)

thanks,

greg k-h

Re: [PATCH] Allow userspace control of runtime disabling/enabling of driver probing

[Matthew Garrett]

On Wed, Jan 4, 2017 at 3:32 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> Ick, hiding this in the power management code?  That's messy, and
> complex, as Rafael pointed out.

It's in code that's used in the power management layer, not in power
management code. This is all in the driver core.

> Turning on and off at random times "new devices can not be bound, wait,
> now they can!" is ripe for loads of issues and is going to be a pain to
> properly maintain over time.

What kind of issues?

> What's wrong with the facility that the USB layer provides today to
> allow only "authenticated" devices to be enabled?  That has been used
> for a few years now to prevent these "don't allow random devices that
> are plugged into the computer to be enabled" type attacks.  Doing much
> the same thing, in a different manner, is ripe for problems (how do the
> two interact now?)

The USB authentication feature was intended for handling wireless USB
devices - it can be reused for this, but the code isn't generic enough
to apply to other bus types. The two interact in exactly the way you'd
expect, ie they don't. If you use both, then you need to handle both.

> If you are worried about PCI devices, why not just implement what USB
> did for PCI?  Or better yet, move the USB functionality into the driver
> core, adding that type of authentication ability to any bus that wishes
> to have it (and not break existing working systems that are using the
> USB solution today.)

The USB approach requires userspace to keep track of which devices
were added while the session was locked, whereas the kernel already
has the logic to do all of this. All the complexity already exists and
is used every time anybody suspends and resumes, so why add additional
complexity?

I'm not clear on how this patch breaks anybody using the existing USB solution?

Re: [PATCH] Allow userspace control of runtime disabling/enabling of driver probing

[Matthew Garrett]

On Wed, Jan 4, 2017 at 12:10 PM, Matthew Garrett <mjg59@coreos.com> wrote:
>
> The USB authentication feature was intended for handling wireless USB
> devices - it can be reused for this, but the code isn't generic enough
> to apply to other bus types. The two interact in exactly the way you'd
> expect, ie they don't. If you use both, then you need to handle both.

And as an example of why the USB authorisation feature isn't
sufficient - the interface configuration isn't picked until after
you've authorised the device, which means you can't necessarily tell
the difference between a keyboard and an ethernet adapter until after
you've authorised it. That defeats the object, but it can't be changed
without breaking the wireless USB case.

Re: [PATCH] Allow userspace control of runtime disabling/enabling of driver probing

[Greg Kroah-Hartman]

On Wed, Jan 04, 2017 at 12:10:04PM -0600, Matthew Garrett wrote:
> On Wed, Jan 4, 2017 at 3:32 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > Ick, hiding this in the power management code?  That's messy, and
> > complex, as Rafael pointed out.
> 
> It's in code that's used in the power management layer, not in power
> management code. This is all in the driver core.

Yes, but it's in the power management layer, not the "main" portion of
the driver bind/unbind logic.  It's kind of hidden, don't you think?

> > Turning on and off at random times "new devices can not be bound, wait,
> > now they can!" is ripe for loads of issues and is going to be a pain to
> > properly maintain over time.
> 
> What kind of issues?

How are you going to test this to ensure it continues to work properly?

> > What's wrong with the facility that the USB layer provides today to
> > allow only "authenticated" devices to be enabled?  That has been used
> > for a few years now to prevent these "don't allow random devices that
> > are plugged into the computer to be enabled" type attacks.  Doing much
> > the same thing, in a different manner, is ripe for problems (how do the
> > two interact now?)
> 
> The USB authentication feature was intended for handling wireless USB
> devices - it can be reused for this, but the code isn't generic enough
> to apply to other bus types. The two interact in exactly the way you'd
> expect, ie they don't. If you use both, then you need to handle both.

The feature was originally created for wireless USB devices, but it will
work for any USB device, and has been successfully used for this type of
thing (i.e. protecting kiosk-systems) for a while now.

> > If you are worried about PCI devices, why not just implement what USB
> > did for PCI?  Or better yet, move the USB functionality into the driver
> > core, adding that type of authentication ability to any bus that wishes
> > to have it (and not break existing working systems that are using the
> > USB solution today.)
> 
> The USB approach requires userspace to keep track of which devices
> were added while the session was locked, whereas the kernel already
> has the logic to do all of this. All the complexity already exists and
> is used every time anybody suspends and resumes, so why add additional
> complexity?

The kernel knows nothing about a "session", you know that.  Userspace
can disable enabling any new USB device when a session is "locked" today
quite easily, why not do that?

> I'm not clear on how this patch breaks anybody using the existing USB
> solution?

You now have two different ways to enable access to a USB device, please
let's keep to only one type of path.

thanks,

greg k-h

Re: [PATCH] Allow userspace control of runtime disabling/enabling of driver probing

[Greg Kroah-Hartman]

On Wed, Jan 04, 2017 at 12:31:45PM -0600, Matthew Garrett wrote:
> On Wed, Jan 4, 2017 at 12:10 PM, Matthew Garrett <mjg59@coreos.com> wrote:
> >
> > The USB authentication feature was intended for handling wireless USB
> > devices - it can be reused for this, but the code isn't generic enough
> > to apply to other bus types. The two interact in exactly the way you'd
> > expect, ie they don't. If you use both, then you need to handle both.
> 
> And as an example of why the USB authorisation feature isn't
> sufficient - the interface configuration isn't picked until after
> you've authorised the device, which means you can't necessarily tell
> the difference between a keyboard and an ethernet adapter until after
> you've authorised it.

You know the device type and vendor/product id before you authorize it,
you should be able to do this type of detection otherwise it seems
pretty pointless :)

> That defeats the object, but it can't be changed without breaking the
> wireless USB case.

No one has wireless USB devices, this all works the same for any USB
device :)

thanks,

greg k-h

Re: [PATCH] Allow userspace control of runtime disabling/enabling of driver probing

[Matthew Garrett]

On Wed, Jan 4, 2017 at 1:46 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Wed, Jan 04, 2017 at 12:10:04PM -0600, Matthew Garrett wrote:
>> On Wed, Jan 4, 2017 at 3:32 AM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > Ick, hiding this in the power management code?  That's messy, and
>> > complex, as Rafael pointed out.
>>
>> It's in code that's used in the power management layer, not in power
>> management code. This is all in the driver core.
>
> Yes, but it's in the power management layer, not the "main" portion of
> the driver bind/unbind logic.  It's kind of hidden, don't you think?

No? It's in dd.c, and defer_all_probes is checked in really_probe()
which is part of the normal probe path. The only user currently is in
the power management code, but there's nothing directly power
management related going on here.

>> > Turning on and off at random times "new devices can not be bound, wait,
>> > now they can!" is ripe for loads of issues and is going to be a pain to
>> > properly maintain over time.
>>
>> What kind of issues?
>
> How are you going to test this to ensure it continues to work properly?

It's code that's already used over every suspend and resume. What kind
of breakage are you expecting? How do we test any feature to ensure it
continues to work properly?

>> > What's wrong with the facility that the USB layer provides today to
>> > allow only "authenticated" devices to be enabled?  That has been used
>> > for a few years now to prevent these "don't allow random devices that
>> > are plugged into the computer to be enabled" type attacks.  Doing much
>> > the same thing, in a different manner, is ripe for problems (how do the
>> > two interact now?)
>>
>> The USB authentication feature was intended for handling wireless USB
>> devices - it can be reused for this, but the code isn't generic enough
>> to apply to other bus types. The two interact in exactly the way you'd
>> expect, ie they don't. If you use both, then you need to handle both.
>
> The feature was originally created for wireless USB devices, but it will
> work for any USB device, and has been successfully used for this type of
> thing (i.e. protecting kiosk-systems) for a while now.

It will work for any USB device, but doesn't have the desired
semantics and the implementation is directly tied to USB.

>> > If you are worried about PCI devices, why not just implement what USB
>> > did for PCI?  Or better yet, move the USB functionality into the driver
>> > core, adding that type of authentication ability to any bus that wishes
>> > to have it (and not break existing working systems that are using the
>> > USB solution today.)
>>
>> The USB approach requires userspace to keep track of which devices
>> were added while the session was locked, whereas the kernel already
>> has the logic to do all of this. All the complexity already exists and
>> is used every time anybody suspends and resumes, so why add additional
>> complexity?
>
> The kernel knows nothing about a "session", you know that.  Userspace
> can disable enabling any new USB device when a session is "locked" today
> quite easily, why not do that?

Because the existing feature doesn't allow you to identify what kind
of device has been plugged in, which means it's not usable.

>> I'm not clear on how this patch breaks anybody using the existing USB
>> solution?
>
> You now have two different ways to enable access to a USB device, please
> let's keep to only one type of path.

So it won't break the existing USB solution?

Re: [PATCH] Allow userspace control of runtime disabling/enabling of driver probing

[Matthew Garrett]

On Wed, Jan 4, 2017 at 1:47 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> You know the device type and vendor/product id before you authorize it,
> you should be able to do this type of detection otherwise it seems
> pretty pointless :)

You know the vendor and product ID, which doesn't tell you whether one
of the endpoints is a network device or a keyboard. You need to know
that.

Re: [PATCH] Allow userspace control of runtime disabling/enabling of driver probing

[Greg Kroah-Hartman]

On Wed, Jan 04, 2017 at 02:01:00PM -0600, Matthew Garrett wrote:
> On Wed, Jan 4, 2017 at 1:47 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > You know the device type and vendor/product id before you authorize it,
> > you should be able to do this type of detection otherwise it seems
> > pretty pointless :)
> 
> You know the vendor and product ID, which doesn't tell you whether one
> of the endpoints is a network device or a keyboard. You need to know
> that.

Are you sure you don't have the configuration information as well?  That
should tell you...

And for network devices, they are almost all just vendor/product ids,
not many use the class protocol.

thanks,

greg k-h

Re: [PATCH] Allow userspace control of runtime disabling/enabling of driver probing

[Matthew Garrett]

Ugh let's try that again in plain text (How does the Android gmail
client still not get this right‽)

On Wed, Jan 4, 2017 at 2:47 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Wed, Jan 04, 2017 at 02:01:00PM -0600, Matthew Garrett wrote:
>> On Wed, Jan 4, 2017 at 1:47 PM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > You know the device type and vendor/product id before you authorize it,
>> > you should be able to do this type of detection otherwise it seems
>> > pretty pointless :)
>>
>> You know the vendor and product ID, which doesn't tell you whether one
>> of the endpoints is a network device or a keyboard. You need to know
>> that.
>
> Are you sure you don't have the configuration information as well?  That
> should tell you...

usb_choose_configuration() hasn't been called at this point, so no -
we don't create any device entries, so there's no way for userspace to
know anything (there isn't even a uevent on device plug). And even if
you could scrape the info, you still have no way of knowing what
configuration the kernel will choose if the device has multiple.

> And for network devices, they are almost all just vendor/product ids,
> not many use the class protocol.

Plenty of stuff uses CDC.

Re: [PATCH] Allow userspace control of runtime disabling/enabling of driver probing

[Matthew Garrett]

On Wed, Jan 4, 2017 at 3:53 PM, Matthew Garrett <mjg59@coreos.com> wrote:
> usb_choose_configuration() hasn't been called at this point, so no -
> we don't create any device entries, so there's no way for userspace to
> know anything (there isn't even a uevent on device plug). And even if
> you could scrape the info, you still have no way of knowing what
> configuration the kernel will choose if the device has multiple.

Ah, I had things slightly misconfigured (accidentally deauthorised the
hub itself, so unsurprising that children wouldn't show up) - you do
get an event for the device itself, but you still have no means of
identifying how the endpoints will show up without parsing the binary
descriptors and then hoping that you have the same heuristics as the
kernel.

Re: [PATCH] Allow userspace control of runtime disabling/enabling of driver probing

[Tomeu Vizoso]

On 4 January 2017 at 20:59, Matthew Garrett <mjg59@coreos.com> wrote:
> On Wed, Jan 4, 2017 at 1:46 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>> On Wed, Jan 04, 2017 at 12:10:04PM -0600, Matthew Garrett wrote:
>>> On Wed, Jan 4, 2017 at 3:32 AM, Greg Kroah-Hartman
>>> <gregkh@linuxfoundation.org> wrote:
>>> > Turning on and off at random times "new devices can not be bound, wait,
>>> > now they can!" is ripe for loads of issues and is going to be a pain to
>>> > properly maintain over time.
>>>
>>> What kind of issues?
>>
>> How are you going to test this to ensure it continues to work properly?
>
> It's code that's already used over every suspend and resume. What kind
> of breakage are you expecting? How do we test any feature to ensure it
> continues to work properly?

In this specific case, we could use the Chamelium board to emulate USB
devices, automating the plugging of various USB device types at
different system states:

https://www.chromium.org/chromium-os/testing/chamelium

We are currently working on improving LAVA support for helper devices
such as the Chamelium, mainly for emulating displays in graphics
tests, but it can be useful for exercising other kernel ABIs such as
disable_device_probe. The goal is to have the result of these tests
reported to kernelci.org for spotting regressions early.

Regards,

Tomeu