| Message ID | 20210512213457.1310774-2-rajatja@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/2] driver core: Move the "removable" attribute from USB to core | expand |
On Wed, May 12, 2021 at 02:34:57PM -0700, Rajat Jain wrote: > A PCI device is "external_facing" if it's a Root Port with the ACPI > "ExternalFacingPort" property or if it has the DT "external-facing" > property. We consider everything downstream from such a device to > be removable by user. > > We're mainly concerned with consumer platforms with user accessible > thunderbolt ports that are vulnerable to DMA attacks, and we expect those > ports to be identified as "ExternalFacingPort". Devices in traditional > hotplug slots can technically be removed, but the expectation is that > unless the port is marked with "ExternalFacingPort", such devices are less > accessible to user / may not be removed by end user, and thus not exposed > as "removable" to userspace. > > Set pci_dev_type.supports_removable so the device core exposes the > "removable" file in sysfs, and tell the device core about removable > devices. > > This can be used by userspace to implment any policies it wants to, > tailored specifically for user removable devices. Eg usage: > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2591812 > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2795038 > (code uses such an attribute to remove external PCI devicces or disable > features on them as needed by the policy desired) > > Signed-off-by: Rajat Jain <rajatja@google.com> > --- > v3: - commit log updated > - Rename set_pci_dev_removable() -> pci_set_removable() > - Call it after applying early PCI quirks. > v2: Add documentation > > Documentation/ABI/testing/sysfs-devices-removable | 3 ++- > drivers/pci/pci-sysfs.c | 1 + > drivers/pci/probe.c | 12 ++++++++++++ > 3 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/Documentation/ABI/testing/sysfs-devices-removable b/Documentation/ABI/testing/sysfs-devices-removable > index 9dabcad7cdcd..ec0b243f5db4 100644 > --- a/Documentation/ABI/testing/sysfs-devices-removable > +++ b/Documentation/ABI/testing/sysfs-devices-removable > @@ -14,4 +14,5 @@ Description: > > Currently this is only supported by USB (which infers the > information from a combination of hub descriptor bits and > - platform-specific data such as ACPI). > + platform-specific data such as ACPI) and PCI (which gets this > + from ACPI / device tree). > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > index beb8d1f4fafe..38b3259ba333 100644 > --- a/drivers/pci/pci-sysfs.c > +++ b/drivers/pci/pci-sysfs.c > @@ -1541,4 +1541,5 @@ static const struct attribute_group *pci_dev_attr_groups[] = { > > const struct device_type pci_dev_type = { > .groups = pci_dev_attr_groups, > + .supports_removable = true, > }; > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 3a62d09b8869..3515afeeaba8 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1575,6 +1575,16 @@ static void set_pcie_untrusted(struct pci_dev *dev) > dev->untrusted = true; > } > > +static void pci_set_removable(struct pci_dev *dev) > +{ > + struct pci_dev *parent = pci_upstream_bridge(dev); > + if (parent && > + (parent->external_facing || dev_is_removable(&parent->dev))) > + dev_set_removable(&dev->dev, DEVICE_REMOVABLE); > + else > + dev_set_removable(&dev->dev, DEVICE_FIXED); > +} Always run checkpatch.pl so you don't get grumpy maintainers telling you to run checkpatch.pl :( And why does external_facing come into play here? I know you say it above, but you should also put it here into the code for when we need to look at it in a few months and wonder what in the world this is doing. Also, are you SURE this is correct and will handle the hotpluggable PCI devices in things like drawers and the like? What is the goal here in exposing this information to userspace, who is going to use it and what is it going to be used for? > + > /** > * pci_ext_cfg_is_aliased - Is ext config space just an alias of std config? > * @dev: PCI device > @@ -1822,6 +1832,8 @@ int pci_setup_device(struct pci_dev *dev) > /* Early fixups, before probing the BARs */ > pci_fixup_device(pci_fixup_early, dev); > > + pci_set_removable(dev); > + > pci_info(dev, "[%04x:%04x] type %02x class %#08x\n", > dev->vendor, dev->device, dev->hdr_type, dev->class); > > -- > 2.31.1.607.g51e8a6a459-goog >
Hello, On Thu, May 13, 2021 at 6:58 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Wed, May 12, 2021 at 02:34:57PM -0700, Rajat Jain wrote: > > A PCI device is "external_facing" if it's a Root Port with the ACPI > > "ExternalFacingPort" property or if it has the DT "external-facing" > > property. We consider everything downstream from such a device to > > be removable by user. > > > > We're mainly concerned with consumer platforms with user accessible > > thunderbolt ports that are vulnerable to DMA attacks, and we expect those > > ports to be identified as "ExternalFacingPort". Devices in traditional > > hotplug slots can technically be removed, but the expectation is that > > unless the port is marked with "ExternalFacingPort", such devices are less > > accessible to user / may not be removed by end user, and thus not exposed > > as "removable" to userspace. > > > > Set pci_dev_type.supports_removable so the device core exposes the > > "removable" file in sysfs, and tell the device core about removable > > devices. > > > > This can be used by userspace to implment any policies it wants to, > > tailored specifically for user removable devices. Eg usage: > > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2591812 > > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2795038 > > (code uses such an attribute to remove external PCI devicces or disable > > features on them as needed by the policy desired) > > > > Signed-off-by: Rajat Jain <rajatja@google.com> > > --- > > v3: - commit log updated > > - Rename set_pci_dev_removable() -> pci_set_removable() > > - Call it after applying early PCI quirks. > > v2: Add documentation > > > > Documentation/ABI/testing/sysfs-devices-removable | 3 ++- > > drivers/pci/pci-sysfs.c | 1 + > > drivers/pci/probe.c | 12 ++++++++++++ > > 3 files changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/ABI/testing/sysfs-devices-removable b/Documentation/ABI/testing/sysfs-devices-removable > > index 9dabcad7cdcd..ec0b243f5db4 100644 > > --- a/Documentation/ABI/testing/sysfs-devices-removable > > +++ b/Documentation/ABI/testing/sysfs-devices-removable > > @@ -14,4 +14,5 @@ Description: > > > > Currently this is only supported by USB (which infers the > > information from a combination of hub descriptor bits and > > - platform-specific data such as ACPI). > > + platform-specific data such as ACPI) and PCI (which gets this > > + from ACPI / device tree). > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > > index beb8d1f4fafe..38b3259ba333 100644 > > --- a/drivers/pci/pci-sysfs.c > > +++ b/drivers/pci/pci-sysfs.c > > @@ -1541,4 +1541,5 @@ static const struct attribute_group *pci_dev_attr_groups[] = { > > > > const struct device_type pci_dev_type = { > > .groups = pci_dev_attr_groups, > > + .supports_removable = true, > > }; > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > index 3a62d09b8869..3515afeeaba8 100644 > > --- a/drivers/pci/probe.c > > +++ b/drivers/pci/probe.c > > @@ -1575,6 +1575,16 @@ static void set_pcie_untrusted(struct pci_dev *dev) > > dev->untrusted = true; > > } > > > > +static void pci_set_removable(struct pci_dev *dev) > > +{ > > + struct pci_dev *parent = pci_upstream_bridge(dev); > > + if (parent && > > + (parent->external_facing || dev_is_removable(&parent->dev))) > > + dev_set_removable(&dev->dev, DEVICE_REMOVABLE); > > + else > > + dev_set_removable(&dev->dev, DEVICE_FIXED); > > +} > > Always run checkpatch.pl so you don't get grumpy maintainers telling you > to run checkpatch.pl :( Yes, I did (it gave me 0 errors and 0 warnings). Please let me know if I need to fix something and I'll be happy to fix that. > > And why does external_facing come into play here? I know you say it > above, but you should also put it here into the code for when we need to > look at it in a few months and wonder what in the world this is doing. Ack, will do. > > Also, are you SURE this is correct and will handle the hotpluggable PCI > devices in things like drawers and the like? Yes, me and Bjorn discussed this in the v2 of this patch (https://patchwork.kernel.org/project/linux-usb/patch/20210424021631.1972022-2-rajatja@google.com/), and yes, this can take care of the hot-pluggable trays if the firmware marks the slots external-facing. > > What is the goal here in exposing this information to userspace, who is > going to use it and what is it going to be used for? The goal here is to implement policies regarding usage of external PCI devices, in userspace. ChromeOS is using it for things like: - Remove external PCI devices when a user logs out. - Don't allow new external PCI devices while the screen is locked. - collect metrics about usage of external PCI devices (how many users actually use it etc). - disable certain features (that are deemed to be dangerous) for external PCI network cards. - etc. Thanks, Rajat > > > > + > > /** > > * pci_ext_cfg_is_aliased - Is ext config space just an alias of std config? > > * @dev: PCI device > > @@ -1822,6 +1832,8 @@ int pci_setup_device(struct pci_dev *dev) > > /* Early fixups, before probing the BARs */ > > pci_fixup_device(pci_fixup_early, dev); > > > > + pci_set_removable(dev); > > + > > pci_info(dev, "[%04x:%04x] type %02x class %#08x\n", > > dev->vendor, dev->device, dev->hdr_type, dev->class); > > > > -- > > 2.31.1.607.g51e8a6a459-goog > >
On Thu, May 13, 2021 at 09:39:58AM -0700, Rajat Jain wrote: > Hello, > > On Thu, May 13, 2021 at 6:58 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Wed, May 12, 2021 at 02:34:57PM -0700, Rajat Jain wrote: > > > A PCI device is "external_facing" if it's a Root Port with the ACPI > > > "ExternalFacingPort" property or if it has the DT "external-facing" > > > property. We consider everything downstream from such a device to > > > be removable by user. > > > > > > We're mainly concerned with consumer platforms with user accessible > > > thunderbolt ports that are vulnerable to DMA attacks, and we expect those > > > ports to be identified as "ExternalFacingPort". Devices in traditional > > > hotplug slots can technically be removed, but the expectation is that > > > unless the port is marked with "ExternalFacingPort", such devices are less > > > accessible to user / may not be removed by end user, and thus not exposed > > > as "removable" to userspace. > > > > > > Set pci_dev_type.supports_removable so the device core exposes the > > > "removable" file in sysfs, and tell the device core about removable > > > devices. > > > > > > This can be used by userspace to implment any policies it wants to, > > > tailored specifically for user removable devices. Eg usage: > > > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2591812 > > > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2795038 > > > (code uses such an attribute to remove external PCI devicces or disable > > > features on them as needed by the policy desired) > > > > > > Signed-off-by: Rajat Jain <rajatja@google.com> > > > --- > > > v3: - commit log updated > > > - Rename set_pci_dev_removable() -> pci_set_removable() > > > - Call it after applying early PCI quirks. > > > v2: Add documentation > > > > > > Documentation/ABI/testing/sysfs-devices-removable | 3 ++- > > > drivers/pci/pci-sysfs.c | 1 + > > > drivers/pci/probe.c | 12 ++++++++++++ > > > 3 files changed, 15 insertions(+), 1 deletion(-) > > > > > > diff --git a/Documentation/ABI/testing/sysfs-devices-removable b/Documentation/ABI/testing/sysfs-devices-removable > > > index 9dabcad7cdcd..ec0b243f5db4 100644 > > > --- a/Documentation/ABI/testing/sysfs-devices-removable > > > +++ b/Documentation/ABI/testing/sysfs-devices-removable > > > @@ -14,4 +14,5 @@ Description: > > > > > > Currently this is only supported by USB (which infers the > > > information from a combination of hub descriptor bits and > > > - platform-specific data such as ACPI). > > > + platform-specific data such as ACPI) and PCI (which gets this > > > + from ACPI / device tree). > > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > > > index beb8d1f4fafe..38b3259ba333 100644 > > > --- a/drivers/pci/pci-sysfs.c > > > +++ b/drivers/pci/pci-sysfs.c > > > @@ -1541,4 +1541,5 @@ static const struct attribute_group *pci_dev_attr_groups[] = { > > > > > > const struct device_type pci_dev_type = { > > > .groups = pci_dev_attr_groups, > > > + .supports_removable = true, > > > }; > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > > index 3a62d09b8869..3515afeeaba8 100644 > > > --- a/drivers/pci/probe.c > > > +++ b/drivers/pci/probe.c > > > @@ -1575,6 +1575,16 @@ static void set_pcie_untrusted(struct pci_dev *dev) > > > dev->untrusted = true; > > > } > > > > > > +static void pci_set_removable(struct pci_dev *dev) > > > +{ > > > + struct pci_dev *parent = pci_upstream_bridge(dev); > > > + if (parent && > > > + (parent->external_facing || dev_is_removable(&parent->dev))) > > > + dev_set_removable(&dev->dev, DEVICE_REMOVABLE); > > > + else > > > + dev_set_removable(&dev->dev, DEVICE_FIXED); > > > +} > > > > Always run checkpatch.pl so you don't get grumpy maintainers telling you > > to run checkpatch.pl :( > > Yes, I did (it gave me 0 errors and 0 warnings). Please let me know if > I need to fix something and I'll be happy to fix that. > > > > > And why does external_facing come into play here? I know you say it > > above, but you should also put it here into the code for when we need to > > look at it in a few months and wonder what in the world this is doing. > > Ack, will do. > > > > > Also, are you SURE this is correct and will handle the hotpluggable PCI > > devices in things like drawers and the like? > > Yes, me and Bjorn discussed this in the v2 of this patch > (https://patchwork.kernel.org/project/linux-usb/patch/20210424021631.1972022-2-rajatja@google.com/), > and yes, this can take care of the hot-pluggable trays if the firmware > marks the slots external-facing. Ok, I'll trust you two :) > > What is the goal here in exposing this information to userspace, who is > > going to use it and what is it going to be used for? > > The goal here is to implement policies regarding usage of external PCI > devices, in userspace. ChromeOS is using it for things like: > - Remove external PCI devices when a user logs out. remove them how? disconnect the device from the system through what method? > - Don't allow new external PCI devices while the screen is locked. Don't allow how? Don't allow the binding of a driver to a device, or the device to be discovered at all? What controls this? > - collect metrics about usage of external PCI devices (how many users > actually use it etc). > - disable certain features (that are deemed to be dangerous) for > external PCI network cards. What is a "dangerous" network feature, RDMA? thanks, greg k-h
On Thu, May 13, 2021 at 10:42 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Thu, May 13, 2021 at 09:39:58AM -0700, Rajat Jain wrote: > > Hello, > > > > On Thu, May 13, 2021 at 6:58 AM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > On Wed, May 12, 2021 at 02:34:57PM -0700, Rajat Jain wrote: > > > > A PCI device is "external_facing" if it's a Root Port with the ACPI > > > > "ExternalFacingPort" property or if it has the DT "external-facing" > > > > property. We consider everything downstream from such a device to > > > > be removable by user. > > > > > > > > We're mainly concerned with consumer platforms with user accessible > > > > thunderbolt ports that are vulnerable to DMA attacks, and we expect those > > > > ports to be identified as "ExternalFacingPort". Devices in traditional > > > > hotplug slots can technically be removed, but the expectation is that > > > > unless the port is marked with "ExternalFacingPort", such devices are less > > > > accessible to user / may not be removed by end user, and thus not exposed > > > > as "removable" to userspace. > > > > > > > > Set pci_dev_type.supports_removable so the device core exposes the > > > > "removable" file in sysfs, and tell the device core about removable > > > > devices. > > > > > > > > This can be used by userspace to implment any policies it wants to, > > > > tailored specifically for user removable devices. Eg usage: > > > > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2591812 > > > > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2795038 > > > > (code uses such an attribute to remove external PCI devicces or disable > > > > features on them as needed by the policy desired) > > > > > > > > Signed-off-by: Rajat Jain <rajatja@google.com> > > > > --- > > > > v3: - commit log updated > > > > - Rename set_pci_dev_removable() -> pci_set_removable() > > > > - Call it after applying early PCI quirks. > > > > v2: Add documentation > > > > > > > > Documentation/ABI/testing/sysfs-devices-removable | 3 ++- > > > > drivers/pci/pci-sysfs.c | 1 + > > > > drivers/pci/probe.c | 12 ++++++++++++ > > > > 3 files changed, 15 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/Documentation/ABI/testing/sysfs-devices-removable b/Documentation/ABI/testing/sysfs-devices-removable > > > > index 9dabcad7cdcd..ec0b243f5db4 100644 > > > > --- a/Documentation/ABI/testing/sysfs-devices-removable > > > > +++ b/Documentation/ABI/testing/sysfs-devices-removable > > > > @@ -14,4 +14,5 @@ Description: > > > > > > > > Currently this is only supported by USB (which infers the > > > > information from a combination of hub descriptor bits and > > > > - platform-specific data such as ACPI). > > > > + platform-specific data such as ACPI) and PCI (which gets this > > > > + from ACPI / device tree). > > > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > > > > index beb8d1f4fafe..38b3259ba333 100644 > > > > --- a/drivers/pci/pci-sysfs.c > > > > +++ b/drivers/pci/pci-sysfs.c > > > > @@ -1541,4 +1541,5 @@ static const struct attribute_group *pci_dev_attr_groups[] = { > > > > > > > > const struct device_type pci_dev_type = { > > > > .groups = pci_dev_attr_groups, > > > > + .supports_removable = true, > > > > }; > > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > > > index 3a62d09b8869..3515afeeaba8 100644 > > > > --- a/drivers/pci/probe.c > > > > +++ b/drivers/pci/probe.c > > > > @@ -1575,6 +1575,16 @@ static void set_pcie_untrusted(struct pci_dev *dev) > > > > dev->untrusted = true; > > > > } > > > > > > > > +static void pci_set_removable(struct pci_dev *dev) > > > > +{ > > > > + struct pci_dev *parent = pci_upstream_bridge(dev); > > > > + if (parent && > > > > + (parent->external_facing || dev_is_removable(&parent->dev))) > > > > + dev_set_removable(&dev->dev, DEVICE_REMOVABLE); > > > > + else > > > > + dev_set_removable(&dev->dev, DEVICE_FIXED); > > > > +} > > > > > > Always run checkpatch.pl so you don't get grumpy maintainers telling you > > > to run checkpatch.pl :( > > > > Yes, I did (it gave me 0 errors and 0 warnings). Please let me know if > > I need to fix something and I'll be happy to fix that. > > > > > > > > And why does external_facing come into play here? I know you say it > > > above, but you should also put it here into the code for when we need to > > > look at it in a few months and wonder what in the world this is doing. > > > > Ack, will do. > > > > > > > > Also, are you SURE this is correct and will handle the hotpluggable PCI > > > devices in things like drawers and the like? > > > > Yes, me and Bjorn discussed this in the v2 of this patch > > (https://patchwork.kernel.org/project/linux-usb/patch/20210424021631.1972022-2-rajatja@google.com/), > > and yes, this can take care of the hot-pluggable trays if the firmware > > marks the slots external-facing. > > Ok, I'll trust you two :) > > > > What is the goal here in exposing this information to userspace, who is > > > going to use it and what is it going to be used for? > > > > The goal here is to implement policies regarding usage of external PCI > > devices, in userspace. ChromeOS is using it for things like: > > - Remove external PCI devices when a user logs out. > > remove them how? disconnect the device from the system through what > method? echo 1 > /sys/bus/pci/devices/<device>/remove > > > - Don't allow new external PCI devices while the screen is locked. > > Don't allow how? Don't allow the binding of a driver to a device, or > the device to be discovered at all? What controls this? Actually Sorry, this was a wrong recollection. > > > - collect metrics about usage of external PCI devices (how many users > > actually use it etc). > > - disable certain features (that are deemed to be dangerous) for > > external PCI network cards. > > What is a "dangerous" network feature, RDMA? For now, we disable offloading of receive path generic / segmentation / checksum features to the external PCI hardware, based on our security team's recommendations. Thanks, Rajat > > thanks, > > greg k-h
Hi On Wed, May 12, 2021 at 2:35 PM Rajat Jain <rajatja@google.com> wrote: > > A PCI device is "external_facing" if it's a Root Port with the ACPI > "ExternalFacingPort" property or if it has the DT "external-facing" > property. We consider everything downstream from such a device to > be removable by user. > > We're mainly concerned with consumer platforms with user accessible > thunderbolt ports that are vulnerable to DMA attacks, and we expect those > ports to be identified as "ExternalFacingPort". Devices in traditional > hotplug slots can technically be removed, but the expectation is that > unless the port is marked with "ExternalFacingPort", such devices are less > accessible to user / may not be removed by end user, and thus not exposed > as "removable" to userspace. > > Set pci_dev_type.supports_removable so the device core exposes the > "removable" file in sysfs, and tell the device core about removable > devices. > > This can be used by userspace to implment any policies it wants to, > tailored specifically for user removable devices. Eg usage: > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2591812 > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2795038 > (code uses such an attribute to remove external PCI devicces or disable > features on them as needed by the policy desired) > > Signed-off-by: Rajat Jain <rajatja@google.com> > --- > v3: - commit log updated > - Rename set_pci_dev_removable() -> pci_set_removable() > - Call it after applying early PCI quirks. > v2: Add documentation > > Documentation/ABI/testing/sysfs-devices-removable | 3 ++- > drivers/pci/pci-sysfs.c | 1 + > drivers/pci/probe.c | 12 ++++++++++++ > 3 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/Documentation/ABI/testing/sysfs-devices-removable b/Documentation/ABI/testing/sysfs-devices-removable > index 9dabcad7cdcd..ec0b243f5db4 100644 > --- a/Documentation/ABI/testing/sysfs-devices-removable > +++ b/Documentation/ABI/testing/sysfs-devices-removable > @@ -14,4 +14,5 @@ Description: > > Currently this is only supported by USB (which infers the > information from a combination of hub descriptor bits and > - platform-specific data such as ACPI). > + platform-specific data such as ACPI) and PCI (which gets this > + from ACPI / device tree). > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > index beb8d1f4fafe..38b3259ba333 100644 > --- a/drivers/pci/pci-sysfs.c > +++ b/drivers/pci/pci-sysfs.c > @@ -1541,4 +1541,5 @@ static const struct attribute_group *pci_dev_attr_groups[] = { > > const struct device_type pci_dev_type = { > .groups = pci_dev_attr_groups, > + .supports_removable = true, > }; > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 3a62d09b8869..3515afeeaba8 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1575,6 +1575,16 @@ static void set_pcie_untrusted(struct pci_dev *dev) > dev->untrusted = true; > } > > +static void pci_set_removable(struct pci_dev *dev) > +{ > + struct pci_dev *parent = pci_upstream_bridge(dev); > + if (parent && > + (parent->external_facing || dev_is_removable(&parent->dev))) > + dev_set_removable(&dev->dev, DEVICE_REMOVABLE); > + else > + dev_set_removable(&dev->dev, DEVICE_FIXED); > +} Copying comments from Krzysztof from another thread: [Krzysztof] We were also wondering if we should only set DEVICE_REMOVABLE for devices known to be behind an external-facing port, and let everything else be set to "unknown" (or whatever the default would be). [Rajat]: I think I'm fine with this proposal if Bjorn & PCI community also sees this as a better idea. Essentially the question here is, would it be better for the non-removable PCI devices to be shown as "fixed" or "unknown"? Thanks, Rajat > + > /** > * pci_ext_cfg_is_aliased - Is ext config space just an alias of std config? > * @dev: PCI device > @@ -1822,6 +1832,8 @@ int pci_setup_device(struct pci_dev *dev) > /* Early fixups, before probing the BARs */ > pci_fixup_device(pci_fixup_early, dev); > > + pci_set_removable(dev); > + > pci_info(dev, "[%04x:%04x] type %02x class %#08x\n", > dev->vendor, dev->device, dev->hdr_type, dev->class); > > -- > 2.31.1.607.g51e8a6a459-goog >
On Thu, May 13, 2021 at 11:02:10AM -0700, Rajat Jain wrote: > On Wed, May 12, 2021 at 2:35 PM Rajat Jain <rajatja@google.com> wrote: > > > > A PCI device is "external_facing" if it's a Root Port with the ACPI > > "ExternalFacingPort" property or if it has the DT "external-facing" > > property. We consider everything downstream from such a device to > > be removable by user. > > > > We're mainly concerned with consumer platforms with user accessible > > thunderbolt ports that are vulnerable to DMA attacks, and we expect those > > ports to be identified as "ExternalFacingPort". Devices in traditional > > hotplug slots can technically be removed, but the expectation is that > > unless the port is marked with "ExternalFacingPort", such devices are less > > accessible to user / may not be removed by end user, and thus not exposed > > as "removable" to userspace. s/thunderbolt/Thunderbolt/ since I think it's a trademark s/identified as/identified by firmware as/ > > Set pci_dev_type.supports_removable so the device core exposes the > > "removable" file in sysfs, and tell the device core about removable > > devices. > > > > This can be used by userspace to implment any policies it wants to, > > tailored specifically for user removable devices. Eg usage: > > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2591812 > > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2795038 > > (code uses such an attribute to remove external PCI devicces or disable > > features on them as needed by the policy desired) s/implment/implement/ s/devicces/devices/ Or maybe something like: This can be used to implement userspace policies tailored for user-removable devices. Not sure exactly what "remove external PCI devices" means. You're talking about the *code* doing something, so I don't think it means physically unplugging the device from the system. Maybe preventing a driver from binding to it or something similar? I hesitate slightly to rely on URLs like googlesource.com in commit logs because we don't know how long they will remain valid. But I guess there's no real alternative here, since this code probably hasn't been posted to any public mailing lists like the ones archived at https://lore.kernel.org/lists.html, right? > > Signed-off-by: Rajat Jain <rajatja@google.com> > > +static void pci_set_removable(struct pci_dev *dev) > > +{ > > + struct pci_dev *parent = pci_upstream_bridge(dev); > > + if (parent && > > + (parent->external_facing || dev_is_removable(&parent->dev))) > > + dev_set_removable(&dev->dev, DEVICE_REMOVABLE); > > + else > > + dev_set_removable(&dev->dev, DEVICE_FIXED); > > +} > > Copying comments from Krzysztof from another thread: > > [Krzysztof] We were also wondering if we should only set DEVICE_REMOVABLE for > devices known to be behind an external-facing port, and let everything > else be set to "unknown" (or whatever the default would be). > > [Rajat]: I think I'm fine with this proposal if Bjorn & PCI community > also sees this as a better idea. Essentially the question here is, > would it be better for the non-removable PCI devices to be shown as > "fixed" or "unknown"? I think I would rather see this as: struct pci_dev *parent = pci_upstream_bridge(dev); if (parent && (parent->external_facing || dev_is_removable(&parent->dev))) dev_set_removable(&dev->dev, DEVICE_REMOVABLE); In other words, assume only that everything below an "external-facing" device is removable. In the absence of an "external-facing" property, we don't know anything about the connection, and I'd rather use the default (probably "unknown") instead of assuming "fixed." I don't think we have anything that depends on "fixed," so I don't think there's value in setting it. (Note the blank line between local variables and the "if"; maybe that's what Greg hinted at?) Bjorn
On Thu, May 13, 2021 at 1:05 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Thu, May 13, 2021 at 11:02:10AM -0700, Rajat Jain wrote: > > On Wed, May 12, 2021 at 2:35 PM Rajat Jain <rajatja@google.com> wrote: > > > > > > A PCI device is "external_facing" if it's a Root Port with the ACPI > > > "ExternalFacingPort" property or if it has the DT "external-facing" > > > property. We consider everything downstream from such a device to > > > be removable by user. > > > > > > We're mainly concerned with consumer platforms with user accessible > > > thunderbolt ports that are vulnerable to DMA attacks, and we expect those > > > ports to be identified as "ExternalFacingPort". Devices in traditional > > > hotplug slots can technically be removed, but the expectation is that > > > unless the port is marked with "ExternalFacingPort", such devices are less > > > accessible to user / may not be removed by end user, and thus not exposed > > > as "removable" to userspace. > > s/thunderbolt/Thunderbolt/ since I think it's a trademark > s/identified as/identified by firmware as/ Ack, will do. > > > > Set pci_dev_type.supports_removable so the device core exposes the > > > "removable" file in sysfs, and tell the device core about removable > > > devices. > > > > > > This can be used by userspace to implment any policies it wants to, > > > tailored specifically for user removable devices. Eg usage: > > > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2591812 > > > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2795038 > > > (code uses such an attribute to remove external PCI devicces or disable > > > features on them as needed by the policy desired) > > s/implment/implement/ > s/devicces/devices/ > > Or maybe something like: > > This can be used to implement userspace policies tailored for > user-removable devices. Ack, will do. > > Not sure exactly what "remove external PCI devices" means. You're > talking about the *code* doing something, so I don't think it means > physically unplugging the device from the system. Maybe preventing a > driver from binding to it or something similar? echo 1 > /sys/bus/pci/devices/<device>/remove > > I hesitate slightly to rely on URLs like googlesource.com in commit > logs because we don't know how long they will remain valid. But I > guess there's no real alternative here, since this code probably > hasn't been posted to any public mailing lists like the ones archived > at https://lore.kernel.org/lists.html, right? Yes, chromium reviews (userspace code that shall use the new attribute) happen over gerrit, and so the publicly available links would be googlesource.com. > > > > Signed-off-by: Rajat Jain <rajatja@google.com> > > > > +static void pci_set_removable(struct pci_dev *dev) > > > +{ > > > + struct pci_dev *parent = pci_upstream_bridge(dev); > > > + if (parent && > > > + (parent->external_facing || dev_is_removable(&parent->dev))) > > > + dev_set_removable(&dev->dev, DEVICE_REMOVABLE); > > > + else > > > + dev_set_removable(&dev->dev, DEVICE_FIXED); > > > +} > > > > Copying comments from Krzysztof from another thread: > > > > [Krzysztof] We were also wondering if we should only set DEVICE_REMOVABLE for > > devices known to be behind an external-facing port, and let everything > > else be set to "unknown" (or whatever the default would be). > > > > [Rajat]: I think I'm fine with this proposal if Bjorn & PCI community > > also sees this as a better idea. Essentially the question here is, > > would it be better for the non-removable PCI devices to be shown as > > "fixed" or "unknown"? > > I think I would rather see this as: > > struct pci_dev *parent = pci_upstream_bridge(dev); > > if (parent && > (parent->external_facing || dev_is_removable(&parent->dev))) > dev_set_removable(&dev->dev, DEVICE_REMOVABLE); > > In other words, assume only that everything below an "external-facing" > device is removable. > > In the absence of an "external-facing" property, we don't know > anything about the connection, and I'd rather use the default > (probably "unknown") instead of assuming "fixed." Ack, will do. One question: Under Greg's latest suggestion, the decision to show this attribute does not have to be bus wide / device_type wide i.e. subsystem can choose for this attribute to show up only under certain devices. So if it is more preferable, I can have this attribute show under ONLY PCI devices that attach below "external-facing" PCI devices (and any other PCI devices will not have this attribute show up at all). I guess this sounds better than having "unknown" show up on the rest of the devices that are not removable? > > I don't think we have anything that depends on "fixed," so I don't > think there's value in setting it. > > (Note the blank line between local variables and the "if"; maybe > that's what Greg hinted at?) Ack, will remove the blank line (didn't know that blank lines between variables and code is not preferred). Thanks, Rajat > > Bjorn
On Thu, May 13, 2021 at 01:34:23PM -0700, Rajat Jain wrote: > On Thu, May 13, 2021 at 1:05 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > On Thu, May 13, 2021 at 11:02:10AM -0700, Rajat Jain wrote: > > > On Wed, May 12, 2021 at 2:35 PM Rajat Jain <rajatja@google.com> wrote: > > > > > > > > A PCI device is "external_facing" if it's a Root Port with the ACPI > > > > "ExternalFacingPort" property or if it has the DT "external-facing" > > > > property. We consider everything downstream from such a device to > > > > be removable by user. > > > > > > > > We're mainly concerned with consumer platforms with user accessible > > > > thunderbolt ports that are vulnerable to DMA attacks, and we expect those > > > > ports to be identified as "ExternalFacingPort". Devices in traditional > > > > hotplug slots can technically be removed, but the expectation is that > > > > unless the port is marked with "ExternalFacingPort", such devices are less > > > > accessible to user / may not be removed by end user, and thus not exposed > > > > as "removable" to userspace. > > > > s/thunderbolt/Thunderbolt/ since I think it's a trademark > > s/identified as/identified by firmware as/ > > Ack, will do. > > > > > > > Set pci_dev_type.supports_removable so the device core exposes the > > > > "removable" file in sysfs, and tell the device core about removable > > > > devices. > > > > > > > > This can be used by userspace to implment any policies it wants to, > > > > tailored specifically for user removable devices. Eg usage: > > > > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2591812 > > > > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2795038 > > > > (code uses such an attribute to remove external PCI devicces or disable > > > > features on them as needed by the policy desired) > > > > s/implment/implement/ > > s/devicces/devices/ > > > > Or maybe something like: > > > > This can be used to implement userspace policies tailored for > > user-removable devices. > > Ack, will do. > > > > > Not sure exactly what "remove external PCI devices" means. You're > > talking about the *code* doing something, so I don't think it means > > physically unplugging the device from the system. Maybe preventing a > > driver from binding to it or something similar? > > echo 1 > /sys/bus/pci/devices/<device>/remove > > > > > I hesitate slightly to rely on URLs like googlesource.com in commit > > logs because we don't know how long they will remain valid. But I > > guess there's no real alternative here, since this code probably > > hasn't been posted to any public mailing lists like the ones archived > > at https://lore.kernel.org/lists.html, right? > > Yes, chromium reviews (userspace code that shall use the new > attribute) happen over gerrit, and so the publicly available links > would be googlesource.com. > > > > > > > Signed-off-by: Rajat Jain <rajatja@google.com> > > > > > > +static void pci_set_removable(struct pci_dev *dev) > > > > +{ > > > > + struct pci_dev *parent = pci_upstream_bridge(dev); > > > > + if (parent && > > > > + (parent->external_facing || dev_is_removable(&parent->dev))) > > > > + dev_set_removable(&dev->dev, DEVICE_REMOVABLE); > > > > + else > > > > + dev_set_removable(&dev->dev, DEVICE_FIXED); > > > > +} > > > > > > Copying comments from Krzysztof from another thread: > > > > > > [Krzysztof] We were also wondering if we should only set DEVICE_REMOVABLE for > > > devices known to be behind an external-facing port, and let everything > > > else be set to "unknown" (or whatever the default would be). > > > > > > [Rajat]: I think I'm fine with this proposal if Bjorn & PCI community > > > also sees this as a better idea. Essentially the question here is, > > > would it be better for the non-removable PCI devices to be shown as > > > "fixed" or "unknown"? > > > > I think I would rather see this as: > > > > struct pci_dev *parent = pci_upstream_bridge(dev); > > > > if (parent && > > (parent->external_facing || dev_is_removable(&parent->dev))) > > dev_set_removable(&dev->dev, DEVICE_REMOVABLE); > > > > In other words, assume only that everything below an "external-facing" > > device is removable. > > > > In the absence of an "external-facing" property, we don't know > > anything about the connection, and I'd rather use the default > > (probably "unknown") instead of assuming "fixed." > > Ack, will do. > > One question: Under Greg's latest suggestion, the decision to show > this attribute does not have to be bus wide / device_type wide i.e. > subsystem can choose for this attribute to show up only under certain > devices. So if it is more preferable, I can have this attribute show > under ONLY PCI devices that attach below "external-facing" PCI devices > (and any other PCI devices will not have this attribute show up at > all). I guess this sounds better than having "unknown" show up on the > rest of the devices that are not removable? If you can make the file appear only for removable devices, that sounds even better. > > I don't think we have anything that depends on "fixed," so I don't > > think there's value in setting it. > > > > (Note the blank line between local variables and the "if"; maybe > > that's what Greg hinted at?) > > Ack, will remove the blank line (didn't know that blank lines between > variables and code is not preferred). The blank line *is* preferred, but your patch didn't include one. Bjorn
diff --git a/Documentation/ABI/testing/sysfs-devices-removable b/Documentation/ABI/testing/sysfs-devices-removable index 9dabcad7cdcd..ec0b243f5db4 100644 --- a/Documentation/ABI/testing/sysfs-devices-removable +++ b/Documentation/ABI/testing/sysfs-devices-removable @@ -14,4 +14,5 @@ Description: Currently this is only supported by USB (which infers the information from a combination of hub descriptor bits and - platform-specific data such as ACPI). + platform-specific data such as ACPI) and PCI (which gets this + from ACPI / device tree). diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c index beb8d1f4fafe..38b3259ba333 100644 --- a/drivers/pci/pci-sysfs.c +++ b/drivers/pci/pci-sysfs.c @@ -1541,4 +1541,5 @@ static const struct attribute_group *pci_dev_attr_groups[] = { const struct device_type pci_dev_type = { .groups = pci_dev_attr_groups, + .supports_removable = true, }; diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 3a62d09b8869..3515afeeaba8 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1575,6 +1575,16 @@ static void set_pcie_untrusted(struct pci_dev *dev) dev->untrusted = true; } +static void pci_set_removable(struct pci_dev *dev) +{ + struct pci_dev *parent = pci_upstream_bridge(dev); + if (parent && + (parent->external_facing || dev_is_removable(&parent->dev))) + dev_set_removable(&dev->dev, DEVICE_REMOVABLE); + else + dev_set_removable(&dev->dev, DEVICE_FIXED); +} + /** * pci_ext_cfg_is_aliased - Is ext config space just an alias of std config? * @dev: PCI device @@ -1822,6 +1832,8 @@ int pci_setup_device(struct pci_dev *dev) /* Early fixups, before probing the BARs */ pci_fixup_device(pci_fixup_early, dev); + pci_set_removable(dev); + pci_info(dev, "[%04x:%04x] type %02x class %#08x\n", dev->vendor, dev->device, dev->hdr_type, dev->class);
A PCI device is "external_facing" if it's a Root Port with the ACPI "ExternalFacingPort" property or if it has the DT "external-facing" property. We consider everything downstream from such a device to be removable by user. We're mainly concerned with consumer platforms with user accessible thunderbolt ports that are vulnerable to DMA attacks, and we expect those ports to be identified as "ExternalFacingPort". Devices in traditional hotplug slots can technically be removed, but the expectation is that unless the port is marked with "ExternalFacingPort", such devices are less accessible to user / may not be removed by end user, and thus not exposed as "removable" to userspace. Set pci_dev_type.supports_removable so the device core exposes the "removable" file in sysfs, and tell the device core about removable devices. This can be used by userspace to implment any policies it wants to, tailored specifically for user removable devices. Eg usage: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2591812 https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2795038 (code uses such an attribute to remove external PCI devicces or disable features on them as needed by the policy desired) Signed-off-by: Rajat Jain <rajatja@google.com> --- v3: - commit log updated - Rename set_pci_dev_removable() -> pci_set_removable() - Call it after applying early PCI quirks. v2: Add documentation Documentation/ABI/testing/sysfs-devices-removable | 3 ++- drivers/pci/pci-sysfs.c | 1 + drivers/pci/probe.c | 12 ++++++++++++ 3 files changed, 15 insertions(+), 1 deletion(-)