[PATCH 0/6] [B]iommu: add kernel dma protection

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/6] [B]iommu: add kernel dma protection

Aaron Ma
BugLink: https://bugs.launchpad.net/bugs/1820153

[Impact]
OS can use IOMMU to defend against DMA attacks from a PCI device like thunderbolt one.
Intel adds DMA_CTRL_PLATFORM_OPT_IN_FLAG flag in DMAR ACPI table.
Use this flag to enable IOMMU and use _DSD to identify untrusted PCI devices.

[Fix]
Enable IOMMU when BIOS supports DMA opt in flag and ExternalFacingPort in _DSD.
Disable ATS on the untrusted PCI device.

[Test]
Tested on 2 Intel platforms that supports DMA opt in flag with a thunderbolt dock station.
iommu enabled as expected with this fix.

[Regression Potential]
Upstream fix, Verified on supported platforms, no affection on not supported platforms.
Backported changes are fairly minimal.

These patches are included in 5.0 kernel, disco is good.

Erik Schmauss (1):
  ACPICA: AML parser: attempt to continue loading table after error

Lu Baolu (1):
  iommu/vt-d: Force IOMMU on for platform opt in hint

Mika Westerberg (4):
  ACPI / property: Allow multiple property compatible _DSD entries
  PCI / ACPI: Identify untrusted PCI devices
  iommu/vt-d: Do not enable ATS for untrusted devices
  thunderbolt: Export IOMMU based DMA protection support to userspace

 .../ABI/testing/sysfs-bus-thunderbolt         |   9 ++
 Documentation/admin-guide/thunderbolt.rst     |  20 ++++
 drivers/acpi/acpica/psloop.c                  |  51 ++++++++-
 drivers/acpi/acpica/psobject.c                |  30 +++++
 drivers/acpi/property.c                       | 105 +++++++++++++-----
 drivers/acpi/x86/apple.c                      |   2 +-
 drivers/gpio/gpiolib-acpi.c                   |   2 +-
 drivers/iommu/dmar.c                          |  25 +++++
 drivers/iommu/intel-iommu.c                   |  56 +++++++++-
 drivers/pci/pci-acpi.c                        |  19 ++++
 drivers/pci/probe.c                           |  15 +++
 drivers/thunderbolt/domain.c                  |  17 +++
 include/acpi/acpi_bus.h                       |   8 +-
 include/linux/acpi.h                          |   9 ++
 include/linux/dmar.h                          |   8 ++
 include/linux/pci.h                           |   8 ++
 16 files changed, 351 insertions(+), 33 deletions(-)

--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/6] ACPICA: AML parser: attempt to continue loading table after error

Aaron Ma
From: Erik Schmauss <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1820153

This change alters the parser so that the table load does not abort
upon an error.

Notable changes:

If there is an error while parsing an element of the termlist, we
will skip parsing the current termlist element and continue parsing
to the next opcode in the termlist.

If we get an error while parsing the conditional of If/Else/While or
the device name of Scope, we will skip the body of the statement all
together and pop the parser_state.

If we get an error while parsing the base offset and length of an
operation region declaration, we will remove the operation region
from the namespace.

Signed-off-by: Erik Schmauss <[hidden email]>
Signed-off-by: Rafael J. Wysocki <[hidden email]>
(backported from commit 5088814a6e931350e5bd29f5d59fa40c6dbbdf10)
Signed-off-by: Aaron Ma <[hidden email]>
---
 drivers/acpi/acpica/psloop.c   | 51 +++++++++++++++++++++++++++++++++-
 drivers/acpi/acpica/psobject.c | 30 ++++++++++++++++++++
 2 files changed, 80 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/acpica/psloop.c b/drivers/acpi/acpica/psloop.c
index c275fbff23fd..a1852ce16c94 100644
--- a/drivers/acpi/acpica/psloop.c
+++ b/drivers/acpi/acpica/psloop.c
@@ -543,6 +543,22 @@ acpi_status acpi_ps_parse_loop(struct acpi_walk_state *walk_state)
  if (ACPI_FAILURE(status)) {
  return_ACPI_STATUS(status);
  }
+ if (walk_state->opcode == AML_SCOPE_OP) {
+ /*
+ * If the scope op fails to parse, skip the body of the
+ * scope op because the parse failure indicates that the
+ * device may not exist.
+ */
+ walk_state->parser_state.aml =
+    walk_state->aml + 1;
+ walk_state->parser_state.aml =
+    acpi_ps_get_next_package_end
+    (&walk_state->parser_state);
+ walk_state->aml =
+    walk_state->parser_state.aml;
+ ACPI_ERROR((AE_INFO,
+    "Skipping Scope block"));
+ }
 
  continue;
  }
@@ -585,7 +601,40 @@ acpi_status acpi_ps_parse_loop(struct acpi_walk_state *walk_state)
  if (ACPI_FAILURE(status)) {
  return_ACPI_STATUS(status);
  }
