linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1] driver: base: Add driver filter support
@ 2021-08-04 17:43 Kuppuswamy Sathyanarayanan
  2021-08-04 18:08 ` Matthew Wilcox
  2021-08-04 19:29 ` Greg Kroah-Hartman
  0 siblings, 2 replies; 44+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2021-08-04 17:43 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Rafael J . Wysocki, Jonathan Corbet
  Cc: Dan Williams, Andi Kleen, Kuppuswamy Sathyanarayanan,
	linux-kernel, linux-doc

Intel's Trust Domain Extensions (TDX) is a new Intel technology that
adds support for VMs to maintain confidentiality and integrity in the
presence of an untrusted VMM.

Given the VMM is an untrusted entity and the VMM presents emulated
hardware to the guest, a threat vector similar to Thunderclap [1] is
present. Also, for ease of deployment, it is useful to be able to use
the same kernel binary on host and guest, but that presents a wide
attack surface given the range of hardware supported in typical
builds. However, TDX guests only require a small number of drivers, a
number small enough to audit for Thunderclap style attacks, and the
rest can be disabled via policy. Add a mechanism to filter drivers
based on a "protected-guest trusted" indication.

An alternative solution is to disable untrusted drivers using a custom
kernel config for TDX, but such solution will break our goal of using
same kernel binary in guest/host or in standard OS distributions. So
it is not considered.

Driver filter framework adds support to filter drivers at boot
time by using platform specific allow list. This is a generic
solution that can be reused by TDX. Driver filter framework adds a
new hook (driver_allowed()) in driver_register() interface which
will update the "allowed" status of the driver based on platform
specific driver allow list or deny list. During driver bind process,
trusted status is used to determine whether to continue or deny the
bind process. If platform does not register any allow or deny list,
all devices will be allowed. Platforms can use wildcard like "ALL:ALL"
in bus_name and driver_name of filter node to allow or deny all
bus and drivers.

Per driver opt-in model is also considered as an alternative design
choice, but central allow or deny list is chosen because it is
easier to maintain and audit. Also, "driver self serve" might be
abused by mistake by driver authors cutting and pasting the code.

Also add an option in kernel command line and sysfs to update the
allowed or denied drivers list. Driver filter allowed status
can be read using following command.

cat /sys/bus/$bus/drivers/$driver/allowed

Details about TDX technology can be found in following link:

https://software.intel.com/content/www/br/pt/develop/articles/intel-trust-domain-extensions.html

[1]: http://thunderclap.io/

Reviewed-by: Andi Kleen <ak@linux.intel.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 .../admin-guide/kernel-parameters.txt         |  15 ++
 drivers/base/Makefile                         |   2 +-
 drivers/base/base.h                           |   8 +
 drivers/base/bus.c                            |  34 +++
 drivers/base/filter.c                         | 217 ++++++++++++++++++
 include/linux/device/filter.h                 |  35 +++
 6 files changed, 310 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/filter.c
 create mode 100644 include/linux/device/filter.h

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 10776a743e74..2af8b60b227b 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1399,6 +1399,21 @@
 			See Documentation/admin-guide/sysctl/net.rst for
 			fb_tunnels_only_for_init_ns
 
+	filter_deny_drivers=
+	filter_allow_drivers=[KNL]
+			Format: bus:driver
+			Override the default driver filter allowed or blocked
+			list. Multiple drivers can be specified, separated by
+			comma. Multiple bus/driver combinations can be
+			specified separated by semicolon. For example to allow
+			the e1000 driver use filter_allow_drivers=pci:e1000. To
+			allow all drivers in pci use
+			filter_allow_drivers=pci:ALL. Similarly to deny e1000
+			driver, use filter_deny_drivers=pci:e1000.
+			Also note that, driver allow list is searched first. If
+			an entry exist in allow list, deny list will not be
+			searched.
+
 	floppy=		[HW]
 			See Documentation/admin-guide/blockdev/floppy.rst.
 
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index ef8e44a7d288..d06b698e2796 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -6,7 +6,7 @@ obj-y			:= component.o core.o bus.o dd.o syscore.o \
 			   cpu.o firmware.o init.o map.o devres.o \
 			   attribute_container.o transport_class.o \
 			   topology.o container.o property.o cacheinfo.o \
-			   swnode.o
+			   swnode.o filter.o
 obj-$(CONFIG_AUXILIARY_BUS) += auxiliary.o
 obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
 obj-y			+= power/
diff --git a/drivers/base/base.h b/drivers/base/base.h
index 404db83ee5ec..1c4c19cab670 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -61,6 +61,8 @@ struct driver_private {
 	struct klist_node knode_bus;
 	struct module_kobject *mkobj;
 	struct device_driver *driver;
+	/* Used by driver filter framework to cache allowed status */
+	bool allowed;
 };
 #define to_driver(obj) container_of(obj, struct driver_private, kobj)
 
