Hello, On Wed, Feb 9, 2022 at 10:49 AM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote: > > On Wed, Feb 9, 2022 at 7:39 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > > > > On Wed, Feb 09, 2022 at 06:46:12AM +0100, Greg Kroah-Hartman wrote: > > > On Tue, Feb 08, 2022 at 04:23:27PM -0800, Rajat Jain wrote: > > > > On Tue, Feb 1, 2022 at 6:01 PM Rajat Jain <rajatja@xxxxxxxxxx> wrote: > > > > > > > > > > Today the pci_dev->untrusted is set for any devices sitting downstream > > > > > an external facing port (determined via "ExternalFacingPort" or the > > > > > "external-facing" properties). > > > > > > > > > > However, currently there is no way for internal devices to be marked as > > > > > untrusted. > > > > > > > > > > There are use-cases though, where a platform would like to treat an > > > > > internal device as untrusted (perhaps because it runs untrusted firmware > > > > > or offers an attack surface by handling untrusted network data etc). > > > > > > > > > > Introduce a new "UntrustedDevice" property that can be used by the > > > > > firmware to mark any device as untrusted. > > > > > > > > Just to unite the threads (from > > > > https://www.spinics.net/lists/linux-pci/msg120221.html). I did reach > > > > out to Microsoft but they haven't acknowledged my email. I also pinged > > > > them again yesterday, but I suspect I may not be able to break the > > > > ice. So this patch may be ready to go in my opinion. > > > > > > > > I don't see any outstanding comments on this patch, but please let me > > > > know if you have any comments. > > > > > > > > > Signed-off-by: Rajat Jain <rajatja@xxxxxxxxxx> > > > > > --- > > > > > v2: * Also use the same property for device tree based systems. > > > > > * Add documentation (next patch) > > > > > > > > > > drivers/pci/of.c | 2 ++ > > > > > drivers/pci/pci-acpi.c | 1 + > > > > > drivers/pci/pci.c | 9 +++++++++ > > > > > drivers/pci/pci.h | 2 ++ > > > > > 4 files changed, 14 insertions(+) > > > > > > > > > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c > > > > > index cb2e8351c2cc..e8b804664b69 100644 > > > > > --- a/drivers/pci/of.c > > > > > +++ b/drivers/pci/of.c > > > > > @@ -24,6 +24,8 @@ void pci_set_of_node(struct pci_dev *dev) > > > > > dev->devfn); > > > > > if (dev->dev.of_node) > > > > > dev->dev.fwnode = &dev->dev.of_node->fwnode; > > > > > + > > > > > + pci_set_untrusted(dev); > > > > > } > > > > > > > > > > void pci_release_of_node(struct pci_dev *dev) > > > > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > > > > > index a42dbf448860..2bffbd5c6114 100644 > > > > > --- a/drivers/pci/pci-acpi.c > > > > > +++ b/drivers/pci/pci-acpi.c > > > > > @@ -1356,6 +1356,7 @@ void pci_acpi_setup(struct device *dev, struct acpi_device *adev) > > > > > > > > > > pci_acpi_optimize_delay(pci_dev, adev->handle); > > > > > pci_acpi_set_external_facing(pci_dev); > > > > > + pci_set_untrusted(pci_dev); > > > > > pci_acpi_add_edr_notifier(pci_dev); > > > > > > > > > > pci_acpi_add_pm_notifier(adev, pci_dev); > > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > > > > index 9ecce435fb3f..41e887c27004 100644 > > > > > --- a/drivers/pci/pci.c > > > > > +++ b/drivers/pci/pci.c > > > > > @@ -6869,3 +6869,12 @@ static int __init pci_realloc_setup_params(void) > > > > > return 0; > > > > > } > > > > > pure_initcall(pci_realloc_setup_params); > > > > > + > > > > > +void pci_set_untrusted(struct pci_dev *pdev) > > > > > +{ > > > > > + u8 val; > > > > > + > > > > > + if (!device_property_read_u8(&pdev->dev, "UntrustedDevice", &val) > > > > If we do this, can we combine it with set_pcie_untrusted(), where we > > already set pdev->untrusted? Maybe that needs to be renamed; I don't > > see anything PCIe-specific there, and it looks like it works for > > conventional PCI as well. Yes, I agree it makes sense to combine with set_pcie_untrusted(). I can do that in the next iteration of my patch, that I intend to work on after we reach some sort of conclusion on the other major comments below. > > > > > Please no, "Untrusted" does not really convey much, if anything here. > > > You are taking an odd in-kernel-value and making it a user api. > > > > > > Where is this "trust" defined? Who defines it? What policy does the > > > kernel impose on it? > > > > I'm a bit hesitant about this, too. It really doesn't have anything > > in particular to do with the PCI core. It's not part of the PCI > > specs, and it could apply to any kind of device, not just PCI (ACPI, > > platform, USB, etc). > > > > We have: > > > > dev->removable # struct device > > pdev->is_thunderbolt > > pdev->untrusted > > pdev->external_facing > > > > and it feels a little hard to keep everything straight. Most of them > > are "discovered" based on some DT or ACPI firmware property. None of > > them really has anything specifically to do with *PCI*, and I don't > > think the PCI core depends on any of them. I think > > pdev->is_thunderbolt is the only one we discover based on a PCI > > feature (the Thunderbolt Capability), and the things we *use* it for > > are actually not things specified by that capability [1]. > > > > Could drivers just look for these properties directly instead of > > relying on the PCI core to get in the middle? Most callers of > > device_property_read_*() are in drivers. I do see that doing it in > > the PCI core might help enforce standard usage in DT/ACPI, but we > > could probably do that in other ways, too. > > FWIW, I agree that looking at these things in drivers would be better. The pci_dev->untrusted property is currently used by: - IOMMU drivers to determine whether bounce buffers should be used, and whether flush queue should be used for these devices. - PCI subsystem to determine ACS settings (ATS / TB etc) As we can see from the usage above, the current primary use of untrusted property in the kernel is to flag and protect against devices that can create a DMA attack on the host physical memory address space (also documented for these properties in [1][2]). IMHO, this property belongs to PCI devices because: * I do not know of any other bus (other than PCI) that can allow DMA access of the host memory, to a device on that bus. * There is some use of this property within the PCI (see above), although I agree it is not much. * The existing properties are currently documented [1][2] to be part of PCIe root ports / PCI-PCI bridges (only): [1] https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports [2] Documentation/devicetree/bindings/pci/pci.txt One can possibly read the device properties in IOMMU drivers, but they'd need to keep it in some device structure. I understand moving the pci_dev->untrusted into struct device has been brought up a couple of times in the past, and has met with much stronger resistance. The discussion turned into a discussion on security, and the semantics of this property, and allowing userspace to change this property etc, requiring major changes, and thus fizzled out of motivation. I'd like to mention that I'm not proposing any changes to the way (already existing) pci_dev->untrusted is being used, or the semantics of this flag. I'm only trying to solve a corner case here i.e. internal devices don't have a way to specify this attribute. Thus requiring us (Chromeos) to carry hacks like [3]. I believe there are others who are also looking for this corner case. From [4]: ============================== We have a similar trust issue with the BMC in servers even though they're internal devices. They're typically network accessible and infrequently updated so treating them as trustworthy isn't a great idea. We have been slowly de-privileging the BMC over the last few years, but the PCIe interface isn't locked down enough for my liking since the SoCs we use do allow software to set the VDID and perform arbitrary DMAs (thankfully limited to 32bit). If we're going to add in infrastructure for handling possibly untrustworthy PCI devices then I'd like to use that for BMCs too. ============================= [3] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3171209 [4] https://lkml.org/lkml/2020/6/9/1467 So from what I see, there is a need to solve this problem for internal PCI devices. And presently what I have, seemed like the path of least resistance to me (i.e. without running into big discussions, and major code changes). I'd greatly appreciate if you could please consider the information I presented above, and suggest an approach that you think is more acceptable. Thanks & Best Regards, Rajat > > > [1] https://lore.kernel.org/r/20220204222956.GA220908@bhelgaas