-
+ if ((walk_state->control_state) &&
+    ((walk_state->control_state->control.
+      opcode == AML_IF_OP)
+     || (walk_state->control_state->control.
+ opcode == AML_WHILE_OP))) {
+ /*
+ * If the if/while op fails to parse, we will skip parsing
+ * the body of the op.
+ */
+ parser_state->aml =
+    walk_state->control_state->control.
+    aml_predicate_start + 1;
+ parser_state->aml =
+    acpi_ps_get_next_package_end
+    (parser_state);
+ walk_state->aml = parser_state->aml;
+
+ ACPI_ERROR((AE_INFO,
+    "Skipping While/If block"));
+ if (*walk_state->aml == AML_ELSE_OP) {
+ ACPI_ERROR((AE_INFO,
+    "Skipping Else block"));
+ walk_state->parser_state.aml =
+    walk_state->aml + 1;
+ walk_state->parser_state.aml =
+    acpi_ps_get_next_package_end
+    (parser_state);
+ walk_state->aml =
+    parser_state->aml;
+ }
+ ACPI_FREE(acpi_ut_pop_generic_state
+  (&walk_state->control_state));
+ }
+ op = NULL;
  continue;
  }
  }
diff --git a/drivers/acpi/acpica/psobject.c b/drivers/acpi/acpica/psobject.c
index 0bef6df71bba..9e9d56597e6b 100644
--- a/drivers/acpi/acpica/psobject.c
+++ b/drivers/acpi/acpica/psobject.c
@@ -46,6 +46,7 @@
 #include "acparser.h"
 #include "amlcode.h"
 #include "acconvert.h"
+#include "acnamesp.h"
 
 #define _COMPONENT          ACPI_PARSER
 ACPI_MODULE_NAME("psobject")
