LinuxLists.cc - [PATCH v2 0/6] Add device filter support

[permalink] [raw]

Subject: [PATCH v2 3/6] driver core: Allow arch to initialize the authorized attribute

Authorized device attribute is used to authorize or deauthorize
the driver probe of the given device. Currently this attribute
is initialized to "true" (allow all) by default.

But for platforms like TDX guest, in which the host is an untrusted
entity, it has a requirement to disable all devices by default and
allow only a trusted list of devices with hardened drivers. So
define a variable "dev_default_authorization" which is used to
initialize the "authorized" attribute in device_initialize(). Also
allow arch code to override the default value by updating
dev_default_authorization value.

More discussion about the need for device/driver filter and the use
of allow list can be found in article [1] titled "firewall for
device drivers".

Also note that USB and Thunderbolt both override this initial value
in their respective device initializations so this is not a regression
for those buses.

[1] - https://lwn.net/Articles/865918/

Reviewed-by: Dan Williams <[email protected]>
Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
---
drivers/base/core.c | 7 +++++++
include/linux/device.h | 2 ++
2 files changed, 9 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index e65dd803a453..98717f00b90b 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -47,6 +47,12 @@ static int __init sysfs_deprecated_setup(char *arg)
early_param("sysfs.deprecated", sysfs_deprecated_setup);
#endif

+/*
+ * Default authorization status set as allow all. It can be
+ * overridden by arch code.
+ */
+bool __ro_after_init dev_default_authorization = true;
+
/* Device links support. */
static LIST_HEAD(deferred_sync);
static unsigned int defer_sync_state_count = 1;
@@ -2855,6 +2861,7 @@ void device_initialize(struct device *dev)
#ifdef CONFIG_SWIOTLB
dev->dma_io_tlb_mem = &io_tlb_default_mem;
#endif
+ dev->authorized = dev_default_authorization;
}
EXPORT_SYMBOL_GPL(device_initialize);

diff --git a/include/linux/device.h b/include/linux/device.h
index 899be9a2c0cb..c97b1e59d23a 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -959,6 +959,8 @@ int devtmpfs_mount(void);
static inline int devtmpfs_mount(void) { return 0; }
#endif

+extern bool dev_default_authorization;
+
/* drivers/base/power/shutdown.c */
void device_shutdown(void);

--
2.25.1

2021-09-30 01:10:45

[permalink] [raw]

Subject: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

While the common case for device-authorization is to skip probe of
unauthorized devices, some buses may still want to emit a message on
probe failure (Thunderbolt), or base probe failures on the
authorization status of a related device like a parent (USB). So add
an option (has_probe_authorization) in struct bus_type for the bus
driver to own probe authorization policy.

Reviewed-by: Dan Williams <[email protected]>
Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
---
drivers/base/dd.c | 5 +++++
drivers/thunderbolt/domain.c | 1 +
drivers/usb/core/driver.c | 1 +
include/linux/device/bus.h | 4 ++++
4 files changed, 11 insertions(+)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 68ea1f949daa..0cd03ac7d3b1 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -544,6 +544,11 @@ static int really_probe(struct device *dev, struct device_driver *drv)
!drv->suppress_bind_attrs;
int ret;

+ if (!dev->authorized && !dev->bus->has_probe_authorization) {
+ dev_dbg(dev, "Device is not authorized\n");
+ return -ENODEV;
+ }
+
if (defer_all_probes) {
/*
* Value of defer_all_probes can be set only by
diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c
index 3e39686eff14..6de8a366b796 100644
--- a/drivers/thunderbolt/domain.c
+++ b/drivers/thunderbolt/domain.c
@@ -321,6 +321,7 @@ struct bus_type tb_bus_type = {
.probe = tb_service_probe,
.remove = tb_service_remove,
.shutdown = tb_service_shutdown,
+ .has_probe_authorization = true,
};

static void tb_domain_release(struct device *dev)
diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index fb476665f52d..f57b5a7a90ca 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,
+ .has_probe_authorization = true,
};
diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h
index 062777a45a74..571a2f6e7c1d 100644
--- a/include/linux/device/bus.h
+++ b/include/linux/device/bus.h
@@ -69,6 +69,9 @@ struct fwnode_handle;
* @lock_key: Lock class key for use by the lock validator
* @need_parent_lock: When probing or removing a device on this bus, the
* device core should lock the device's parent.
+ * @has_probe_authorization: Set true to indicate to the driver-core to skip
+ * the authorization checks and let bus drivers
+ * handle it locally.
*
* A bus is a channel between the processor and one or more devices. For the
* purposes of the device model, all devices are connected via a bus, even if
@@ -112,6 +115,7 @@ struct bus_type {
struct lock_class_key lock_key;

bool need_parent_lock;
+ bool has_probe_authorization;
};

extern int __must_check bus_register(struct bus_type *bus);
--
2.25.1

2021-09-30 01:10:46

[permalink] [raw]

Subject: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

Confidential guest platforms like TDX have a requirement to allow
only trusted devices. By default the confidential-guest core will
arrange for all devices to default to unauthorized (via
dev_default_authorization) in device_initialize(). Since virtio
driver is already hardened against the attack from the un-trusted host,
override the confidential computing default unauthorized state

Reviewed-by: Dan Williams <[email protected]>
Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
---
drivers/virtio/virtio.c | 9 +++++++++
1 file changed, 9 insertions(+)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 588e02fb91d3..377b0ccdc503 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -5,6 +5,8 @@
#include <linux/module.h>
#include <linux/idr.h>
#include <linux/of.h>
+#include <linux/cc_platform.h>
+#include <linux/device.h>
#include <uapi/linux/virtio_ids.h>

/* Unique numbering for virtio devices. */
@@ -390,6 +392,13 @@ int register_virtio_device(struct virtio_device *dev)
dev->config_enabled = false;
dev->config_change_pending = false;

+ /*
+ * For Confidential guest (like TDX), virtio devices are
+ * trusted. So set authorized status as true.
+ */
+ if (cc_platform_has(CC_ATTR_GUEST_DEVICE_FILTER))
+ dev->dev.authorized = true;
+
/* We always start by resetting the device, in case a previous
* driver messed it up. This also tests that code path a little. */
dev->config->reset(dev);
--
2.25.1

2021-09-30 01:10:52

[permalink] [raw]

Subject: [PATCH v2 6/6] PCI: Initialize authorized attribute for confidential guest

Confidential guest platforms like TDX have a requirement to allow
only trusted devices. So initialize the "authorized" attribute
using cc_guest_dev_authorized().

By default the confidential-guest core arranges for all devices to
default to unauthorized (via dev_default_authorization) in
device_initialize(). So, consult a core list of allowed devices to
override that default.

ARCH code will use its device allow list in cc_guest_dev_authorized()
to determine the status of the authorized attribute.

Reviewed-by: Dan Williams <[email protected]>
Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
---
drivers/pci/probe.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index d9fc02a71baa..aab9d1917d52 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -20,6 +20,8 @@
#include <linux/irqdomain.h>
#include <linux/pm_runtime.h>
#include <linux/bitfield.h>
+#include <linux/cc_platform.h>
+#include <linux/device.h>
#include "pci.h"

#define CARDBUS_LATENCY_TIMER 176 /* secondary latency timer */
@@ -2491,6 +2493,8 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
pci_configure_device(dev);

device_initialize(&dev->dev);
+ if (cc_platform_has(CC_ATTR_GUEST_DEVICE_FILTER))
+ dev->dev.authorized = cc_guest_dev_authorized(&dev->dev);
dev->dev.release = pci_release_dev;

set_dev_node(&dev->dev, pcibus_to_node(bus));
--
2.25.1

2021-09-30 01:11:09

[permalink] [raw]

Subject: [PATCH v2 5/6] x86/tdx: Add device filter support for x86 TDX guest platform

For Confidential VM guests like TDX, the host is untrusted and hence
the devices emulated by the host or any data coming from the host
cannot be trusted. So the drivers that interact with the outside world
have to be hardened and the allowed devices have to be filtered. More
details about the need for device/driver filter in confidential guest
can be found in article [1] titled "firewall for device drivers".

So use the "authorized" device attribute to allow only the trusted list
of the devices. Add support for cc_guest_dev_authorized() which can be
used by BUS drivers to consult the arch specific device allow list and
initialize the "authorized" attribute. In order to deny probing for all
but the allowed list of devices @dev_default_authorization is set to
false.

The default audited list of drivers that a protected guest may trust
are:

* virtio-blk
* virtio-console
* virtio-net
* virtio-pci
* virtio_rproc_serial

Add a new flag CC_ATTR_GUEST_DEVICE_FILTER to conditionally enable
device filter related code in generic drivers (using cc_platform_has()
API).

[1] - https://lwn.net/Articles/865918/

Reviewed-by: Andi Kleen <[email protected]>
Reviewed-by: Dan Williams <[email protected]>
Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
---
arch/x86/include/asm/tdx.h | 9 ++++++
arch/x86/kernel/Makefile | 2 +-
arch/x86/kernel/cc_platform.c | 20 ++++++++++++
arch/x86/kernel/cpu/intel.c | 1 +
arch/x86/kernel/tdx-filter.c | 56 +++++++++++++++++++++++++++++++++
arch/x86/kernel/tdx.c | 2 ++
include/linux/cc_platform.h | 10 ++++++
include/linux/device.h | 11 +++++++
include/uapi/linux/virtio_ids.h | 8 +++++
9 files changed, 118 insertions(+), 1 deletion(-)
create mode 100644 arch/x86/kernel/tdx-filter.c

diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 78d146e8a163..c18920703503 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -5,6 +5,7 @@

#include <linux/cpufeature.h>
#include <linux/types.h>
+#include <linux/device.h>
#include <vdso/limits.h>
#include <asm/vmx.h>

@@ -69,6 +70,7 @@ enum tdx_map_type {

void __init tdx_early_init(void);
bool cpuid_has_tdx_guest(void);
+void __init tdx_filter_init(void);

/* Helper function used to communicate with the TDX module */
u64 __tdx_module_call(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
@@ -96,6 +98,8 @@ int tdx_hcall_get_quote(u64 data);

extern void (*tdx_event_notify_handler)(void);

+bool tdx_guest_dev_authorized(struct device *dev);
+
/*
* To support I/O port access in decompressor or early kernel init
* code, since #VE exception handler cannot be used, use paravirt
@@ -169,6 +173,11 @@ static inline int tdx_hcall_gpa_intent(phys_addr_t gpa, int numpages,
return -ENODEV;
}

+static inline bool tdx_guest_dev_authorized(struct device *dev)
+{
+ return dev->authorized;
+}
+
#endif /* CONFIG_INTEL_TDX_GUEST */

#if defined(CONFIG_KVM_GUEST) && defined(CONFIG_INTEL_TDX_GUEST)
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 3d8876f60d5b..2b6f7b0065f5 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -128,7 +128,7 @@ obj-$(CONFIG_PARAVIRT_CLOCK) += pvclock.o
obj-$(CONFIG_X86_PMEM_LEGACY_DEVICE) += pmem.o

obj-$(CONFIG_JAILHOUSE_GUEST) += jailhouse.o
-obj-$(CONFIG_INTEL_TDX_GUEST) += tdcall.o tdx.o
+obj-$(CONFIG_INTEL_TDX_GUEST) += tdcall.o tdx.o tdx-filter.o

obj-$(CONFIG_EISA) += eisa.o
obj-$(CONFIG_PCSPKR_PLATFORM) += pcspeaker.o
diff --git a/arch/x86/kernel/cc_platform.c b/arch/x86/kernel/cc_platform.c
index 3fd2c628e028..3544ed78dd16 100644
--- a/arch/x86/kernel/cc_platform.c
+++ b/arch/x86/kernel/cc_platform.c
@@ -11,6 +11,9 @@
#include <linux/cc_platform.h>
#include <linux/mem_encrypt.h>
#include <linux/processor.h>
+#include <linux/device.h>
+
+#include <asm/tdx.h>

#include <asm/tdx.h>

@@ -24,3 +27,20 @@ bool cc_platform_has(enum cc_attr attr)
return false;
}
EXPORT_SYMBOL_GPL(cc_platform_has);
+
+/*
+ * cc_guest_dev_authorized() - Used to get ARCH specific authorized status
+ * of the given device.
+ * @dev - device structure
+ *
+ * Return True to allow the device or False to deny it.
+ *
+ */
+bool cc_guest_dev_authorized(struct device *dev)
+{
+ if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+ return tdx_guest_dev_authorized(dev);
+
+ return dev->authorized;
+}
+EXPORT_SYMBOL_GPL(cc_guest_dev_authorized);
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 7fbb7f6eb523..1d405750bc16 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -84,6 +84,7 @@ bool intel_cc_platform_has(enum cc_attr attr)
case CC_ATTR_GUEST_MEM_ENCRYPT:
case CC_ATTR_GUEST_SHARED_MAPPING_INIT:
case CC_ATTR_MEM_ENCRYPT:
+ case CC_ATTR_GUEST_DEVICE_FILTER:
return cpu_feature_enabled(X86_FEATURE_TDX_GUEST);
default:
return false;
diff --git a/arch/x86/kernel/tdx-filter.c b/arch/x86/kernel/tdx-filter.c
new file mode 100644
index 000000000000..534cc2cf5851
--- /dev/null
+++ b/arch/x86/kernel/tdx-filter.c
@@ -0,0 +1,56 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2020 Intel Corporation
+ */
+#define pr_fmt(fmt) "TDX: " fmt
+
+#include <linux/acpi.h>
+#include <linux/pci.h>
+#include <linux/device.h>
+#include <linux/cc_platform.h>
+#include <linux/export.h>
+#include <uapi/linux/virtio_ids.h>
+
+#include <asm/tdx.h>
+#include <asm/cmdline.h>
+
+/*
+ * Allow list for PCI bus
+ *
+ * NOTE: Device ID is duplicated here. But for small list
+ * of devices, it is easier to maintain the duplicated list
+ * here verses exporting the device ID table from the driver
+ * and use it.
+ */
+struct pci_device_id pci_allow_ids[] = {
+ { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, VIRTIO_TRANS_ID_NET) },
+ { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, VIRTIO_TRANS_ID_BLOCK) },
+ { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, VIRTIO_TRANS_ID_CONSOLE) },
+ { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, VIRTIO_TRANS_ID_9P) },
+ { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, VIRTIO1_ID_NET) },
+ { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, VIRTIO1_ID_BLOCK) },
+ { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, VIRTIO1_ID_CONSOLE) },
+ { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, VIRTIO1_ID_9P) },
+ { 0, },
+};
+
+bool tdx_guest_dev_authorized(struct device *dev)
+{
+ if (!dev_is_pci(dev))
+ return dev->authorized;
+
+ if (pci_match_id(pci_allow_ids, to_pci_dev(dev)))
+ return true;
+
+ return dev_default_authorization;
+}
+
+void __init tdx_filter_init(void)
+{
+ if (!cc_platform_has(CC_ATTR_GUEST_DEVICE_FILTER))
+ return;
+
+ dev_default_authorization = false;
+
+ pr_info("Enabled TDX guest device filter\n");
+}
diff --git a/arch/x86/kernel/tdx.c b/arch/x86/kernel/tdx.c
index aeeab647e62d..b1d660bd98c6 100644
--- a/arch/x86/kernel/tdx.c
+++ b/arch/x86/kernel/tdx.c
@@ -797,6 +797,8 @@ void __init tdx_early_init(void)

tdx_get_info();

+ tdx_filter_init();
+
pv_ops.irq.safe_halt = tdx_safe_halt;
pv_ops.irq.halt = tdx_halt;

diff --git a/include/linux/cc_platform.h b/include/linux/cc_platform.h
index d39370cfbda1..a0b608bba1ee 100644
--- a/include/linux/cc_platform.h
+++ b/include/linux/cc_platform.h
@@ -94,6 +94,16 @@ enum cc_attr {
*/
CC_ATTR_GUEST_SHARED_MAPPING_INIT,

+ /**
+ * @CC_ATTR_GUEST_DEVICE_FILTER: Filter device enumeration as per
+ * platform specific allow list.
+ *
+ * The platform/OS is running as a guest/virtual machine and allows or
+ * dis-allows device enumeration as per platform specific allow list.
+ *
+ * Examples include TDX guest.
+ */
+ CC_ATTR_GUEST_DEVICE_FILTER,
};

#ifdef CONFIG_ARCH_HAS_CC_PLATFORM
diff --git a/include/linux/device.h b/include/linux/device.h
index c97b1e59d23a..125590e80c35 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -990,4 +990,15 @@ extern long sysfs_deprecated;
#define sysfs_deprecated 0
#endif

+#ifndef __ASSEMBLY__
+#ifdef CONFIG_ARCH_HAS_CC_PLATFORM
+bool cc_guest_dev_authorized(struct device *dev);
+#else
+static inline bool cc_guest_dev_authorized(struct device *dev)
+{
+ return dev->authorized;
+}
+#endif /* CONFIG_ARCH_HAS_CC_PLATFORM */
+#endif /* __ASSEMBLY__ */
+
#endif /* _DEVICE_H_ */
diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
index 80d76b75bccd..68c68d449ea5 100644
--- a/include/uapi/linux/virtio_ids.h
+++ b/include/uapi/linux/virtio_ids.h
@@ -81,4 +81,12 @@
#define VIRTIO_TRANS_ID_RNG 1005 /* transitional virtio rng */
#define VIRTIO_TRANS_ID_9P 1009 /* transitional virtio 9p console */

+/*
+ * Virtio IDS (for PCI rev ID > 1)
+ */
+#define VIRTIO1_ID_NET 1041 /* transitional virtio net */
+#define VIRTIO1_ID_BLOCK 1042 /* transitional virtio block */
+#define VIRTIO1_ID_CONSOLE 1043 /* transitional virtio console */
+#define VIRTIO1_ID_9P 1049 /* transitional virtio 9p console */
+
#endif /* _LINUX_VIRTIO_IDS_H */
--
2.25.1

2021-09-30 01:57:23

[permalink] [raw]

Subject: [PATCH v2 1/6] driver core: Move the "authorized" attribute from USB/Thunderbolt to core

Currently bus drivers like "USB" or "Thunderbolt" implement a custom
version of device authorization to selectively authorize the driver
probes. Since there is a common requirement, move the "authorized"
attribute support to the driver core in order to allow it to be used
by other subsystems / buses.

Similar requirements have been discussed in the PCI [1] community for
PCI bus drivers as well.

No functional changes are intended. It just converts authorized
attribute from int to bool and moves it to the driver core. There
should be no user-visible change in the location or semantics of
attributes for USB devices.

Regarding thunderbolt driver, although it declares sw->authorized as
"int" and allows 0,1,2 as valid values for sw->authorized attribute,
but within the driver, in all authorized attribute related checks,
it is treated as bool value. So when converting the authorized
attribute from int to bool value, there should be no functional
changes other than value 2 being not visible to the user.

[1]: https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=VB3pULx_GyT2NkzAogdRQJ9TKSX9A@mail.gmail.com/

Reviewed-by: Dan Williams <[email protected]>
Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
---
drivers/thunderbolt/domain.c | 6 +++---
drivers/thunderbolt/icm.c | 9 +++++----
drivers/thunderbolt/switch.c | 18 ++++++++----------
drivers/thunderbolt/tb.c | 2 +-
drivers/thunderbolt/tb.h | 2 --
drivers/usb/core/driver.c | 2 +-
drivers/usb/core/generic.c | 2 +-
drivers/usb/core/hub.c | 8 ++++----
drivers/usb/core/message.c | 2 +-
drivers/usb/core/sysfs.c | 3 +--
drivers/usb/core/usb.c | 10 +++++++++-
include/linux/device.h | 3 ++-
include/linux/usb.h | 6 ------
13 files changed, 36 insertions(+), 37 deletions(-)

diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c
index 7018d959f775..3e39686eff14 100644
--- a/drivers/thunderbolt/domain.c
+++ b/drivers/thunderbolt/domain.c
@@ -656,7 +656,7 @@ int tb_domain_approve_switch(struct tb *tb, struct tb_switch *sw)

/* The parent switch must be authorized before this one */
parent_sw = tb_to_switch(sw->dev.parent);
- if (!parent_sw || !parent_sw->authorized)
+ if (!parent_sw || !parent_sw->dev.authorized)
return -EINVAL;

return tb->cm_ops->approve_switch(tb, sw);
@@ -683,7 +683,7 @@ int tb_domain_approve_switch_key(struct tb *tb, struct tb_switch *sw)

