On Thu, May 13, 2021 at 1:05 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> > > > > +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