@@ -587,6 +588,21 @@ acpi_ps_complete_op(struct acpi_walk_state *walk_state,
 
  do {
  if (*op) {
+ /*
+ * These Opcodes need to be removed from the namespace because they
+ * get created even if these opcodes cannot be created due to
+ * errors.
+ */
+ if (((*op)->common.aml_opcode == AML_REGION_OP)
+    || ((*op)->common.aml_opcode ==
+ AML_DATA_REGION_OP)) {
+ acpi_ns_delete_children((*op)->common.
+ node);
+ acpi_ns_remove_node((*op)->common.node);
+ (*op)->common.node = NULL;
+ acpi_ps_delete_parse_tree(*op);
+ }
+
  status2 =
     acpi_ps_complete_this_op(walk_state, *op);
  if (ACPI_FAILURE(status2)) {
@@ -612,6 +628,20 @@ acpi_ps_complete_op(struct acpi_walk_state *walk_state,
 #endif
  walk_state->prev_op = NULL;
  walk_state->prev_arg_types = walk_state->arg_types;
+
+ if (walk_state->parse_flags & ACPI_PARSE_MODULE_LEVEL) {
+ /*
+ * There was something that went wrong while executing code at the
+ * module-level. We need to skip parsing whatever caused the
+ * error and keep going. One runtime error during the table load
+ * should not cause the entire table to not be loaded. This is
+ * because there could be correct AML beyond the parts that caused
+ * the runtime error.
+ */
+ ACPI_ERROR((AE_INFO,
+    "Ignore error and continue table load"));
+ return_ACPI_STATUS(AE_OK);
+ }
  return_ACPI_STATUS(status);
  }
 
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 2/6] ACPI / property: Allow multiple property compatible _DSD entries

Aaron Ma
In reply to this post by Aaron Ma
From: Mika Westerberg <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1820153

It is possible to have _DSD entries where the data is compatible with
device properties format but are using different GUID for various reasons.
In addition to that there can be many such _DSD entries for a single device
such as for PCIe root port used to host a Thunderbolt hierarchy:

    Scope (\_SB.PCI0.RP21)
    {
        Name (_DSD, Package () {
            ToUUID ("6211e2c0-58a3-4af3-90e1-927a4e0c55a4"),
            Package () {
                Package () {"HotPlugSupportInD3", 1}
            },

            ToUUID ("efcc06cc-73ac-4bc3-bff0-76143807c389"),
            Package () {
                Package () {"ExternalFacingPort", 1},
                Package () {"UID", 0 }
            }
        })
    }

More information about these new _DSD entries can be found in:

  https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports

To make these available for drivers via unified device property APIs,
modify ACPI property core so that it supports multiple _DSD entries
organized in a linked list. We also store GUID of each _DSD entry in struct
acpi_device_properties in case there is need to differentiate between
entries. The supported GUIDs are then listed in prp_guids array.

Signed-off-by: Mika Westerberg <[hidden email]>
Signed-off-by: Bjorn Helgaas <[hidden email]>
Reviewed-by: Rafael J. Wysocki <[hidden email]>
Acked-by: Sakari Ailus <[hidden email]>
(cherry picked from commit 5f5e4890d57a8af5da72c9d73a4efa9bad43a7a3)
Signed-off-by: Aaron Ma <[hidden email]>
---
 drivers/acpi/property.c     | 94 +++++++++++++++++++++++++++----------
 drivers/acpi/x86/apple.c    |  2 +-
 drivers/gpio/gpiolib-acpi.c |  2 +-
 include/acpi/acpi_bus.h     |  8 +++-
 include/linux/acpi.h        |  9 ++++
 5 files changed, 86 insertions(+), 29 deletions(-)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index e26ea209b63e..1fbe5c0fd230 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -24,11 +24,12 @@ static int acpi_data_get_property_array(const struct acpi_device_data *data,
  acpi_object_type type,
  const union acpi_object **obj);
 
-/* ACPI _DSD device properties GUID: daffd814-6eba-4d8c-8a91-bc9bbf4aa301 */
-static const guid_t prp_guid =
+static const guid_t prp_guids[] = {
+ /* ACPI _DSD device properties GUID: daffd814-6eba-4d8c-8a91-bc9bbf4aa301 */
  GUID_INIT(0xdaffd814, 0x6eba, 0x4d8c,
-  0x8a, 0x91, 0xbc, 0x9b, 0xbf, 0x4a, 0xa3, 0x01);
-/* ACPI _DSD data subnodes GUID: dbb8e3e6-5886-4ba6-8795-1319f52a966b */
+  0x8a, 0x91, 0xbc, 0x9b, 0xbf, 0x4a, 0xa3, 0x01),
+};
+
 static const guid_t ads_guid =
  GUID_INIT(0xdbb8e3e6, 0x5886, 0x4ba6,
   0x87, 0x95, 0x13, 0x19, 0xf5, 0x2a, 0x96, 0x6b);
@@ -56,6 +57,7 @@ static bool acpi_nondev_subnode_extract(const union acpi_object *desc,
  dn->name = link->package.elements[0].string.pointer;
  dn->fwnode.ops = &acpi_data_fwnode_ops;
  dn->parent = parent;
+ INIT_LIST_HEAD(&dn->data.properties);
  INIT_LIST_HEAD(&dn->data.subnodes);
 
  result = acpi_extract_properties(desc, &dn->data);
@@ -288,6 +290,35 @@ static void acpi_init_of_compatible(struct acpi_device *adev)
  adev->flags.of_compatible_ok = 1;
 }
 
+static bool acpi_is_property_guid(const guid_t *guid)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(prp_guids); i++) {
+ if (guid_equal(guid, &prp_guids[i]))
+ return true;
+ }
+
+ return false;
+}
+
+struct acpi_device_properties *
+acpi_data_add_props(struct acpi_device_data *data, const guid_t *guid,
+    const union acpi_object *properties)
+{
+ struct acpi_device_properties *props;
+
+ props = kzalloc(sizeof(*props), GFP_KERNEL);
+ if (props) {
+ INIT_LIST_HEAD(&props->list);
+ props->guid = guid;
+ props->properties = properties;
+ list_add_tail(&props->list, &data->properties);
+ }
+
+ return props;
+}
+
 static bool acpi_extract_properties(const union acpi_object *desc,
     struct acpi_device_data *data)
 {
@@ -312,7 +343,7 @@ static bool acpi_extract_properties(const union acpi_object *desc,
     properties->type != ACPI_TYPE_PACKAGE)
  break;
 
- if (!guid_equal((guid_t *)guid->buffer.pointer, &prp_guid))
+ if (!acpi_is_property_guid((guid_t *)guid->buffer.pointer))
  continue;
 
  /*
@@ -320,13 +351,13 @@ static bool acpi_extract_properties(const union acpi_object *desc,
  * package immediately following it.
  */
  if (!acpi_properties_format_valid(properties))
- break;
+ continue;
 
- data->properties = properties;
- return true;
+ acpi_data_add_props(data, (const guid_t *)guid->buffer.pointer,
+    properties);
  }
 
- return false;
+ return !list_empty(&data->properties);
 }
 
 void acpi_init_properties(struct acpi_device *adev)
@@ -336,6 +367,7 @@ void acpi_init_properties(struct acpi_device *adev)
  acpi_status status;
  bool acpi_of = false;
 
+ INIT_LIST_HEAD(&adev->data.properties);
  INIT_LIST_HEAD(&adev->data.subnodes);
 
  if (!adev->handle)