/* The parent switch must be authorized before this one */
parent_sw = tb_to_switch(sw->dev.parent);
- if (!parent_sw || !parent_sw->authorized)
+ if (!parent_sw || !parent_sw->dev.authorized)
return -EINVAL;

ret = tb->cm_ops->add_switch_key(tb, sw);
@@ -720,7 +720,7 @@ int tb_domain_challenge_switch_key(struct tb *tb, struct tb_switch *sw)

/* The parent switch must be authorized before this one */
parent_sw = tb_to_switch(sw->dev.parent);
- if (!parent_sw || !parent_sw->authorized)
+ if (!parent_sw || !parent_sw->dev.authorized)
return -EINVAL;

get_random_bytes(challenge, sizeof(challenge));
diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c
index 6255f1ef9599..f5b784c1cabb 100644
--- a/drivers/thunderbolt/icm.c
+++ b/drivers/thunderbolt/icm.c
@@ -768,7 +768,7 @@ icm_fr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr)
* sure our book keeping matches that.
*/
if (sw->depth == depth && sw_phy_port == phy_port &&
- !!sw->authorized == authorized) {
+ sw->dev.authorized == authorized) {
/*
* It was enumerated through another link so update
* route string accordingly.
@@ -849,7 +849,7 @@ icm_fr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr)
sw->connection_key = pkg->connection_key;
sw->link = link;
sw->depth = depth;
- sw->authorized = authorized;
+ sw->dev.authorized = authorized;
sw->security_level = security_level;
sw->boot = boot;
sw->link_speed = speed_gen3 ? 20 : 10;
@@ -1235,7 +1235,8 @@ __icm_tr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr,
sw = tb_switch_find_by_uuid(tb, &pkg->ep_uuid);
if (sw) {
/* Update the switch if it is still in the same place */
- if (tb_route(sw) == route && !!sw->authorized == authorized) {
+ if (tb_route(sw) == route &&
+ sw->dev.authorized == authorized) {
parent_sw = tb_to_switch(sw->dev.parent);
update_switch(parent_sw, sw, route, pkg->connection_id,
0, 0, 0, boot);
@@ -1272,7 +1273,7 @@ __icm_tr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr,
sw = alloc_switch(parent_sw, route, &pkg->ep_uuid);
if (!IS_ERR(sw)) {
sw->connection_id = pkg->connection_id;
- sw->authorized = authorized;
+ sw->dev.authorized = authorized;
sw->security_level = security_level;
sw->boot = boot;
sw->link_speed = speed_gen3 ? 20 : 10;
diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index 3014146081c1..e640d764499a 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -1494,9 +1494,7 @@ static ssize_t authorized_show(struct device *dev,
struct device_attribute *attr,
char *buf)
{
- struct tb_switch *sw = tb_to_switch(dev);
-
- return sprintf(buf, "%u\n", sw->authorized);
+ return sprintf(buf, "%u\n", dev->authorized);
}

static int disapprove_switch(struct device *dev, void *not_used)
@@ -1505,7 +1503,7 @@ static int disapprove_switch(struct device *dev, void *not_used)
struct tb_switch *sw;

sw = tb_to_switch(dev);
- if (sw && sw->authorized) {
+ if (sw && sw->dev.authorized) {
int ret;

/* First children */
@@ -1517,7 +1515,7 @@ static int disapprove_switch(struct device *dev, void *not_used)
if (ret)
return ret;

- sw->authorized = 0;
+ dev->authorized = false;
kobject_uevent_env(&sw->dev.kobj, KOBJ_CHANGE, envp);
}

@@ -1533,7 +1531,7 @@ static int tb_switch_set_authorized(struct tb_switch *sw, unsigned int val)
if (!mutex_trylock(&sw->tb->lock))
return restart_syscall();

- if (!!sw->authorized == !!val)
+ if (sw->dev.authorized == !!val)
goto unlock;

switch (val) {
@@ -1564,12 +1562,12 @@ static int tb_switch_set_authorized(struct tb_switch *sw, unsigned int val)
}

if (!ret) {
- sw->authorized = val;
+ sw->dev.authorized = !!val;
/*
* Notify status change to the userspace, informing the new
* value of /sys/bus/thunderbolt/devices/.../authorized.
*/
- sprintf(envp_string, "AUTHORIZED=%u", sw->authorized);
+ sprintf(envp_string, "AUTHORIZED=%u", sw->dev.authorized);
kobject_uevent_env(&sw->dev.kobj, KOBJ_CHANGE, envp);
}

@@ -1671,7 +1669,7 @@ static ssize_t key_store(struct device *dev, struct device_attribute *attr,
if (!mutex_trylock(&sw->tb->lock))
return restart_syscall();

- if (sw->authorized) {
+ if (sw->dev.authorized) {
ret = -EBUSY;
} else {
kfree(sw->key);
@@ -2192,7 +2190,7 @@ struct tb_switch *tb_switch_alloc(struct tb *tb, struct device *parent,

/* Root switch is always authorized */
if (!route)
- sw->authorized = true;
+ sw->dev.authorized = true;

device_initialize(&sw->dev);
sw->dev.parent = parent;
diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
index 2897a77d44c3..44d2fa893fa9 100644
--- a/drivers/thunderbolt/tb.c
+++ b/drivers/thunderbolt/tb.c
@@ -1323,7 +1323,7 @@ static int tb_scan_finalize_switch(struct device *dev, void *data)
* send uevent to userspace.
*/
if (sw->boot)
- sw->authorized = 1;
+ sw->dev.authorized = true;

dev_set_uevent_suppress(dev, false);
kobject_uevent(&dev->kobj, KOBJ_ADD);
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index 725104c83e3d..cfe869d8e826 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -130,7 +130,6 @@ struct tb_switch_tmu {
* @safe_mode: The switch is in safe-mode
* @boot: Whether the switch was already authorized on boot or not
* @rpm: The switch supports runtime PM
- * @authorized: Whether the switch is authorized by user or policy
* @security_level: Switch supported security level
* @debugfs_dir: Pointer to the debugfs structure
* @key: Contains the key used to challenge the device or %NULL if not
@@ -180,7 +179,6 @@ struct tb_switch {
bool safe_mode;
bool boot;
bool rpm;
- unsigned int authorized;
enum tb_security_level security_level;
struct dentry *debugfs_dir;
u8 *key;
diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 072968c40ade..fb476665f52d 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -331,7 +331,7 @@ static int usb_probe_interface(struct device *dev)
if (usb_device_is_owned(udev))
return error;

- if (udev->authorized == 0) {
+ if (udev->dev.authorized == false) {
dev_err(&intf->dev, "Device is not authorized for usage\n");
return error;
} else if (intf->authorized == 0) {
diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c
index 26f9fb9f67ca..7fa4ca77fa89 100644
--- a/drivers/usb/core/generic.c
+++ b/drivers/usb/core/generic.c
@@ -230,7 +230,7 @@ int usb_generic_driver_probe(struct usb_device *udev)
/* Choose and set the configuration. This registers the interfaces
* with the driver core and lets interface drivers bind to them.
*/
- if (udev->authorized == 0)
+ if (udev->dev.authorized == false)
dev_err(&udev->dev, "Device is not authorized for usage\n");
else {
c = usb_choose_configuration(udev);
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 86658a81d284..f58b19aa4f5f 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2616,10 +2616,10 @@ int usb_new_device(struct usb_device *udev)
int usb_deauthorize_device(struct usb_device *usb_dev)
{
usb_lock_device(usb_dev);
- if (usb_dev->authorized == 0)
+ if (usb_dev->dev.authorized == false)
goto out_unauthorized;

- usb_dev->authorized = 0;
+ usb_dev->dev.authorized = false;
usb_set_configuration(usb_dev, -1);

out_unauthorized:
@@ -2633,7 +2633,7 @@ int usb_authorize_device(struct usb_device *usb_dev)
int result = 0, c;

usb_lock_device(usb_dev);
- if (usb_dev->authorized == 1)
+ if (usb_dev->dev.authorized == true)
goto out_authorized;

result = usb_autoresume_device(usb_dev);
@@ -2652,7 +2652,7 @@ int usb_authorize_device(struct usb_device *usb_dev)
}
}

- usb_dev->authorized = 1;
+ usb_dev->dev.authorized = true;
/* Choose and set the configuration. This registers the interfaces
* with the driver core and lets interface drivers bind to them.
*/
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 4d59d927ae3e..47548ce1cfb1 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1962,7 +1962,7 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
struct usb_hcd *hcd = bus_to_hcd(dev->bus);
int n, nintf;

- if (dev->authorized == 0 || configuration == -1)
+ if (dev->dev.authorized == false || configuration == -1)
configuration = 0;
else {
for (i = 0; i < dev->descriptor.bNumConfigurations; i++) {
diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
index fa2e49d432ff..3d63e345d0a0 100644
--- a/drivers/usb/core/sysfs.c
+++ b/drivers/usb/core/sysfs.c
@@ -726,8 +726,7 @@ usb_descriptor_attr(bMaxPacketSize0, "%d\n");
static ssize_t authorized_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
- struct usb_device *usb_dev = to_usb_device(dev);
- return snprintf(buf, PAGE_SIZE, "%u\n", usb_dev->authorized);
+ return snprintf(buf, PAGE_SIZE, "%u\n", dev->authorized);
}

/*
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 62368c4ed37a..18f3ad39ccbc 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -579,6 +579,14 @@ static unsigned usb_bus_is_wusb(struct usb_bus *bus)
return hcd->wireless;
}

+/*
+ * usb_dev_authorized() - Used to initialize the "authorized" status of
+ * the USB device.
+ * (user space) policy determines if we authorize this device to be
+ * used or not. By default, wired USB devices are authorized.
+ * WUSB devices are not, until we authorize them from user space.
+ * FIXME -- complete doc
+ */
static bool usb_dev_authorized(struct usb_device *dev, struct usb_hcd *hcd)
{
struct usb_hub *hub;
@@ -717,7 +725,7 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent,
dev->active_duration = -jiffies;
#endif

- dev->authorized = usb_dev_authorized(dev, usb_hcd);
+ dev->dev.authorized = usb_dev_authorized(dev, usb_hcd);
if (!root_hub)
dev->wusb = usb_bus_is_wusb(bus) ? 1 : 0;

diff --git a/include/linux/device.h b/include/linux/device.h
index e270cb740b9e..899be9a2c0cb 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -443,7 +443,7 @@ struct dev_links_info {
* @removable: Whether the device can be removed from the system. This
* should be set by the subsystem / bus driver that discovered
* the device.
- *
+ * @authorized: Whether the device is authorized to bind to a driver.
* @offline_disabled: If set, the device is permanently online.
* @offline: Set after successful invocation of bus type's .offline().
* @of_node_reused: Set if the device-tree node is shared with an ancestor
@@ -562,6 +562,7 @@ struct device {

enum device_removable removable;

+ bool authorized:1;
bool offline_disabled:1;
bool offline:1;
bool of_node_reused:1;
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 7ccaa76a9a96..796df4068e94 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -576,11 +576,6 @@ struct usb3_lpm_parameters {
* @can_submit: URBs may be submitted
* @persist_enabled: USB_PERSIST enabled for this device
* @have_langid: whether string_langid is valid
- * @authorized: policy has said we can use it;
- * (user space) policy determines if we authorize this device to be
- * used or not. By default, wired USB devices are authorized.
- * WUSB devices are not, until we authorize them from user space.
- * FIXME -- complete doc
* @authenticated: Crypto authentication passed
* @wusb: device is Wireless USB
* @lpm_capable: device supports LPM
@@ -662,7 +657,6 @@ struct usb_device {
unsigned can_submit:1;
unsigned persist_enabled:1;
unsigned have_langid:1;
- unsigned authorized:1;
unsigned authenticated:1;
unsigned wusb:1;
unsigned lpm_capable:1;
--
2.25.1

2021-09-30 02:09:33

[permalink] [raw]

Subject: Re: [PATCH v2 1/6] driver core: Move the "authorized" attribute from USB/Thunderbolt to core

On Wed, Sep 29, 2021 at 06:05:06PM -0700, Kuppuswamy Sathyanarayanan wrote:
> Currently bus drivers like "USB" or "Thunderbolt" implement a custom
> version of device authorization to selectively authorize the driver
> probes. Since there is a common requirement, move the "authorized"
> attribute support to the driver core in order to allow it to be used
> by other subsystems / buses.
>
> Similar requirements have been discussed in the PCI [1] community for
> PCI bus drivers as well.
>
> No functional changes are intended. It just converts authorized
> attribute from int to bool and moves it to the driver core. There
> should be no user-visible change in the location or semantics of
> attributes for USB devices.
>
> Regarding thunderbolt driver, although it declares sw->authorized as
> "int" and allows 0,1,2 as valid values for sw->authorized attribute,
> but within the driver, in all authorized attribute related checks,
> it is treated as bool value. So when converting the authorized
> attribute from int to bool value, there should be no functional
> changes other than value 2 being not visible to the user.
>
> [1]: https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=VB3pULx_GyT2NkzAogdRQJ9TKSX9A@mail.gmail.com/
>
> Reviewed-by: Dan Williams <[email protected]>
> Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>

Since you're moving the authorized flag from the USB core to the
driver core, the corresponding sysfs attribute functions should be
moved as well.

Also, you ignored the usb_[de]authorize_interface() functions and
their friends.

Alan Stern

2021-09-30 02:10:44

[permalink] [raw]

Subject: Re: [PATCH v2 1/6] driver core: Move the "authorized" attribute from USB/Thunderbolt to core

On Wed, Sep 29, 2021 at 6:43 PM Alan Stern <[email protected]> wrote:
>
> On Wed, Sep 29, 2021 at 06:05:06PM -0700, Kuppuswamy Sathyanarayanan wrote:
> > Currently bus drivers like "USB" or "Thunderbolt" implement a custom
> > version of device authorization to selectively authorize the driver
> > probes. Since there is a common requirement, move the "authorized"
> > attribute support to the driver core in order to allow it to be used
> > by other subsystems / buses.
> >
> > Similar requirements have been discussed in the PCI [1] community for
> > PCI bus drivers as well.
> >
> > No functional changes are intended. It just converts authorized
> > attribute from int to bool and moves it to the driver core. There
> > should be no user-visible change in the location or semantics of
> > attributes for USB devices.
> >
> > Regarding thunderbolt driver, although it declares sw->authorized as
> > "int" and allows 0,1,2 as valid values for sw->authorized attribute,
> > but within the driver, in all authorized attribute related checks,
> > it is treated as bool value. So when converting the authorized
> > attribute from int to bool value, there should be no functional
> > changes other than value 2 being not visible to the user.
> >
> > [1]: https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=VB3pULx_GyT2NkzAogdRQJ9TKSX9A@mail.gmail.com/
> >
> > Reviewed-by: Dan Williams <[email protected]>
> > Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
>
> Since you're moving the authorized flag from the USB core to the
> driver core, the corresponding sysfs attribute functions should be
> moved as well.

Unlike when 'removable' moved from USB to the driver core there isn't
a common definition for how the 'authorized' sysfs-attribute behaves
across buses. The only common piece is where this flag is stored in
the data structure, i.e. the 'authorized' sysfs interface is
purposefully left bus specific.

> Also, you ignored the usb_[de]authorize_interface() functions and
> their friends.

Ugh, yes.

2021-09-30 03:30:50

[permalink] [raw]

Subject: Re: [PATCH v2 1/6] driver core: Move the "authorized" attribute from USB/Thunderbolt to core

On 9/29/21 6:55 PM, Dan Williams wrote:
>> Also, you ignored the usb_[de]authorize_interface() functions and
>> their friends.
> Ugh, yes.

I did not change it because I am not sure about the interface vs device
dependency.

I think following change should work.

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index f57b5a7a90ca..84969732d09c 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -334,7 +334,7 @@ static int usb_probe_interface(struct device *dev)
if (udev->dev.authorized == false) {
dev_err(&intf->dev, "Device is not authorized for usage\n");
return error;
- } else if (intf->authorized == 0) {
+ } else if (intf->dev.authorized == 0) {
dev_err(&intf->dev, "Interface %d is not authorized for usage\n",
intf->altsetting->desc.bInterfaceNumber);
return error;
@@ -546,7 +546,7 @@ int usb_driver_claim_interface(struct usb_driver *driver,
return -EBUSY;

/* reject claim if interface is not authorized */
- if (!iface->authorized)
+ if (!iface->dev.authorized)
return -ENODEV;

dev->driver = &driver->drvwrap.driver;
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 47548ce1cfb1..ab3c8d1e4db9 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1791,9 +1791,9 @@ void usb_deauthorize_interface(struct usb_interface *intf)

device_lock(dev->parent);

- if (intf->authorized) {
+ if (intf->dev.authorized) {
device_lock(dev);
- intf->authorized = 0;
+ intf->dev.authorized = 0;
device_unlock(dev);

usb_forced_unbind_intf(intf);
@@ -1811,9 +1811,9 @@ void usb_authorize_interface(struct usb_interface *intf)
{
struct device *dev = &intf->dev;

- if (!intf->authorized) {
+ if (!intf->dev.authorized) {
device_lock(dev);
- intf->authorized = 1; /* authorize interface */
+ intf->dev.authorized = 1; /* authorize interface */
device_unlock(dev);
}
}
@@ -2069,7 +2069,6 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
intfc = cp->intf_cache[i];
intf->altsetting = intfc->altsetting;
intf->num_altsetting = intfc->num_altsetting;
- intf->authorized = !!HCD_INTF_AUTHORIZED(hcd);
kref_get(&intfc->ref);

alt = usb_altnum_to_altsetting(intf, 0);
@@ -2101,6 +2100,7 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
INIT_WORK(&intf->reset_ws, __usb_queue_reset_device);
intf->minor = -1;
device_initialize(&intf->dev);
+ intf->dev.authorized = !!HCD_INTF_AUTHORIZED(hcd);
pm_runtime_no_callbacks(&intf->dev);
dev_set_name(&intf->dev, "%d-%s:%d.%d", dev->bus->busnum,
dev->devpath, configuration, ifnum);
diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
index 3d63e345d0a0..666eeb80ff29 100644
--- a/drivers/usb/core/sysfs.c
+++ b/drivers/usb/core/sysfs.c
@@ -1160,9 +1160,7 @@ static DEVICE_ATTR_RO(supports_autosuspend);
static ssize_t interface_authorized_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
- struct usb_interface *intf = to_usb_interface(dev);
-
- return sprintf(buf, "%u\n", intf->authorized);
+ return sprintf(buf, "%u\n", dev->authorized);
}

/*
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 796df4068e94..1e41453c63cb 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -195,8 +195,6 @@ usb_find_last_int_out_endpoint(struct usb_host_interface *alt,
* has been deferred.
* @needs_binding: flag set when the driver should be re-probed or unbound
* following a reset or suspend operation it doesn't support.
- * @authorized: This allows to (de)authorize individual interfaces instead
- * a whole device in contrast to the device authorization.
* @dev: driver model's view of this device
* @usb_dev: if an interface is bound to the USB major, this will point
* to the sysfs representation for that device.
@@ -252,7 +250,6 @@ struct usb_interface {
unsigned needs_altsetting0:1; /* switch to altsetting 0 is pending */
unsigned needs_binding:1; /* needs delayed unbind/rebind */
unsigned resetting_device:1; /* true: bandwidth alloc after reset */
- unsigned authorized:1; /* used for interface authorization */

struct device dev; /* interface specific device info */
struct device *usb_dev;
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

2021-09-30 05:07:59

[permalink] [raw]

Subject: Re: [PATCH v2 1/6] driver core: Move the "authorized" attribute from USB/Thunderbolt to core

On Wed, Sep 29, 2021 at 7:39 PM Kuppuswamy, Sathyanarayanan
<[email protected]> wrote:
>
>
>
> On 9/29/21 6:55 PM, Dan Williams wrote:
> >> Also, you ignored the usb_[de]authorize_interface() functions and
> >> their friends.
> > Ugh, yes.
>
> I did not change it because I am not sure about the interface vs device
> dependency.
>

This is was the rationale for has_probe_authorization flag. USB
performs authorization of child devices based on the authorization
state of the parent interface.

> I think following change should work.
>
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index f57b5a7a90ca..84969732d09c 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -334,7 +334,7 @@ static int usb_probe_interface(struct device *dev)
> if (udev->dev.authorized == false) {
> dev_err(&intf->dev, "Device is not authorized for usage\n");
> return error;
> - } else if (intf->authorized == 0) {
> + } else if (intf->dev.authorized == 0) {

== false.

> dev_err(&intf->dev, "Interface %d is not authorized for usage\n",
> intf->altsetting->desc.bInterfaceNumber);
> return error;
> @@ -546,7 +546,7 @@ int usb_driver_claim_interface(struct usb_driver *driver,
> return -EBUSY;
>
> /* reject claim if interface is not authorized */
> - if (!iface->authorized)
> + if (!iface->dev.authorized)

I'd do == false to keep it consistent with other conversions.

> return -ENODEV;
>
> dev->driver = &driver->drvwrap.driver;
> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index 47548ce1cfb1..ab3c8d1e4db9 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -1791,9 +1791,9 @@ void usb_deauthorize_interface(struct usb_interface *intf)
>
> device_lock(dev->parent);
>
> - if (intf->authorized) {
> + if (intf->dev.authorized) {
> device_lock(dev);
> - intf->authorized = 0;
> + intf->dev.authorized = 0;

= false;

> device_unlock(dev);
>
> usb_forced_unbind_intf(intf);
> @@ -1811,9 +1811,9 @@ void usb_authorize_interface(struct usb_interface *intf)
> {
> struct device *dev = &intf->dev;
>
> - if (!intf->authorized) {
> + if (!intf->dev.authorized) {
> device_lock(dev);
> - intf->authorized = 1; /* authorize interface */
> + intf->dev.authorized = 1; /* authorize interface */

= true

...not sure that comment is worth preserving.

2021-09-30 11:03:31

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

On Wed, Sep 29, 2021 at 06:05:07PM -0700, Kuppuswamy Sathyanarayanan wrote:
> While the common case for device-authorization is to skip probe of
> unauthorized devices, some buses may still want to emit a message on
> probe failure (Thunderbolt), or base probe failures on the
> authorization status of a related device like a parent (USB). So add
> an option (has_probe_authorization) in struct bus_type for the bus
> driver to own probe authorization policy.
>
> Reviewed-by: Dan Williams <[email protected]>
> Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>

So what e.g. the PCI patch
https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=VB3pULx_GyT2NkzAogdRQJ9TKSX9A@mail.gmail.com/
actually proposes is a list of
allowed drivers, not devices. Doing it at the device level
has disadvantages, for example some devices might have a legacy
unsafe driver, or an out of tree driver. It also does not
address drivers that poke at hardware during init.

Accordingly, I think the right thing to do is to skip
driver init for disallowed drivers, not skip probe
for specific devices.

> ---
> drivers/base/dd.c | 5 +++++
> drivers/thunderbolt/domain.c | 1 +
> drivers/usb/core/driver.c | 1 +
> include/linux/device/bus.h | 4 ++++
> 4 files changed, 11 insertions(+)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 68ea1f949daa..0cd03ac7d3b1 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -544,6 +544,11 @@ static int really_probe(struct device *dev, struct device_driver *drv)
> !drv->suppress_bind_attrs;
> int ret;
>
> + if (!dev->authorized && !dev->bus->has_probe_authorization) {
> + dev_dbg(dev, "Device is not authorized\n");
> + return -ENODEV;
> + }
> +
> if (defer_all_probes) {
> /*
> * Value of defer_all_probes can be set only by
> diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c
> index 3e39686eff14..6de8a366b796 100644
> --- a/drivers/thunderbolt/domain.c
> +++ b/drivers/thunderbolt/domain.c
> @@ -321,6 +321,7 @@ struct bus_type tb_bus_type = {
> .probe = tb_service_probe,
> .remove = tb_service_remove,
> .shutdown = tb_service_shutdown,
> + .has_probe_authorization = true,
> };
>
> static void tb_domain_release(struct device *dev)
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index fb476665f52d..f57b5a7a90ca 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,
> + .has_probe_authorization = true,
> };
> diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h
> index 062777a45a74..571a2f6e7c1d 100644
> --- a/include/linux/device/bus.h
> +++ b/include/linux/device/bus.h
> @@ -69,6 +69,9 @@ struct fwnode_handle;
> * @lock_key: Lock class key for use by the lock validator
> * @need_parent_lock: When probing or removing a device on this bus, the
> * device core should lock the device's parent.
> + * @has_probe_authorization: Set true to indicate to the driver-core to skip
> + * the authorization checks and let bus drivers
> + * handle it locally.
> *
> * A bus is a channel between the processor and one or more devices. For the
> * purposes of the device model, all devices are connected via a bus, even if
> @@ -112,6 +115,7 @@ struct bus_type {
> struct lock_class_key lock_key;
>
> bool need_parent_lock;
> + bool has_probe_authorization;
> };
>
> extern int __must_check bus_register(struct bus_type *bus);
> --
> 2.25.1

2021-09-30 12:31:52

by Yehezkel Bernat

[permalink] [raw]

Subject: Re: [PATCH v2 1/6] driver core: Move the "authorized" attribute from USB/Thunderbolt to core

On Thu, Sep 30, 2021 at 4:05 AM Kuppuswamy Sathyanarayanan
<[email protected]> wrote:
>
> no functional
> changes other than value 2 being not visible to the user.
>

Are we sure we don't break any user-facing tool with it? Tools might use this to
"remember" how the device was authorized this time.

2021-09-30 13:52:02

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

On Thu, Sep 30, 2021 at 06:36:18AM -0700, Dan Williams wrote:
> On Thu, Sep 30, 2021 at 4:03 AM Michael S. Tsirkin <[email protected]> wrote:
> >
> > On Wed, Sep 29, 2021 at 06:05:09PM -0700, Kuppuswamy Sathyanarayanan wrote:
> > > Confidential guest platforms like TDX have a requirement to allow
> > > only trusted devices. By default the confidential-guest core will
> > > arrange for all devices to default to unauthorized (via
> > > dev_default_authorization) in device_initialize(). Since virtio
> > > driver is already hardened against the attack from the un-trusted host,
> > > override the confidential computing default unauthorized state
> > >
> > > Reviewed-by: Dan Williams <[email protected]>
> > > Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
> >
> > Architecturally this all looks backwards. IIUC nothing about virtio
> > makes it authorized or trusted. The driver is hardened,
> > true, but this should be set at the driver not the device level.
>
> That's was my initial reaction to this proposal as well, and I ended
> up leading Sathya astray from what Greg wanted. Greg rightly points
> out that the "authorized" attribute from USB and Thunderbolt already
> exists [1] [2]. So the choice is find an awkward way to mix driver
> trust with existing bus-local "authorized" mechanisms, or promote the
> authorized capability to the driver-core. This patch set implements
> the latter to keep the momentum on the already shipping design scheme
> to not add to the driver-core maintenance burden.
>
> [1]: https://lore.kernel.org/all/[email protected]/
> [2]: https://lore.kernel.org/all/YQzF%[email protected]/
>
> > And in particular, not all virtio drivers are hardened -
> > I think at this point blk and scsi drivers have been hardened - so
> > treating them all the same looks wrong.
>
> My understanding was that they have been audited, Sathya?

Please define "audited" and "trusted" here.

thanks,

greg k-h

2021-09-30 14:22:58

by Rafael J. Wysocki

[permalink] [raw]

Subject: Re: [PATCH v2 1/6] driver core: Move the "authorized" attribute from USB/Thunderbolt to core

On Thu, Sep 30, 2021 at 6:59 AM Dan Williams <[email protected]> wrote:
>
> On Wed, Sep 29, 2021 at 7:39 PM Kuppuswamy, Sathyanarayanan
> <[email protected]> wrote:
> >
> >
> >
> > On 9/29/21 6:55 PM, Dan Williams wrote:
> > >> Also, you ignored the usb_[de]authorize_interface() functions and
> > >> their friends.
> > > Ugh, yes.
> >
> > I did not change it because I am not sure about the interface vs device
> > dependency.
> >
>
> This is was the rationale for has_probe_authorization flag. USB
> performs authorization of child devices based on the authorization
> state of the parent interface.
>
> > I think following change should work.
> >
> > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> > index f57b5a7a90ca..84969732d09c 100644
> > --- a/drivers/usb/core/driver.c
> > +++ b/drivers/usb/core/driver.c
> > @@ -334,7 +334,7 @@ static int usb_probe_interface(struct device *dev)
> > if (udev->dev.authorized == false) {
> > dev_err(&intf->dev, "Device is not authorized for usage\n");
> > return error;
> > - } else if (intf->authorized == 0) {
> > + } else if (intf->dev.authorized == 0) {
>
> == false

Or even (!intf->dev.authorized).

> > dev_err(&intf->dev, "Interface %d is not authorized for usage\n",
> > intf->altsetting->desc.bInterfaceNumber);
> > return error;
> > @@ -546,7 +546,7 @@ int usb_driver_claim_interface(struct usb_driver *driver,
> > return -EBUSY;
> >
> > /* reject claim if interface is not authorized */
> > - if (!iface->authorized)
> > + if (!iface->dev.authorized)
>
> I'd do == false to keep it consistent with other conversions.

But this looks odd, FWIW.

2021-09-30 14:28:10

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

On Wed, Sep 29, 2021 at 06:05:09PM -0700, Kuppuswamy Sathyanarayanan wrote:
> Confidential guest platforms like TDX have a requirement to allow
> only trusted devices. By default the confidential-guest core will
> arrange for all devices to default to unauthorized (via
> dev_default_authorization) in device_initialize(). Since virtio
> driver is already hardened against the attack from the un-trusted host,
> override the confidential computing default unauthorized state
>
> Reviewed-by: Dan Williams <[email protected]>
> Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>

Architecturally this all looks backwards. IIUC nothing about virtio
makes it authorized or trusted. The driver is hardened,
true, but this should be set at the driver not the device level.
And in particular, not all virtio drivers are hardened -
I think at this point blk and scsi drivers have been hardened - so
treating them all the same looks wrong.

> ---
> drivers/virtio/virtio.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 588e02fb91d3..377b0ccdc503 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -5,6 +5,8 @@
> #include <linux/module.h>
> #include <linux/idr.h>
> #include <linux/of.h>
> +#include <linux/cc_platform.h>
> +#include <linux/device.h>
> #include <uapi/linux/virtio_ids.h>
>
> /* Unique numbering for virtio devices. */
> @@ -390,6 +392,13 @@ int register_virtio_device(struct virtio_device *dev)
> dev->config_enabled = false;
> dev->config_change_pending = false;
>
> + /*
> + * For Confidential guest (like TDX), virtio devices are
> + * trusted. So set authorized status as true.
> + */
> + if (cc_platform_has(CC_ATTR_GUEST_DEVICE_FILTER))
> + dev->dev.authorized = true;
> +
> /* We always start by resetting the device, in case a previous
> * driver messed it up. This also tests that code path a little. */
> dev->config->reset(dev);
> --
> 2.25.1

2021-09-30 14:44:56

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

On Thu, Sep 30, 2021 at 4:03 AM Michael S. Tsirkin <[email protected]> wrote:
>
> On Wed, Sep 29, 2021 at 06:05:09PM -0700, Kuppuswamy Sathyanarayanan wrote:
> > Confidential guest platforms like TDX have a requirement to allow
> > only trusted devices. By default the confidential-guest core will
> > arrange for all devices to default to unauthorized (via
> > dev_default_authorization) in device_initialize(). Since virtio
> > driver is already hardened against the attack from the un-trusted host,
> > override the confidential computing default unauthorized state
> >
> > Reviewed-by: Dan Williams <[email protected]>
> > Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
>
> Architecturally this all looks backwards. IIUC nothing about virtio
> makes it authorized or trusted. The driver is hardened,
> true, but this should be set at the driver not the device level.

That's was my initial reaction to this proposal as well, and I ended
up leading Sathya astray from what Greg wanted. Greg rightly points
out that the "authorized" attribute from USB and Thunderbolt already
exists [1] [2]. So the choice is find an awkward way to mix driver
trust with existing bus-local "authorized" mechanisms, or promote the
authorized capability to the driver-core. This patch set implements
the latter to keep the momentum on the already shipping design scheme
to not add to the driver-core maintenance burden.

[1]: https://lore.kernel.org/all/[email protected]/
[2]: https://lore.kernel.org/all/YQzF%[email protected]/

> And in particular, not all virtio drivers are hardened -
> I think at this point blk and scsi drivers have been hardened - so
> treating them all the same looks wrong.

My understanding was that they have been audited, Sathya?

2021-09-30 14:49:24

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

On Thu, Sep 30, 2021 at 06:59:36AM -0400, Michael S. Tsirkin wrote:
> On Wed, Sep 29, 2021 at 06:05:07PM -0700, Kuppuswamy Sathyanarayanan wrote:
> > While the common case for device-authorization is to skip probe of
> > unauthorized devices, some buses may still want to emit a message on
> > probe failure (Thunderbolt), or base probe failures on the
> > authorization status of a related device like a parent (USB). So add
> > an option (has_probe_authorization) in struct bus_type for the bus
> > driver to own probe authorization policy.
> >
> > Reviewed-by: Dan Williams <[email protected]>
> > Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
>
>
>
> So what e.g. the PCI patch
> https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=VB3pULx_GyT2NkzAogdRQJ9TKSX9A@mail.gmail.com/
> actually proposes is a list of
> allowed drivers, not devices. Doing it at the device level
> has disadvantages, for example some devices might have a legacy
> unsafe driver, or an out of tree driver. It also does not
> address drivers that poke at hardware during init.

Doing it at a device level is the only sane way to do this.

A user needs to say "this device is allowed to be controlled by this
driver". This is the trust model that USB has had for over a decade and
what thunderbolt also has.

> Accordingly, I think the right thing to do is to skip
> driver init for disallowed drivers, not skip probe
> for specific devices.

What do you mean by "driver init"? module_init()?

No driver should be touching hardware in their module init call. They
should only be touching it in the probe callback as that is the only
time they are ever allowed to talk to hardware. Specifically the device
that has been handed to them.

If there are in-kernel PCI drivers that do not do this, they need to be
fixed today.

We don't care about out-of-tree drivers for obvious reasons that we have
no control over them.

thanks,

greg k-h

2021-09-30 15:09:05

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

On Thu, Sep 30, 2021 at 03:52:52PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Sep 30, 2021 at 06:59:36AM -0400, Michael S. Tsirkin wrote:
> > On Wed, Sep 29, 2021 at 06:05:07PM -0700, Kuppuswamy Sathyanarayanan wrote:
> > > While the common case for device-authorization is to skip probe of
> > > unauthorized devices, some buses may still want to emit a message on
> > > probe failure (Thunderbolt), or base probe failures on the
> > > authorization status of a related device like a parent (USB). So add
> > > an option (has_probe_authorization) in struct bus_type for the bus
> > > driver to own probe authorization policy.
> > >
> > > Reviewed-by: Dan Williams <[email protected]>
> > > Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
> >
> >
> >
> > So what e.g. the PCI patch
> > https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=VB3pULx_GyT2NkzAogdRQJ9TKSX9A@mail.gmail.com/
> > actually proposes is a list of
> > allowed drivers, not devices. Doing it at the device level
> > has disadvantages, for example some devices might have a legacy
> > unsafe driver, or an out of tree driver. It also does not
> > address drivers that poke at hardware during init.
>
> Doing it at a device level is the only sane way to do this.
>
> A user needs to say "this device is allowed to be controlled by this
> driver". This is the trust model that USB has had for over a decade and
> what thunderbolt also has.
>
> > Accordingly, I think the right thing to do is to skip
> > driver init for disallowed drivers, not skip probe
> > for specific devices.
>
> What do you mean by "driver init"? module_init()?
>
> No driver should be touching hardware in their module init call. They
> should only be touching it in the probe callback as that is the only
> time they are ever allowed to talk to hardware. Specifically the device
> that has been handed to them.
>
> If there are in-kernel PCI drivers that do not do this, they need to be
> fixed today.
>
> We don't care about out-of-tree drivers for obvious reasons that we have
> no control over them.
>
> thanks,
>
> greg k-h

Well talk to Andi about it pls :)
https://lore.kernel.org/r/ad1e41d1-3f4e-8982-16ea-18a3b2c04019%40linux.intel.com

--
MST

2021-09-30 15:21:37

[permalink] [raw]

Subject: Re: [PATCH v2 1/6] driver core: Move the "authorized" attribute from USB/Thunderbolt to core

On Wed, Sep 29, 2021 at 06:55:12PM -0700, Dan Williams wrote:
> On Wed, Sep 29, 2021 at 6:43 PM Alan Stern <[email protected]> wrote:
> >
> > On Wed, Sep 29, 2021 at 06:05:06PM -0700, Kuppuswamy Sathyanarayanan wrote:
> > > Currently bus drivers like "USB" or "Thunderbolt" implement a custom
> > > version of device authorization to selectively authorize the driver
> > > probes. Since there is a common requirement, move the "authorized"
> > > attribute support to the driver core in order to allow it to be used
> > > by other subsystems / buses.
> > >
> > > Similar requirements have been discussed in the PCI [1] community for
> > > PCI bus drivers as well.
> > >
> > > No functional changes are intended. It just converts authorized
> > > attribute from int to bool and moves it to the driver core. There
> > > should be no user-visible change in the location or semantics of
> > > attributes for USB devices.
> > >
> > > Regarding thunderbolt driver, although it declares sw->authorized as
> > > "int" and allows 0,1,2 as valid values for sw->authorized attribute,
> > > but within the driver, in all authorized attribute related checks,
> > > it is treated as bool value. So when converting the authorized
> > > attribute from int to bool value, there should be no functional
> > > changes other than value 2 being not visible to the user.
> > >
> > > [1]: https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=VB3pULx_GyT2NkzAogdRQJ9TKSX9A@mail.gmail.com/
> > >
> > > Reviewed-by: Dan Williams <[email protected]>
> > > Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
> >
> > Since you're moving the authorized flag from the USB core to the
> > driver core, the corresponding sysfs attribute functions should be
> > moved as well.
>
> Unlike when 'removable' moved from USB to the driver core there isn't
> a common definition for how the 'authorized' sysfs-attribute behaves
> across buses. The only common piece is where this flag is stored in
> the data structure, i.e. the 'authorized' sysfs interface is
> purposefully left bus specific.

How about implementing "library" versions of show_authorized() and
store_authorized() that the bus-specific attribute routines can call?
These library routines would handle parsing the input values, storing
the new flag, and displaying the stored flag value. That way at
least the common parts of these APIs would be centralized in the
driver core, and any additional functionality could easily be added
by the bus-specific attribute routine.

Alan Stern

2021-09-30 15:23:45

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

On Thu, Sep 30, 2021 at 04:49:23PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Sep 30, 2021 at 10:38:42AM -0400, Michael S. Tsirkin wrote:
> > On Thu, Sep 30, 2021 at 03:52:52PM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Sep 30, 2021 at 06:59:36AM -0400, Michael S. Tsirkin wrote:
> > > > On Wed, Sep 29, 2021 at 06:05:07PM -0700, Kuppuswamy Sathyanarayanan wrote:
> > > > > While the common case for device-authorization is to skip probe of
> > > > > unauthorized devices, some buses may still want to emit a message on
> > > > > probe failure (Thunderbolt), or base probe failures on the
> > > > > authorization status of a related device like a parent (USB). So add
> > > > > an option (has_probe_authorization) in struct bus_type for the bus
> > > > > driver to own probe authorization policy.
> > > > >
> > > > > Reviewed-by: Dan Williams <[email protected]>
> > > > > Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
> > > >
> > > >
> > > >
> > > > So what e.g. the PCI patch
> > > > https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=VB3pULx_GyT2NkzAogdRQJ9TKSX9A@mail.gmail.com/
> > > > actually proposes is a list of
> > > > allowed drivers, not devices. Doing it at the device level
> > > > has disadvantages, for example some devices might have a legacy
> > > > unsafe driver, or an out of tree driver. It also does not
> > > > address drivers that poke at hardware during init.
> > >
> > > Doing it at a device level is the only sane way to do this.
> > >
> > > A user needs to say "this device is allowed to be controlled by this
> > > driver". This is the trust model that USB has had for over a decade and
> > > what thunderbolt also has.
> > >
> > > > Accordingly, I think the right thing to do is to skip
> > > > driver init for disallowed drivers, not skip probe
> > > > for specific devices.
> > >
> > > What do you mean by "driver init"? module_init()?
> > >
> > > No driver should be touching hardware in their module init call. They
> > > should only be touching it in the probe callback as that is the only
> > > time they are ever allowed to talk to hardware. Specifically the device
> > > that has been handed to them.
> > >
> > > If there are in-kernel PCI drivers that do not do this, they need to be
> > > fixed today.
> > >
> > > We don't care about out-of-tree drivers for obvious reasons that we have
> > > no control over them.
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> > Well talk to Andi about it pls :)
> > https://lore.kernel.org/r/ad1e41d1-3f4e-8982-16ea-18a3b2c04019%40linux.intel.com
>
> As Alan said, the minute you allow any driver to get into your kernel,
> it can do anything it wants to.
>
> So just don't allow drivers to be added to your kernel if you care about
> these things. The system owner has that mechanism today.
>
> thanks,
>
> greg k-h

The "it" that I referred to is the claim that no driver should be
touching hardware in their module init call. Andi seems to think
such drivers are worth working around with a special remap API.

--
MST

2021-09-30 15:28:44

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

2021-09-30 15:28:55

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

On 9/30/21 8:20 AM, Michael S. Tsirkin wrote:
> On Thu, Sep 30, 2021 at 08:18:18AM -0700, Kuppuswamy, Sathyanarayanan wrote:
>>
>>
>> On 9/30/21 6:36 AM, Dan Williams wrote:
>>>> And in particular, not all virtio drivers are hardened -
>>>> I think at this point blk and scsi drivers have been hardened - so
>>>> treating them all the same looks wrong.
>>> My understanding was that they have been audited, Sathya?
>>
>> Yes, AFAIK, it has been audited. Andi also submitted some patches
>> related to it. Andi, can you confirm.
>>
>> We also authorize the virtio at PCI ID level. And currently we allow
>> console, block and net virtio PCI devices.
>>
>> { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, VIRTIO_TRANS_ID_NET) },
>> { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, VIRTIO_TRANS_ID_BLOCK) },
>> { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, VIRTIO_TRANS_ID_CONSOLE) },
>
> Presumably modern IDs should be allowed too?

You mean IDs in range 104x right? If yes, we also allow them for above
mentioned types.

>

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

2021-09-30 15:30:45

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

On Thu, Sep 30, 2021 at 11:00:07AM -0400, Michael S. Tsirkin wrote:
> On Thu, Sep 30, 2021 at 04:49:23PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Sep 30, 2021 at 10:38:42AM -0400, Michael S. Tsirkin wrote:
> > > On Thu, Sep 30, 2021 at 03:52:52PM +0200, Greg Kroah-Hartman wrote:
> > > > On Thu, Sep 30, 2021 at 06:59:36AM -0400, Michael S. Tsirkin wrote:
> > > > > On Wed, Sep 29, 2021 at 06:05:07PM -0700, Kuppuswamy Sathyanarayanan wrote:
> > > > > > While the common case for device-authorization is to skip probe of
> > > > > > unauthorized devices, some buses may still want to emit a message on
> > > > > > probe failure (Thunderbolt), or base probe failures on the
> > > > > > authorization status of a related device like a parent (USB). So add
> > > > > > an option (has_probe_authorization) in struct bus_type for the bus
> > > > > > driver to own probe authorization policy.
> > > > > >
> > > > > > Reviewed-by: Dan Williams <[email protected]>
> > > > > > Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
> > > > >
> > > > >
> > > > >
> > > > > So what e.g. the PCI patch
> > > > > https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=VB3pULx_GyT2NkzAogdRQJ9TKSX9A@mail.gmail.com/
> > > > > actually proposes is a list of
> > > > > allowed drivers, not devices. Doing it at the device level
> > > > > has disadvantages, for example some devices might have a legacy
> > > > > unsafe driver, or an out of tree driver. It also does not
> > > > > address drivers that poke at hardware during init.
> > > >
> > > > Doing it at a device level is the only sane way to do this.
> > > >
> > > > A user needs to say "this device is allowed to be controlled by this
> > > > driver". This is the trust model that USB has had for over a decade and
> > > > what thunderbolt also has.
> > > >
> > > > > Accordingly, I think the right thing to do is to skip
> > > > > driver init for disallowed drivers, not skip probe
> > > > > for specific devices.
> > > >
> > > > What do you mean by "driver init"? module_init()?
> > > >
> > > > No driver should be touching hardware in their module init call. They
> > > > should only be touching it in the probe callback as that is the only
> > > > time they are ever allowed to talk to hardware. Specifically the device
> > > > that has been handed to them.
> > > >
> > > > If there are in-kernel PCI drivers that do not do this, they need to be
> > > > fixed today.
> > > >
> > > > We don't care about out-of-tree drivers for obvious reasons that we have
> > > > no control over them.
> > > >
> > > > thanks,
> > > >
> > > > greg k-h
> > >
> > > Well talk to Andi about it pls :)
> > > https://lore.kernel.org/r/ad1e41d1-3f4e-8982-16ea-18a3b2c04019%40linux.intel.com
> >
> > As Alan said, the minute you allow any driver to get into your kernel,
> > it can do anything it wants to.
> >
> > So just don't allow drivers to be added to your kernel if you care about
> > these things. The system owner has that mechanism today.
> >
> > thanks,
> >
> > greg k-h
>
> The "it" that I referred to is the claim that no driver should be
> touching hardware in their module init call. Andi seems to think
> such drivers are worth working around with a special remap API.

Andi is wrong.

2021-09-30 15:33:49

[permalink] [raw]

Subject: Re: [PATCH v2 1/6] driver core: Move the "authorized" attribute from USB/Thunderbolt to core

On Thu, Sep 30, 2021 at 8:00 AM Alan Stern <[email protected]> wrote:
>
> On Wed, Sep 29, 2021 at 06:55:12PM -0700, Dan Williams wrote:
> > On Wed, Sep 29, 2021 at 6:43 PM Alan Stern <[email protected]> wrote:
> > >
> > > On Wed, Sep 29, 2021 at 06:05:06PM -0700, Kuppuswamy Sathyanarayanan wrote:
> > > > Currently bus drivers like "USB" or "Thunderbolt" implement a custom
> > > > version of device authorization to selectively authorize the driver
> > > > probes. Since there is a common requirement, move the "authorized"
> > > > attribute support to the driver core in order to allow it to be used
> > > > by other subsystems / buses.
> > > >
> > > > Similar requirements have been discussed in the PCI [1] community for
> > > > PCI bus drivers as well.
> > > >
> > > > No functional changes are intended. It just converts authorized
> > > > attribute from int to bool and moves it to the driver core. There
> > > > should be no user-visible change in the location or semantics of
> > > > attributes for USB devices.
> > > >
> > > > Regarding thunderbolt driver, although it declares sw->authorized as
> > > > "int" and allows 0,1,2 as valid values for sw->authorized attribute,
> > > > but within the driver, in all authorized attribute related checks,
> > > > it is treated as bool value. So when converting the authorized
> > > > attribute from int to bool value, there should be no functional
> > > > changes other than value 2 being not visible to the user.
> > > >
> > > > [1]: https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=VB3pULx_GyT2NkzAogdRQJ9TKSX9A@mail.gmail.com/
> > > >
> > > > Reviewed-by: Dan Williams <[email protected]>
> > > > Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
> > >
> > > Since you're moving the authorized flag from the USB core to the
> > > driver core, the corresponding sysfs attribute functions should be
> > > moved as well.
> >
> > Unlike when 'removable' moved from USB to the driver core there isn't
> > a common definition for how the 'authorized' sysfs-attribute behaves
> > across buses. The only common piece is where this flag is stored in
> > the data structure, i.e. the 'authorized' sysfs interface is
> > purposefully left bus specific.
>
> How about implementing "library" versions of show_authorized() and
> store_authorized() that the bus-specific attribute routines can call?
> These library routines would handle parsing the input values, storing
> the new flag, and displaying the stored flag value. That way at
> least the common parts of these APIs would be centralized in the
> driver core, and any additional functionality could easily be added
> by the bus-specific attribute routine.
>

While show_authorized() seems like it could be standardized, have a
look at what the different store_authorized() implementations do.
Thunderbolt wants "switch approval" vs "switch challenge" and USB has
a bunch of bus-specific work to do when the authorization state
changes. I don't see much room for a library to help there as more
buses add authorization support. That said I do think it would be
useful to have a common implementation available for generic probe
authorization to toggle the flag if the bus does not have any
authorization work to do, but that seems a follow-on once this core is
accepted.

2021-09-30 15:36:32

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

On Thu, Sep 30, 2021 at 10:48:54AM -0400, Michael S. Tsirkin wrote:
> On Thu, Sep 30, 2021 at 10:43:05AM -0400, Alan Stern wrote:
> > I don't see any point in talking about "untrusted drivers". If a
> > driver isn't trusted then it doesn't belong in your kernel. Period.
> > When you load a driver into your kernel, you are implicitly trusting
> > it (aside from limitations imposed by security modules). The code
> > it contains, the module_init code in particular, runs with full
> > superuser permissions.
> >
> > What use is there in loading a driver but telling the kernel "I don't
> > trust this driver, so don't allow it to probe any devices"? Why not
> > just blacklist it so that it never gets modprobed in the first place?
> >
> > Alan Stern
>
> When the driver is built-in, it seems useful to be able to block it
> without rebuilding the kernel. This is just flipping it around
> and using an allow-list for cases where you want to severly
> limit the available functionality.

Does this make sense?

The only way to tell the kernel to block a built-in driver is by
using some boot-command-line option. Otherwise the driver's init
code will run before you have a chance to tell the kernel anything at
all.

So if you change your mind about whether a driver should be blocked,
all you have to do is remove the blocking option from the command
line and reboot. No kernel rebuild is necessary.

Alan Stern

2021-09-30 15:37:52

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

On Thu, Sep 30, 2021 at 10:58:07AM -0400, Michael S. Tsirkin wrote:
> On Thu, Sep 30, 2021 at 10:43:05AM -0400, Alan Stern wrote:
> > I don't see any point in talking about "untrusted drivers". If a
> > driver isn't trusted then it doesn't belong in your kernel. Period.
> > When you load a driver into your kernel, you are implicitly trusting
> > it (aside from limitations imposed by security modules).
>
> Trusting it to do what? Historically a ton of drivers did not
> validate input from devices they drive. Most still don't.

Trusting it to behave properly (i.e., not destroy your system, among
other things).

The fact that many drivers haven't been trustworthy is beside the
point. By loading them into your kernel, you are trusting them
regardless. In the end, you may regret having done so. :-(

Alan Stern

2021-09-30 16:01:40

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

On Thu, Sep 30, 2021 at 11:35:09AM -0400, Alan Stern wrote:
> On Thu, Sep 30, 2021 at 10:58:07AM -0400, Michael S. Tsirkin wrote:
> > On Thu, Sep 30, 2021 at 10:43:05AM -0400, Alan Stern wrote:
> > > I don't see any point in talking about "untrusted drivers". If a
> > > driver isn't trusted then it doesn't belong in your kernel. Period.
> > > When you load a driver into your kernel, you are implicitly trusting
> > > it (aside from limitations imposed by security modules).
> >
> > Trusting it to do what? Historically a ton of drivers did not
> > validate input from devices they drive. Most still don't.
>
> Trusting it to behave properly (i.e., not destroy your system, among
> other things).

I don't think the current mitigations under discussion here are about
keeping the system working. In fact most encrypted VM configs tend to
stop booting as a preferred way to handle security issues.

> The fact that many drivers haven't been trustworthy is beside the
> point. By loading them into your kernel, you are trusting them
> regardless. In the end, you may regret having done so. :-(
>
> Alan Stern

--
MST

2021-09-30 17:19:56

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

>> The "it" that I referred to is the claim that no driver should be
>> touching hardware in their module init call. Andi seems to think
>> such drivers are worth working around with a special remap API.
> Andi is wrong.

While overall it's a small percentage of the total, there are still
quite a few drivers that do touch hardware in init functions. Sometimes
for good reasons -- they need to do some extra probing to discover
something that is not enumerated -- sometimes just because it's very old
legacy code that predates the modern driver model.

The legacy drivers could be fixed, but nobody really wants to touch them
anymore and they're impossible to test.

The drivers that probe something that is not enumerated in a standard
way have no choice, it cannot be implemented in a different way.

So instead we're using a "firewall" the prevents these drivers from
doing bad things by not allowing ioremap access unless opted in, and
also do some filtering on the IO ports The device filter is still the
primary mechanism, the ioremap filtering is just belts and suspenders
for those odd cases.

If you want we can send an exact list, we did some analysis using a
patched smatch tool.

-Andi

2021-09-30 17:26:46

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

On Thu, Sep 30, 2021 at 10:17:09AM -0700, Andi Kleen wrote:
>
> > > The "it" that I referred to is the claim that no driver should be
> > > touching hardware in their module init call. Andi seems to think
> > > such drivers are worth working around with a special remap API.
> > Andi is wrong.
>
> While overall it's a small percentage of the total, there are still quite a
> few drivers that do touch hardware in init functions. Sometimes for good
> reasons -- they need to do some extra probing to discover something that is
> not enumerated -- sometimes just because it's very old legacy code that
> predates the modern driver model.

Are any of them in the kernel today?

PCI drivers should not be messing with this, we have had well over a
decade to fix that up.

> The legacy drivers could be fixed, but nobody really wants to touch them
> anymore and they're impossible to test.

Pointers to them?

> The drivers that probe something that is not enumerated in a standard way
> have no choice, it cannot be implemented in a different way.

PCI devices are not enumerated in a standard way???

> So instead we're using a "firewall" the prevents these drivers from doing
> bad things by not allowing ioremap access unless opted in, and also do some
> filtering on the IO ports The device filter is still the primary mechanism,
> the ioremap filtering is just belts and suspenders for those odd cases.

That's horrible, don't try to protect the kernel from itself. Just fix
the drivers.

If you point me at them, I will be glad to have a look and throw some
interns on them.

But really, you all could have fixed them up by now if Intel really
cared about it :(

> If you want we can send an exact list, we did some analysis using a patched
> smatch tool.

Please do.

thanks,

greg k-h

2021-09-30 18:29:18

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

On Thu, Sep 30, 2021 at 03:52:52PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Sep 30, 2021 at 06:59:36AM -0400, Michael S. Tsirkin wrote:
> > On Wed, Sep 29, 2021 at 06:05:07PM -0700, Kuppuswamy Sathyanarayanan wrote:
> > > While the common case for device-authorization is to skip probe of
> > > unauthorized devices, some buses may still want to emit a message on
> > > probe failure (Thunderbolt), or base probe failures on the
> > > authorization status of a related device like a parent (USB). So add
> > > an option (has_probe_authorization) in struct bus_type for the bus
> > > driver to own probe authorization policy.
> > >
> > > Reviewed-by: Dan Williams <[email protected]>
> > > Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
> >
> >
> >
> > So what e.g. the PCI patch
> > https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=VB3pULx_GyT2NkzAogdRQJ9TKSX9A@mail.gmail.com/
> > actually proposes is a list of
> > allowed drivers, not devices. Doing it at the device level
> > has disadvantages, for example some devices might have a legacy
> > unsafe driver, or an out of tree driver. It also does not
> > address drivers that poke at hardware during init.
>
> Doing it at a device level is the only sane way to do this.
>
> A user needs to say "this device is allowed to be controlled by this
> driver". This is the trust model that USB has had for over a decade and
> what thunderbolt also has.
>
> > Accordingly, I think the right thing to do is to skip
> > driver init for disallowed drivers, not skip probe
> > for specific devices.
>
> What do you mean by "driver init"? module_init()?
>
> No driver should be touching hardware in their module init call. They
> should only be touching it in the probe callback as that is the only
> time they are ever allowed to talk to hardware. Specifically the device
> that has been handed to them.
>
> If there are in-kernel PCI drivers that do not do this, they need to be
> fixed today.
>
> We don't care about out-of-tree drivers for obvious reasons that we have
> no control over them.

I don't see any point in talking about "untrusted drivers". If a
driver isn't trusted then it doesn't belong in your kernel. Period.
When you load a driver into your kernel, you are implicitly trusting
it (aside from limitations imposed by security modules). The code
it contains, the module_init code in particular, runs with full
superuser permissions.

What use is there in loading a driver but telling the kernel "I don't
trust this driver, so don't allow it to probe any devices"? Why not
just blacklist it so that it never gets modprobed in the first place?

Alan Stern

2021-09-30 18:29:50

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

On Thu, Sep 30, 2021 at 10:38:42AM -0400, Michael S. Tsirkin wrote:
> On Thu, Sep 30, 2021 at 03:52:52PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Sep 30, 2021 at 06:59:36AM -0400, Michael S. Tsirkin wrote:
> > > On Wed, Sep 29, 2021 at 06:05:07PM -0700, Kuppuswamy Sathyanarayanan wrote:
> > > > While the common case for device-authorization is to skip probe of
> > > > unauthorized devices, some buses may still want to emit a message on
> > > > probe failure (Thunderbolt), or base probe failures on the
> > > > authorization status of a related device like a parent (USB). So add
> > > > an option (has_probe_authorization) in struct bus_type for the bus
> > > > driver to own probe authorization policy.
> > > >
> > > > Reviewed-by: Dan Williams <[email protected]>
> > > > Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
> > >
> > >
> > >
> > > So what e.g. the PCI patch
> > > https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=VB3pULx_GyT2NkzAogdRQJ9TKSX9A@mail.gmail.com/
> > > actually proposes is a list of
> > > allowed drivers, not devices. Doing it at the device level
> > > has disadvantages, for example some devices might have a legacy
> > > unsafe driver, or an out of tree driver. It also does not
> > > address drivers that poke at hardware during init.
> >
> > Doing it at a device level is the only sane way to do this.
> >
> > A user needs to say "this device is allowed to be controlled by this
> > driver". This is the trust model that USB has had for over a decade and
> > what thunderbolt also has.
> >
> > > Accordingly, I think the right thing to do is to skip
> > > driver init for disallowed drivers, not skip probe
> > > for specific devices.
> >
> > What do you mean by "driver init"? module_init()?
> >
> > No driver should be touching hardware in their module init call. They
> > should only be touching it in the probe callback as that is the only
> > time they are ever allowed to talk to hardware. Specifically the device
> > that has been handed to them.
> >
> > If there are in-kernel PCI drivers that do not do this, they need to be
> > fixed today.
> >
> > We don't care about out-of-tree drivers for obvious reasons that we have
> > no control over them.
> >
> > thanks,
> >
> > greg k-h
>
> Well talk to Andi about it pls :)
> https://lore.kernel.org/r/ad1e41d1-3f4e-8982-16ea-18a3b2c04019%40linux.intel.com

As Alan said, the minute you allow any driver to get into your kernel,
it can do anything it wants to.

So just don't allow drivers to be added to your kernel if you care about
these things. The system owner has that mechanism today.

thanks,

greg k-h

2021-09-30 18:31:34

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

On Thu, Sep 30, 2021 at 10:43:05AM -0400, Alan Stern wrote:
> I don't see any point in talking about "untrusted drivers". If a
> driver isn't trusted then it doesn't belong in your kernel. Period.
> When you load a driver into your kernel, you are implicitly trusting
> it (aside from limitations imposed by security modules).

Trusting it to do what? Historically a ton of drivers did not
validate input from devices they drive. Most still don't.

> The code
> it contains, the module_init code in particular, runs with full
> superuser permissions.

--
MST

2021-09-30 18:43:01

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

On Thu, Sep 30, 2021 at 10:43:05AM -0400, Alan Stern wrote:
> On Thu, Sep 30, 2021 at 03:52:52PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Sep 30, 2021 at 06:59:36AM -0400, Michael S. Tsirkin wrote:
> > > On Wed, Sep 29, 2021 at 06:05:07PM -0700, Kuppuswamy Sathyanarayanan wrote:
> > > > While the common case for device-authorization is to skip probe of
> > > > unauthorized devices, some buses may still want to emit a message on
> > > > probe failure (Thunderbolt), or base probe failures on the
> > > > authorization status of a related device like a parent (USB). So add
> > > > an option (has_probe_authorization) in struct bus_type for the bus
> > > > driver to own probe authorization policy.
> > > >
> > > > Reviewed-by: Dan Williams <[email protected]>
> > > > Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
> > >
> > >
> > >
> > > So what e.g. the PCI patch
> > > https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=VB3pULx_GyT2NkzAogdRQJ9TKSX9A@mail.gmail.com/
> > > actually proposes is a list of
> > > allowed drivers, not devices. Doing it at the device level
> > > has disadvantages, for example some devices might have a legacy
> > > unsafe driver, or an out of tree driver. It also does not
> > > address drivers that poke at hardware during init.
> >
> > Doing it at a device level is the only sane way to do this.
> >
> > A user needs to say "this device is allowed to be controlled by this
> > driver". This is the trust model that USB has had for over a decade and
> > what thunderbolt also has.
> >
> > > Accordingly, I think the right thing to do is to skip
> > > driver init for disallowed drivers, not skip probe
> > > for specific devices.
> >
> > What do you mean by "driver init"? module_init()?
> >
> > No driver should be touching hardware in their module init call. They
> > should only be touching it in the probe callback as that is the only
> > time they are ever allowed to talk to hardware. Specifically the device
> > that has been handed to them.
> >
> > If there are in-kernel PCI drivers that do not do this, they need to be
> > fixed today.
> >
> > We don't care about out-of-tree drivers for obvious reasons that we have
> > no control over them.
>
> I don't see any point in talking about "untrusted drivers". If a
> driver isn't trusted then it doesn't belong in your kernel. Period.
> When you load a driver into your kernel, you are implicitly trusting
> it (aside from limitations imposed by security modules). The code
> it contains, the module_init code in particular, runs with full
> superuser permissions.
>
> What use is there in loading a driver but telling the kernel "I don't
> trust this driver, so don't allow it to probe any devices"? Why not
> just blacklist it so that it never gets modprobed in the first place?
>
> Alan Stern

When the driver is built-in, it seems useful to be able to block it
without rebuilding the kernel. This is just flipping it around
and using an allow-list for cases where you want to severly
limit the available functionality.

--
MST

2021-09-30 18:52:04

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

On 9/30/21 6:36 AM, Dan Williams wrote:
>> And in particular, not all virtio drivers are hardened -
>> I think at this point blk and scsi drivers have been hardened - so
>> treating them all the same looks wrong.
> My understanding was that they have been audited, Sathya?

Yes, AFAIK, it has been audited. Andi also submitted some patches
related to it. Andi, can you confirm.

We also authorize the virtio at PCI ID level. And currently we allow
console, block and net virtio PCI devices.

{ PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, VIRTIO_TRANS_ID_NET) },
{ PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, VIRTIO_TRANS_ID_BLOCK) },
{ PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, VIRTIO_TRANS_ID_CONSOLE) },

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

2021-09-30 18:52:10

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

On Thu, Sep 30, 2021 at 08:18:18AM -0700, Kuppuswamy, Sathyanarayanan wrote:
>
>
> On 9/30/21 6:36 AM, Dan Williams wrote:
> > > And in particular, not all virtio drivers are hardened -
> > > I think at this point blk and scsi drivers have been hardened - so
> > > treating them all the same looks wrong.
> > My understanding was that they have been audited, Sathya?
>
> Yes, AFAIK, it has been audited. Andi also submitted some patches
> related to it. Andi, can you confirm.
>
> We also authorize the virtio at PCI ID level. And currently we allow
> console, block and net virtio PCI devices.
>
> { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, VIRTIO_TRANS_ID_NET) },
> { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, VIRTIO_TRANS_ID_BLOCK) },
> { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, VIRTIO_TRANS_ID_CONSOLE) },

Presumably modern IDs should be allowed too?

--
MST

2021-09-30 18:52:30

[permalink] [raw]

Subject: Re: [PATCH v2 1/6] driver core: Move the "authorized" attribute from USB/Thunderbolt to core

On Thu, Sep 30, 2021 at 4:20 AM Yehezkel Bernat <[email protected]> wrote:
>
> On Thu, Sep 30, 2021 at 4:05 AM Kuppuswamy Sathyanarayanan
> <[email protected]> wrote:
> >
> > no functional
> > changes other than value 2 being not visible to the user.
> >
>
> Are we sure we don't break any user-facing tool with it? Tools might use this to
> "remember" how the device was authorized this time.

That's why it was highlighted in the changelog. Hopefully a
Thunderbolt developer can confirm if it is a non-issue.
Documentation/ABI/testing/sysfs-bus-thunderbolt does not seem to
answer this question about whether authorized_show and
authorized_store need to be symmetric.

2021-09-30 18:53:16

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

On Thu, Sep 30, 2021 at 11:32:41AM -0400, Alan Stern wrote:
> On Thu, Sep 30, 2021 at 10:48:54AM -0400, Michael S. Tsirkin wrote:
> > On Thu, Sep 30, 2021 at 10:43:05AM -0400, Alan Stern wrote:
> > > I don't see any point in talking about "untrusted drivers". If a
> > > driver isn't trusted then it doesn't belong in your kernel. Period.
> > > When you load a driver into your kernel, you are implicitly trusting
> > > it (aside from limitations imposed by security modules). The code
> > > it contains, the module_init code in particular, runs with full
> > > superuser permissions.
> > >
> > > What use is there in loading a driver but telling the kernel "I don't
> > > trust this driver, so don't allow it to probe any devices"? Why not
> > > just blacklist it so that it never gets modprobed in the first place?
> > >
> > > Alan Stern
> >
> > When the driver is built-in, it seems useful to be able to block it
> > without rebuilding the kernel. This is just flipping it around
> > and using an allow-list for cases where you want to severly
> > limit the available functionality.
>
> Does this make sense?
>
> The only way to tell the kernel to block a built-in driver is by
> using some boot-command-line option. Otherwise the driver's init
> code will run before you have a chance to tell the kernel anything at
> all.
>
> So if you change your mind about whether a driver should be blocked,
> all you have to do is remove the blocking option from the command
> line and reboot. No kernel rebuild is necessary.
>
> Alan Stern

Right.

--
MST

2021-09-30 19:51:15

[permalink] [raw]

Subject: Re: [PATCH v2 1/6] driver core: Move the "authorized" attribute from USB/Thunderbolt to core

On Thu, Sep 30, 2021 at 11:25 AM Yehezkel Bernat <[email protected]> wrote:
>
> On Thu, Sep 30, 2021 at 6:28 PM Dan Williams <[email protected]> wrote:
> >
> > On Thu, Sep 30, 2021 at 4:20 AM Yehezkel Bernat <[email protected]> wrote:
> > >
> > > On Thu, Sep 30, 2021 at 4:05 AM Kuppuswamy Sathyanarayanan
> > > <[email protected]> wrote:
> > > >
> > > > no functional
> > > > changes other than value 2 being not visible to the user.
> > > >
> > >
> > > Are we sure we don't break any user-facing tool with it? Tools might use this to
> > > "remember" how the device was authorized this time.
> >
> > That's why it was highlighted in the changelog. Hopefully a
> > Thunderbolt developer can confirm if it is a non-issue.
> > Documentation/ABI/testing/sysfs-bus-thunderbolt does not seem to
> > answer this question about whether authorized_show and
> > authorized_store need to be symmetric.
>
> Apparently, Bolt does read it [1] and cares about it [2].

Ah, thank you!

Yeah, looks like the conversion to bool was indeed too hopeful.

>
> [1] https://gitlab.freedesktop.org/bolt/bolt/-/blob/130e09d1c7ff02c09e4ad1c9c36e9940b68e58d8/boltd/bolt-sysfs.c#L511
> [2] https://gitlab.freedesktop.org/bolt/bolt/-/blob/130e09d1c7ff02c09e4ad1c9c36e9940b68e58d8/boltd/bolt-device.c#L639

2021-09-30 19:53:41

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

On 9/30/2021 10:23 AM, Greg Kroah-Hartman wrote:
> On Thu, Sep 30, 2021 at 10:17:09AM -0700, Andi Kleen wrote:
>>>> The "it" that I referred to is the claim that no driver should be
>>>> touching hardware in their module init call. Andi seems to think
>>>> such drivers are worth working around with a special remap API.
>>> Andi is wrong.
>> While overall it's a small percentage of the total, there are still quite a
>> few drivers that do touch hardware in init functions. Sometimes for good
>> reasons -- they need to do some extra probing to discover something that is
>> not enumerated -- sometimes just because it's very old legacy code that
>> predates the modern driver model.
> Are any of them in the kernel today?
>
> PCI drivers should not be messing with this, we have had well over a
> decade to fix that up.

It's not just PCI driver, it's every driver that can do io port / MMIO /
MSR / config space accesses.

Maybe read the excellent article from Jon on this:

https://lwn.net/Articles/865918/

>
>> The legacy drivers could be fixed, but nobody really wants to touch them
>> anymore and they're impossible to test.
> Pointers to them?

For example if you look over old SCSI drivers in drivers/scsi/*.c there
is a substantial number that has a module init longer than just
registering a driver. As a single example look at drivers/scsi/BusLogic.c

There were also quite a few platform drivers like this.

>
>> The drivers that probe something that is not enumerated in a standard way
>> have no choice, it cannot be implemented in a different way.
> PCI devices are not enumerated in a standard way???

The pci devices are enumerated in a standard way, but typically the
driver also needs something else outside PCI that needs some other
probing mechanism.

>
>> So instead we're using a "firewall" the prevents these drivers from doing
>> bad things by not allowing ioremap access unless opted in, and also do some
>> filtering on the IO ports The device filter is still the primary mechanism,
>> the ioremap filtering is just belts and suspenders for those odd cases.
> That's horrible, don't try to protect the kernel from itself. Just fix
> the drivers.

I thought we had already established this last time we discussed it.

That's completely impractical. We cannot harden thousands of drivers,
especially since it would be all wasted work since nobody will ever need
them in virtual guests. Even if we could harden them how would such a
work be maintained long term? Using a firewall and filtering mechanism
is much saner for everyone.

-Andi

2021-09-30 19:55:12

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

+Elena

On 9/30/21 12:04 PM, Kuppuswamy, Sathyanarayanan wrote:
>
>
> On 9/30/21 8:23 AM, Greg Kroah-Hartman wrote:
>> On Thu, Sep 30, 2021 at 08:18:18AM -0700, Kuppuswamy, Sathyanarayanan wrote:
>>>
>>>
>>> On 9/30/21 6:36 AM, Dan Williams wrote:
>>>>> And in particular, not all virtio drivers are hardened -
>>>>> I think at this point blk and scsi drivers have been hardened - so
>>>>> treating them all the same looks wrong.
>>>> My understanding was that they have been audited, Sathya?
>>>
>>> Yes, AFAIK, it has been audited. Andi also submitted some patches
>>> related to it. Andi, can you confirm.
>>
>> What is the official definition of "audited"?
>
>
> In our case (Confidential Computing platform), the host is an un-trusted
> entity. So any interaction with host from the drivers will have to be
> protected against the possible attack from the host. For example, if we
> are accessing a memory based on index value received from host, we have
> to make sure it does not lead to out of bound access or when sharing the
> memory with the host, we need to make sure only the required region is
> shared with the host and the memory is un-shared after use properly.
>
> Elena can share more details, but it was achieved with static analysis
> and fuzzing. Here is a presentation she is sharing about the work at the
> Linux Security Summit:
> https://static.sched.com/hosted_files/lssna2021/b6/LSS-HardeningLinuxGuestForCCC.pdf
>
> Andi, can talk more about the specific driver changes that came out of this
> effort.
>
> In our case the driver is considered "hardened" / "audited" if it can handle
> the above scenarios.
>
>>
>> thanks,
>>
>> greg k-h
>>
>

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

2021-09-30 19:57:08

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

> I don't think the current mitigations under discussion here are about
> keeping the system working. In fact most encrypted VM configs tend to
> stop booting as a preferred way to handle security issues.

Maybe we should avoid the "trusted" term here. We're only really using
it because USB is using it and we're now using a common framework like
Greg requested. But I don't think it's the right way to think about it.

We usually call the drivers "hardened". The requirement for a hardened
driver is that all interactions through MMIO/port/config space IO/MSRs
are sanitized and do not cause memory safety issues or other information
leaks. Other than that there is no requirement on the functionality. In
particular DOS is ok since a malicious hypervisor can decide to not run
the guest at any time anyways.

Someone loading an malicious driver inside the guest would be out of
scope. If an attacker can do that inside the guest you already violated
the security mechanisms and there are likely easier ways to take over
the guest or leak data.

The goal of the device filter mechanism is to prevent loading unhardened
drivers that could be exploited without them being themselves malicious.

-Andi

2021-09-30 19:58:08

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

On 9/30/2021 12:04 PM, Kuppuswamy, Sathyanarayanan wrote:
>
>
> On 9/30/21 8:23 AM, Greg Kroah-Hartman wrote:
>> On Thu, Sep 30, 2021 at 08:18:18AM -0700, Kuppuswamy, Sathyanarayanan
>> wrote:
>>>
>>>
>>> On 9/30/21 6:36 AM, Dan Williams wrote:
>>>>> And in particular, not all virtio drivers are hardened -
>>>>> I think at this point blk and scsi drivers have been hardened - so
>>>>> treating them all the same looks wrong.
>>>> My understanding was that they have been audited, Sathya?
>>>
>>> Yes, AFAIK, it has been audited. Andi also submitted some patches
>>> related to it. Andi, can you confirm.
>>
>> What is the official definition of "audited"?
>
>
> In our case (Confidential Computing platform), the host is an un-trusted
> entity. So any interaction with host from the drivers will have to be
> protected against the possible attack from the host. For example, if we
> are accessing a memory based on index value received from host, we have
> to make sure it does not lead to out of bound access or when sharing the
> memory with the host, we need to make sure only the required region is
> shared with the host and the memory is un-shared after use properly.
>
> Elena can share more details, but it was achieved with static analysis
> and fuzzing. Here is a presentation she is sharing about the work at the
> Linux Security Summit:
> https://static.sched.com/hosted_files/lssna2021/b6/LSS-HardeningLinuxGuestForCCC.pdf
>
>
> Andi, can talk more about the specific driver changes that came out of
> this
> effort.

The original virtio was quite easy to exploit because it put its free
list into the shared ring buffer.

We had a patchkit to harden virtio originally, but after some discussion
we instead switched to Jason Wang's patchkit to move the virtio metadata
into protected memory, which fixed near all of the issues. These patches
have been already merged. There is one additional patch to limit the
virtio modes.

There's an ongoing effort to audit (mostly finished I believe) and fuzz
the three virtio drivers (fuzzing is still ongoing).

There was also a range of changes outside virtio for code outside the
device model. Most of it was just disabling it though.

-Andi

2021-09-30 20:04:06

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

+Elena

On 9/30/21 12:30 PM, Andi Kleen wrote:
>
> On 9/30/2021 12:04 PM, Kuppuswamy, Sathyanarayanan wrote:
>>
>>
>> On 9/30/21 8:23 AM, Greg Kroah-Hartman wrote:
>>> On Thu, Sep 30, 2021 at 08:18:18AM -0700, Kuppuswamy, Sathyanarayanan wrote:
>>>>
>>>>
>>>> On 9/30/21 6:36 AM, Dan Williams wrote:
>>>>>> And in particular, not all virtio drivers are hardened -
>>>>>> I think at this point blk and scsi drivers have been hardened - so
>>>>>> treating them all the same looks wrong.
>>>>> My understanding was that they have been audited, Sathya?
>>>>
>>>> Yes, AFAIK, it has been audited. Andi also submitted some patches
>>>> related to it. Andi, can you confirm.
>>>
>>> What is the official definition of "audited"?
>>
>>
>> In our case (Confidential Computing platform), the host is an un-trusted
>> entity. So any interaction with host from the drivers will have to be
>> protected against the possible attack from the host. For example, if we
>> are accessing a memory based on index value received from host, we have
>> to make sure it does not lead to out of bound access or when sharing the
>> memory with the host, we need to make sure only the required region is
>> shared with the host and the memory is un-shared after use properly.
>>
>> Elena can share more details, but it was achieved with static analysis
>> and fuzzing. Here is a presentation she is sharing about the work at the
>> Linux Security Summit:
>> https://static.sched.com/hosted_files/lssna2021/b6/LSS-HardeningLinuxGuestForCCC.pdf
>>
>> Andi, can talk more about the specific driver changes that came out of this
>> effort.
>
> The original virtio was quite easy to exploit because it put its free list into the shared ring buffer.
>
> We had a patchkit to harden virtio originally, but after some discussion we instead switched to
> Jason Wang's patchkit to move the virtio metadata into protected memory, which fixed near all of the
> issues. These patches have been already merged. There is one additional patch to limit the virtio
> modes.
>
> There's an ongoing effort to audit (mostly finished I believe) and fuzz the three virtio drivers
> (fuzzing is still ongoing).
>
> There was also a range of changes outside virtio for code outside the device model. Most of it was
> just disabling it though.
>
> -Andi
>

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

2021-09-30 20:13:16

[permalink] [raw]

Subject: Re: [PATCH v2 1/6] driver core: Move the "authorized" attribute from USB/Thunderbolt to core

On 9/30/21 12:04 PM, Dan Williams wrote:
>>> That's why it was highlighted in the changelog. Hopefully a
>>> Thunderbolt developer can confirm if it is a non-issue.
>>> Documentation/ABI/testing/sysfs-bus-thunderbolt does not seem to
>>> answer this question about whether authorized_show and
>>> authorized_store need to be symmetric.
>> Apparently, Bolt does read it [1] and cares about it [2].
> Ah, thank you!
>
> Yeah, looks like the conversion to bool was indeed too hopeful.
>

IIUC, the end result of value "2" in authorized sysfs is to just
"authorize" or "de-authorize". In that case, can the user space
driver adapt to this int->bool change? Just want to know the
possibility.

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

2021-09-30 21:19:03

by Yehezkel Bernat

[permalink] [raw]

Subject: Re: [PATCH v2 1/6] driver core: Move the "authorized" attribute from USB/Thunderbolt to core

On Thu, Sep 30, 2021 at 6:28 PM Dan Williams <[email protected]> wrote:
>
> On Thu, Sep 30, 2021 at 4:20 AM Yehezkel Bernat <[email protected]> wrote:
> >
> > On Thu, Sep 30, 2021 at 4:05 AM Kuppuswamy Sathyanarayanan
> > <[email protected]> wrote:
> > >
> > > no functional
> > > changes other than value 2 being not visible to the user.
> > >
> >
> > Are we sure we don't break any user-facing tool with it? Tools might use this to
> > "remember" how the device was authorized this time.
>
> That's why it was highlighted in the changelog. Hopefully a
> Thunderbolt developer can confirm if it is a non-issue.
> Documentation/ABI/testing/sysfs-bus-thunderbolt does not seem to
> answer this question about whether authorized_show and
> authorized_store need to be symmetric.

Apparently, Bolt does read it [1] and cares about it [2].

[1] https://gitlab.freedesktop.org/bolt/bolt/-/blob/130e09d1c7ff02c09e4ad1c9c36e9940b68e58d8/boltd/bolt-sysfs.c#L511
[2] https://gitlab.freedesktop.org/bolt/bolt/-/blob/130e09d1c7ff02c09e4ad1c9c36e9940b68e58d8/boltd/bolt-device.c#L639

2021-09-30 21:24:56

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

On 9/30/21 8:23 AM, Greg Kroah-Hartman wrote:
> On Thu, Sep 30, 2021 at 08:18:18AM -0700, Kuppuswamy, Sathyanarayanan wrote:
>>
>>
>> On 9/30/21 6:36 AM, Dan Williams wrote:
>>>> And in particular, not all virtio drivers are hardened -
>>>> I think at this point blk and scsi drivers have been hardened - so
>>>> treating them all the same looks wrong.
>>> My understanding was that they have been audited, Sathya?
>>
>> Yes, AFAIK, it has been audited. Andi also submitted some patches
>> related to it. Andi, can you confirm.
>
> What is the official definition of "audited"?

In our case (Confidential Computing platform), the host is an un-trusted
entity. So any interaction with host from the drivers will have to be
protected against the possible attack from the host. For example, if we
are accessing a memory based on index value received from host, we have
to make sure it does not lead to out of bound access or when sharing the
memory with the host, we need to make sure only the required region is
shared with the host and the memory is un-shared after use properly.

Elena can share more details, but it was achieved with static analysis
and fuzzing. Here is a presentation she is sharing about the work at the
Linux Security Summit:
https://static.sched.com/hosted_files/lssna2021/b6/LSS-HardeningLinuxGuestForCCC.pdf

Andi, can talk more about the specific driver changes that came out of this
effort.

In our case the driver is considered "hardened" / "audited" if it can handle
the above scenarios.

>
> thanks,
>
> greg k-h
>

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

2021-09-30 21:27:04

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

On 9/30/2021 8:18 AM, Kuppuswamy, Sathyanarayanan wrote:
>
>
> On 9/30/21 6:36 AM, Dan Williams wrote:
>>> And in particular, not all virtio drivers are hardened -
>>> I think at this point blk and scsi drivers have been hardened - so
>>> treating them all the same looks wrong.
>> My understanding was that they have been audited, Sathya?
>
> Yes, AFAIK, it has been audited. Andi also submitted some patches
> related to it. Andi, can you confirm.
>
> We also authorize the virtio at PCI ID level. And currently we allow
> console, block and net virtio PCI devices.
>
> { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, VIRTIO_TRANS_ID_NET) },
> { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, VIRTIO_TRANS_ID_BLOCK) },
> { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, VIRTIO_TRANS_ID_CONSOLE) },
>
The only drivers that are being audited and fuzzed are these three
virtio drivers (in addition to some other x86 code outside the driver model)

-Andi

2021-09-30 21:36:41

[permalink] [raw]

Subject: Re: [PATCH v2 1/6] driver core: Move the "authorized" attribute from USB/Thunderbolt to core

On Thu, Sep 30, 2021 at 12:50 PM Kuppuswamy, Sathyanarayanan
<[email protected]> wrote:
>
>
>
> On 9/30/21 12:04 PM, Dan Williams wrote:
> >>> That's why it was highlighted in the changelog. Hopefully a
> >>> Thunderbolt developer can confirm if it is a non-issue.
> >>> Documentation/ABI/testing/sysfs-bus-thunderbolt does not seem to
> >>> answer this question about whether authorized_show and
> >>> authorized_store need to be symmetric.
> >> Apparently, Bolt does read it [1] and cares about it [2].
> > Ah, thank you!
> >
> > Yeah, looks like the conversion to bool was indeed too hopeful.
> >
>
> IIUC, the end result of value "2" in authorized sysfs is to just
> "authorize" or "de-authorize". In that case, can the user space
> driver adapt to this int->bool change? Just want to know the
> possibility.

ABIs are forever. The kernel has to uphold its contract to bolt that
it will return '2' and not '1' after '2' has been written.

2021-09-30 21:37:15

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

On Thu, Sep 30, 2021 at 1:44 PM Alan Stern <[email protected]> wrote:
>
> On Thu, Sep 30, 2021 at 12:23:36PM -0700, Andi Kleen wrote:
> >
> > > I don't think the current mitigations under discussion here are about
> > > keeping the system working. In fact most encrypted VM configs tend to
> > > stop booting as a preferred way to handle security issues.
> >
> > Maybe we should avoid the "trusted" term here. We're only really using it
> > because USB is using it and we're now using a common framework like Greg
> > requested. But I don't think it's the right way to think about it.
> >
> > We usually call the drivers "hardened". The requirement for a hardened
> > driver is that all interactions through MMIO/port/config space IO/MSRs are
> > sanitized and do not cause memory safety issues or other information leaks.
> > Other than that there is no requirement on the functionality. In particular
> > DOS is ok since a malicious hypervisor can decide to not run the guest at
> > any time anyways.
> >
> > Someone loading an malicious driver inside the guest would be out of scope.
> > If an attacker can do that inside the guest you already violated the
> > security mechanisms and there are likely easier ways to take over the guest
> > or leak data.
> >
> > The goal of the device filter mechanism is to prevent loading unhardened
> > drivers that could be exploited without them being themselves malicious.
>
> If all you want to do is prevent someone from loading a bunch of
> drivers that you have identified as unhardened, why not just use a
> modprobe blacklist? Am I missing something?

modules != drivers (i.e. multi-driver modules are a thing) and builtin
modules do not adhere to modprobe policy.

There is also a desire to be able to support a single kernel image
across hosts and guests. So, if you were going to say, "just compile
all unnecessary drivers as modules" that defeats the common kernel
image goal. For confidential computing the expectation is that the
necessary device set is small. As you can see in the patches in this
case it's just a few lines of PCI ids and a hack to the virtio bus to
achieve the goal of disabling all extraneous devices by default.

2021-09-30 21:40:06

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

> If all you want to do is prevent someone from loading a bunch of
> drivers that you have identified as unhardened, why not just use a
> modprobe blacklist?

That wouldn't help for builtin drivers, we cannot control initcalls.

This LWN article has more details on the background.

https://lwn.net/Articles/865918/

-Andi

> Am I missing something?
>
> Alan Stern

2021-09-30 22:37:00

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

On Thu, Sep 30, 2021 at 12:23:36PM -0700, Andi Kleen wrote:
>
> > I don't think the current mitigations under discussion here are about
> > keeping the system working. In fact most encrypted VM configs tend to
> > stop booting as a preferred way to handle security issues.
>
> Maybe we should avoid the "trusted" term here. We're only really using it
> because USB is using it and we're now using a common framework like Greg
> requested. But I don't think it's the right way to think about it.
>
> We usually call the drivers "hardened". The requirement for a hardened
> driver is that all interactions through MMIO/port/config space IO/MSRs are
> sanitized and do not cause memory safety issues or other information leaks.
> Other than that there is no requirement on the functionality. In particular
> DOS is ok since a malicious hypervisor can decide to not run the guest at
> any time anyways.
>
> Someone loading an malicious driver inside the guest would be out of scope.
> If an attacker can do that inside the guest you already violated the
> security mechanisms and there are likely easier ways to take over the guest
> or leak data.
>
> The goal of the device filter mechanism is to prevent loading unhardened
> drivers that could be exploited without them being themselves malicious.

If all you want to do is prevent someone from loading a bunch of
drivers that you have identified as unhardened, why not just use a
modprobe blacklist? Am I missing something?

Alan Stern

2021-10-01 01:43:30

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

On Thu, Sep 30, 2021 at 01:52:59PM -0700, Dan Williams wrote:
> On Thu, Sep 30, 2021 at 1:44 PM Alan Stern <[email protected]> wrote:
> >
> > On Thu, Sep 30, 2021 at 12:23:36PM -0700, Andi Kleen wrote:
> > >
> > > > I don't think the current mitigations under discussion here are about
> > > > keeping the system working. In fact most encrypted VM configs tend to
> > > > stop booting as a preferred way to handle security issues.
> > >
> > > Maybe we should avoid the "trusted" term here. We're only really using it
> > > because USB is using it and we're now using a common framework like Greg
> > > requested. But I don't think it's the right way to think about it.
> > >
> > > We usually call the drivers "hardened". The requirement for a hardened
> > > driver is that all interactions through MMIO/port/config space IO/MSRs are
> > > sanitized and do not cause memory safety issues or other information leaks.
> > > Other than that there is no requirement on the functionality. In particular
> > > DOS is ok since a malicious hypervisor can decide to not run the guest at
> > > any time anyways.
> > >
> > > Someone loading an malicious driver inside the guest would be out of scope.
> > > If an attacker can do that inside the guest you already violated the
> > > security mechanisms and there are likely easier ways to take over the guest
> > > or leak data.
> > >
> > > The goal of the device filter mechanism is to prevent loading unhardened
> > > drivers that could be exploited without them being themselves malicious.
> >
> > If all you want to do is prevent someone from loading a bunch of
> > drivers that you have identified as unhardened, why not just use a
> > modprobe blacklist? Am I missing something?
>
> modules != drivers (i.e. multi-driver modules are a thing) and builtin
> modules do not adhere to modprobe policy.
>
> There is also a desire to be able to support a single kernel image
> across hosts and guests. So, if you were going to say, "just compile
> all unnecessary drivers as modules" that defeats the common kernel
> image goal. For confidential computing the expectation is that the
> necessary device set is small. As you can see in the patches in this
> case it's just a few lines of PCI ids and a hack to the virtio bus to
> achieve the goal of disabling all extraneous devices by default.

If your goal is to prevent some unwanted _drivers_ from operating --
or all but a few desired drivers, as the case may be -- why extend
the "authorized" API to all _devices_? Why not instead develop a
separate API (but of similar form) for drivers?

Wouldn't that make more sense? It corresponds a lot more directly
with what you say you want to accomplish.

What would you do in the theoretical case where two separate drivers
can manage the same device, but one of them is desired (or hardened)
and the other isn't?

Alan Stern

2021-10-01 02:24:08

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

On Thu, Sep 30, 2021 at 6:41 PM Alan Stern <[email protected]> wrote:
>
> On Thu, Sep 30, 2021 at 01:52:59PM -0700, Dan Williams wrote:
> > On Thu, Sep 30, 2021 at 1:44 PM Alan Stern <[email protected]> wrote:
> > >
> > > On Thu, Sep 30, 2021 at 12:23:36PM -0700, Andi Kleen wrote:
> > > >
> > > > > I don't think the current mitigations under discussion here are about
> > > > > keeping the system working. In fact most encrypted VM configs tend to
> > > > > stop booting as a preferred way to handle security issues.
> > > >
> > > > Maybe we should avoid the "trusted" term here. We're only really using it
> > > > because USB is using it and we're now using a common framework like Greg
> > > > requested. But I don't think it's the right way to think about it.
> > > >
> > > > We usually call the drivers "hardened". The requirement for a hardened
> > > > driver is that all interactions through MMIO/port/config space IO/MSRs are
> > > > sanitized and do not cause memory safety issues or other information leaks.
> > > > Other than that there is no requirement on the functionality. In particular
> > > > DOS is ok since a malicious hypervisor can decide to not run the guest at
> > > > any time anyways.
> > > >
> > > > Someone loading an malicious driver inside the guest would be out of scope.
> > > > If an attacker can do that inside the guest you already violated the
> > > > security mechanisms and there are likely easier ways to take over the guest
> > > > or leak data.
> > > >
> > > > The goal of the device filter mechanism is to prevent loading unhardened
> > > > drivers that could be exploited without them being themselves malicious.
> > >
> > > If all you want to do is prevent someone from loading a bunch of
> > > drivers that you have identified as unhardened, why not just use a
> > > modprobe blacklist? Am I missing something?
> >
> > modules != drivers (i.e. multi-driver modules are a thing) and builtin
> > modules do not adhere to modprobe policy.
> >
> > There is also a desire to be able to support a single kernel image
> > across hosts and guests. So, if you were going to say, "just compile
> > all unnecessary drivers as modules" that defeats the common kernel
> > image goal. For confidential computing the expectation is that the
> > necessary device set is small. As you can see in the patches in this
> > case it's just a few lines of PCI ids and a hack to the virtio bus to
> > achieve the goal of disabling all extraneous devices by default.
>
>
>
> If your goal is to prevent some unwanted _drivers_ from operating --
> or all but a few desired drivers, as the case may be -- why extend
> the "authorized" API to all _devices_? Why not instead develop a
> separate API (but of similar form) for drivers?
>
> Wouldn't that make more sense? It corresponds a lot more directly
> with what you say you want to accomplish.

This was v1. v1 was NAKd [1] [2]:

[1]: https://lore.kernel.org/all/[email protected]/
[2]: https://lore.kernel.org/all/[email protected]/

> What would you do in the theoretical case where two separate drivers
> can manage the same device, but one of them is desired (or hardened)
> and the other isn't?

Allow for user override, just like we do today for new_id, remove_id,
bind, and unbind when default driver policy is insufficient.

echo 1 > /sys/bus/$bus/devices/$device/authorized
echo $device > /sys/bus/$bus/drivers/$desired_driver/bind

The device filter is really only necessary to bootstrap until you can
run override policy scripts. The driver firewall approach was overkill
in that regard.

2021-10-01 07:04:40

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

On Thu, Sep 30, 2021 at 12:15:16PM -0700, Andi Kleen wrote:
>
> On 9/30/2021 10:23 AM, Greg Kroah-Hartman wrote:
> > On Thu, Sep 30, 2021 at 10:17:09AM -0700, Andi Kleen wrote:
> > > The legacy drivers could be fixed, but nobody really wants to touch them
> > > anymore and they're impossible to test.
> > Pointers to them?
>
> For example if you look over old SCSI drivers in drivers/scsi/*.c there is a
> substantial number that has a module init longer than just registering a
> driver. As a single example look at drivers/scsi/BusLogic.c

Great, send patches to fix them up instead of adding new infrastructure
to the kernel. It is better to remove code than add it. You can rip
the ISA code out of that driver and then you will not have the issue
anymore.

Or again, just add that module to the deny list and never load it from
userspace.

> There were also quite a few platform drivers like this.

Of course, platform drivers are horrible abusers of this. Just like the
recent one submitted by Intel that would bind to any machine it was
loaded on and did not actually do any hardware detection assuming that
it owned the platform:
https://lore.kernel.org/r/[email protected]

So yes, some drivers are horrible, it is our job to catch that and fix
it. If you don't want to load those drivers on your system, we have
userspace solutions for that (you can have allow/deny lists there.)

> > > The drivers that probe something that is not enumerated in a standard way
> > > have no choice, it cannot be implemented in a different way.
> > PCI devices are not enumerated in a standard way???
>
> The pci devices are enumerated in a standard way, but typically the driver
> also needs something else outside PCI that needs some other probing
> mechanism.

Like what? What PCI drivers need outside connections to control the
hardware?

> > > So instead we're using a "firewall" the prevents these drivers from doing
> > > bad things by not allowing ioremap access unless opted in, and also do some
> > > filtering on the IO ports The device filter is still the primary mechanism,
> > > the ioremap filtering is just belts and suspenders for those odd cases.
> > That's horrible, don't try to protect the kernel from itself. Just fix
> > the drivers.
>
> I thought we had already established this last time we discussed it.
>
> That's completely impractical. We cannot harden thousands of drivers,
> especially since it would be all wasted work since nobody will ever need
> them in virtual guests. Even if we could harden them how would such a work
> be maintained long term? Using a firewall and filtering mechanism is much
> saner for everyone.

I agree, you can not "harden" anything here. That is why I asked you to
use the existing process that explicitly moves the model to userspace
where a user can say "do I want this device to be controlled by the
kernel or not" which then allows you to pick and choose what drivers you
want to have in your system.

You need to trust devices, and not worry about trusting drivers as you
yourself admit :)

The kernel's trust model is that once we bind to them, we trust almost
all device types almost explicitly. If you wish to change that model,
that's great, but it is a much larger discussion than this tiny patchset
would require.

thanks,

greg k-h

2021-10-01 07:07:10

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

On Thu, Sep 30, 2021 at 12:04:05PM -0700, Kuppuswamy, Sathyanarayanan wrote:
>
>
> On 9/30/21 8:23 AM, Greg Kroah-Hartman wrote:
> > On Thu, Sep 30, 2021 at 08:18:18AM -0700, Kuppuswamy, Sathyanarayanan wrote:
> > >
> > >
> > > On 9/30/21 6:36 AM, Dan Williams wrote:
> > > > > And in particular, not all virtio drivers are hardened -
> > > > > I think at this point blk and scsi drivers have been hardened - so
> > > > > treating them all the same looks wrong.
> > > > My understanding was that they have been audited, Sathya?
> > >
> > > Yes, AFAIK, it has been audited. Andi also submitted some patches
> > > related to it. Andi, can you confirm.
> >
> > What is the official definition of "audited"?
>
>
> In our case (Confidential Computing platform), the host is an un-trusted
> entity. So any interaction with host from the drivers will have to be
> protected against the possible attack from the host. For example, if we
> are accessing a memory based on index value received from host, we have
> to make sure it does not lead to out of bound access or when sharing the
> memory with the host, we need to make sure only the required region is
> shared with the host and the memory is un-shared after use properly.

You have not defined the term "audited" here at all in any way that can
be reviewed or verified by anyone from what I can tell.

You have only described a new model that you wish the kernel to run in,
one in which it does not trust the hardware at all. That is explicitly
NOT what the kernel has been designed for so far, and if you wish to
change that, lots of things need to be done outside of simply running
some fuzzers on a few random drivers.

For one example, how do you ensure that the memory you are reading from
hasn't been modified by the host between writing to it the last time you
did? Do you have a list of specific drivers and kernel options that you
feel you now "trust"? If so, how long does that trust last for? Until
someonen else modifies that code? What about modifications to functions
that your "audited" code touches? Who is doing this auditing? How do
you know the auditing has been done correctly? Who has reviewed and
audited the tools that are doing the auditing? Where is the
specification that has been agreed on how the auditing must be done?
And so on...

I feel like there are a lot of different things all being mixed up here
into one "oh we want this to happen!" type of thread. Please let's just
stick to the one request that I had here, which was to move the way that
busses are allowed to authorize the devices they wish to control into a
generic way instead of being bus-specific logic.

Any requests outside of that type of functionality are just that,
outside the scope of this patchset and should get their own patch series
and discussion.

thanks,

greg k-h

2021-10-01 16:08:47

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

> Forget about trust for the moment. Let's say the goal is to prevent
> the kernel from creating any bindings other that those in some small
> "allowed" set. To fully specify one of the allowed bindings, you
> would have to provide both a device ID and a driver name. But in
> practice this isn't necessary, since a device with a given ID will
> bind to only one driver in almost all cases, and hence giving just
> the device ID is enough.
>
> So to do what they want, all that's needed is to forbid any bindings
> except where the device ID is "allowed". Or to put it another way,
> where the device's authorized flag (which can be initialized based on
> the device ID) is set.
>
> (The opposite approach, in which the drivers are "allowed" rather
> than the device IDs, apparently has already been discussed and
> rejected. I'm not convinced that was a good decision, but...)
>
> Does this seem like a fair description of the situation?

Yes. That's roughly what the patchkit under discussion implements.

-Andi

2021-10-01 16:17:27

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

On Fri, Oct 1, 2021 at 12:03 AM Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Thu, Sep 30, 2021 at 12:04:05PM -0700, Kuppuswamy, Sathyanarayanan wrote:
> >
> >
> > On 9/30/21 8:23 AM, Greg Kroah-Hartman wrote:
> > > On Thu, Sep 30, 2021 at 08:18:18AM -0700, Kuppuswamy, Sathyanarayanan wrote:
> > > >
> > > >
> > > > On 9/30/21 6:36 AM, Dan Williams wrote:
> > > > > > And in particular, not all virtio drivers are hardened -
> > > > > > I think at this point blk and scsi drivers have been hardened - so
> > > > > > treating them all the same looks wrong.
> > > > > My understanding was that they have been audited, Sathya?
> > > >
> > > > Yes, AFAIK, it has been audited. Andi also submitted some patches
> > > > related to it. Andi, can you confirm.
> > >
> > > What is the official definition of "audited"?
> >
> >
> > In our case (Confidential Computing platform), the host is an un-trusted
> > entity. So any interaction with host from the drivers will have to be
> > protected against the possible attack from the host. For example, if we
> > are accessing a memory based on index value received from host, we have
> > to make sure it does not lead to out of bound access or when sharing the
> > memory with the host, we need to make sure only the required region is
> > shared with the host and the memory is un-shared after use properly.
>
> You have not defined the term "audited" here at all in any way that can
> be reviewed or verified by anyone from what I can tell.

Agree.

>
> You have only described a new model that you wish the kernel to run in,
> one in which it does not trust the hardware at all. That is explicitly
> NOT what the kernel has been designed for so far, and if you wish to
> change that, lots of things need to be done outside of simply running
> some fuzzers on a few random drivers.
>
> For one example, how do you ensure that the memory you are reading from
> hasn't been modified by the host between writing to it the last time you
> did? Do you have a list of specific drivers and kernel options that you
> feel you now "trust"? If so, how long does that trust last for? Until
> someonen else modifies that code? What about modifications to functions
> that your "audited" code touches? Who is doing this auditing? How do
> you know the auditing has been done correctly? Who has reviewed and
> audited the tools that are doing the auditing? Where is the
> specification that has been agreed on how the auditing must be done?
> And so on...
>
> I feel like there are a lot of different things all being mixed up here
> into one "oh we want this to happen!" type of thread. Please let's just
> stick to the one request that I had here, which was to move the way that
> busses are allowed to authorize the devices they wish to control into a
> generic way instead of being bus-specific logic.

I want to continue to shave this proposal down, but that last sentence
reads as self-contradictory.

Bear with me, and perhaps it's a lack of imagination on my part, but I
don't see how to get to a globally generic "authorized" sysfs ABI
given that USB and Thunderbolt want to do bus specific actions on
authorization toggle events. Certainly a default generic authorized
attribute can be defined for all the other buses that don't have
legacy here, but Thunderbolt will still require support for '2' as an
authorized value, and USB will still want to base probe decisions on
the authorization state of both the usb_device and the usb_interface.

> Any requests outside of that type of functionality are just that,
> outside the scope of this patchset and should get their own patch series
> and discussion.

A way to shave this further would be to default authorize just enough
devices to do the rest of the authorization from initramfs. That would
seem to cut out 'virtio-net' and 'virtio-storage' from default
authorized list.

The generic authorized attribute would only need to appear when
'dev_default_authorization' is set to false.

2021-10-01 16:49:34

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

On Fri, Oct 01, 2021 at 09:13:54AM -0700, Dan Williams wrote:
> Bear with me, and perhaps it's a lack of imagination on my part, but I
> don't see how to get to a globally generic "authorized" sysfs ABI
> given that USB and Thunderbolt want to do bus specific actions on
> authorization toggle events. Certainly a default generic authorized
> attribute can be defined for all the other buses that don't have
> legacy here, but Thunderbolt will still require support for '2' as an
> authorized value, and USB will still want to base probe decisions on
> the authorization state of both the usb_device and the usb_interface.

The USB part isn't really accurate (I can't speak for Thunderbolt).
When a usb_device is deauthorized, the device will be unconfigured,
deleting all its interfaces and removing the need for any probe
decisions about them. In other words, the probe decision for a
usb_device or usb_interface depends only on the device's/interface's
own authorization state.

True, the interface binding code does contain a test of the device's
authorization setting. That test is redundant and can be removed.

The actions that USB wants to take on authorization toggle events for
usb_devices are: for authorize, select and install a configuration;
for deauthorize, unconfigure the device. Each of these could be
handled simply enough just by binding/unbinding the device. (There
is some special code for handling wireless USB devices, but wireless
USB is now defunct.)

Alan Stern

2021-10-01 18:12:51

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

On Fri, Oct 1, 2021 at 9:47 AM Alan Stern <[email protected]> wrote:
>
> On Fri, Oct 01, 2021 at 09:13:54AM -0700, Dan Williams wrote:
> > Bear with me, and perhaps it's a lack of imagination on my part, but I
> > don't see how to get to a globally generic "authorized" sysfs ABI
> > given that USB and Thunderbolt want to do bus specific actions on
> > authorization toggle events. Certainly a default generic authorized
> > attribute can be defined for all the other buses that don't have
> > legacy here, but Thunderbolt will still require support for '2' as an
> > authorized value, and USB will still want to base probe decisions on
> > the authorization state of both the usb_device and the usb_interface.
>
> The USB part isn't really accurate (I can't speak for Thunderbolt).
> When a usb_device is deauthorized, the device will be unconfigured,
> deleting all its interfaces and removing the need for any probe
> decisions about them. In other words, the probe decision for a
> usb_device or usb_interface depends only on the device's/interface's
> own authorization state.
>
> True, the interface binding code does contain a test of the device's
> authorization setting. That test is redundant and can be removed.
>
> The actions that USB wants to take on authorization toggle events for
> usb_devices are: for authorize, select and install a configuration;
> for deauthorize, unconfigure the device. Each of these could be
> handled simply enough just by binding/unbinding the device. (There
> is some special code for handling wireless USB devices, but wireless
> USB is now defunct.)

Ah, so are you saying that it would be sufficient for USB if the
generic authorized implementation did something like:

dev->authorized = 1;
device_attach(dev);

...for the authorize case, and:

dev->authorize = 0;
device_release_driver(dev);

...for the deauthorize case?

2021-10-01 18:39:04

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

On Fri, Oct 01, 2021 at 08:29:36AM +0200, Greg Kroah-Hartman wrote:
> On Thu, Sep 30, 2021 at 12:15:16PM -0700, Andi Kleen wrote:
> >
> > On 9/30/2021 10:23 AM, Greg Kroah-Hartman wrote:
> > > On Thu, Sep 30, 2021 at 10:17:09AM -0700, Andi Kleen wrote:
> > > > The legacy drivers could be fixed, but nobody really wants to touch them
> > > > anymore and they're impossible to test.
> > > Pointers to them?
> >
> > For example if you look over old SCSI drivers in drivers/scsi/*.c there is a
> > substantial number that has a module init longer than just registering a
> > driver. As a single example look at drivers/scsi/BusLogic.c
>
> Great, send patches to fix them up instead of adding new infrastructure
> to the kernel. It is better to remove code than add it. You can rip
> the ISA code out of that driver and then you will not have the issue
> anymore.
>
> Or again, just add that module to the deny list and never load it from
> userspace.
>
> > There were also quite a few platform drivers like this.
>
> Of course, platform drivers are horrible abusers of this. Just like the
> recent one submitted by Intel that would bind to any machine it was
> loaded on and did not actually do any hardware detection assuming that
> it owned the platform:
> https://lore.kernel.org/r/[email protected]
>
> So yes, some drivers are horrible, it is our job to catch that and fix
> it. If you don't want to load those drivers on your system, we have
> userspace solutions for that (you can have allow/deny lists there.)
>
> > > > The drivers that probe something that is not enumerated in a standard way
> > > > have no choice, it cannot be implemented in a different way.
> > > PCI devices are not enumerated in a standard way???
> >
> > The pci devices are enumerated in a standard way, but typically the driver
> > also needs something else outside PCI that needs some other probing
> > mechanism.
>
> Like what? What PCI drivers need outside connections to control the
> hardware?
>
> > > > So instead we're using a "firewall" the prevents these drivers from doing
> > > > bad things by not allowing ioremap access unless opted in, and also do some
> > > > filtering on the IO ports The device filter is still the primary mechanism,
> > > > the ioremap filtering is just belts and suspenders for those odd cases.
> > > That's horrible, don't try to protect the kernel from itself. Just fix
> > > the drivers.
> >
> > I thought we had already established this last time we discussed it.
> >
> > That's completely impractical. We cannot harden thousands of drivers,
> > especially since it would be all wasted work since nobody will ever need
> > them in virtual guests. Even if we could harden them how would such a work
> > be maintained long term? Using a firewall and filtering mechanism is much
> > saner for everyone.
>
> I agree, you can not "harden" anything here. That is why I asked you to
> use the existing process that explicitly moves the model to userspace
> where a user can say "do I want this device to be controlled by the
> kernel or not" which then allows you to pick and choose what drivers you
> want to have in your system.
>
> You need to trust devices, and not worry about trusting drivers as you
> yourself admit :)

That isn't the way they see it. In their approach you can never
trust any devices at all. Therefore devices can only be allowed to
bind to a very small set of "hardened" drivers. Never mind how they
decide whether or not a driver is sufficiently "hardened".

> The kernel's trust model is that once we bind to them, we trust almost
> all device types almost explicitly. If you wish to change that model,
> that's great, but it is a much larger discussion than this tiny patchset
> would require.

Forget about trust for the moment. Let's say the goal is to prevent
the kernel from creating any bindings other that those in some small
"allowed" set. To fully specify one of the allowed bindings, you
would have to provide both a device ID and a driver name. But in
practice this isn't necessary, since a device with a given ID will
bind to only one driver in almost all cases, and hence giving just
the device ID is enough.

So to do what they want, all that's needed is to forbid any bindings
except where the device ID is "allowed". Or to put it another way,
where the device's authorized flag (which can be initialized based on
the device ID) is set.

(The opposite approach, in which the drivers are "allowed" rather
than the device IDs, apparently has already been discussed and
rejected. I'm not convinced that was a good decision, but...)

Does this seem like a fair description of the situation?

Alan Stern

2021-10-01 19:15:27

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

On Fri, Oct 01, 2021 at 11:09:52AM -0700, Dan Williams wrote:
> On Fri, Oct 1, 2021 at 9:47 AM Alan Stern <[email protected]> wrote:
> >
> > On Fri, Oct 01, 2021 at 09:13:54AM -0700, Dan Williams wrote:
> > > Bear with me, and perhaps it's a lack of imagination on my part, but I
> > > don't see how to get to a globally generic "authorized" sysfs ABI
> > > given that USB and Thunderbolt want to do bus specific actions on
> > > authorization toggle events. Certainly a default generic authorized
> > > attribute can be defined for all the other buses that don't have
> > > legacy here, but Thunderbolt will still require support for '2' as an
> > > authorized value, and USB will still want to base probe decisions on
> > > the authorization state of both the usb_device and the usb_interface.
> >
> > The USB part isn't really accurate (I can't speak for Thunderbolt).
> > When a usb_device is deauthorized, the device will be unconfigured,
> > deleting all its interfaces and removing the need for any probe
> > decisions about them. In other words, the probe decision for a
> > usb_device or usb_interface depends only on the device's/interface's
> > own authorization state.
> >
> > True, the interface binding code does contain a test of the device's
> > authorization setting. That test is redundant and can be removed.
> >
> > The actions that USB wants to take on authorization toggle events for
> > usb_devices are: for authorize, select and install a configuration;
> > for deauthorize, unconfigure the device. Each of these could be
> > handled simply enough just by binding/unbinding the device. (There
> > is some special code for handling wireless USB devices, but wireless
> > USB is now defunct.)
>
> Ah, so are you saying that it would be sufficient for USB if the
> generic authorized implementation did something like:
>
> dev->authorized = 1;
> device_attach(dev);
>
> ...for the authorize case, and:
>
> dev->authorize = 0;
> device_release_driver(dev);
>
> ...for the deauthorize case?

Yes, I think so. But I haven't tried making this change to test and
see what really happens.

Alan Stern

2021-10-01 19:48:33

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

On 10/1/2021 12:03 AM, Greg Kroah-Hartman wrote:
> On Thu, Sep 30, 2021 at 12:04:05PM -0700, Kuppuswamy, Sathyanarayanan wrote:
>>
>> On 9/30/21 8:23 AM, Greg Kroah-Hartman wrote:
>>> On Thu, Sep 30, 2021 at 08:18:18AM -0700, Kuppuswamy, Sathyanarayanan wrote:
>>>>
>>>> On 9/30/21 6:36 AM, Dan Williams wrote:
>>>>>> And in particular, not all virtio drivers are hardened -
>>>>>> I think at this point blk and scsi drivers have been hardened - so
>>>>>> treating them all the same looks wrong.
>>>>> My understanding was that they have been audited, Sathya?
>>>> Yes, AFAIK, it has been audited. Andi also submitted some patches
>>>> related to it. Andi, can you confirm.
>>> What is the official definition of "audited"?
>>
>> In our case (Confidential Computing platform), the host is an un-trusted
>> entity. So any interaction with host from the drivers will have to be
>> protected against the possible attack from the host. For example, if we
>> are accessing a memory based on index value received from host, we have
>> to make sure it does not lead to out of bound access or when sharing the
>> memory with the host, we need to make sure only the required region is
>> shared with the host and the memory is un-shared after use properly.
> You have not defined the term "audited" here at all in any way that can
> be reviewed or verified by anyone from what I can tell.
>
> You have only described a new model that you wish the kernel to run in,
> one in which it does not trust the hardware at all. That is explicitly
> NOT what the kernel has been designed for so far,

It has been already done for a few USB/TB drivers, but yes not for the
majority of the kernel.

> and if you wish to
> change that, lots of things need to be done outside of simply running
> some fuzzers on a few random drivers.

The goal is to do similar work as USB/TB did, but do it for a small set
of virtio drivers and use a custom allow list for those for the specific
secure guest cases.

(there are some other goals, but let's not discuss them here for now)

>
> For one example, how do you ensure that the memory you are reading from
> hasn't been modified by the host between writing to it the last time you
> did?

It's similar techniques as we do on user space accesses. For example if
you bound check some value the code needs to ensure it is cached in
private memory, not reread from MMIO or shared memory. Of course that's
a good idea anyways for performance because MMIO is slow.

In the concrete cases of virtio the main problem was the free list in
shared memory, but that has been addressed now.

> Do you have a list of specific drivers and kernel options that you
> feel you now "trust"?

For TDX it's currently only virtio net/block/console

But we expect this list to grow slightly over time, but not at a high
rate (so hopefully <10)

> If so, how long does that trust last for? Until
> someonen else modifies that code? What about modifications to functions
> that your "audited" code touches? Who is doing this auditing? How do
> you know the auditing has been done correctly? Who has reviewed and
> audited the tools that are doing the auditing? Where is the
> specification that has been agreed on how the auditing must be done?
> And so on...

Well, I mean we already have a similar situation with user space APIs.
So it's not a new problem. For those we've done it for many years, with
audits and extra fuzzing.

There are people working on the audit and fuzzing today. How exactly it
will be ensured long term is still be worked out, but I expect we can
work out something.

>
> I feel like there are a lot of different things all being mixed up here
> into one "oh we want this to happen!" type of thread.

Agreed. The thread ended up about a lot of stuff which is outside the
scope of the patches.

> Please let's just
> stick to the one request that I had here, which was to move the way that
> busses are allowed to authorize the devices they wish to control into a
> generic way instead of being bus-specific logic.
>
> Any requests outside of that type of functionality are just that,
> outside the scope of this patchset and should get their own patch series
> and discussion.

Yes that's the intention. This patch kit is only about controlling what
devices can enumerate.

Also please let's avoid the "trusted" term. It's really misleading and
confusing in the context of confidential computing.

-Andi

2021-10-01 20:59:21

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

On 10/1/21 12:00 PM, Alan Stern wrote:
> On Fri, Oct 01, 2021 at 11:09:52AM -0700, Dan Williams wrote:
>> On Fri, Oct 1, 2021 at 9:47 AM Alan Stern <[email protected]> wrote:
>>>
>>> On Fri, Oct 01, 2021 at 09:13:54AM -0700, Dan Williams wrote:
>>>> Bear with me, and perhaps it's a lack of imagination on my part, but I
>>>> don't see how to get to a globally generic "authorized" sysfs ABI
>>>> given that USB and Thunderbolt want to do bus specific actions on
>>>> authorization toggle events. Certainly a default generic authorized
>>>> attribute can be defined for all the other buses that don't have
>>>> legacy here, but Thunderbolt will still require support for '2' as an
>>>> authorized value, and USB will still want to base probe decisions on
>>>> the authorization state of both the usb_device and the usb_interface.
>>>
>>> The USB part isn't really accurate (I can't speak for Thunderbolt).
>>> When a usb_device is deauthorized, the device will be unconfigured,
>>> deleting all its interfaces and removing the need for any probe
>>> decisions about them. In other words, the probe decision for a
>>> usb_device or usb_interface depends only on the device's/interface's
>>> own authorization state.
>>>
>>> True, the interface binding code does contain a test of the device's
>>> authorization setting. That test is redundant and can be removed.
>>>
>>> The actions that USB wants to take on authorization toggle events for
>>> usb_devices are: for authorize, select and install a configuration;
>>> for deauthorize, unconfigure the device. Each of these could be
>>> handled simply enough just by binding/unbinding the device. (There
>>> is some special code for handling wireless USB devices, but wireless
>>> USB is now defunct.)
>>
>> Ah, so are you saying that it would be sufficient for USB if the
>> generic authorized implementation did something like:
>>
>> dev->authorized = 1;
>> device_attach(dev);
>>
>> ...for the authorize case, and:
>>
>> dev->authorize = 0;
>> device_release_driver(dev);
>>
>> ...for the deauthorize case?
>
> Yes, I think so. But I haven't tried making this change to test and
> see what really happens.
>

For thunderbolt driver, it looks much more complicated. Unless you
define some callbacks in struct bus_type, we cannot easily generalize
it (but such callbacks are not recommended because it brings bus specific
operations to core layer).

sysfs_read()
-> simple read

sysfs_write()
-> tb_switch_set_authorized()
-> disapprove_switch()
-> tb_domain_disapprove_switch()
-> tb->cm_ops->disapprove_switch() (product specific call)
-> tb_domain_approve_switch_key()
-> tb->cm_ops->add_switch_key
-> tb->cm_ops->approve_switch() (product specific call)
-> tb_domain_approve_switch()
-> tb->cm_ops->approve_switch() (product specific call)
-> tb_domain_challenge_switch_key()
-> tb->cm_ops->challenge_switch_key()
-> crypto_alloc_shash()
-> crypto_shash_setkey()
-> crypto_shash_digest()
-> tb->cm_ops->approve_switch() (product specific call)

> Alan Stern
>

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

2021-10-01 20:59:44

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

On Fri, Oct 1, 2021 at 12:02 PM Alan Stern <[email protected]> wrote:
>
> On Fri, Oct 01, 2021 at 11:09:52AM -0700, Dan Williams wrote:
> > On Fri, Oct 1, 2021 at 9:47 AM Alan Stern <[email protected]> wrote:
> > >
> > > On Fri, Oct 01, 2021 at 09:13:54AM -0700, Dan Williams wrote:
> > > > Bear with me, and perhaps it's a lack of imagination on my part, but I
> > > > don't see how to get to a globally generic "authorized" sysfs ABI
> > > > given that USB and Thunderbolt want to do bus specific actions on
> > > > authorization toggle events. Certainly a default generic authorized
> > > > attribute can be defined for all the other buses that don't have
> > > > legacy here, but Thunderbolt will still require support for '2' as an
> > > > authorized value, and USB will still want to base probe decisions on
> > > > the authorization state of both the usb_device and the usb_interface.
> > >
> > > The USB part isn't really accurate (I can't speak for Thunderbolt).
> > > When a usb_device is deauthorized, the device will be unconfigured,
> > > deleting all its interfaces and removing the need for any probe
> > > decisions about them. In other words, the probe decision for a
> > > usb_device or usb_interface depends only on the device's/interface's
> > > own authorization state.
> > >
> > > True, the interface binding code does contain a test of the device's
> > > authorization setting. That test is redundant and can be removed.
> > >
> > > The actions that USB wants to take on authorization toggle events for
> > > usb_devices are: for authorize, select and install a configuration;
> > > for deauthorize, unconfigure the device. Each of these could be
> > > handled simply enough just by binding/unbinding the device. (There
> > > is some special code for handling wireless USB devices, but wireless
> > > USB is now defunct.)
> >
> > Ah, so are you saying that it would be sufficient for USB if the
> > generic authorized implementation did something like:
> >
> > dev->authorized = 1;
> > device_attach(dev);
> >
> > ...for the authorize case, and:
> >
> > dev->authorize = 0;
> > device_release_driver(dev);
> >
> > ...for the deauthorize case?
>
> Yes, I think so. But I haven't tried making this change to test and
> see what really happens.

Sounds like a useful path for this effort to explore. Especially as
Greg seems to want the proposed "has_probe_authorization" flag in the
bus_type to disappear and make this all generic. It just seems that
Thunderbolt would need deeper surgery to move what it does in the
authorization toggle path into the probe and remove paths.

Mika, do you see a path for Thunderbolt to align its authorization
paths behind bus ->probe() ->remove() events similar to what USB might
be able to support for a generic authorization path?

2021-10-02 11:15:41

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

On Fri, Oct 01, 2021 at 08:49:28AM -0700, Andi Kleen wrote:
> > Do you have a list of specific drivers and kernel options that you
> > feel you now "trust"?
>
> For TDX it's currently only virtio net/block/console
>
> But we expect this list to grow slightly over time, but not at a high rate
> (so hopefully <10)

Well there are already >10 virtio drivers and I think it's reasonable
that all of these will be used with encrypted guests. The list will
grow.

--
MST

2021-10-02 11:39:27

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

On Sat, Oct 02, 2021 at 07:04:28AM -0400, Michael S. Tsirkin wrote:
> On Fri, Oct 01, 2021 at 08:49:28AM -0700, Andi Kleen wrote:
> > > Do you have a list of specific drivers and kernel options that you
> > > feel you now "trust"?
> >
> > For TDX it's currently only virtio net/block/console
> >
> > But we expect this list to grow slightly over time, but not at a high rate
> > (so hopefully <10)
>
> Well there are already >10 virtio drivers and I think it's reasonable
> that all of these will be used with encrypted guests. The list will
> grow.

What is keeping "all" drivers from being on this list? How exactly are
you determining what should, and should not, be allowed? How can
drivers move on, or off, of it over time?

And why not just put all of that into userspace and have it pick and
choose? That should be the end-goal here, you don't want to encode
policy like this in the kernel, right?

thanks,

greg k-h

2021-10-02 14:26:15

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

On 10/2/2021 4:14 AM, Greg Kroah-Hartman wrote:
> On Sat, Oct 02, 2021 at 07:04:28AM -0400, Michael S. Tsirkin wrote:
>> On Fri, Oct 01, 2021 at 08:49:28AM -0700, Andi Kleen wrote:
>>>> Do you have a list of specific drivers and kernel options that you
>>>> feel you now "trust"?
>>> For TDX it's currently only virtio net/block/console
>>>
>>> But we expect this list to grow slightly over time, but not at a high rate
>>> (so hopefully <10)
>> Well there are already >10 virtio drivers and I think it's reasonable
>> that all of these will be used with encrypted guests. The list will
>> grow.
> What is keeping "all" drivers from being on this list?

It would be too much work to harden them all, and it would be pointless
because all these drivers are never legitimately needed in a virtualized
environment which only virtualize a very small number of devices.

> How exactly are
> you determining what should, and should not, be allowed?

Everything that has had reasonable effort at hardening can be added. But
if someone proposes to add a driver that should trigger additional
scrutiny in code review. We should also request them to do some fuzzing.

It's a bit similar to someone trying to add a new syscall interface.
That also triggers much additional scrutiny for good reasons and people
start fuzzing it.

> How can
> drivers move on, or off, of it over time?

Adding something is submitting a patch to the allow list.

I'm not sure the "off" case would happen, unless the driver is
completely removed, or maybe it has some unfixable security problem. But
that is all rather unlikely.

>
> And why not just put all of that into userspace and have it pick and
> choose? That should be the end-goal here, you don't want to encode
> policy like this in the kernel, right?

How would user space know what drivers have been hardened? This is
really something that the kernel needs to determine. I don't think we
can outsource it to anyone else.

Also BTW of course user space can still override it, but really the
defaults should be a kernel policy.

There's also the additional problem that one of the goals of
confidential guest is to just move existing guest virtual images into
them without much changes. So it's better for such a case if as much as
possible of the policy is in the kernel. But that's more a secondary
consideration, the first point is really the important part.

-Andi

2021-10-02 14:46:45

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

On Sat, Oct 02, 2021 at 07:20:22AM -0700, Andi Kleen wrote:
>
> On 10/2/2021 4:14 AM, Greg Kroah-Hartman wrote:
> > On Sat, Oct 02, 2021 at 07:04:28AM -0400, Michael S. Tsirkin wrote:
> > > On Fri, Oct 01, 2021 at 08:49:28AM -0700, Andi Kleen wrote:
> > > > > Do you have a list of specific drivers and kernel options that you
> > > > > feel you now "trust"?
> > > > For TDX it's currently only virtio net/block/console
> > > >
> > > > But we expect this list to grow slightly over time, but not at a high rate
> > > > (so hopefully <10)
> > > Well there are already >10 virtio drivers and I think it's reasonable
> > > that all of these will be used with encrypted guests. The list will
> > > grow.
> > What is keeping "all" drivers from being on this list?
>
> It would be too much work to harden them all, and it would be pointless
> because all these drivers are never legitimately needed in a virtualized
> environment which only virtualize a very small number of devices.

Why would you not want to properly review and fix up all kernel drivers?
That feels like you are being lazy.

What exactly are you meaning by "harden"? Why isn't it automated? Who
is doing this work? Where is it being done?

Come on, you have a small number of virtio drivers, to somehow say that
they are now divided up into trusted/untrusted feels very very odd.

Just do the real work here, everyone will benefit, including yourself.

> > How exactly are
> > you determining what should, and should not, be allowed?
>
> Everything that has had reasonable effort at hardening can be added. But if
> someone proposes to add a driver that should trigger additional scrutiny in
> code review. We should also request them to do some fuzzing.

You can provide that fuzzing right now, why isn't syzbot running on
these interfaces today?

And again, what _exactly_ do you all mean by "hardening" that has
happened? Where is that documented and who did that work?

> > And why not just put all of that into userspace and have it pick and
> > choose? That should be the end-goal here, you don't want to encode
> > policy like this in the kernel, right?
>
> How would user space know what drivers have been hardened? This is really
> something that the kernel needs to determine. I don't think we can outsource
> it to anyone else.

It would "know" just as well as you know today in the kernel. There is
no difference here.

Just do the real work here, and "harden" all of the virtio drivers
please. What prevents that?

> Also BTW of course user space can still override it, but really the defaults
> should be a kernel policy.
>
> There's also the additional problem that one of the goals of confidential
> guest is to just move existing guest virtual images into them without much
> changes. So it's better for such a case if as much as possible of the policy
> is in the kernel. But that's more a secondary consideration, the first point
> is really the important part.

Where exactly are all of these "goals" and "requirements" documented and
who is defining them and forcing them on us without actually doing any
of the work involved?

thanks,

greg k-h

2021-10-02 18:47:25

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

On Sat, Oct 02, 2021 at 07:20:22AM -0700, Andi Kleen wrote:
>
> On 10/2/2021 4:14 AM, Greg Kroah-Hartman wrote:
> > On Sat, Oct 02, 2021 at 07:04:28AM -0400, Michael S. Tsirkin wrote:
> > > On Fri, Oct 01, 2021 at 08:49:28AM -0700, Andi Kleen wrote:
> > > > > Do you have a list of specific drivers and kernel options that you
> > > > > feel you now "trust"?
> > > > For TDX it's currently only virtio net/block/console
> > > >
> > > > But we expect this list to grow slightly over time, but not at a high rate
> > > > (so hopefully <10)
> > > Well there are already >10 virtio drivers and I think it's reasonable
> > > that all of these will be used with encrypted guests. The list will
> > > grow.
> > What is keeping "all" drivers from being on this list?
>
> It would be too much work to harden them all, and it would be pointless
> because all these drivers are never legitimately needed in a virtualized
> environment which only virtualize a very small number of devices.
>
> > How exactly are
> > you determining what should, and should not, be allowed?
>
> Everything that has had reasonable effort at hardening can be added. But if
> someone proposes to add a driver that should trigger additional scrutiny in
> code review. We should also request them to do some fuzzing.

Looks like out of tree modules get a free pass then.
Which is exactly the reverse of what it should be,
people who spent the time to get their drivers into the kernel
expect that if kernel decides to change some API their
driver is automatically updated. This was always the social
contract, was it not?

> It's a bit similar to someone trying to add a new syscall interface. That
> also triggers much additional scrutiny for good reasons and people start
> fuzzing it.
>
>
> > How can
> > drivers move on, or off, of it over time?
>
> Adding something is submitting a patch to the allow list.
>
> I'm not sure the "off" case would happen, unless the driver is completely
> removed, or maybe it has some unfixable security problem. But that is all
> rather unlikely.
>
>
> >
> > And why not just put all of that into userspace and have it pick and
> > choose? That should be the end-goal here, you don't want to encode
> > policy like this in the kernel, right?
>
> How would user space know what drivers have been hardened? This is really
> something that the kernel needs to determine. I don't think we can outsource
> it to anyone else.

IIUC userspace is the distro. It can also do more than a binary on/off,
e.g. it can decide "only virtio", "no out of tree drivers".
A distro can also ship configs with a specific features
enabled/disabled. E.g. I can see where some GPU drivers will be
included by some distros since they are so useful, and excluded
by others since they are so big and hard to audit.
I don't see how the kernel can reasonably make a stand here.
Is "some audit and some fuzzing" a good policy? How much is enough?

> Also BTW of course user space can still override it, but really the defaults
> should be a kernel policy.

Well if userspace sets the policy then I'm not sure we also want
a kernel one ... but if yes I'd like it to be in a central
place so whoever is building the kernel can tweak it easily
and rebuild, without poking at individual drivers.

> There's also the additional problem that one of the goals of confidential
> guest is to just move existing guest virtual images into them without much
> changes. So it's better for such a case if as much as possible of the policy
> is in the kernel.

If it's e.g. the kernel command line then we can set that
when building the kernel right? No need for a dedicated interface
just for this ...

> But that's more a secondary consideration, the first point
> is really the important part.
>
>
> -Andi

2021-10-03 06:41:49

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

On Sat, Oct 02, 2021 at 02:40:55PM -0400, Michael S. Tsirkin wrote:
> On Sat, Oct 02, 2021 at 07:20:22AM -0700, Andi Kleen wrote:
> >
> > On 10/2/2021 4:14 AM, Greg Kroah-Hartman wrote:
> > > On Sat, Oct 02, 2021 at 07:04:28AM -0400, Michael S. Tsirkin wrote:
> > > > On Fri, Oct 01, 2021 at 08:49:28AM -0700, Andi Kleen wrote:
> > > > > > Do you have a list of specific drivers and kernel options that you
> > > > > > feel you now "trust"?
> > > > > For TDX it's currently only virtio net/block/console
> > > > >
> > > > > But we expect this list to grow slightly over time, but not at a high rate
> > > > > (so hopefully <10)
> > > > Well there are already >10 virtio drivers and I think it's reasonable
> > > > that all of these will be used with encrypted guests. The list will
> > > > grow.
> > > What is keeping "all" drivers from being on this list?
> >
> > It would be too much work to harden them all, and it would be pointless
> > because all these drivers are never legitimately needed in a virtualized
> > environment which only virtualize a very small number of devices.
> >
> > > How exactly are
> > > you determining what should, and should not, be allowed?
> >
> > Everything that has had reasonable effort at hardening can be added. But if
> > someone proposes to add a driver that should trigger additional scrutiny in
> > code review. We should also request them to do some fuzzing.
>
> Looks like out of tree modules get a free pass then.

That's not good. As we already know if a module is in or out of the
tree, you all should be banning all out-of-tree modules if you care
about these things. That should be very easy to do if you care.

> > How would user space know what drivers have been hardened? This is really
> > something that the kernel needs to determine. I don't think we can outsource
> > it to anyone else.
>
> IIUC userspace is the distro. It can also do more than a binary on/off,
> e.g. it can decide "only virtio", "no out of tree drivers".
> A distro can also ship configs with a specific features
> enabled/disabled. E.g. I can see where some GPU drivers will be
> included by some distros since they are so useful, and excluded
> by others since they are so big and hard to audit.
> I don't see how the kernel can reasonably make a stand here.
> Is "some audit and some fuzzing" a good policy? How much is enough?

Agreed, that is why the policy for this should be in userspace.

> Well if userspace sets the policy then I'm not sure we also want
> a kernel one ... but if yes I'd like it to be in a central
> place so whoever is building the kernel can tweak it easily
> and rebuild, without poking at individual drivers.

And here I thought the requirement was that no one could rebuild their
kernel as it was provided by someone else.

Again, these requirements seem contradicting, but as no one has actually
pointed me at the real list of them, who knows what they are?

thanks,

greg k-h

2021-10-04 05:26:22

by Mika Westerberg

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

Hi,

On Fri, Oct 01, 2021 at 12:57:18PM -0700, Dan Williams wrote:
> > > Ah, so are you saying that it would be sufficient for USB if the
> > > generic authorized implementation did something like:
> > >
> > > dev->authorized = 1;
> > > device_attach(dev);
> > >
> > > ...for the authorize case, and:
> > >
> > > dev->authorize = 0;
> > > device_release_driver(dev);
> > >
> > > ...for the deauthorize case?
> >
> > Yes, I think so. But I haven't tried making this change to test and
> > see what really happens.
>
> Sounds like a useful path for this effort to explore. Especially as
> Greg seems to want the proposed "has_probe_authorization" flag in the
> bus_type to disappear and make this all generic. It just seems that
> Thunderbolt would need deeper surgery to move what it does in the
> authorization toggle path into the probe and remove paths.
>
> Mika, do you see a path for Thunderbolt to align its authorization
> paths behind bus ->probe() ->remove() events similar to what USB might
> be able to support for a generic authorization path?

In Thunderbolt "authorization" actually means whether there is a PCIe
tunnel to the device or not. There is no driver bind/unbind happening
when authorization toggles (well on Thunderbolt bus, there can be on PCI
bus after the tunnel is established) so I'm not entirely sure how we
could use the bus ->probe() or ->remove for that to be honest.

2021-10-04 23:49:09

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

On Sat, Oct 2, 2021 at 7:20 AM Andi Kleen <[email protected]> wrote:
>
>
> On 10/2/2021 4:14 AM, Greg Kroah-Hartman wrote:
> > On Sat, Oct 02, 2021 at 07:04:28AM -0400, Michael S. Tsirkin wrote:
> >> On Fri, Oct 01, 2021 at 08:49:28AM -0700, Andi Kleen wrote:
> >>>> Do you have a list of specific drivers and kernel options that you
> >>>> feel you now "trust"?
> >>> For TDX it's currently only virtio net/block/console
> >>>
> >>> But we expect this list to grow slightly over time, but not at a high rate
> >>> (so hopefully <10)
> >> Well there are already >10 virtio drivers and I think it's reasonable
> >> that all of these will be used with encrypted guests. The list will
> >> grow.
> > What is keeping "all" drivers from being on this list?
>
> It would be too much work to harden them all, and it would be pointless
> because all these drivers are never legitimately needed in a virtualized
> environment which only virtualize a very small number of devices.
>
> > How exactly are
> > you determining what should, and should not, be allowed?
>
> Everything that has had reasonable effort at hardening can be added. But
> if someone proposes to add a driver that should trigger additional
> scrutiny in code review. We should also request them to do some fuzzing.
>
> It's a bit similar to someone trying to add a new syscall interface.
> That also triggers much additional scrutiny for good reasons and people
> start fuzzing it.
>
>
> > How can
> > drivers move on, or off, of it over time?
>
> Adding something is submitting a patch to the allow list.
>
> I'm not sure the "off" case would happen, unless the driver is
> completely removed, or maybe it has some unfixable security problem. But
> that is all rather unlikely.
>
>
> >
> > And why not just put all of that into userspace and have it pick and
> > choose? That should be the end-goal here, you don't want to encode
> > policy like this in the kernel, right?
>
> How would user space know what drivers have been hardened? This is
> really something that the kernel needs to determine. I don't think we
> can outsource it to anyone else.

How it is outsourcing by moving that same allow list over the kernel /
user boundary. It can be maintained by the same engineers and get
deployed by something like:

dracut --authorize-device-list=confidential-computing-default $kernel-version

With that distributions can deploy kernel-specific authorizations and
admins can deploy site-specific authorizations. Then the kernel
implementation is minimized to authorize just enough drivers by
default to let userspace take over the policy.

> Also BTW of course user space can still override it, but really the
> defaults should be a kernel policy.

The default is secure, trust nothing but bootstrap devices.

> There's also the additional problem that one of the goals of
> confidential guest is to just move existing guest virtual images into
> them without much changes. So it's better for such a case if as much as
> possible of the policy is in the kernel. But that's more a secondary
> consideration, the first point is really the important part.

The same image can be used on host and guest in this "do it in
userspace" proposal.

2021-10-05 22:38:11

[permalink] [raw]

Subject: Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

On Sun, Oct 3, 2021 at 10:16 PM Mika Westerberg
<[email protected]> wrote:
>
> Hi,
>
> On Fri, Oct 01, 2021 at 12:57:18PM -0700, Dan Williams wrote:
> > > > Ah, so are you saying that it would be sufficient for USB if the
> > > > generic authorized implementation did something like:
> > > >
> > > > dev->authorized = 1;
> > > > device_attach(dev);
> > > >
> > > > ...for the authorize case, and:
> > > >
> > > > dev->authorize = 0;
> > > > device_release_driver(dev);
> > > >
> > > > ...for the deauthorize case?
> > >
> > > Yes, I think so. But I haven't tried making this change to test and
> > > see what really happens.
> >
> > Sounds like a useful path for this effort to explore. Especially as
> > Greg seems to want the proposed "has_probe_authorization" flag in the
> > bus_type to disappear and make this all generic. It just seems that
> > Thunderbolt would need deeper surgery to move what it does in the
> > authorization toggle path into the probe and remove paths.
> >
> > Mika, do you see a path for Thunderbolt to align its authorization
> > paths behind bus ->probe() ->remove() events similar to what USB might
> > be able to support for a generic authorization path?
>
> In Thunderbolt "authorization" actually means whether there is a PCIe
> tunnel to the device or not. There is no driver bind/unbind happening
> when authorization toggles (well on Thunderbolt bus, there can be on PCI
> bus after the tunnel is established) so I'm not entirely sure how we
> could use the bus ->probe() or ->remove for that to be honest.

Greg, per your comment:

"... which was to move the way that busses are allowed to authorize
the devices they wish to control into a generic way instead of being
bus-specific logic."

We have USB and TB that have already diverged on the ABI here. The USB
behavior is more in line with the "probe authorization" concept, while
TB is about tunnel establishment and not cleanly tied to probe
authorization. So while I see a path to a common authorization
implementation for USB and other buses (per the insight from Alan), TB
needs to retain the ability to record the authorization state as an
enum rather than a bool, and emit a uevent on authorization status
change.

So how about something like the following that moves the attribute
into the core, but still calls back to TB and USB to perform their
legacy authorization work. This new authorized attribute only shows up
when devices default to not authorized, i.e. when userspace owns the
allow list past critical-boot built-in drivers, or if the bus (USB /
TB) implements ->authorize().

diff --git a/drivers/base/core.c b/drivers/base/core.c
index e65dd803a453..8f8fbe2637d1 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2414,6 +2414,58 @@ static ssize_t online_store(struct device *dev,
struct device_attribute *attr,
}
static DEVICE_ATTR_RW(online);

+static ssize_t authorized_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return sysfs_emit(buf, "%u\n", dev->authorized);
+}
+
+static ssize_t authorized_store(struct device *dev,
+ struct device_attribute *attr, const char *buf,
+ size_t count)
+{
+ unsigned int val, save;
+ ssize_t rc;
+
+ rc = kstrtouint(buf, 0, &val);
+ if (rc < 0)
+ return rc;
+
+ /* some buses (Thunderbolt) support authorized values > 1 */
+ if (val > 1 && !dev->bus->authorize)
+ return -EINVAL;
+
+ device_lock(dev);
+ save = dev->authorized;
+ if (save == val) {
+ rc = count;
+ goto err;
+ }
+
+ dev->authorized = val;
+ if (dev->bus->authorize) {
+ /* notify bus about change in authorization state */
+ rc = dev->bus->authorize(dev);
+ if (rc) {
+ dev->authorized = save;
+ goto err;
+ }
+ }
+ device_unlock(dev);
+
+ if (dev->authorized) {
+ if (!device_attach(dev))
+ dev_dbg(dev, "failed to probe after authorize\n");
+ } else
+ device_release_driver(dev);
+ return count;
+
+err:
+ device_unlock(dev);
+ return rc < 0 ? rc : count;
+}
+static DEVICE_ATTR_RW(authorized);
+
static ssize_t removable_show(struct device *dev, struct
device_attribute *attr,
char *buf)
{
@@ -2616,8 +2668,16 @@ static int device_add_attrs(struct device *dev)
goto err_remove_dev_waiting_for_supplier;
}

+ if (dev_needs_authorization(dev)) {
+ error = device_create_file(dev, &dev_attr_authorized);
+ if (error)
+ goto err_remove_dev_removable;
+ }
+
return 0;

+ err_remove_dev_removable:
+ device_remove_file(dev, &dev_attr_removable);
err_remove_dev_waiting_for_supplier:
device_remove_file(dev, &dev_attr_waiting_for_supplier);
err_remove_dev_online:
@@ -2639,6 +2699,7 @@ static void device_remove_attrs(struct device *dev)
struct class *class = dev->class;
const struct device_type *type = dev->type;

+ device_remove_file(dev, &dev_attr_authorized);
device_remove_file(dev, &dev_attr_removable);
device_remove_file(dev, &dev_attr_waiting_for_supplier);
device_remove_file(dev, &dev_attr_online);
@@ -2805,6 +2866,8 @@ static void klist_children_put(struct klist_node *n)
put_device(dev);
}

+unsigned int dev_default_authorization;
+
/**
* device_initialize - init device structure.
* @dev: device.
diff --git a/include/linux/device.h b/include/linux/device.h
index e270cb740b9e..fbb83e46af9d 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -561,6 +561,7 @@ struct device {
struct dev_iommu *iommu;

enum device_removable removable;
+ unsigned int authorized;

bool offline_disabled:1;
bool offline:1;
@@ -814,6 +815,19 @@ static inline bool dev_removable_is_valid(struct
device *dev)
return dev->removable != DEVICE_REMOVABLE_NOT_SUPPORTED;
}

+extern unsigned int dev_default_authorization;
+
+/*
+ * If the bus has custom authorization, or if devices default to not
+ * authorized, register the 'authorized' attribute for @dev.
+ */
+static inline bool dev_needs_authorization(struct device *dev)
+{
+ if (dev->bus->authorize || dev_default_authorization == 0)
+ return true;
+ return false;
+}
+
/*
* High level routines for use by the bus drivers
*/
diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h
index 062777a45a74..3202a2b13374 100644
--- a/include/linux/device/bus.h
+++ b/include/linux/device/bus.h
@@ -40,6 +40,11 @@ struct fwnode_handle;
* that generate uevents to add the environment variables.
* @probe: Called when a new device or driver add to this bus, and callback
* the specific driver's probe to initial the matched device.
+ * @authorize: Called after authorized_store() changes the
+ * authorization state of the device. Do not use for new
+ * bus implementations, revalidate dev->authorized in
+ * @probe and @remove to take any bus specific
+ * authorization actions.
* @sync_state: Called to sync device state to software state
after all the
* state tracking consumers linked to this device (present at
* the time of late_initcall) have successfully bound to a
@@ -90,6 +95,7 @@ struct bus_type {
int (*match)(struct device *dev, struct device_driver *drv);
int (*uevent)(struct device *dev, struct kobj_uevent_env *env);
int (*probe)(struct device *dev);
+ int (*authorize)(struct device *dev);
void (*sync_state)(struct device *dev);
void (*remove)(struct device *dev);
void (*shutdown)(struct device *dev);

2021-10-06 05:47:31