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 <[email protected]>
Reviewed-by: Dan Williams <[email protected]>
Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
---
.../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
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...
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
> 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
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
> <[email protected]> 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
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
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
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
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
>
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
On Thu, Aug 5, 2021 at 11:44 AM Andi Kleen <[email protected]> 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?
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
On Thu, Aug 5, 2021 at 12:12 PM Greg Kroah-Hartman
<[email protected]> 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.
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
> <[email protected]> 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
[ add Jonathan ]
On Thu, Aug 5, 2021 at 12:28 PM Greg Kroah-Hartman
<[email protected]> 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
> > <[email protected]> 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/[email protected]/
[2]: https://lore.kernel.org/r/[email protected]
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
On Thu, Aug 5, 2021 at 12:58 AM Greg Kroah-Hartman
<[email protected]> 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,
};
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
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
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
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...
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
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...
On Thu, Aug 5, 2021 at 2:10 PM Andi Kleen <[email protected]> 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.
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
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
On Thu, 5 Aug 2021 12:52:30 -0700
Dan Williams <[email protected]> wrote:
> [ add Jonathan ]
>
> On Thu, Aug 5, 2021 at 12:28 PM Greg Kroah-Hartman
> <[email protected]> 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
> > > <[email protected]> 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/[email protected]/
> [2]: https://lore.kernel.org/r/[email protected]
On Thu, Aug 5, 2021 at 10:17 PM Greg Kroah-Hartman
<[email protected]> 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.