@@ -398,11 +430,16 @@ static void acpi_destroy_nondev_subnodes(struct list_head *list)
 
 void acpi_free_properties(struct acpi_device *adev)
 {
+ struct acpi_device_properties *props, *tmp;
+
  acpi_destroy_nondev_subnodes(&adev->data.subnodes);
  ACPI_FREE((void *)adev->data.pointer);
  adev->data.of_compatible = NULL;
  adev->data.pointer = NULL;
- adev->data.properties = NULL;
+ list_for_each_entry_safe(props, tmp, &adev->data.properties, list) {
+ list_del(&props->list);
+ kfree(props);
+ }
 }
 
 /**
@@ -427,32 +464,37 @@ static int acpi_data_get_property(const struct acpi_device_data *data,
   const char *name, acpi_object_type type,
   const union acpi_object **obj)
 {
- const union acpi_object *properties;
- int i;
+ const struct acpi_device_properties *props;
 
  if (!data || !name)
  return -EINVAL;
 
- if (!data->pointer || !data->properties)
+ if (!data->pointer || list_empty(&data->properties))
  return -EINVAL;
 
- properties = data->properties;
- for (i = 0; i < properties->package.count; i++) {
- const union acpi_object *propname, *propvalue;
- const union acpi_object *property;
+ list_for_each_entry(props, &data->properties, list) {
+ const union acpi_object *properties;
+ unsigned int i;
 
- property = &properties->package.elements[i];
+ properties = props->properties;
+ for (i = 0; i < properties->package.count; i++) {
+ const union acpi_object *propname, *propvalue;
+ const union acpi_object *property;
 
- propname = &property->package.elements[0];
- propvalue = &property->package.elements[1];
+ property = &properties->package.elements[i];
 
- if (!strcmp(name, propname->string.pointer)) {
- if (type != ACPI_TYPE_ANY && propvalue->type != type)
- return -EPROTO;
- if (obj)
- *obj = propvalue;
+ propname = &property->package.elements[0];
+ propvalue = &property->package.elements[1];
 
- return 0;
+ if (!strcmp(name, propname->string.pointer)) {
+ if (type != ACPI_TYPE_ANY &&
+    propvalue->type != type)
+ return -EPROTO;
+ if (obj)
+ *obj = propvalue;
+
+ return 0;
+ }
  }
  }
  return -EINVAL;
diff --git a/drivers/acpi/x86/apple.c b/drivers/acpi/x86/apple.c
index 51b4cf9f25da..130df1c8ed7d 100644
--- a/drivers/acpi/x86/apple.c
+++ b/drivers/acpi/x86/apple.c
@@ -132,8 +132,8 @@ void acpi_extract_apple_properties(struct acpi_device *adev)
  }
  WARN_ON(free_space != (void *)newprops + newsize);
 
- adev->data.properties = newprops;
  adev->data.pointer = newprops;
+ acpi_data_add_props(&adev->data, &apple_prp_guid, newprops);
 
 out_free:
  ACPI_FREE(props);
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 13eb9dc4689b..1de68064481b 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -1134,7 +1134,7 @@ int acpi_gpio_count(struct device *dev, const char *con_id)
 bool acpi_can_fallback_to_crs(struct acpi_device *adev, const char *con_id)
 {
  /* Never allow fallback if the device has properties */
- if (adev->data.properties || adev->driver_gpios)
+ if (acpi_dev_has_props(adev) || adev->driver_gpios)
  return false;
 
  return con_id == NULL;
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 367f0fb99a35..2a2101bfd7fc 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -343,10 +343,16 @@ struct acpi_device_physical_node {
  bool put_online:1;
 };
 
+struct acpi_device_properties {
+ const guid_t *guid;
+ const union acpi_object *properties;
+ struct list_head list;
+};
+
 /* ACPI Device Specific Data (_DSD) */
 struct acpi_device_data {
  const union acpi_object *pointer;
- const union acpi_object *properties;
+ struct list_head properties;
  const union acpi_object *of_compatible;
  struct list_head subnodes;
 };
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index a4b3475bcf2e..5aefebd32aae 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1051,6 +1051,15 @@ static inline int acpi_node_get_property_reference(
  MAX_ACPI_REFERENCE_ARGS, args);
 }
 
+static inline bool acpi_dev_has_props(const struct acpi_device *adev)
+{
+ return !list_empty(&adev->data.properties);
+}
+
+struct acpi_device_properties *
+acpi_data_add_props(struct acpi_device_data *data, const guid_t *guid,
+    const union acpi_object *properties);
+
 int acpi_node_prop_get(const struct fwnode_handle *fwnode, const char *propname,
        void **valptr);
 int acpi_dev_prop_read_single(struct acpi_device *adev,
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 3/6] PCI / ACPI: Identify untrusted PCI devices

Aaron Ma
In reply to this post by Aaron Ma
From: Mika Westerberg <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1820153

A malicious PCI device may use DMA to attack the system. An external
Thunderbolt port is a convenient point to attach such a device. The OS
may use IOMMU to defend against DMA attacks.

Some BIOSes mark these externally facing root ports with this
ACPI _DSD [1]:

  Name (_DSD, Package () {
      ToUUID ("efcc06cc-73ac-4bc3-bff0-76143807c389"),
      Package () {
          Package () {"ExternalFacingPort", 1},
          Package () {"UID", 0 }
      }
  })

If we find such a root port, mark it and all its children as untrusted.
The rest of the OS may use this information to enable DMA protection
against malicious devices. For instance the device may be put behind an
IOMMU to keep it from accessing memory outside of what the driver has
allocated for it.

While at it, add a comment on top of prp_guids array explaining the
possible caveat resulting when these GUIDs are treated equivalent.