@@ -144,6 +146,9 @@ extern void device_set_deferred_probe_reason(const struct device *dev,
 static inline int driver_match_device(struct device_driver *drv,
 				      struct device *dev)
 {
+	if (!drv->p->allowed)
+		return 0;
+
 	return drv->bus->match ? drv->bus->match(dev, drv) : 1;
 }
 extern bool driver_allows_async_probing(struct device_driver *drv);
@@ -202,3 +207,6 @@ int devtmpfs_delete_node(struct device *dev);
 static inline int devtmpfs_create_node(struct device *dev) { return 0; }
 static inline int devtmpfs_delete_node(struct device *dev) { return 0; }
 #endif
+
+bool driver_allowed(struct device_driver *drv);
+bool driver_filter_enabled(void);
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 1f6b4bd61056..85fb4063459f 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -583,6 +583,28 @@ static ssize_t uevent_store(struct device_driver *drv, const char *buf,
 }
 static DRIVER_ATTR_WO(uevent);
 
+static ssize_t allowed_store(struct device_driver *drv, const char *buf,
+			     size_t count)
+{
+	int ret;
+	bool status;
+
+	ret = kstrtobool(buf, &status);
+	if (ret)
+		return ret;
+
+	drv->p->allowed = status;
+
+	return count;
+}
+
+static ssize_t allowed_show(struct device_driver *drv, char *buf)
+{
+	return sysfs_emit(buf, "%s:%s %d\n", drv->bus ? drv->bus->name : "NULL",
+			  drv->name, drv->p->allowed);
+}
+static DRIVER_ATTR_RW(allowed);
+
 /**
  * bus_add_driver - Add a driver to the bus.
  * @drv: driver.
@@ -607,6 +629,7 @@ int bus_add_driver(struct device_driver *drv)
 	klist_init(&priv->klist_devices, NULL, NULL);
 	priv->driver = drv;
 	drv->p = priv;
+	drv->p->allowed = driver_allowed(drv);
 	priv->kobj.kset = bus->p->drivers_kset;
 	error = kobject_init_and_add(&priv->kobj, &driver_ktype, NULL,
 				     "%s", drv->name);
@@ -626,6 +649,15 @@ int bus_add_driver(struct device_driver *drv)
 		printk(KERN_ERR "%s: uevent attr (%s) failed\n",
 			__func__, drv->name);
 	}
+
+	if (driver_filter_enabled()) {
+		error = driver_create_file(drv, &driver_attr_allowed);
+		if (error) {
+			printk(KERN_ERR "%s: allowed attr (%s) failed\n",
+			       __func__, drv->name);
+		}
+	}
+
 	error = driver_add_groups(drv, bus->drv_groups);
 	if (error) {
 		/* How the hell do we get out of this pickle? Give up */
@@ -670,6 +702,8 @@ void bus_remove_driver(struct device_driver *drv)
 		remove_bind_files(drv);
 	driver_remove_groups(drv, drv->bus->drv_groups);
 	driver_remove_file(drv, &driver_attr_uevent);
+	if (driver_filter_enabled())
+		driver_remove_file(drv, &driver_attr_allowed);
 	klist_remove(&drv->p->knode_bus);
 	pr_debug("bus: '%s': remove driver %s\n", drv->bus->name, drv->name);
 	driver_detach(drv);
diff --git a/drivers/base/filter.c b/drivers/base/filter.c
new file mode 100644
index 000000000000..772e947ba933
--- /dev/null
+++ b/drivers/base/filter.c
@@ -0,0 +1,217 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * filter.c - Add driver filter framework.
+ *
+ * Implements APIs required for registering platform specific
+ * driver filter.
+ *
+ * Copyright (c) 2021 Intel Corporation
+ */
+
+#define pr_fmt(fmt) "filter: " fmt
+
+#include <linux/init.h>
+#include <linux/device/filter.h>
+#include <linux/acpi.h>
+#include <linux/protected_guest.h>
+
+#include "base.h"
+
+#define MAX_FILTER_NODES 10
+#define MAX_CMDLINE_LEN  500
+
+/* Buffer used by command line parser */
+static char allowed_drivers[MAX_CMDLINE_LEN];
+static char denied_drivers[MAX_CMDLINE_LEN];
+
+/* List of filter nodes for command line parser */
+static struct drv_filter_node allowed_nodes[MAX_FILTER_NODES];
+static struct drv_filter_node denied_nodes[MAX_FILTER_NODES];
+
+/* Driver allow list */
+static LIST_HEAD(driver_allow_list);
+/* Driver deny list */
+static LIST_HEAD(driver_deny_list);
+
+/* Protects driver_filter_list add/read operations*/
+static DEFINE_SPINLOCK(driver_filter_lock);
+
+/*
+ * Compares the driver name with given filter node.
+ *
+ * Return true if driver name matches with filter node.
+ */
+static bool match_driver(struct device_driver *drv,
+			 struct drv_filter_node *node)
+{
+	const char *n;
+	int len;
+
+	/* Make sure input entries are valid */
+	if (!drv || !node || !drv->bus)
+		return false;
+
+	/* If bus_name and drv_list matches "ALL", return true */
+	if (!strcmp(node->bus_name, "ALL") && !strcmp(node->drv_list, "ALL"))
+		return true;
+
+	/*
+	 * Since next step involves bus specific comparison, make
+	 * sure the bus name matches with filter node. If not
+	 * return false.
+	 */
+	if (strcmp(node->bus_name, drv->bus->name))
+		return false;
+
+	/* If allow list is "ALL", allow all */
+	if (!strcmp(node->drv_list, "ALL"))
+		return true;
+
+	for (n = node->drv_list; *n; n += len) {
+		if (*n == ',')
+			n++;
+		len = strcspn(n, ",");
+		if (!strncmp(drv->name, n, len) && drv->name[len] == 0)
+			return true;
+	}
+
+	return false;
+}
+
+/*
+ * driver_allowed() - Check whether given driver is allowed or not
+ *		      based on platform specific driver filter list.
+ */
+bool driver_allowed(struct device_driver *drv)
+{
+	bool status = true;
+	struct drv_filter_node *node;
+
+	spin_lock(&driver_filter_lock);
+
+	/*
+	 * Check whether the driver is allowed using platform
+	 * registered allow list.
+	 */
+	list_for_each_entry(node, &driver_allow_list, list) {
+		if (match_driver(drv, node)) {
+			status = true;
+			goto done;
+		}
+	}
+
+	/*
+	 * Check whether the driver is denied using platform
+	 * registered deny list.
+	 */
+	list_for_each_entry(node, &driver_deny_list, list) {
+		if (match_driver(drv, node)) {
+			status = false;
+			break;
+		}
+	}
+
+done:
+	pr_debug("bus:%s driver:%s %s\n", drv->bus ? drv->bus->name : "null",
+		 drv->name, status ? "allowed" : "denied");
+
+	spin_unlock(&driver_filter_lock);
+
+	return status;
+}
+
+bool driver_filter_enabled(void)
+{
+	return !list_empty(&driver_allow_list) ||
+		!list_empty(&driver_deny_list);
+}
+
+/*
+ * Helper function for filter node validity checks and
+ * adding filter node to allow or deny list.
+ */
+static int add_node_to_list(struct drv_filter_node *node,
+			    struct list_head *flist)
+{
+	/* If filter node is NULL, return error */
+	if (!node)
+		return -EINVAL;
+
+	/* Make sure node input is valid */
+	if (!node->bus_name || !node->drv_list)
+		return -EINVAL;
+
+	spin_lock(&driver_filter_lock);
+
+	list_add_tail(&node->list, flist);
+
+	spin_unlock(&driver_filter_lock);
+
+	return 0;
+}
+
+int register_filter_allow_node(struct drv_filter_node *node)
+{
+	return add_node_to_list(node, &driver_allow_list);
+}
+
+int register_filter_deny_node(struct drv_filter_node *node)
+{
+	return add_node_to_list(node, &driver_deny_list);
+}
+
+static __init void add_custom_driver_filter(char *p, bool allow)
+{
+	struct drv_filter_node *n;
+	int j = 0;
+	char *k;
+
+	while ((k = strsep(&p, ";")) != NULL) {
+		if (j >= MAX_FILTER_NODES) {
+			pr_err("Driver filter nodes exceed MAX_FILTER_NODES\n");
+			break;
+		}
+
+		if (allow)
+			n = &allowed_nodes[j++];
+		else
+			n = &denied_nodes[j++];
+
+		n->bus_name = strsep(&k, ":");
+
+		n->drv_list = p;
+
+		if (allow)
+			register_filter_allow_node(n);
+		else
+			register_filter_deny_node(n);
+	}
+}
+
+/* Command line option to update driver allow list */
+static int __init setup_allowed_drivers(char *buf)
+{
+	if (strlen(buf) >= MAX_CMDLINE_LEN)
+		pr_warn("Allowed list exceeds %d chars\n", MAX_CMDLINE_LEN);
+
+	strscpy(allowed_drivers, buf, MAX_CMDLINE_LEN);
+
+	add_custom_driver_filter(allowed_drivers, 1);
+
+	return 0;
+}
+__setup("filter_allow_drivers=", setup_allowed_drivers);
+
+/* Command line option to update driver deny list */
+static int __init setup_denied_drivers(char *buf)
+{
+	if (strlen(buf) >= MAX_CMDLINE_LEN)
+		pr_warn("Allowed list exceeds %d chars\n", MAX_CMDLINE_LEN);
+
+	strscpy(denied_drivers, buf, MAX_CMDLINE_LEN);
+
+	add_custom_driver_filter(denied_drivers, 0);
+
+	return 0;
+}
+__setup("filter_deny_drivers=", setup_denied_drivers);
diff --git a/include/linux/device/filter.h b/include/linux/device/filter.h
new file mode 100644
index 000000000000..a7510aa4642f
--- /dev/null
+++ b/include/linux/device/filter.h
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * filter.h - Driver filter specific header
+ *
+ * Copyright (c) 2021 Intel Corporation
+ *
+ */
+
+#ifndef _DEVICE_FILTER_H_
+#define _DEVICE_FILTER_H_
+
+#include <linux/device/bus.h>
+#include <linux/device/driver.h>
+#include <linux/device.h>
+
+/**
+ * struct drv_filter_node - driver filter node
+ *
+ * @bus_name		: Name of the bus.
+ * @drv_list		: Driver names for allow or deny list.
+ *
+ * Passing ALL in bus_name and drv_list will allow or
+ * deny all drivers.
+ */
+struct drv_filter_node {
+	const char *bus_name;
+	const char *drv_list;
+	struct list_head list;
+};
+
+/* Register platform specific allow list */
+int register_filter_allow_node(struct drv_filter_node *node);
+/* Register platform specific deny list */
+int register_filter_deny_node(struct drv_filter_node *node);
+#endif
-- 
2.25.1


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-04 17:43 [PATCH v1] driver: base: Add driver filter support Kuppuswamy Sathyanarayanan
@ 2021-08-04 18:08 ` Matthew Wilcox
  2021-08-04 18:29   ` Kuppuswamy, Sathyanarayanan
  2021-08-04 18:45   ` Dan Williams
  2021-08-04 19:29 ` Greg Kroah-Hartman
  1 sibling, 2 replies; 44+ messages in thread
From: Matthew Wilcox @ 2021-08-04 18:08 UTC (permalink / raw)
  To: Kuppuswamy Sathyanarayanan
  Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Jonathan Corbet,
	Dan Williams, Andi Kleen, Kuppuswamy Sathyanarayanan,
	linux-kernel, linux-doc

On Wed, Aug 04, 2021 at 10:43:22AM -0700, Kuppuswamy Sathyanarayanan wrote:
> +/* Driver allow list */
> +static LIST_HEAD(driver_allow_list);
> +/* Driver deny list */
> +static LIST_HEAD(driver_deny_list);

Why use a doubly-linked-list here?  An allocating xarray should perform
much better and use less memory.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-04 18:08 ` Matthew Wilcox
@ 2021-08-04 18:29   ` Kuppuswamy, Sathyanarayanan
  2021-08-04 18:33     ` Matthew Wilcox
  2021-08-04 19:27     ` Andi Kleen
  2021-08-04 18:45   ` Dan Williams
  1 sibling, 2 replies; 44+ messages in thread
From: Kuppuswamy, Sathyanarayanan @ 2021-08-04 18:29 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Jonathan Corbet,
	Dan Williams, Andi Kleen, Kuppuswamy Sathyanarayanan,
	linux-kernel, linux-doc



On 8/4/21 11:08 AM, Matthew Wilcox wrote:
> Why use a doubly-linked-list here?  An allocating xarray should perform
> much better and use less memory.

We don't expect the list to be too long. So we may not gain any significant
advantage in terms of performance or memory when using alternate lists. Since
linked list easier to use, we chose it.

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-04 18:29   ` Kuppuswamy, Sathyanarayanan
@ 2021-08-04 18:33     ` Matthew Wilcox
  2021-08-04 19:27     ` Andi Kleen
  1 sibling, 0 replies; 44+ messages in thread
From: Matthew Wilcox @ 2021-08-04 18:33 UTC (permalink / raw)
  To: Kuppuswamy, Sathyanarayanan
  Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Jonathan Corbet,
	Dan Williams, Andi Kleen, Kuppuswamy Sathyanarayanan,
	linux-kernel, linux-doc

On Wed, Aug 04, 2021 at 11:29:31AM -0700, Kuppuswamy, Sathyanarayanan wrote:
> 
> 
> On 8/4/21 11:08 AM, Matthew Wilcox wrote:
> > Why use a doubly-linked-list here?  An allocating xarray should perform
> > much better and use less memory.
> 
> We don't expect the list to be too long. So we may not gain any significant
> advantage in terms of performance or memory when using alternate lists. Since
> linked list easier to use, we chose it.

It is not easier to use.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-04 18:08 ` Matthew Wilcox
  2021-08-04 18:29   ` Kuppuswamy, Sathyanarayanan
@ 2021-08-04 18:45   ` Dan Williams
  1 sibling, 0 replies; 44+ messages in thread
From: Dan Williams @ 2021-08-04 18:45 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Kuppuswamy Sathyanarayanan, Greg Kroah-Hartman,
	Rafael J . Wysocki, Jonathan Corbet, Andi Kleen,
	Kuppuswamy Sathyanarayanan, Linux Kernel Mailing List,
	Linux Doc Mailing List

On Wed, Aug 4, 2021 at 11:09 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Wed, Aug 04, 2021 at 10:43:22AM -0700, Kuppuswamy Sathyanarayanan wrote:
> > +/* Driver allow list */
> > +static LIST_HEAD(driver_allow_list);
> > +/* Driver deny list */
> > +static LIST_HEAD(driver_deny_list);
>
> Why use a doubly-linked-list here?  An allocating xarray should perform
> much better and use less memory.

Sounds reasonable to me.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-04 18:29   ` Kuppuswamy, Sathyanarayanan
  2021-08-04 18:33     ` Matthew Wilcox
@ 2021-08-04 19:27     ` Andi Kleen
  1 sibling, 0 replies; 44+ messages in thread
From: Andi Kleen @ 2021-08-04 19:27 UTC (permalink / raw)
  To: Kuppuswamy, Sathyanarayanan, Matthew Wilcox
  Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Jonathan Corbet,
	Dan Williams, Kuppuswamy Sathyanarayanan, linux-kernel,
	linux-doc


On 8/4/2021 11:29 AM, Kuppuswamy, Sathyanarayanan wrote:
>
>
> On 8/4/21 11:08 AM, Matthew Wilcox wrote:
>> Why use a doubly-linked-list here?  An allocating xarray should perform
>> much better and use less memory.
>
> We don't expect the list to be too long. So we may not gain any 
> significant
> advantage in terms of performance or memory when using alternate 
> lists. Since
> linked list easier to use, we chose it.
>
Also even if it was long it wouldn't matter because this isn't a fast 
path at all.

All that matters it to write the code as clearly as possible.

-Andi


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-04 17:43 [PATCH v1] driver: base: Add driver filter support Kuppuswamy Sathyanarayanan
  2021-08-04 18:08 ` Matthew Wilcox
@ 2021-08-04 19:29 ` Greg Kroah-Hartman
  2021-08-04 19:50   ` Andi Kleen
  2021-08-04 20:11   ` Dan Williams
  1 sibling, 2 replies; 44+ messages in thread
From: Greg Kroah-Hartman @ 2021-08-04 19:29 UTC (permalink / raw)
  To: Kuppuswamy Sathyanarayanan
  Cc: Rafael J . Wysocki, Jonathan Corbet, Dan Williams, Andi Kleen,
	Kuppuswamy Sathyanarayanan, linux-kernel, linux-doc

On Wed, Aug 04, 2021 at 10:43:22AM -0700, Kuppuswamy Sathyanarayanan wrote:
> Intel's Trust Domain Extensions (TDX) is a new Intel technology that
> adds support for VMs to maintain confidentiality and integrity in the
> presence of an untrusted VMM.
> 
> Given the VMM is an untrusted entity and the VMM presents emulated
> hardware to the guest, a threat vector similar to Thunderclap [1] is
> present. Also, for ease of deployment, it is useful to be able to use
> the same kernel binary on host and guest, but that presents a wide
> attack surface given the range of hardware supported in typical
> builds. However, TDX guests only require a small number of drivers, a
> number small enough to audit for Thunderclap style attacks, and the
> rest can be disabled via policy. Add a mechanism to filter drivers
> based on a "protected-guest trusted" indication.

So you are trying to work around the "problem" that distro kernels
provides drivers for everything by adding additional kernel complexity?

What prevents you from using a sane, stripped down, kernel image for
these vms which would keep things sane and simpler and easier to audit
and most importantly, prove, that the image is not doing something you
don't expect it to do?

Why not just make distros that want to support this type of platform,
also provide these tiny kernel images?  Why are you pushing this work on
the kernel community instead?

> An alternative solution is to disable untrusted drivers using a custom
> kernel config for TDX, but such solution will break our goal of using
> same kernel binary in guest/host or in standard OS distributions. So
> it is not considered.

Why is that a goal that you _have_ to have?  Who is making that
requirement?

> Driver filter framework adds support to filter drivers at boot
> time by using platform specific allow list. This is a generic
> solution that can be reused by TDX. Driver filter framework adds a
> new hook (driver_allowed()) in driver_register() interface which
> will update the "allowed" status of the driver based on platform
> specific driver allow list or deny list. During driver bind process,
> trusted status is used to determine whether to continue or deny the
> bind process. If platform does not register any allow or deny list,
> all devices will be allowed. Platforms can use wildcard like "ALL:ALL"
> in bus_name and driver_name of filter node to allow or deny all
> bus and drivers.
> 
> Per driver opt-in model is also considered as an alternative design
> choice, but central allow or deny list is chosen because it is
> easier to maintain and audit. Also, "driver self serve" might be
> abused by mistake by driver authors cutting and pasting the code.
> 
> Also add an option in kernel command line and sysfs to update the
> allowed or denied drivers list. Driver filter allowed status
> can be read using following command.
> 
> cat /sys/bus/$bus/drivers/$driver/allowed

You added a sysfs file without Documentation/ABI/ update as well?

{sigh}

And what's wrong with the current method of removing drivers from
devices that you do not want them to be bound to?  We offer that support
for all busses now that want to do it, what driver types are you needing
to "control" here that does not take advantage of the existing
infrastructure that we currently have for this type of thing?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-04 19:29 ` Greg Kroah-Hartman
@ 2021-08-04 19:50   ` Andi Kleen
  2021-08-04 20:09     ` Kuppuswamy, Sathyanarayanan
  2021-08-05  7:49     ` Greg Kroah-Hartman
  2021-08-04 20:11   ` Dan Williams
  1 sibling, 2 replies; 44+ messages in thread
From: Andi Kleen @ 2021-08-04 19:50 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Kuppuswamy Sathyanarayanan
  Cc: Rafael J . Wysocki, Jonathan Corbet, Dan Williams,
	Kuppuswamy Sathyanarayanan, linux-kernel, linux-doc


> So you are trying to work around the "problem" that distro kernels
> provides drivers for everything by adding additional kernel complexity?
>
> What prevents you from using a sane, stripped down, kernel image for
> these vms which would keep things sane and simpler and easier to audit
> and most importantly, prove, that the image is not doing something you
> don't expect it to do?
>
> Why not just make distros that want to support this type of platform,
> also provide these tiny kernel images?  Why are you pushing this work on
> the kernel community instead?


Greg, I'm surprised by your comment. Traditionally we've been tried to 
support all platforms in unified binary kernels and gone to considerable 
complications to do so. That has been standard Linux practice for at 
least the 90ies. In some cases we went to considerable pain to support 
that, for example for the 5 level page tables or for paravirt ops.

Imagine there were 10 new features or platforms and they would all ask 
distributions to produce custom kernels for them, they would need to 
maintain 10 different kernel packages forever for all these different 
cases. It's totally reasonable that they don't want to do that.

Also even if they were willing to do custom configs it's not clear how 
this could be maintained and distributed. We would either have a 
standard TDX config and get everyone to agree on it (very difficult). Or 
some enforcement mechanism at the Kconfig level that forces most drivers 
to be disabled when TDX is on, which would be also a considerable new 
mechanism and complication. In addition there are drivers which are not 
covered by Kconfig today (like quite a few of the basic platform 
drivers), but which we still want to filter. So to implement a full 
build time mechanism would likely need more changes than this relatively 
straight forward run time mechanism based on the driver model.

And of course there other use cases for a run time filter mechanism. For 
example let's say you want filtering for Thunderbolt security. In this 
case it has to be done at runtime because it's not practical to have a 
kernel that is only built for Thunderbolt drivers, but doesn't support 
anything else that is on the SOC. The only sane way to handle such a 
case is to make a runtime distinction.

> And what's wrong with the current method of removing drivers from
> devices that you do not want them to be bound to?  We offer that support
> for all busses now that want to do it, what driver types are you needing
> to "control" here that does not take advantage of the existing
> infrastructure that we currently have for this type of thing?

I'm not sure what mechanism you're referring to here, but in general 
don't want the drivers to initialize at all because they might get 
exploited in any code that they execute.The intention is to disable all 
drivers except for a small allow list, because it's not practical to 
harden all 25M lines of Linux code.

-Andi


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-04 19:50   ` Andi Kleen
@ 2021-08-04 20:09     ` Kuppuswamy, Sathyanarayanan
  2021-08-05  7:50       ` Greg Kroah-Hartman
  2021-08-05  7:49     ` Greg Kroah-Hartman
  1 sibling, 1 reply; 44+ messages in thread
From: Kuppuswamy, Sathyanarayanan @ 2021-08-04 20:09 UTC (permalink / raw)
  To: Andi Kleen, Greg Kroah-Hartman
  Cc: Rafael J . Wysocki, Jonathan Corbet, Dan Williams,
	Kuppuswamy Sathyanarayanan, linux-kernel, linux-doc



On 8/4/21 12:50 PM, Andi Kleen wrote:
> 
>> And what's wrong with the current method of removing drivers from
>> devices that you do not want them to be bound to?  We offer that support
>> for all busses now that want to do it, what driver types are you needing
>> to "control" here that does not take advantage of the existing
>> infrastructure that we currently have for this type of thing?
> 
> I'm not sure what mechanism you're referring to here, but in general don't want the drivers to 
> initialize at all because they might get exploited in any code that they execute.The intention is to 
> disable all drivers except for a small allow list, because it's not practical to harden all 25M 
> lines of Linux code.

Yes, we are not trying to remove the drivers via sysfs. If driver
filter is enabled, "allowed" sysfs file is used to view the driver
filter status (allowed/denied). And a write to that file changes
the allowed/denied status of the driver. It has nothing to do
with bind/unbind operations.

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-04 19:29 ` Greg Kroah-Hartman
  2021-08-04 19:50   ` Andi Kleen
@ 2021-08-04 20:11   ` Dan Williams
  2021-08-04 20:29     ` Kuppuswamy, Sathyanarayanan
  2021-08-04 21:07     ` Matthew Wilcox
  1 sibling, 2 replies; 44+ messages in thread
From: Dan Williams @ 2021-08-04 20:11 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Kuppuswamy Sathyanarayanan, Rafael J . Wysocki, Jonathan Corbet,
	Andi Kleen, Kuppuswamy Sathyanarayanan,
	Linux Kernel Mailing List, Linux Doc Mailing List

On Wed, Aug 4, 2021 at 12:29 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Aug 04, 2021 at 10:43:22AM -0700, Kuppuswamy Sathyanarayanan wrote:
> > Intel's Trust Domain Extensions (TDX) is a new Intel technology that
> > adds support for VMs to maintain confidentiality and integrity in the
> > presence of an untrusted VMM.
> >
> > Given the VMM is an untrusted entity and the VMM presents emulated
> > hardware to the guest, a threat vector similar to Thunderclap [1] is
> > present. Also, for ease of deployment, it is useful to be able to use
> > the same kernel binary on host and guest, but that presents a wide
> > attack surface given the range of hardware supported in typical
> > builds. However, TDX guests only require a small number of drivers, a
> > number small enough to audit for Thunderclap style attacks, and the
> > rest can be disabled via policy. Add a mechanism to filter drivers
> > based on a "protected-guest trusted" indication.
>
> So you are trying to work around the "problem" that distro kernels
> provides drivers for everything by adding additional kernel complexity?
>
> What prevents you from using a sane, stripped down, kernel image for
> these vms which would keep things sane and simpler and easier to audit
> and most importantly, prove, that the image is not doing something you
> don't expect it to do?
>
> Why not just make distros that want to support this type of platform,
> also provide these tiny kernel images?  Why are you pushing this work on
> the kernel community instead?

In fact, these questions are where I started when first encountering
this proposal. Andi has addressed the single kernel image constraint,
but I want to pick up on this "pushing work to the kernel community"
contention. The small list of vetted drivers that a TDX guest needs
will be built-in and maintained in the kernel by the protected guest
developer community, so no "pushing work" there. However, given that
any driver disable mechanism needs to touch the driver core I
advocated to go ahead and make this a general purpose capability to
pick up where this [1] conversation left off. I.e. a general facility
for the corner cases that modprobe and kernel config policy can not
reach. Corner cases like VMM attacking the VM, or broken hardware with
a built-in driver that can't be unbound after the fact.

[1]: https://lore.kernel.org/lkml/20170928090901.GC12599@kroah.com/

>
> > An alternative solution is to disable untrusted drivers using a custom
> > kernel config for TDX, but such solution will break our goal of using
> > same kernel binary in guest/host or in standard OS distributions. So
> > it is not considered.
>
> Why is that a goal that you _have_ to have?  Who is making that
> requirement?
>
> > Driver filter framework adds support to filter drivers at boot
> > time by using platform specific allow list. This is a generic
> > solution that can be reused by TDX. Driver filter framework adds a
> > new hook (driver_allowed()) in driver_register() interface which
> > will update the "allowed" status of the driver based on platform
> > specific driver allow list or deny list. During driver bind process,
> > trusted status is used to determine whether to continue or deny the
> > bind process. If platform does not register any allow or deny list,
> > all devices will be allowed. Platforms can use wildcard like "ALL:ALL"
> > in bus_name and driver_name of filter node to allow or deny all
> > bus and drivers.
> >
> > Per driver opt-in model is also considered as an alternative design
> > choice, but central allow or deny list is chosen because it is
> > easier to maintain and audit. Also, "driver self serve" might be
> > abused by mistake by driver authors cutting and pasting the code.
> >
> > Also add an option in kernel command line and sysfs to update the
> > allowed or denied drivers list. Driver filter allowed status
> > can be read using following command.
> >
> > cat /sys/bus/$bus/drivers/$driver/allowed
>
> You added a sysfs file without Documentation/ABI/ update as well?
>
> {sigh}

Argh, my fault, that one slipped through in the internal review.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-04 20:11   ` Dan Williams
@ 2021-08-04 20:29     ` Kuppuswamy, Sathyanarayanan
  2021-08-04 21:07     ` Matthew Wilcox
  1 sibling, 0 replies; 44+ messages in thread
From: Kuppuswamy, Sathyanarayanan @ 2021-08-04 20:29 UTC (permalink / raw)
  To: Dan Williams, Greg Kroah-Hartman
  Cc: Rafael J . Wysocki, Jonathan Corbet, Andi Kleen,
	Kuppuswamy Sathyanarayanan, Linux Kernel Mailing List,
	Linux Doc Mailing List



On 8/4/21 1:11 PM, Dan Williams wrote:
>> You added a sysfs file without Documentation/ABI/ update as well?
>>
>> {sigh}
> Argh, my fault, that one slipped through in the internal review.

Sorry, I will add this in next version.

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-04 20:11   ` Dan Williams
  2021-08-04 20:29     ` Kuppuswamy, Sathyanarayanan
@ 2021-08-04 21:07     ` Matthew Wilcox
  2021-08-04 21:28       ` Dan Williams
  1 sibling, 1 reply; 44+ messages in thread
From: Matthew Wilcox @ 2021-08-04 21:07 UTC (permalink / raw)
  To: Dan Williams
  Cc: Greg Kroah-Hartman, Kuppuswamy Sathyanarayanan,
	Rafael J . Wysocki, Jonathan Corbet, Andi Kleen,
	Kuppuswamy Sathyanarayanan, Linux Kernel Mailing List,
	Linux Doc Mailing List

On Wed, Aug 04, 2021 at 01:11:27PM -0700, Dan Williams wrote:
> On Wed, Aug 4, 2021 at 12:29 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > Why not just make distros that want to support this type of platform,
> > also provide these tiny kernel images?  Why are you pushing this work on
> > the kernel community instead?
> 
> In fact, these questions are where I started when first encountering
> this proposal. Andi has addressed the single kernel image constraint,
> but I want to pick up on this "pushing work to the kernel community"
> contention. The small list of vetted drivers that a TDX guest needs
> will be built-in and maintained in the kernel by the protected guest
> developer community, so no "pushing work" there. However, given that
> any driver disable mechanism needs to touch the driver core I
> advocated to go ahead and make this a general purpose capability to
> pick up where this [1] conversation left off. I.e. a general facility
> for the corner cases that modprobe and kernel config policy can not
> reach. Corner cases like VMM attacking the VM, or broken hardware with
> a built-in driver that can't be unbound after the fact.

I don't understand how this defends against a hypervisor attacking a
guest.  If the hardware exists, the hypervisor can access it, regardless
of whether the driver is default-disabled by configuration.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-04 21:07     ` Matthew Wilcox
@ 2021-08-04 21:28       ` Dan Williams
  2021-08-05  4:45         ` Andi Kleen
  2021-08-05  7:59         ` Greg Kroah-Hartman
  0 siblings, 2 replies; 44+ messages in thread
From: Dan Williams @ 2021-08-04 21:28 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Greg Kroah-Hartman, Kuppuswamy Sathyanarayanan,
	Rafael J . Wysocki, Jonathan Corbet, Andi Kleen,
	Kuppuswamy Sathyanarayanan, Linux Kernel Mailing List,
	Linux Doc Mailing List

On Wed, Aug 4, 2021 at 2:07 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Wed, Aug 04, 2021 at 01:11:27PM -0700, Dan Williams wrote:
> > On Wed, Aug 4, 2021 at 12:29 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > > Why not just make distros that want to support this type of platform,
> > > also provide these tiny kernel images?  Why are you pushing this work on
> > > the kernel community instead?
> >
> > In fact, these questions are where I started when first encountering
> > this proposal. Andi has addressed the single kernel image constraint,
> > but I want to pick up on this "pushing work to the kernel community"
> > contention. The small list of vetted drivers that a TDX guest needs
> > will be built-in and maintained in the kernel by the protected guest
> > developer community, so no "pushing work" there. However, given that
> > any driver disable mechanism needs to touch the driver core I
> > advocated to go ahead and make this a general purpose capability to
> > pick up where this [1] conversation left off. I.e. a general facility
> > for the corner cases that modprobe and kernel config policy can not
> > reach. Corner cases like VMM attacking the VM, or broken hardware with
> > a built-in driver that can't be unbound after the fact.
>
> I don't understand how this defends against a hypervisor attacking a
> guest.  If the hardware exists, the hypervisor can access it, regardless
> of whether the driver is default-disabled by configuration.

The "hardware" in this case is virtual devices presented by the VMM to
the VM. So if a driver misbehaves in a useful way for an attacker to
exploit, they can stimulate that behavior with a custom crafted
virtual device, and that driver will autoload unaware of the threat
without this filter for vetted drivers.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-04 21:28       ` Dan Williams
@ 2021-08-05  4:45         ` Andi Kleen
  2021-08-05  7:59         ` Greg Kroah-Hartman
  1 sibling, 0 replies; 44+ messages in thread
From: Andi Kleen @ 2021-08-05  4:45 UTC (permalink / raw)
  To: Dan Williams, Matthew Wilcox
  Cc: Greg Kroah-Hartman, Kuppuswamy Sathyanarayanan,
	Rafael J . Wysocki, Jonathan Corbet, Kuppuswamy Sathyanarayanan,
	Linux Kernel Mailing List, Linux Doc Mailing List


On 8/4/2021 2:28 PM, Dan Williams wrote
> The "hardware" in this case is virtual devices presented by the VMM to
> the VM. So if a driver misbehaves in a useful way for an attacker to
> exploit, they can stimulate that behavior with a custom crafted
> virtual device, and that driver will autoload unaware of the threat
> without this filter for vetted drivers.

Another way to see it is: the confidential guest is protected against 
the host, except for the places where it chooses to communicate with the 
host through MMIOs, port IOs, some (not all) MSRs. It's somewhat 
analogous to a network server in a hostile network which can be attacked 
through network packets. We typically use a firewall to limit the 
network exposure only to especially hardened network services. Each low 
level MMIO etc. is like a network access communicating with a hostile 
network. The device filter is the firewall for these vulnerable low 
level interactions. It reduces the hardening problem from being 
completely infeasible to tractable.

-Andi




^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-04 19:50   ` Andi Kleen
  2021-08-04 20:09     ` Kuppuswamy, Sathyanarayanan
@ 2021-08-05  7:49     ` Greg Kroah-Hartman
  2021-08-05  7:55       ` Greg Kroah-Hartman
  1 sibling, 1 reply; 44+ messages in thread
From: Greg Kroah-Hartman @ 2021-08-05  7:49 UTC (permalink / raw)
  To: Andi Kleen
  Cc: Kuppuswamy Sathyanarayanan, Rafael J . Wysocki, Jonathan Corbet,
	Dan Williams, Kuppuswamy Sathyanarayanan, linux-kernel,
	linux-doc

On Wed, Aug 04, 2021 at 12:50:24PM -0700, Andi Kleen wrote:
> 
> > So you are trying to work around the "problem" that distro kernels
> > provides drivers for everything by adding additional kernel complexity?
> > 
> > What prevents you from using a sane, stripped down, kernel image for
> > these vms which would keep things sane and simpler and easier to audit
> > and most importantly, prove, that the image is not doing something you
> > don't expect it to do?
> > 
> > Why not just make distros that want to support this type of platform,
> > also provide these tiny kernel images?  Why are you pushing this work on
> > the kernel community instead?
> 
> 
> Greg, I'm surprised by your comment. Traditionally we've been tried to
> support all platforms in unified binary kernels and gone to considerable
> complications to do so. That has been standard Linux practice for at least
> the 90ies. In some cases we went to considerable pain to support that, for
> example for the 5 level page tables or for paravirt ops.
> 
> Imagine there were 10 new features or platforms and they would all ask
> distributions to produce custom kernels for them, they would need to
> maintain 10 different kernel packages forever for all these different cases.
> It's totally reasonable that they don't want to do that.

Ok, but that's not what it sounded like you wanted to have here.

It looked like you wanted something like a "stripped down kernel with
only a specific number of drivers allowed to work", which for a virtual
system, would seem to imply a uniform kernel image/configuration.

> Also even if they were willing to do custom configs it's not clear how this
> could be maintained and distributed. We would either have a standard TDX
> config and get everyone to agree on it (very difficult).

Why not try that?  Can't hurt and no need to change anything in the
kernel at all.  'make tdx_defconfig' should be trivial enough for you
all to maintain a single config file template, right?

> Or some enforcement
> mechanism at the Kconfig level that forces most drivers to be disabled when
> TDX is on, which would be also a considerable new mechanism and
> complication. In addition there are drivers which are not covered by Kconfig
> today (like quite a few of the basic platform drivers), but which we still
> want to filter. So to implement a full build time mechanism would likely
> need more changes than this relatively straight forward run time mechanism
> based on the driver model.
> 
> And of course there other use cases for a run time filter mechanism. For
> example let's say you want filtering for Thunderbolt security. In this case
> it has to be done at runtime because it's not practical to have a kernel
> that is only built for Thunderbolt drivers, but doesn't support anything
> else that is on the SOC. The only sane way to handle such a case is to make
> a runtime distinction.

We already have filtering for thunderbolt driver security in the kernel,
why do you want to add more?

> > And what's wrong with the current method of removing drivers from
> > devices that you do not want them to be bound to?  We offer that support
> > for all busses now that want to do it, what driver types are you needing
> > to "control" here that does not take advantage of the existing
> > infrastructure that we currently have for this type of thing?
> 
> I'm not sure what mechanism you're referring to here, but in general don't
> want the drivers to initialize at all because they might get exploited in
> any code that they execute.

That is exactly the mechanism we have today in the kernel for all busses
if they wish to take advantage of it.  We have had this for all USB
drivers for well over a decade now, this is not a new feature.  Please
use that instead.

> The intention is to disable all drivers except
> for a small allow list, because it's not practical to harden all 25M lines
> of Linux code.

Great, do that in userspace using the functionality we have today
please.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-04 20:09     ` Kuppuswamy, Sathyanarayanan
@ 2021-08-05  7:50       ` Greg Kroah-Hartman
  0 siblings, 0 replies; 44+ messages in thread
From: Greg Kroah-Hartman @ 2021-08-05  7:50 UTC (permalink / raw)
  To: Kuppuswamy, Sathyanarayanan
  Cc: Andi Kleen, Rafael J . Wysocki, Jonathan Corbet, Dan Williams,
	Kuppuswamy Sathyanarayanan, linux-kernel, linux-doc

On Wed, Aug 04, 2021 at 01:09:07PM -0700, Kuppuswamy, Sathyanarayanan wrote:
> 
> 
> On 8/4/21 12:50 PM, Andi Kleen wrote:
> > 
> > > And what's wrong with the current method of removing drivers from
> > > devices that you do not want them to be bound to?  We offer that support
> > > for all busses now that want to do it, what driver types are you needing
> > > to "control" here that does not take advantage of the existing
> > > infrastructure that we currently have for this type of thing?
> > 
> > I'm not sure what mechanism you're referring to here, but in general
> > don't want the drivers to initialize at all because they might get
> > exploited in any code that they execute.The intention is to disable all
> > drivers except for a small allow list, because it's not practical to
> > harden all 25M lines of Linux code.
> 
> Yes, we are not trying to remove the drivers via sysfs. If driver
> filter is enabled, "allowed" sysfs file is used to view the driver
> filter status (allowed/denied). And a write to that file changes
> the allowed/denied status of the driver. It has nothing to do
> with bind/unbind operations.

Again, we have this already today, with full sysfs control in userspace.
Why add yet-another-way to do this?  What is lacking in the existing
functionality that needs to be expanded on?

greg k-h

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-05  7:49     ` Greg Kroah-Hartman
@ 2021-08-05  7:55       ` Greg Kroah-Hartman
  2021-08-05  7:58         ` Greg Kroah-Hartman
  0 siblings, 1 reply; 44+ messages in thread
From: Greg Kroah-Hartman @ 2021-08-05  7:55 UTC (permalink / raw)
  To: Andi Kleen
  Cc: Kuppuswamy Sathyanarayanan, Rafael J . Wysocki, Jonathan Corbet,
	Dan Williams, Kuppuswamy Sathyanarayanan, linux-kernel,
	linux-doc

On Thu, Aug 05, 2021 at 09:49:29AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Aug 04, 2021 at 12:50:24PM -0700, Andi Kleen wrote:
> > > And what's wrong with the current method of removing drivers from
> > > devices that you do not want them to be bound to?  We offer that support
> > > for all busses now that want to do it, what driver types are you needing
> > > to "control" here that does not take advantage of the existing
> > > infrastructure that we currently have for this type of thing?
> > 
> > I'm not sure what mechanism you're referring to here, but in general don't
> > want the drivers to initialize at all because they might get exploited in
> > any code that they execute.
> 
> That is exactly the mechanism we have today in the kernel for all busses
> if they wish to take advantage of it.  We have had this for all USB
> drivers for well over a decade now, this is not a new feature.  Please
> use that instead.

Hm, wait, maybe that didn't get merged yet, let me dig...


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-05  7:55       ` Greg Kroah-Hartman
@ 2021-08-05  7:58         ` Greg Kroah-Hartman
  2021-08-05 13:52           ` Andi Kleen
  2021-08-05 16:37           ` Dan Williams
  0 siblings, 2 replies; 44+ messages in thread
From: Greg Kroah-Hartman @ 2021-08-05  7:58 UTC (permalink / raw)
  To: Andi Kleen
  Cc: Kuppuswamy Sathyanarayanan, Rafael J . Wysocki, Jonathan Corbet,
	Dan Williams, Kuppuswamy Sathyanarayanan, linux-kernel,
	linux-doc

On Thu, Aug 05, 2021 at 09:55:33AM +0200, Greg Kroah-Hartman wrote:
> On Thu, Aug 05, 2021 at 09:49:29AM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Aug 04, 2021 at 12:50:24PM -0700, Andi Kleen wrote:
> > > > And what's wrong with the current method of removing drivers from
> > > > devices that you do not want them to be bound to?  We offer that support
> > > > for all busses now that want to do it, what driver types are you needing
> > > > to "control" here that does not take advantage of the existing
> > > > infrastructure that we currently have for this type of thing?
> > > 
> > > I'm not sure what mechanism you're referring to here, but in general don't
> > > want the drivers to initialize at all because they might get exploited in
> > > any code that they execute.
> > 
> > That is exactly the mechanism we have today in the kernel for all busses
> > if they wish to take advantage of it.  We have had this for all USB
> > drivers for well over a decade now, this is not a new feature.  Please
> > use that instead.
> 
> Hm, wait, maybe that didn't get merged yet, let me dig...
> 

Ok, my fault, I was thinking of the generic "removable" support that
recently got added.

Both thunderbolt and USB have the idea of "authorized" devices, that is
the logic that should be made generic and available for all busses to
use, by moving it to the driver core, just like the "removable" logic
got moved to the driver core recently (see 70f400d4d957 ("driver core:
Move the "removable" attribute from USB to core")

Please use that type of interface, as we already have userspace tools
using it, and expand it for all busses in the system to use if they
want.  Otherwise with this proposal you will end up with multiple ways
to control the same bus type with different types of "filtering",
ensuring a mess.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-04 21:28       ` Dan Williams
  2021-08-05  4:45         ` Andi Kleen
@ 2021-08-05  7:59         ` Greg Kroah-Hartman
  1 sibling, 0 replies; 44+ messages in thread
From: Greg Kroah-Hartman @ 2021-08-05  7:59 UTC (permalink / raw)
  To: Dan Williams
  Cc: Matthew Wilcox, Kuppuswamy Sathyanarayanan, Rafael J . Wysocki,
	Jonathan Corbet, Andi Kleen, Kuppuswamy Sathyanarayanan,
	Linux Kernel Mailing List, Linux Doc Mailing List

On Wed, Aug 04, 2021 at 02:28:32PM -0700, Dan Williams wrote:
> On Wed, Aug 4, 2021 at 2:07 PM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Wed, Aug 04, 2021 at 01:11:27PM -0700, Dan Williams wrote:
> > > On Wed, Aug 4, 2021 at 12:29 PM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > > Why not just make distros that want to support this type of platform,
> > > > also provide these tiny kernel images?  Why are you pushing this work on
> > > > the kernel community instead?
> > >
> > > In fact, these questions are where I started when first encountering
> > > this proposal. Andi has addressed the single kernel image constraint,
> > > but I want to pick up on this "pushing work to the kernel community"
> > > contention. The small list of vetted drivers that a TDX guest needs
> > > will be built-in and maintained in the kernel by the protected guest
> > > developer community, so no "pushing work" there. However, given that
> > > any driver disable mechanism needs to touch the driver core I
> > > advocated to go ahead and make this a general purpose capability to
> > > pick up where this [1] conversation left off. I.e. a general facility
> > > for the corner cases that modprobe and kernel config policy can not
> > > reach. Corner cases like VMM attacking the VM, or broken hardware with
> > > a built-in driver that can't be unbound after the fact.
> >
> > I don't understand how this defends against a hypervisor attacking a
> > guest.  If the hardware exists, the hypervisor can access it, regardless
> > of whether the driver is default-disabled by configuration.
> 
> The "hardware" in this case is virtual devices presented by the VMM to
> the VM. So if a driver misbehaves in a useful way for an attacker to
> exploit, they can stimulate that behavior with a custom crafted
> virtual device, and that driver will autoload unaware of the threat
> without this filter for vetted drivers.

As I just said elsewhere in this thread, we have support for this today
for thunderbolt and USB, please just expand that to all bus types and
that should be fine.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-05  7:58         ` Greg Kroah-Hartman
@ 2021-08-05 13:52           ` Andi Kleen
  2021-08-05 17:51             ` Greg Kroah-Hartman
  2021-08-05 16:37           ` Dan Williams
  1 sibling, 1 reply; 44+ messages in thread
From: Andi Kleen @ 2021-08-05 13:52 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Kuppuswamy Sathyanarayanan, Rafael J . Wysocki, Jonathan Corbet,
	Dan Williams, Kuppuswamy Sathyanarayanan, linux-kernel,
	linux-doc


> Both thunderbolt and USB have the idea of "authorized" devices, that is
> the logic that should be made generic and available for all busses to
> use, by moving it to the driver core, just like the "removable" logic
> got moved to the driver core recently (see 70f400d4d957 ("driver core:
> Move the "removable" attribute from USB to core")

This looks like it's controlled by udev?  Have a default per bus, and 
let user space override it before setting up the device.

This doesn't help us handle builtin drivers that initialize before user 
space is up.

We need something that works for all drivers. Also cannot just use a 
default at bootup because some drivers (like virtio or rtc) need to be 
initialized in early boot to make the system functional at all. So you 
need a way to distinguish these two cases in the pre user space boot.

That's basically what this patch implements the infrastructure for.

>
> Please use that type of interface, as we already have userspace tools
> using it, and expand it for all busses in the system to use if they
> want.  Otherwise with this proposal you will end up with multiple ways
> to control the same bus type with different types of "filtering",
> ensuring a mess.

How would such a proposal work for a platform driver that doesn't have a 
bus?

-Andi


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-05  7:58         ` Greg Kroah-Hartman
  2021-08-05 13:52           ` Andi Kleen
@ 2021-08-05 16:37           ` Dan Williams
  2021-08-05 17:25             ` Kuppuswamy, Sathyanarayanan
  2021-08-05 17:49             ` Greg Kroah-Hartman
  1 sibling, 2 replies; 44+ messages in thread
From: Dan Williams @ 2021-08-05 16:37 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Andi Kleen, Kuppuswamy Sathyanarayanan, Rafael J . Wysocki,
	Jonathan Corbet, Kuppuswamy Sathyanarayanan,
	Linux Kernel Mailing List, Linux Doc Mailing List

On Thu, Aug 5, 2021 at 12:58 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Aug 05, 2021 at 09:55:33AM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Aug 05, 2021 at 09:49:29AM +0200, Greg Kroah-Hartman wrote:
> > > On Wed, Aug 04, 2021 at 12:50:24PM -0700, Andi Kleen wrote:
> > > > > And what's wrong with the current method of removing drivers from
> > > > > devices that you do not want them to be bound to?  We offer that support
> > > > > for all busses now that want to do it, what driver types are you needing
> > > > > to "control" here that does not take advantage of the existing
> > > > > infrastructure that we currently have for this type of thing?
> > > >
> > > > I'm not sure what mechanism you're referring to here, but in general don't
> > > > want the drivers to initialize at all because they might get exploited in
> > > > any code that they execute.
> > >
> > > That is exactly the mechanism we have today in the kernel for all busses
> > > if they wish to take advantage of it.  We have had this for all USB
> > > drivers for well over a decade now, this is not a new feature.  Please
> > > use that instead.
> >
> > Hm, wait, maybe that didn't get merged yet, let me dig...
> >
>
> Ok, my fault, I was thinking of the generic "removable" support that
> recently got added.
>
> Both thunderbolt and USB have the idea of "authorized" devices, that is
> the logic that should be made generic and available for all busses to
> use, by moving it to the driver core, just like the "removable" logic
> got moved to the driver core recently (see 70f400d4d957 ("driver core:
> Move the "removable" attribute from USB to core")
>
> Please use that type of interface, as we already have userspace tools
> using it, and expand it for all busses in the system to use if they
> want.  Otherwise with this proposal you will end up with multiple ways
> to control the same bus type with different types of "filtering",
> ensuring a mess.

I overlooked the "authorized" attribute in usb and thunderbolt. The
collision problem makes sense. Are you open to a core "authorized"
attribute that buses like usb and thunderbolt would override in favor
of their local implementation? I.e. similar to suppress_bind_attrs:

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index daeb9b5763ae..d1780f026d1a 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -511,6 +511,10 @@ static int call_driver_probe(struct device *dev,
struct device_driver *drv)
 {
        int ret = 0;

+       if (driver_core_auth_enabled && !dev->bus->suppress_authorized_attrs &&
+           !driver_core_authorized(dev))
+               return -ENODEV;
+
        if (dev->bus->probe)
                ret = dev->bus->probe(dev);
        else if (drv->probe)
diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c
index a062befcb3b2..e835be9bee4f 100644
--- a/drivers/thunderbolt/domain.c
+++ b/drivers/thunderbolt/domain.c
@@ -323,6 +323,7 @@ struct bus_type tb_bus_type = {
        .probe = tb_service_probe,
        .remove = tb_service_remove,
        .shutdown = tb_service_shutdown,
+       .suppress_authorized_attrs = true,
 };

 static void tb_domain_release(struct device *dev)
diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 072968c40ade..2cf9c3cc12b4 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -2028,4 +2028,5 @@ struct bus_type usb_bus_type = {
        .match =        usb_device_match,
        .uevent =       usb_uevent,
        .need_parent_lock =     true,
+       .suppress_authorized_attrs = true,
 };

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-05 16:37           ` Dan Williams
@ 2021-08-05 17:25             ` Kuppuswamy, Sathyanarayanan
  2021-08-05 17:48               ` Greg Kroah-Hartman
  2021-08-05 17:52               ` Andi Kleen
  2021-08-05 17:49             ` Greg Kroah-Hartman
  1 sibling, 2 replies; 44+ messages in thread
From: Kuppuswamy, Sathyanarayanan @ 2021-08-05 17:25 UTC (permalink / raw)
  To: Dan Williams, Greg Kroah-Hartman
  Cc: Andi Kleen, Rafael J . Wysocki, Jonathan Corbet,
	Kuppuswamy Sathyanarayanan, Linux Kernel Mailing List,
	Linux Doc Mailing List



On 8/5/21 9:37 AM, Dan Williams wrote:
> I overlooked the "authorized" attribute in usb and thunderbolt. The
> collision problem makes sense. Are you open to a core "authorized"
> attribute that buses like usb and thunderbolt would override in favor
> of their local implementation? I.e. similar to suppress_bind_attrs:

Even if such overriding is allowed in default boot, it should not be
allowed in protected guest + driver_filter model.

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-05 17:25             ` Kuppuswamy, Sathyanarayanan
@ 2021-08-05 17:48               ` Greg Kroah-Hartman
  2021-08-05 17:52               ` Andi Kleen
  1 sibling, 0 replies; 44+ messages in thread
From: Greg Kroah-Hartman @ 2021-08-05 17:48 UTC (permalink / raw)
  To: Kuppuswamy, Sathyanarayanan
  Cc: Dan Williams, Andi Kleen, Rafael J . Wysocki, Jonathan Corbet,
	Kuppuswamy Sathyanarayanan, Linux Kernel Mailing List,
	Linux Doc Mailing List

On Thu, Aug 05, 2021 at 10:25:32AM -0700, Kuppuswamy, Sathyanarayanan wrote:
> 
> 
> On 8/5/21 9:37 AM, Dan Williams wrote:
> > I overlooked the "authorized" attribute in usb and thunderbolt. The
> > collision problem makes sense. Are you open to a core "authorized"
> > attribute that buses like usb and thunderbolt would override in favor
> > of their local implementation? I.e. similar to suppress_bind_attrs:
> 
> Even if such overriding is allowed in default boot, it should not be
> allowed in protected guest + driver_filter model.

The kernel has no idea of "protected guest" at the moment so I do not
know what you mean here...

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-05 16:37           ` Dan Williams
  2021-08-05 17:25             ` Kuppuswamy, Sathyanarayanan
@ 2021-08-05 17:49             ` Greg Kroah-Hartman
  1 sibling, 0 replies; 44+ messages in thread
From: Greg Kroah-Hartman @ 2021-08-05 17:49 UTC (permalink / raw)
  To: Dan Williams
  Cc: Andi Kleen, Kuppuswamy Sathyanarayanan, Rafael J . Wysocki,
	Jonathan Corbet, Kuppuswamy Sathyanarayanan,
	Linux Kernel Mailing List, Linux Doc Mailing List

On Thu, Aug 05, 2021 at 09:37:28AM -0700, Dan Williams wrote:
> On Thu, Aug 5, 2021 at 12:58 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Aug 05, 2021 at 09:55:33AM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Aug 05, 2021 at 09:49:29AM +0200, Greg Kroah-Hartman wrote:
> > > > On Wed, Aug 04, 2021 at 12:50:24PM -0700, Andi Kleen wrote:
> > > > > > And what's wrong with the current method of removing drivers from
> > > > > > devices that you do not want them to be bound to?  We offer that support
> > > > > > for all busses now that want to do it, what driver types are you needing
> > > > > > to "control" here that does not take advantage of the existing
> > > > > > infrastructure that we currently have for this type of thing?
> > > > >
> > > > > I'm not sure what mechanism you're referring to here, but in general don't
> > > > > want the drivers to initialize at all because they might get exploited in
> > > > > any code that they execute.
> > > >
> > > > That is exactly the mechanism we have today in the kernel for all busses
> > > > if they wish to take advantage of it.  We have had this for all USB
> > > > drivers for well over a decade now, this is not a new feature.  Please
> > > > use that instead.
> > >
> > > Hm, wait, maybe that didn't get merged yet, let me dig...
> > >
> >
> > Ok, my fault, I was thinking of the generic "removable" support that
> > recently got added.
> >
> > Both thunderbolt and USB have the idea of "authorized" devices, that is
> > the logic that should be made generic and available for all busses to
> > use, by moving it to the driver core, just like the "removable" logic
> > got moved to the driver core recently (see 70f400d4d957 ("driver core:
> > Move the "removable" attribute from USB to core")
> >
> > Please use that type of interface, as we already have userspace tools
> > using it, and expand it for all busses in the system to use if they
> > want.  Otherwise with this proposal you will end up with multiple ways
> > to control the same bus type with different types of "filtering",
> > ensuring a mess.
> 
> I overlooked the "authorized" attribute in usb and thunderbolt. The
> collision problem makes sense. Are you open to a core "authorized"
> attribute that buses like usb and thunderbolt would override in favor
> of their local implementation? I.e. similar to suppress_bind_attrs:

What about doing it the other way around and making it generic like was
done for the "removable" attribute?  That way the logic from both
thunderbolt and USB moves into the driver core as they really should be
shared.

See the above git commit as an example of what I mean.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-05 13:52           ` Andi Kleen
@ 2021-08-05 17:51             ` Greg Kroah-Hartman
  2021-08-05 17:58               ` Andi Kleen
  0 siblings, 1 reply; 44+ messages in thread
From: Greg Kroah-Hartman @ 2021-08-05 17:51 UTC (permalink / raw)
  To: Andi Kleen
  Cc: Kuppuswamy Sathyanarayanan, Rafael J . Wysocki, Jonathan Corbet,
	Dan Williams, Kuppuswamy Sathyanarayanan, linux-kernel,
	linux-doc

On Thu, Aug 05, 2021 at 06:52:25AM -0700, Andi Kleen wrote:
> 
> > Both thunderbolt and USB have the idea of "authorized" devices, that is
> > the logic that should be made generic and available for all busses to
> > use, by moving it to the driver core, just like the "removable" logic
> > got moved to the driver core recently (see 70f400d4d957 ("driver core:
> > Move the "removable" attribute from USB to core")
> 
> This looks like it's controlled by udev?  Have a default per bus, and let
> user space override it before setting up the device.

It's controlled by whatever you want to use in userspace.  usbguard has
been handling this logic in userspace for over a decade now just fine.

> This doesn't help us handle builtin drivers that initialize before user
> space is up.

Then have the default setting for your bus be "unauthorized" like we
allow for some busses today.

> We need something that works for all drivers. Also cannot just use a default
> at bootup because some drivers (like virtio or rtc) need to be initialized
> in early boot to make the system functional at all. So you need a way to
> distinguish these two cases in the pre user space boot.
> 
> That's basically what this patch implements the infrastructure for.

It also ignores the existing implementation we already have for this for
some busses, please do not do that.

> > Please use that type of interface, as we already have userspace tools
> > using it, and expand it for all busses in the system to use if they
> > want.  Otherwise with this proposal you will end up with multiple ways
> > to control the same bus type with different types of "filtering",
> > ensuring a mess.
> 
> How would such a proposal work for a platform driver that doesn't have a
> bus?

There is a platform bus, it's just a fake one.  The platform bus code
does the binding just like any other bus does, why is platform somehow
"special"?  Except for how it is abused...

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-05 17:25             ` Kuppuswamy, Sathyanarayanan
  2021-08-05 17:48               ` Greg Kroah-Hartman
@ 2021-08-05 17:52               ` Andi Kleen
  2021-08-05 18:11                 ` Greg Kroah-Hartman
  1 sibling, 1 reply; 44+ messages in thread
From: Andi Kleen @ 2021-08-05 17:52 UTC (permalink / raw)
  To: Kuppuswamy, Sathyanarayanan, Dan Williams, Greg Kroah-Hartman
  Cc: Rafael J . Wysocki, Jonathan Corbet, Kuppuswamy Sathyanarayanan,
	Linux Kernel Mailing List, Linux Doc Mailing List


On 8/5/2021 10:25 AM, Kuppuswamy, Sathyanarayanan wrote:
>
>
> On 8/5/21 9:37 AM, Dan Williams wrote:
>> I overlooked the "authorized" attribute in usb and thunderbolt. The
>> collision problem makes sense. Are you open to a core "authorized"
>> attribute that buses like usb and thunderbolt would override in favor
>> of their local implementation? I.e. similar to suppress_bind_attrs:
>
> Even if such overriding is allowed in default boot, it should not be
> allowed in protected guest + driver_filter model.


Allowing overriding would be acceptable, as long as nobody does it by 
default. In theory a (root) user program can already do other things 
that make the guest insecure.

Still it's not clear to me how this proposal solves the builtin and 
platform drivers problem. AFAIK that needs a builtin allowlist in any 
case. And once we have that likely we don't need anything else for 
current TDX at least, because the allowlist is so small and there is no 
concept of hotplug or similar.

Also another consideration is that we were trying to avoid relying too 
much on user space for this. One of the goals was to move an existing 
guest image to a confidential guest with only minor changes (new kernel 
/ enable attestation). Complex changes for securing it would make that 
much harder.

-Andi


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-05 17:51             ` Greg Kroah-Hartman
@ 2021-08-05 17:58               ` Andi Kleen
  2021-08-05 18:09                 ` Greg Kroah-Hartman
  0 siblings, 1 reply; 44+ messages in thread
From: Andi Kleen @ 2021-08-05 17:58 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Kuppuswamy Sathyanarayanan, Rafael J . Wysocki, Jonathan Corbet,
	Dan Williams, Kuppuswamy Sathyanarayanan, linux-kernel,
	linux-doc


On 8/5/2021 10:51 AM, Greg Kroah-Hartman wrote:
>
> It's controlled by whatever you want to use in userspace.  usbguard has
> been handling this logic in userspace for over a decade now just fine.


So how does that work with builtin USB drivers? Do you delay the USB 
binding until usbguard starts up?

>
>> This doesn't help us handle builtin drivers that initialize before user
>> space is up.
> Then have the default setting for your bus be "unauthorized" like we
> allow for some busses today.

We need some early boot drivers, just not all of them. For example in 
your scheme we would need to reject all early platform drivers, which 
would break booting. Or allow all early platform drivers, but then we 
exactly get into the problem that there are far too many of them to 
properly harden.


> There is a platform bus, it's just a fake one.  The platform bus code
> does the binding just like any other bus does, why is platform somehow
> "special"?  Except for how it is abused...

For once it's usually all initialized early, so there's no way for user 
space to do anything.


-andi


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-05 17:58               ` Andi Kleen
@ 2021-08-05 18:09                 ` Greg Kroah-Hartman
  2021-08-05 18:44                   ` Andi Kleen
  2021-08-05 18:53                   ` Kuppuswamy, Sathyanarayanan
  0 siblings, 2 replies; 44+ messages in thread
From: Greg Kroah-Hartman @ 2021-08-05 18:09 UTC (permalink / raw)
  To: Andi Kleen
  Cc: Kuppuswamy Sathyanarayanan, Rafael J . Wysocki, Jonathan Corbet,
	Dan Williams, Kuppuswamy Sathyanarayanan, linux-kernel,
	linux-doc

On Thu, Aug 05, 2021 at 10:58:46AM -0700, Andi Kleen wrote:
> 
> On 8/5/2021 10:51 AM, Greg Kroah-Hartman wrote:
> > 
> > It's controlled by whatever you want to use in userspace.  usbguard has
> > been handling this logic in userspace for over a decade now just fine.
> 
> 
> So how does that work with builtin USB drivers? Do you delay the USB binding
> until usbguard starts up?

Yes.

> > > This doesn't help us handle builtin drivers that initialize before user
> > > space is up.
> > Then have the default setting for your bus be "unauthorized" like we
> > allow for some busses today.
> 
> We need some early boot drivers, just not all of them. For example in your
> scheme we would need to reject all early platform drivers, which would break
> booting. Or allow all early platform drivers, but then we exactly get into
> the problem that there are far too many of them to properly harden.

Define "harden" please.  Why not just allow all platform drivers, they
should all be trusted.  If not, well, you have bigger problems...

Anyway, feel free to build on top of the existing scheme please, but do
not ignore it and try to create yet-another-way-to-do-this that I have
to maintain for the next 20+ years.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-05 17:52               ` Andi Kleen
@ 2021-08-05 18:11                 ` Greg Kroah-Hartman
  0 siblings, 0 replies; 44+ messages in thread
From: Greg Kroah-Hartman @ 2021-08-05 18:11 UTC (permalink / raw)
  To: Andi Kleen
  Cc: Kuppuswamy, Sathyanarayanan, Dan Williams, Rafael J . Wysocki,
	Jonathan Corbet, Kuppuswamy Sathyanarayanan,
	Linux Kernel Mailing List, Linux Doc Mailing List

On Thu, Aug 05, 2021 at 10:52:10AM -0700, Andi Kleen wrote:
> 
> On 8/5/2021 10:25 AM, Kuppuswamy, Sathyanarayanan wrote:
> > 
> > 
> > On 8/5/21 9:37 AM, Dan Williams wrote:
> > > I overlooked the "authorized" attribute in usb and thunderbolt. The
> > > collision problem makes sense. Are you open to a core "authorized"
> > > attribute that buses like usb and thunderbolt would override in favor
> > > of their local implementation? I.e. similar to suppress_bind_attrs:
> > 
> > Even if such overriding is allowed in default boot, it should not be
> > allowed in protected guest + driver_filter model.
> 
> 
> Allowing overriding would be acceptable, as long as nobody does it by
> default. In theory a (root) user program can already do other things that
> make the guest insecure.
> 
> Still it's not clear to me how this proposal solves the builtin and platform
> drivers problem. AFAIK that needs a builtin allowlist in any case. And once
> we have that likely we don't need anything else for current TDX at least,
> because the allowlist is so small and there is no concept of hotplug or
> similar.

What specific platform drivers do you need on these systems that you
would ever want to exclude some and not just allow them all?

> Also another consideration is that we were trying to avoid relying too much
> on user space for this. One of the goals was to move an existing guest image
> to a confidential guest with only minor changes (new kernel / enable
> attestation). Complex changes for securing it would make that much harder.

Just deny all and only allow the ones you "trust".  That is a
well-defined policy that (/me checks notes) Intel created for USB a very
long time ago.

greg k-h

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-05 18:09                 ` Greg Kroah-Hartman
@ 2021-08-05 18:44                   ` Andi Kleen
  2021-08-05 19:01                     ` Dan Williams
  2021-08-06  5:07                     ` Greg Kroah-Hartman
  2021-08-05 18:53                   ` Kuppuswamy, Sathyanarayanan
  1 sibling, 2 replies; 44+ messages in thread
From: Andi Kleen @ 2021-08-05 18:44 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Kuppuswamy Sathyanarayanan, Rafael J . Wysocki, Jonathan Corbet,
	Dan Williams, Kuppuswamy Sathyanarayanan, linux-kernel,
	linux-doc


On 8/5/2021 11:09 AM, Greg Kroah-Hartman wrote:
> On Thu, Aug 05, 2021 at 10:58:46AM -0700, Andi Kleen wrote:
>> On 8/5/2021 10:51 AM, Greg Kroah-Hartman wrote:
>>> It's controlled by whatever you want to use in userspace.  usbguard has
>>> been handling this logic in userspace for over a decade now just fine.
>>
>> So how does that work with builtin USB drivers? Do you delay the USB binding
>> until usbguard starts up?
> Yes.

That won't work for confidential guests, e.g. we need a virtio driver 
for the console and some other things.


>
>>>> This doesn't help us handle builtin drivers that initialize before user
>>>> space is up.
>>> Then have the default setting for your bus be "unauthorized" like we
>>> allow for some busses today.
>> We need some early boot drivers, just not all of them. For example in your
>> scheme we would need to reject all early platform drivers, which would break
>> booting. Or allow all early platform drivers, but then we exactly get into
>> the problem that there are far too many of them to properly harden.
> Define "harden" please.  Why not just allow all platform drivers, they
> should all be trusted.  If not, well, you have bigger problems...

Trusted here means someone audited them and also fuzzed them. That's all 
a lot of work and also needs to be maintained forever so we're trying to 
do only a minimum set. There are actually quite a few platform drivers, 
it's difficult to audit them all.


>
> Anyway, feel free to build on top of the existing scheme please, but do
> not ignore it and try to create yet-another-way-to-do-this that I have
> to maintain for the next 20+ years.

We have to establish the existing scheme solves the problem statement 
first. So far it seems it doesn't seem to solve the problem at all for 
early drivers that are needed for booting. Unless I'm missing something?

For late (e.g. modular) drivers it would probably be usable, but it 
would complicate deployment quite a bit with complex user space changes, 
so I can't say it looks very attractive.

But if we solve the problem for the early drivers then I don't think we 
need the user space controlled scheme at all, because it should work all 
the same.

So it seems they existing approach is not really cutting it.

That's why I think the builtin allow list hook is still needed. Thoughts?


-Andi


>

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-05 18:09                 ` Greg Kroah-Hartman
  2021-08-05 18:44                   ` Andi Kleen
@ 2021-08-05 18:53                   ` Kuppuswamy, Sathyanarayanan
  2021-08-05 19:12                     ` Greg Kroah-Hartman
  1 sibling, 1 reply; 44+ messages in thread
From: Kuppuswamy, Sathyanarayanan @ 2021-08-05 18:53 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Andi Kleen
  Cc: Rafael J . Wysocki, Jonathan Corbet, Dan Williams,
	Kuppuswamy Sathyanarayanan, linux-kernel, linux-doc



On 8/5/21 11:09 AM, Greg Kroah-Hartman wrote:
> On Thu, Aug 05, 2021 at 10:58:46AM -0700, Andi Kleen wrote:
>>
>> On 8/5/2021 10:51 AM, Greg Kroah-Hartman wrote:
>>>
>>> It's controlled by whatever you want to use in userspace.  usbguard has
>>> been handling this logic in userspace for over a decade now just fine.
>>
>>
>> So how does that work with builtin USB drivers? Do you delay the USB binding
>> until usbguard starts up?
> 
> Yes.
> 
>>>> This doesn't help us handle builtin drivers that initialize before user
>>>> space is up.
>>> Then have the default setting for your bus be "unauthorized" like we
>>> allow for some busses today.
>>
>> We need some early boot drivers, just not all of them. For example in your
>> scheme we would need to reject all early platform drivers, which would break
>> booting. Or allow all early platform drivers, but then we exactly get into
>> the problem that there are far too many of them to properly harden.
> 
> Define "harden" please.  Why not just allow all platform drivers, they
> should all be trusted.  If not, well, you have bigger problems...

This driver filter framework will be mainly (at-least for now) used by
protected guest. "Protected guest" is the term we use define a VM
guest which ensures memory and data isolation when working with
untrusted VMM. You can find some basic introduction to it in following
links.

https://lwn.net/Articles/860352/
https://software.intel.com/content/www/br/pt/develop/articles/intel-trust-domain-extensions.html

In protected guest, since VMM is untrusted, device drivers that deals
with IO-memory and data had to audited and hardened against attack from
malicious VMM.

With this driver filter support, we can ensure only hardened drivers
are allowed to bind with device. This is applicable to built-in and
loadable kernel drivers.

I don't think there is a existing framework which does this right?

I am not sure how USB and Thunderbolt "authorzied" model works. But I
don't think it prevents built-in driver probes during kernel boot right?
I will also check this framework and get back to you.

> 
> Anyway, feel free to build on top of the existing scheme please, but do
> not ignore it and try to create yet-another-way-to-do-this that I have
> to maintain for the next 20+ years.
> 
> thanks,
> 
> greg k-h
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-05 18:44                   ` Andi Kleen
@ 2021-08-05 19:01                     ` Dan Williams
  2021-08-05 19:08                       ` Kuppuswamy, Sathyanarayanan
  2021-08-05 21:10                       ` Andi Kleen
  2021-08-06  5:07                     ` Greg Kroah-Hartman
  1 sibling, 2 replies; 44+ messages in thread
From: Dan Williams @ 2021-08-05 19:01 UTC (permalink / raw)
  To: Andi Kleen
  Cc: Greg Kroah-Hartman, Kuppuswamy Sathyanarayanan,
	Rafael J . Wysocki, Jonathan Corbet, Kuppuswamy Sathyanarayanan,
	Linux Kernel Mailing List, Linux Doc Mailing List

On Thu, Aug 5, 2021 at 11:44 AM Andi Kleen <ak@linux.intel.com> wrote:
>
>
> On 8/5/2021 11:09 AM, Greg Kroah-Hartman wrote:
> > On Thu, Aug 05, 2021 at 10:58:46AM -0700, Andi Kleen wrote:
> >> On 8/5/2021 10:51 AM, Greg Kroah-Hartman wrote:
> >>> It's controlled by whatever you want to use in userspace.  usbguard has
> >>> been handling this logic in userspace for over a decade now just fine.
> >>
> >> So how does that work with builtin USB drivers? Do you delay the USB binding
> >> until usbguard starts up?
> > Yes.
>
> That won't work for confidential guests, e.g. we need a virtio driver
> for the console and some other things.
>
>
> >
> >>>> This doesn't help us handle builtin drivers that initialize before user
> >>>> space is up.
> >>> Then have the default setting for your bus be "unauthorized" like we
> >>> allow for some busses today.
> >> We need some early boot drivers, just not all of them. For example in your
> >> scheme we would need to reject all early platform drivers, which would break
> >> booting. Or allow all early platform drivers, but then we exactly get into
> >> the problem that there are far too many of them to properly harden.
> > Define "harden" please.  Why not just allow all platform drivers, they
> > should all be trusted.  If not, well, you have bigger problems...
>
> Trusted here means someone audited them and also fuzzed them. That's all
> a lot of work and also needs to be maintained forever so we're trying to
> do only a minimum set. There are actually quite a few platform drivers,
> it's difficult to audit them all.
>

What's wrong with the generic authorized proposal? The core can
default to deauthorizing devices on the platform bus, or any bus,
unless on an allow list. It's a bit more work to uplevel the local
"authorized" implementations from USB and Thunderbolt to the core, but
it's functionally identical to the "filter" approach in terms of
protection, i.e. avoiding probe of unnecessary unvetted drivers.

[..]
> That's why I think the builtin allow list hook is still needed. Thoughts?

I see nothing that prevents a built-in allow list to augment the
driver-core default. Is there a gap I'm missing?

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-05 19:01                     ` Dan Williams
@ 2021-08-05 19:08                       ` Kuppuswamy, Sathyanarayanan
  2021-08-05 19:28                         ` Greg Kroah-Hartman
  2021-08-05 21:10                       ` Andi Kleen
  1 sibling, 1 reply; 44+ messages in thread
From: Kuppuswamy, Sathyanarayanan @ 2021-08-05 19:08 UTC (permalink / raw)
  To: Dan Williams, Andi Kleen
  Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Jonathan Corbet,
	Kuppuswamy Sathyanarayanan, Linux Kernel Mailing List,
	Linux Doc Mailing List



On 8/5/21 12:01 PM, Dan Williams wrote:
> What's wrong with the generic authorized proposal? The core can
> default to deauthorizing devices on the platform bus, or any bus,
> unless on an allow list. It's a bit more work to uplevel the local
> "authorized" implementations from USB and Thunderbolt to the core, but
> it's functionally identical to the "filter" approach in terms of
> protection, i.e. avoiding probe of unnecessary unvetted drivers.

I have not yet read about the "authorized" model in USB and Thunderbolt.
So bear with me if my question is basic or obvious. In the case USB
authorized model, who maintains the allow list? kernel or userspace?

If we are clubbing it with the driver filter model, I think
allow list in kernel should take precedence. Agree?

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-05 18:53                   ` Kuppuswamy, Sathyanarayanan
@ 2021-08-05 19:12                     ` Greg Kroah-Hartman
  2021-08-05 19:18                       ` Dan Williams
  0 siblings, 1 reply; 44+ messages in thread
From: Greg Kroah-Hartman @ 2021-08-05 19:12 UTC (permalink / raw)
  To: Kuppuswamy, Sathyanarayanan
  Cc: Andi Kleen, Rafael J . Wysocki, Jonathan Corbet, Dan Williams,
	Kuppuswamy Sathyanarayanan, linux-kernel, linux-doc

On Thu, Aug 05, 2021 at 11:53:52AM -0700, Kuppuswamy, Sathyanarayanan wrote:
> I am not sure how USB and Thunderbolt "authorzied" model works. But I
> don't think it prevents built-in driver probes during kernel boot right?

Yes it does.

Again Intel created this framework well over a decade ago for busses
that it deemed that it did not want to "trust" to instantly probe
drivers for and made it part of the Wireless USB specification.

Then Intel went and added the same framework to Thunderbolt for the same
reason.

To ignore this work is quite odd, you might want to talk to your
coworkers...

greg k-h

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-05 19:12                     ` Greg Kroah-Hartman
@ 2021-08-05 19:18                       ` Dan Williams
  2021-08-05 19:28                         ` Greg Kroah-Hartman
  0 siblings, 1 reply; 44+ messages in thread
From: Dan Williams @ 2021-08-05 19:18 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Kuppuswamy, Sathyanarayanan, Andi Kleen, Rafael J . Wysocki,
	Jonathan Corbet, Kuppuswamy Sathyanarayanan,
	Linux Kernel Mailing List, Linux Doc Mailing List

On Thu, Aug 5, 2021 at 12:12 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Aug 05, 2021 at 11:53:52AM -0700, Kuppuswamy, Sathyanarayanan wrote:
> > I am not sure how USB and Thunderbolt "authorzied" model works. But I
> > don't think it prevents built-in driver probes during kernel boot right?
>
> Yes it does.
>
> Again Intel created this framework well over a decade ago for busses
> that it deemed that it did not want to "trust" to instantly probe
> drivers for and made it part of the Wireless USB specification.
>
> Then Intel went and added the same framework to Thunderbolt for the same
> reason.
>
> To ignore this work is quite odd, you might want to talk to your
> coworkers...

Sometimes we need upstream to connect us wayward drones back into the
hive mind. Forgive me for not immediately recognizing that the
existing 'authorized' mechanisms might be repurposed for this use
case.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-05 19:18                       ` Dan Williams
@ 2021-08-05 19:28                         ` Greg Kroah-Hartman
  2021-08-05 19:52                           ` Dan Williams
  0 siblings, 1 reply; 44+ messages in thread
From: Greg Kroah-Hartman @ 2021-08-05 19:28 UTC (permalink / raw)
  To: Dan Williams
  Cc: Kuppuswamy, Sathyanarayanan, Andi Kleen, Rafael J . Wysocki,
	Jonathan Corbet, Kuppuswamy Sathyanarayanan,
	Linux Kernel Mailing List, Linux Doc Mailing List

On Thu, Aug 05, 2021 at 12:18:12PM -0700, Dan Williams wrote:
> On Thu, Aug 5, 2021 at 12:12 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Aug 05, 2021 at 11:53:52AM -0700, Kuppuswamy, Sathyanarayanan wrote:
> > > I am not sure how USB and Thunderbolt "authorzied" model works. But I
> > > don't think it prevents built-in driver probes during kernel boot right?
> >
> > Yes it does.
> >
> > Again Intel created this framework well over a decade ago for busses
> > that it deemed that it did not want to "trust" to instantly probe
> > drivers for and made it part of the Wireless USB specification.
> >
> > Then Intel went and added the same framework to Thunderbolt for the same
> > reason.
> >
> > To ignore this work is quite odd, you might want to talk to your
> > coworkers...
> 
> Sometimes we need upstream to connect us wayward drones back into the
> hive mind. Forgive me for not immediately recognizing that the
> existing 'authorized' mechanisms might be repurposed for this use
> case.

Not your fault, I'm more amazed that Andi doesn't remember this, he's
been around longer :)

But the first instinct should not be "let's go add a new feature", but
rather, "how has this problem been solved by others first" because,
really, this is not a new issue at all.  You should not rely on just me
to point out existing kernel features, we do have documentation you
know...

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-05 19:08                       ` Kuppuswamy, Sathyanarayanan
@ 2021-08-05 19:28                         ` Greg Kroah-Hartman
  0 siblings, 0 replies; 44+ messages in thread
From: Greg Kroah-Hartman @ 2021-08-05 19:28 UTC (permalink / raw)
  To: Kuppuswamy, Sathyanarayanan
  Cc: Dan Williams, Andi Kleen, Rafael J . Wysocki, Jonathan Corbet,
	Kuppuswamy Sathyanarayanan, Linux Kernel Mailing List,
	Linux Doc Mailing List

On Thu, Aug 05, 2021 at 12:08:52PM -0700, Kuppuswamy, Sathyanarayanan wrote:
> 
> 
> On 8/5/21 12:01 PM, Dan Williams wrote:
> > What's wrong with the generic authorized proposal? The core can
> > default to deauthorizing devices on the platform bus, or any bus,
> > unless on an allow list. It's a bit more work to uplevel the local
> > "authorized" implementations from USB and Thunderbolt to the core, but
> > it's functionally identical to the "filter" approach in terms of
> > protection, i.e. avoiding probe of unnecessary unvetted drivers.
> 
> I have not yet read about the "authorized" model in USB and Thunderbolt.
> So bear with me if my question is basic or obvious. In the case USB
> authorized model, who maintains the allow list? kernel or userspace?

Please go read the documentation and don't ask others to do your work
for you...


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-05 19:28                         ` Greg Kroah-Hartman
@ 2021-08-05 19:52                           ` Dan Williams
  2021-08-06 11:15                             ` Jonathan Cameron
  0 siblings, 1 reply; 44+ messages in thread
From: Dan Williams @ 2021-08-05 19:52 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Kuppuswamy, Sathyanarayanan, Andi Kleen, Rafael J . Wysocki,
	Jonathan Corbet, Kuppuswamy Sathyanarayanan,
	Linux Kernel Mailing List, Linux Doc Mailing List,
	Jonathan Cameron

[ add Jonathan ]

On Thu, Aug 5, 2021 at 12:28 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Aug 05, 2021 at 12:18:12PM -0700, Dan Williams wrote:
> > On Thu, Aug 5, 2021 at 12:12 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Thu, Aug 05, 2021 at 11:53:52AM -0700, Kuppuswamy, Sathyanarayanan wrote:
> > > > I am not sure how USB and Thunderbolt "authorzied" model works. But I
> > > > don't think it prevents built-in driver probes during kernel boot right?
> > >
> > > Yes it does.
> > >
> > > Again Intel created this framework well over a decade ago for busses
> > > that it deemed that it did not want to "trust" to instantly probe
> > > drivers for and made it part of the Wireless USB specification.
> > >
> > > Then Intel went and added the same framework to Thunderbolt for the same
> > > reason.
> > >
> > > To ignore this work is quite odd, you might want to talk to your
> > > coworkers...
> >
> > Sometimes we need upstream to connect us wayward drones back into the
> > hive mind. Forgive me for not immediately recognizing that the
> > existing 'authorized' mechanisms might be repurposed for this use
> > case.
>
> Not your fault, I'm more amazed that Andi doesn't remember this, he's
> been around longer :)
>

In the driver core? No, not so much, and I do remember it flying by,
just did not connect the dots. In fact, it had just gone upstream when
you and I had that thread about blocking PCI drivers [1], September
2017 vs June 2017 when the Thunderbolt connection manager was merged.
There was no internal review process back then so I failed to
internalize its implications for this TDX filter. You had taken the
time to review it in a way that I had not.

> But the first instinct should not be "let's go add a new feature", but
> rather, "how has this problem been solved by others first" because,
> really, this is not a new issue at all.  You should not rely on just me
> to point out existing kernel features, we do have documentation you
> know...

I have added, "review driver core attribute proposal for duplication
of bus-local capabilities" to my review checklist.

The good news is I think this generic authorization support in the
core may answer one of Jonathan's questions about how to integrate PCI
SPDM/CMA support [2].

[1]: https://lore.kernel.org/lkml/20170928090901.GC12599@kroah.com/
[2]: https://lore.kernel.org/r/20210804161839.3492053-1-Jonathan.Cameron@huawei.com

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-05 19:01                     ` Dan Williams
  2021-08-05 19:08                       ` Kuppuswamy, Sathyanarayanan
@ 2021-08-05 21:10                       ` Andi Kleen
  2021-08-06  1:00                         ` Dan Williams
  1 sibling, 1 reply; 44+ messages in thread
From: Andi Kleen @ 2021-08-05 21:10 UTC (permalink / raw)
  To: Dan Williams
  Cc: Greg Kroah-Hartman, Kuppuswamy Sathyanarayanan,
	Rafael J . Wysocki, Jonathan Corbet, Kuppuswamy Sathyanarayanan,
	Linux Kernel Mailing List, Linux Doc Mailing List


On 8/5/2021 12:01 PM, Dan Williams wrote:

>> That's why I think the builtin allow list hook is still needed. Thoughts?
> I see nothing that prevents a built-in allow list to augment the
> driver-core default. Is there a gap I'm missing?


Okay so you're suggesting to build the builtin allow list on top of the 
existing framework?

I thought Greg's suggestion was to only rely on user space only.

But if we have a way to change the authorized defaults by device (not 
just bus) from inside the kernel at early boot that could well work.

Doing it only on the bus level I suspect wouldn't work though.

-Andi



^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-05 21:10                       ` Andi Kleen
@ 2021-08-06  1:00                         ` Dan Williams
  2021-08-06  5:17                           ` Greg Kroah-Hartman
  0 siblings, 1 reply; 44+ messages in thread
From: Dan Williams @ 2021-08-06  1:00 UTC (permalink / raw)
  To: Andi Kleen
  Cc: Greg Kroah-Hartman, Kuppuswamy Sathyanarayanan,
	Rafael J . Wysocki, Jonathan Corbet, Kuppuswamy Sathyanarayanan,
	Linux Kernel Mailing List, Linux Doc Mailing List

On Thu, Aug 5, 2021 at 2:10 PM Andi Kleen <ak@linux.intel.com> wrote:
>
>
> On 8/5/2021 12:01 PM, Dan Williams wrote:
>
> >> That's why I think the builtin allow list hook is still needed. Thoughts?
> > I see nothing that prevents a built-in allow list to augment the
> > driver-core default. Is there a gap I'm missing?
>
>
> Okay so you're suggesting to build the builtin allow list on top of the
> existing framework?
>
> I thought Greg's suggestion was to only rely on user space only.
>
> But if we have a way to change the authorized defaults by device (not
> just bus) from inside the kernel at early boot that could well work.

The default usb authorization is set at device creation time inherited
from controller policy, which is in turn inherited from usbcore
policy. So appending a built-in way to augment that policy further
seems doable.

> Doing it only on the bus level I suspect wouldn't work though.

I think /sys/devices/.../$dev/authorized attribute can be used
generically as the override interface, not that the TDX use case cares
about user overrides, but that was the bulk of the unnecessary
reinvention. That also addresses the ABI confusion so tools like
usbguard don't need to look in 2 places to find a device is not
authorized.

That said, per-device authorization is a little bit different than
per-driver trust. Driver trust is easy to reason about for a built-in
policy, while per-device authorization is easy for userspace to reason
about for "why is this device not talking to its driver?".

So a strawman (I'm willing to watch go up in flames like
driver-filter) is an arch overridable callback in driver_sysfs_add()
as a central location where a device could have its authorization
state set if it has not been previously changed from its
initialization value. That callback could then consider device-name,
bus-name, and/or driver-name for the default authorization.

driver_sysfs_add() should also catch drivers that are manually bound
without a bus.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-05 18:44                   ` Andi Kleen
  2021-08-05 19:01                     ` Dan Williams
@ 2021-08-06  5:07                     ` Greg Kroah-Hartman
  1 sibling, 0 replies; 44+ messages in thread
From: Greg Kroah-Hartman @ 2021-08-06  5:07 UTC (permalink / raw)
  To: Andi Kleen
  Cc: Kuppuswamy Sathyanarayanan, Rafael J . Wysocki, Jonathan Corbet,
	Dan Williams, Kuppuswamy Sathyanarayanan, linux-kernel,
	linux-doc

On Thu, Aug 05, 2021 at 11:44:47AM -0700, Andi Kleen wrote:
> 
> On 8/5/2021 11:09 AM, Greg Kroah-Hartman wrote:
> > On Thu, Aug 05, 2021 at 10:58:46AM -0700, Andi Kleen wrote:
> > > On 8/5/2021 10:51 AM, Greg Kroah-Hartman wrote:
> > > > It's controlled by whatever you want to use in userspace.  usbguard has
> > > > been handling this logic in userspace for over a decade now just fine.
> > > 
> > > So how does that work with builtin USB drivers? Do you delay the USB binding
> > > until usbguard starts up?
> > Yes.
> 
> That won't work for confidential guests, e.g. we need a virtio driver for
> the console and some other things.
> 
> 
> > 
> > > > > This doesn't help us handle builtin drivers that initialize before user
> > > > > space is up.
> > > > Then have the default setting for your bus be "unauthorized" like we
> > > > allow for some busses today.
> > > We need some early boot drivers, just not all of them. For example in your
> > > scheme we would need to reject all early platform drivers, which would break
> > > booting. Or allow all early platform drivers, but then we exactly get into
> > > the problem that there are far too many of them to properly harden.
> > Define "harden" please.  Why not just allow all platform drivers, they
> > should all be trusted.  If not, well, you have bigger problems...
> 
> Trusted here means someone audited them and also fuzzed them. That's all a
> lot of work and also needs to be maintained forever so we're trying to do
> only a minimum set. There are actually quite a few platform drivers, it's
> difficult to audit them all.

Note, this model is wrong and backwards and will not work out at all in
the end.

But given that this is coming from a hardware company, it makes sense.
You are coming from the model of "the hardware is trusted, but the code
is untrusted".  That is the exact opposite of what we have been working
with for the past 5+ years now.

Look at all of the work that has happened in just USB drivers over the
recent years thanks to fuzzing efforts.  None of this was done because
we did not trust the kernel code, it was because we had to now not trust
the hardware to actually do the right thing.  So we have had to "harden"
the kernel side to deal with untrusted hardware.

People are now looking at doing the same for PCI devices, but that's a
huge effort that no one has started to take seriously.

Same thing for any other hardware "bug", that is software having to fix
hardware errors as it is the thing that is incorrect, not the software.
Spectre/meltdown is a huge example of that, but there are many more.

The model the kernel has right now for "authorized" devices is that it
is up to some entity to determine if the _device_ is trusted, not if the
_driver_ is trusted.

By virtue of you all saying that you want to use a generic kernel image
from a vendor, you are implying that you have to trust that software as
you have no control over that kernel image.  What you need to validate
is "can I trust this device to be controlled by the kernel".

So work on providing "trusted" hardware/device signals to the kernel
please.  That is the only way this is going to work.

And yes, auditing drivers is wonderful and grand please do that and set
up automated testing for it along with good static analysis and all of
that.  But that is NOT going to provide you with what you want here, as
the most "perfictly audited" body of code will fall apart when
confronted with "hardware" that does not work as documented/planned.

That's the fatal flaw in the formal methods camp, they have to trust the
model of the running system in order to be able to then validate the
software.  But when the model turns out to be wrong (again, spectre is
an example of this), it turns out that the software ends up not working
"correctly".

So go work on providing some sort of authentication of hardware to
attest that it really is what it says it is, in order to allow a kernel
to be able to determine if it should start talking to it or not.

good luck!

greg k-h

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-06  1:00                         ` Dan Williams
@ 2021-08-06  5:17                           ` Greg Kroah-Hartman
  2021-08-06 14:36                             ` Dan Williams
  0 siblings, 1 reply; 44+ messages in thread
From: Greg Kroah-Hartman @ 2021-08-06  5:17 UTC (permalink / raw)
  To: Dan Williams
  Cc: Andi Kleen, Kuppuswamy Sathyanarayanan, Rafael J . Wysocki,
	Jonathan Corbet, Kuppuswamy Sathyanarayanan,
	Linux Kernel Mailing List, Linux Doc Mailing List

On Thu, Aug 05, 2021 at 06:00:25PM -0700, Dan Williams wrote:
> That said, per-device authorization is a little bit different than
> per-driver trust. Driver trust is easy to reason about for a built-in
> policy, while per-device authorization is easy for userspace to reason
> about for "why is this device not talking to its driver?".

See my other email about how the "per driver" trust is the wrong model,
you need to stick to "per device" trust.  Especially given that you are
giving control of your kernel drivers over to third parties, you already
trust them to do the right thing.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-05 19:52                           ` Dan Williams
@ 2021-08-06 11:15                             ` Jonathan Cameron
  0 siblings, 0 replies; 44+ messages in thread
From: Jonathan Cameron @ 2021-08-06 11:15 UTC (permalink / raw)
  To: Dan Williams
  Cc: Greg Kroah-Hartman, Kuppuswamy, Sathyanarayanan, Andi Kleen,
	Rafael J . Wysocki, Jonathan Corbet, Kuppuswamy Sathyanarayanan,
	Linux Kernel Mailing List, Linux Doc Mailing List

On Thu, 5 Aug 2021 12:52:30 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> [ add Jonathan ]
> 
> On Thu, Aug 5, 2021 at 12:28 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Aug 05, 2021 at 12:18:12PM -0700, Dan Williams wrote:  
> > > On Thu, Aug 5, 2021 at 12:12 PM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:  
> > > >
> > > > On Thu, Aug 05, 2021 at 11:53:52AM -0700, Kuppuswamy, Sathyanarayanan wrote:  
> > > > > I am not sure how USB and Thunderbolt "authorzied" model works. But I
> > > > > don't think it prevents built-in driver probes during kernel boot right?  
> > > >
> > > > Yes it does.
> > > >
> > > > Again Intel created this framework well over a decade ago for busses
> > > > that it deemed that it did not want to "trust" to instantly probe
> > > > drivers for and made it part of the Wireless USB specification.
> > > >
> > > > Then Intel went and added the same framework to Thunderbolt for the same
> > > > reason.
> > > >
> > > > To ignore this work is quite odd, you might want to talk to your
> > > > coworkers...  
> > >
> > > Sometimes we need upstream to connect us wayward drones back into the
> > > hive mind. Forgive me for not immediately recognizing that the
> > > existing 'authorized' mechanisms might be repurposed for this use
> > > case.  
> >
> > Not your fault, I'm more amazed that Andi doesn't remember this, he's
> > been around longer :)
> >  
> 
> In the driver core? No, not so much, and I do remember it flying by,
> just did not connect the dots. In fact, it had just gone upstream when
> you and I had that thread about blocking PCI drivers [1], September
> 2017 vs June 2017 when the Thunderbolt connection manager was merged.
> There was no internal review process back then so I failed to
> internalize its implications for this TDX filter. You had taken the
> time to review it in a way that I had not.
> 
> > But the first instinct should not be "let's go add a new feature", but
> > rather, "how has this problem been solved by others first" because,
> > really, this is not a new issue at all.  You should not rely on just me
> > to point out existing kernel features, we do have documentation you
> > know...  
> 
> I have added, "review driver core attribute proposal for duplication
> of bus-local capabilities" to my review checklist.
> 
> The good news is I think this generic authorization support in the
> core may answer one of Jonathan's questions about how to integrate PCI
> SPDM/CMA support [2].

Definitely an interesting discussion, and the SPDM stuff
feeds into Greg's point about establishing trust with hardware.

If anyone is looking at the USB authentication specification (which is
more or less SPDM), would be good to align on that.

My current model is really basic (driver checks and fails probe if
failure occurs). Definitely better to bolt into standard approach.

*Goes off to read up on this topic*

Thanks for highlighting this thread Dan,

Jonathan

> 
> [1]: https://lore.kernel.org/lkml/20170928090901.GC12599@kroah.com/
> [2]: https://lore.kernel.org/r/20210804161839.3492053-1-Jonathan.Cameron@huawei.com


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH v1] driver: base: Add driver filter support
  2021-08-06  5:17                           ` Greg Kroah-Hartman
@ 2021-08-06 14:36                             ` Dan Williams
  0 siblings, 0 replies; 44+ messages in thread
From: Dan Williams @ 2021-08-06 14:36 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Andi Kleen, Kuppuswamy Sathyanarayanan, Rafael J . Wysocki,
	Jonathan Corbet, Kuppuswamy Sathyanarayanan,
	Linux Kernel Mailing List, Linux Doc Mailing List

On Thu, Aug 5, 2021 at 10:17 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Aug 05, 2021 at 06:00:25PM -0700, Dan Williams wrote:
> > That said, per-device authorization is a little bit different than
> > per-driver trust. Driver trust is easy to reason about for a built-in
> > policy, while per-device authorization is easy for userspace to reason
> > about for "why is this device not talking to its driver?".
>
> See my other email about how the "per driver" trust is the wrong model,
> you need to stick to "per device" trust.  Especially given that you are
> giving control of your kernel drivers over to third parties, you already
> trust them to do the right thing.

Andi, if the number of TDX devices is small could they grow an SPDM
over virtio channel? Then you can measure trust from the VM to the VMM
to the attestation server.

^ permalink raw reply	[flat|nested] 44+ messages in thread

end of thread, other threads:[~2021-08-06 14:36 UTC | newest]

Thread overview: 44+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-04 17:43 [PATCH v1] driver: base: Add driver filter support Kuppuswamy Sathyanarayanan
2021-08-04 18:08 ` Matthew Wilcox
2021-08-04 18:29   ` Kuppuswamy, Sathyanarayanan
2021-08-04 18:33     ` Matthew Wilcox
2021-08-04 19:27     ` Andi Kleen
2021-08-04 18:45   ` Dan Williams
2021-08-04 19:29 ` Greg Kroah-Hartman
2021-08-04 19:50   ` Andi Kleen
2021-08-04 20:09     ` Kuppuswamy, Sathyanarayanan
2021-08-05  7:50       ` Greg Kroah-Hartman
2021-08-05  7:49     ` Greg Kroah-Hartman
2021-08-05  7:55       ` Greg Kroah-Hartman
2021-08-05  7:58         ` Greg Kroah-Hartman
2021-08-05 13:52           ` Andi Kleen
2021-08-05 17:51             ` Greg Kroah-Hartman
2021-08-05 17:58               ` Andi Kleen
2021-08-05 18:09                 ` Greg Kroah-Hartman
2021-08-05 18:44                   ` Andi Kleen
2021-08-05 19:01                     ` Dan Williams
2021-08-05 19:08                       ` Kuppuswamy, Sathyanarayanan
2021-08-05 19:28                         ` Greg Kroah-Hartman
2021-08-05 21:10                       ` Andi Kleen
2021-08-06  1:00                         ` Dan Williams
2021-08-06  5:17                           ` Greg Kroah-Hartman
2021-08-06 14:36                             ` Dan Williams
2021-08-06  5:07                     ` Greg Kroah-Hartman
2021-08-05 18:53                   ` Kuppuswamy, Sathyanarayanan
2021-08-05 19:12                     ` Greg Kroah-Hartman
2021-08-05 19:18                       ` Dan Williams
2021-08-05 19:28                         ` Greg Kroah-Hartman
2021-08-05 19:52                           ` Dan Williams
2021-08-06 11:15                             ` Jonathan Cameron
2021-08-05 16:37           ` Dan Williams
2021-08-05 17:25             ` Kuppuswamy, Sathyanarayanan
2021-08-05 17:48               ` Greg Kroah-Hartman
2021-08-05 17:52               ` Andi Kleen
2021-08-05 18:11                 ` Greg Kroah-Hartman
2021-08-05 17:49             ` Greg Kroah-Hartman
2021-08-04 20:11   ` Dan Williams
2021-08-04 20:29     ` Kuppuswamy, Sathyanarayanan
2021-08-04 21:07     ` Matthew Wilcox
2021-08-04 21:28       ` Dan Williams
2021-08-05  4:45         ` Andi Kleen
2021-08-05  7:59         ` Greg Kroah-Hartman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).