[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-externally-exposed-pcie-root-ports

Signed-off-by: Mika Westerberg <[hidden email]>
Acked-by: Rafael J. Wysocki <[hidden email]>
Acked-by: Bjorn Helgaas <[hidden email]>
(backported from commit 617654aae50eb59dd98aa53fb562e850937f4cde)
Signed-off-by: Aaron Ma <[hidden email]>
---
 drivers/acpi/property.c | 11 +++++++++++
 drivers/pci/pci-acpi.c  | 19 +++++++++++++++++++
 drivers/pci/probe.c     | 15 +++++++++++++++
 include/linux/pci.h     |  8 ++++++++
 4 files changed, 53 insertions(+)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 1fbe5c0fd230..6c2c53b741e1 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -24,10 +24,21 @@ static int acpi_data_get_property_array(const struct acpi_device_data *data,
  acpi_object_type type,
  const union acpi_object **obj);
 
+/*
+ * The GUIDs here are made equivalent to each other in order to avoid extra
+ * complexity in the properties handling code, with the caveat that the
+ * kernel will accept certain combinations of GUID and properties that are
+ * not defined without a warning. For instance if any of the properties
+ * from different GUID appear in a property list of another, it will be
+ * accepted by the kernel. Firmware validation tools should catch these.
+ */
 static const guid_t prp_guids[] = {
  /* ACPI _DSD device properties GUID: daffd814-6eba-4d8c-8a91-bc9bbf4aa301 */
  GUID_INIT(0xdaffd814, 0x6eba, 0x4d8c,
   0x8a, 0x91, 0xbc, 0x9b, 0xbf, 0x4a, 0xa3, 0x01),
+ /* External facing port GUID: efcc06cc-73ac-4bc3-bff0-76143807c389 */
+ GUID_INIT(0xefcc06cc, 0x73ac, 0x4bc3,
+  0xbf, 0xf0, 0x76, 0x14, 0x38, 0x07, 0xc3, 0x89),
 };
 
 static const guid_t ads_guid =
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 41cd931232d6..67c4bbd73bc2 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -735,6 +735,24 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev,
  ACPI_FREE(obj);
 }
 
+static void pci_acpi_set_untrusted(struct pci_dev *dev)
+{
+ u8 val;
+
+ if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
+ return;
+ if (device_property_read_u8(&dev->dev, "ExternalFacingPort", &val))
+ return;
+
+ /*
+ * These root ports expose PCIe (including DMA) outside of the
+ * system so make sure we treat them and everything behind as
+ * untrusted.
+ */
+ if (val)
+ dev->untrusted = 1;
+}
+
 static void pci_acpi_setup(struct device *dev)
 {
  struct pci_dev *pci_dev = to_pci_dev(dev);
@@ -744,6 +762,7 @@ static void pci_acpi_setup(struct device *dev)
  return;
 
  pci_acpi_optimize_delay(pci_dev, adev->handle);
+ pci_acpi_set_untrusted(pci_dev);
 
  pci_acpi_add_pm_notifier(adev, pci_dev);
  if (!adev->wakeup.flags.valid)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 7ec4357d991c..6ba8174fab03 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1296,6 +1296,19 @@ static void set_pcie_thunderbolt(struct pci_dev *dev)
  }
 }
 
+static void set_pcie_untrusted(struct pci_dev *dev)
+{
+ struct pci_dev *parent;
+
+ /*
+ * If the upstream bridge is untrusted we treat this device
+ * untrusted as well.
+ */
+ parent = pci_upstream_bridge(dev);
+ if (parent && parent->untrusted)
+ dev->untrusted = true;
+}
+
 /**
  * pci_ext_cfg_is_aliased - is ext config space just an alias of std config?
  * @dev: PCI device
@@ -1479,6 +1492,8 @@ int pci_setup_device(struct pci_dev *dev)
  /* need to have dev->cfg_size ready */
  set_pcie_thunderbolt(dev);
 
+ set_pcie_untrusted(dev);
+
  /* "Unknown power state" */
  dev->current_state = PCI_UNKNOWN;
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 9074d6cad0da..18f8f490b90a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -393,6 +393,14 @@ struct pci_dev {
  unsigned int reset_fn:1;
  unsigned int    is_hotplug_bridge:1;
  unsigned int is_thunderbolt:1; /* Thunderbolt controller */
+ /*
+ * Devices marked being untrusted are the ones that can potentially
+ * execute DMA attacks and similar. They are typically connected
+ * through external ports such as Thunderbolt but not limited to
+ * that. When an IOMMU is enabled they should be getting full
+ * mappings to make sure they cannot access arbitrary memory.
+ */
+ unsigned int untrusted:1;
  unsigned int    __aer_firmware_first_valid:1;
  unsigned int __aer_firmware_first:1;
  unsigned int broken_intx_masking:1; /* INTx masking can't be used */
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 4/6] iommu/vt-d: Force IOMMU on for platform opt in hint

Aaron Ma
In reply to this post by Aaron Ma
From: Lu Baolu <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1820153

Intel VT-d spec added a new DMA_CTRL_PLATFORM_OPT_IN_FLAG flag in DMAR
ACPI table [1] for BIOS to report compliance about platform initiated
DMA restricted to RMRR ranges when transferring control to the OS. This
means that during OS boot, before it enables IOMMU none of the connected
devices can bypass DMA protection for instance by overwriting the data
structures used by the IOMMU. The OS also treats this as a hint that the
IOMMU should be enabled to prevent DMA attacks from possible malicious
devices.

A use of this flag is Kernel DMA protection for Thunderbolt [2] which in
practice means that IOMMU should be enabled for PCIe devices connected
to the Thunderbolt ports. With IOMMU enabled for these devices, all DMA
operations are limited in the range reserved for it, thus the DMA
attacks are prevented. All these devices are enumerated in the PCI/PCIe
module and marked with an untrusted flag.

This forces IOMMU to be enabled if DMA_CTRL_PLATFORM_OPT_IN_FLAG is set
in DMAR ACPI table and there are PCIe devices marked as untrusted in the
system. This can be turned off by adding "intel_iommu=off" in the kernel
command line, if any problems are found.

[1] https://software.intel.com/sites/default/files/managed/c5/15/vt-directed-io-spec.pdf
[2] https://docs.microsoft.com/en-us/windows/security/information-protection/kernel-dma-protection-for-thunderbolt

Cc: Jacob Pan <[hidden email]>
Cc: Sohil Mehta <[hidden email]>
Signed-off-by: Lu Baolu <[hidden email]>
Signed-off-by: Mika Westerberg <[hidden email]>
Reviewed-by: Ashok Raj <[hidden email]>
Reviewed-by: Joerg Roedel <[hidden email]>
Acked-by: Joerg Roedel <[hidden email]>
(cherry picked from commit 89a6079df791aeace2044ea93be1b397195824ec)
Signed-off-by: Aaron Ma <[hidden email]>
---
 drivers/iommu/dmar.c        | 25 +++++++++++++++++
 drivers/iommu/intel-iommu.c | 53 +++++++++++++++++++++++++++++++++++--
 include/linux/dmar.h        |  8 ++++++
 3 files changed, 84 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 4e3e3d2f51c8..96e3c784f197 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -2044,3 +2044,28 @@ int dmar_device_remove(acpi_handle handle)
 {
  return dmar_device_hotplug(handle, false);
 }
+
+/*
+ * dmar_platform_optin - Is %DMA_CTRL_PLATFORM_OPT_IN_FLAG set in DMAR table
+ *
+ * Returns true if the platform has %DMA_CTRL_PLATFORM_OPT_IN_FLAG set in
+ * the ACPI DMAR table. This means that the platform boot firmware has made
+ * sure no device can issue DMA outside of RMRR regions.
+ */
+bool dmar_platform_optin(void)
+{
+ struct acpi_table_dmar *dmar;
+ acpi_status status;
+ bool ret;
+
+ status = acpi_get_table(ACPI_SIG_DMAR, 0,
+ (struct acpi_table_header **)&dmar);
+ if (ACPI_FAILURE(status))
+ return false;
+
+ ret = !!(dmar->flags & DMAR_PLATFORM_OPT_IN);
+ acpi_put_table((struct acpi_table_header *)dmar);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(dmar_platform_optin);
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 869f37d1f3b7..8f3ab84163b2 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -182,6 +182,7 @@ static int rwbf_quirk;
  */
 static int force_on = 0;
 int intel_iommu_tboot_noforce;
+static int no_platform_optin;
 
 /*
  * 0: Present
@@ -559,6 +560,7 @@ static int __init intel_iommu_setup(char *str)
  pr_info("IOMMU enabled\n");
  } else if (!strncmp(str, "off", 3)) {
  dmar_disabled = 1;
+ no_platform_optin = 1;
  pr_info("IOMMU disabled\n");
  } else if (!strncmp(str, "igfx_off", 8)) {
  dmar_map_gfx = 0;
@@ -2886,6 +2888,13 @@ static int iommu_should_identity_map(struct device *dev, int startup)
  if (device_is_rmrr_locked(dev))
  return 0;
 
+ /*
+ * Prevent any device marked as untrusted from getting
+ * placed into the statically identity mapping domain.
+ */
+ if (pdev->untrusted)
+ return 0;
+
  if ((iommu_identity_mapping & IDENTMAP_AZALIA) && IS_AZALIA(pdev))
  return 1;
 
@@ -4722,14 +4731,54 @@ const struct attribute_group *intel_iommu_groups[] = {
  NULL,
 };
 
+static int __init platform_optin_force_iommu(void)
+{
+ struct pci_dev *pdev = NULL;
+ bool has_untrusted_dev = false;
+
+ if (!dmar_platform_optin() || no_platform_optin)
+ return 0;
+
+ for_each_pci_dev(pdev) {
+ if (pdev->untrusted) {
+ has_untrusted_dev = true;
+ break;
+ }
+ }
+
+ if (!has_untrusted_dev)
+ return 0;
+
+ if (no_iommu || dmar_disabled)
+ pr_info("Intel-IOMMU force enabled due to platform opt in\n");
+
+ /*
+ * If Intel-IOMMU is disabled by default, we will apply identity
+ * map for all devices except those marked as being untrusted.
+ */
+ if (dmar_disabled)
+ iommu_identity_mapping |= IDENTMAP_ALL;
+
+ dmar_disabled = 0;
+#if defined(CONFIG_X86) && defined(CONFIG_SWIOTLB)
+ swiotlb = 0;
+#endif
+ no_iommu = 0;
+
+ return 1;
+}
+
 int __init intel_iommu_init(void)
 {
  int ret = -ENODEV;
  struct dmar_drhd_unit *drhd;
  struct intel_iommu *iommu;
 
- /* VT-d is required for a TXT/tboot launch, so enforce that */
- force_on = tboot_force_iommu();
+ /*
+ * Intel IOMMU is required for a TXT/tboot launch or platform
+ * opt in, so enforce that.
+ */
+ force_on = tboot_force_iommu() || platform_optin_force_iommu();
 
  if (iommu_init_mempool()) {
  if (force_on)
diff --git a/include/linux/dmar.h b/include/linux/dmar.h
index e2433bc50210..44ade0600ad7 100644
--- a/include/linux/dmar.h
+++ b/include/linux/dmar.h
@@ -39,6 +39,7 @@ struct acpi_dmar_header;
 /* DMAR Flags */
 #define DMAR_INTR_REMAP 0x1
 #define DMAR_X2APIC_OPT_OUT 0x2
+#define DMAR_PLATFORM_OPT_IN 0x4
 
 struct intel_iommu;
 
@@ -170,6 +171,8 @@ static inline int dmar_ir_hotplug(struct dmar_drhd_unit *dmaru, bool insert)
 { return 0; }
 #endif /* CONFIG_IRQ_REMAP */
 
+extern bool dmar_platform_optin(void);
+
 #else /* CONFIG_DMAR_TABLE */
 
 static inline int dmar_device_add(void *handle)
@@ -182,6 +185,11 @@ static inline int dmar_device_remove(void *handle)
  return 0;
 }
 
+static inline bool dmar_platform_optin(void)
+{
+ return false;
+}
+
 #endif /* CONFIG_DMAR_TABLE */
 
 struct irte {
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 5/6] iommu/vt-d: Do not enable ATS for untrusted devices

Aaron Ma
In reply to this post by Aaron Ma
From: Mika Westerberg <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1820153

Currently Linux automatically enables ATS (Address Translation Service)
for any device that supports it (and IOMMU is turned on). ATS is used to
accelerate DMA access as the device can cache translations locally so
there is no need to do full translation on IOMMU side. However, as
pointed out in [1] ATS can be used to bypass IOMMU based security
completely by simply sending PCIe read/write transaction with AT
(Address Translation) field set to "translated".

To mitigate this modify the Intel IOMMU code so that it does not enable
ATS for any device that is marked as being untrusted. In case this turns
out to cause performance issues we may selectively allow ATS based on
user decision but currently use big hammer and disable it completely to
be on the safe side.

[1] https://www.repository.cam.ac.uk/handle/1810/274352

Signed-off-by: Mika Westerberg <[hidden email]>
Reviewed-by: Ashok Raj <[hidden email]>
Reviewed-by: Joerg Roedel <[hidden email]>
Acked-by: Joerg Roedel <[hidden email]>
(cherry picked from commit fb58fdcd295b914ece1d829b24df00a17a9624bc)
Signed-off-by: Aaron Ma <[hidden email]>
---
 drivers/iommu/intel-iommu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 8f3ab84163b2..21ecab995cb2 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -1515,7 +1515,8 @@ static void iommu_enable_dev_iotlb(struct device_domain_info *info)
  if (info->pri_supported && !pci_reset_pri(pdev) && !pci_enable_pri(pdev, 32))
  info->pri_enabled = 1;
 #endif
- if (info->ats_supported && !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
+ if (!pdev->untrusted && info->ats_supported &&
+    !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
  info->ats_enabled = 1;
  domain_update_iotlb(info->domain);
  info->ats_qdep = pci_ats_queue_depth(pdev);
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 6/6] thunderbolt: Export IOMMU based DMA protection support to userspace

Aaron Ma
In reply to this post by Aaron Ma
From: Mika Westerberg <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1820153

Recent systems with Thunderbolt ports may support IOMMU natively. In
practice this means that Thunderbolt connected devices are placed behind
an IOMMU during the whole time it is connected (including during boot)
making Thunderbolt security levels redundant. This is called Kernel DMA
protection [1] by Microsoft.

Some of these systems still have Thunderbolt security level set to
"user" in order to support OS downgrade (the older version of the OS
might not support IOMMU based DMA protection so connecting a device
still relies on user approval).

Export this information to userspace by introducing a new sysfs
attribute (iommu_dma_protection). Based on it userspace tools can make
more accurate decision whether or not authorize the connected device.

In addition update Thunderbolt documentation regarding IOMMU based DMA
protection.

[1] https://docs.microsoft.com/en-us/windows/security/information-protection/kernel-dma-protection-for-thunderbolt

Signed-off-by: Mika Westerberg <[hidden email]>
Reviewed-by: Yehezkel Bernat <[hidden email]>
(cherry picked from commit dcc3c9e37fbd70e728d08cce0e50121605390fa0)
Signed-off-by: Aaron Ma <[hidden email]>
---
 .../ABI/testing/sysfs-bus-thunderbolt         |  9 +++++++++
 Documentation/admin-guide/thunderbolt.rst     | 20 +++++++++++++++++++
 drivers/thunderbolt/domain.c                  | 17 ++++++++++++++++
 3 files changed, 46 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-thunderbolt b/Documentation/ABI/testing/sysfs-bus-thunderbolt
index 151584a1f950..b21fba14689b 100644
--- a/Documentation/ABI/testing/sysfs-bus-thunderbolt
+++ b/Documentation/ABI/testing/sysfs-bus-thunderbolt
@@ -21,6 +21,15 @@ Description: Holds a comma separated list of device unique_ids that
  If a device is authorized automatically during boot its
  boot attribute is set to 1.
 
+What: /sys/bus/thunderbolt/devices/.../domainX/iommu_dma_protection
+Date: Mar 2019
+KernelVersion: 4.21
+Contact: [hidden email]
+Description: This attribute tells whether the system uses IOMMU
+ for DMA protection. Value of 1 means IOMMU is used 0 means
+ it is not (DMA protection is solely based on Thunderbolt
+ security levels).
+
 What: /sys/bus/thunderbolt/devices/.../domainX/security
 Date: Sep 2017
 KernelVersion: 4.13
diff --git a/Documentation/admin-guide/thunderbolt.rst b/Documentation/admin-guide/thunderbolt.rst
index 35fccba6a9a6..898ad78f3cc7 100644
--- a/Documentation/admin-guide/thunderbolt.rst
+++ b/Documentation/admin-guide/thunderbolt.rst
@@ -133,6 +133,26 @@ If the user still wants to connect the device they can either approve
 the device without a key or write a new key and write 1 to the
 ``authorized`` file to get the new key stored on the device NVM.
 
+DMA protection utilizing IOMMU
+------------------------------
+Recent systems from 2018 and forward with Thunderbolt ports may natively
+support IOMMU. This means that Thunderbolt security is handled by an IOMMU
+so connected devices cannot access memory regions outside of what is
+allocated for them by drivers. When Linux is running on such system it
+automatically enables IOMMU if not enabled by the user already. These
+systems can be identified by reading ``1`` from
+``/sys/bus/thunderbolt/devices/domainX/iommu_dma_protection`` attribute.
+
+The driver does not do anything special in this case but because DMA
+protection is handled by the IOMMU, security levels (if set) are
+redundant. For this reason some systems ship with security level set to
+``none``. Other systems have security level set to ``user`` in order to
+support downgrade to older OS, so users who want to automatically
+authorize devices when IOMMU DMA protection is enabled can use the
+following ``udev`` rule::
+
+  ACTION=="add", SUBSYSTEM=="thunderbolt", ATTRS{iommu_dma_protection}=="1", ATTR{authorized}=="0", ATTR{authorized}="1"
+
 Upgrading NVM on Thunderbolt device or host
 -------------------------------------------
 Since most of the functionality is handled in firmware running on a
diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c
index 092381e2accf..a6672ecdf176 100644
--- a/drivers/thunderbolt/domain.c
+++ b/drivers/thunderbolt/domain.c
@@ -10,7 +10,9 @@
  */
 
 #include <linux/device.h>
+#include <linux/dmar.h>
 #include <linux/idr.h>
+#include <linux/iommu.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
 #include <linux/slab.h>
@@ -239,6 +241,20 @@ static ssize_t boot_acl_store(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RW(boot_acl);
 
+static ssize_t iommu_dma_protection_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ /*
+ * Kernel DMA protection is a feature where Thunderbolt security is
+ * handled natively using IOMMU. It is enabled when IOMMU is
+ * enabled and ACPI DMAR table has DMAR_PLATFORM_OPT_IN set.
+ */
+ return sprintf(buf, "%d\n",
+       iommu_present(&pci_bus_type) && dmar_platform_optin());
+}
+static DEVICE_ATTR_RO(iommu_dma_protection);
+
 static ssize_t security_show(struct device *dev, struct device_attribute *attr,
      char *buf)
 {
@@ -254,6 +270,7 @@ static DEVICE_ATTR_RO(security);
 
 static struct attribute *domain_attrs[] = {
  &dev_attr_boot_acl.attr,
+ &dev_attr_iommu_dma_protection.attr,
  &dev_attr_security.attr,
  NULL,
 };
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team