From: Saravana Kannan <saravanak@google.com> To: Rajat Jain <rajatja@google.com> Cc: David Woodhouse <dwmw2@infradead.org>, Lu Baolu <baolu.lu@linux.intel.com>, Joerg Roedel <joro@8bytes.org>, Bjorn Helgaas <bhelgaas@google.com>, "Rafael J. Wysocki" <rjw@rjwysocki.net>, Len Brown <lenb@kernel.org>, iommu@lists.linux-foundation.org, LKML <linux-kernel@vger.kernel.org>, Linux PCI <linux-pci@vger.kernel.org>, ACPI Devel Maling List <linux-acpi@vger.kernel.org>, Raj Ashok <ashok.raj@intel.com>, lalithambika.krishnakumar@intel.com, Mika Westerberg <mika.westerberg@linux.intel.com>, Jean-Philippe Brucker <jean-philippe@linaro.org>, Prashant Malani <pmalani@google.com>, Benson Leung <bleung@google.com>, Todd Broch <tbroch@google.com>, Alex Levin <levinale@google.com>, Mattias Nissler <mnissler@google.com>, Rajat Jain <rajatxjain@gmail.com>, Bernie Keany <bernie.keany@intel.com>, Aaron Durbin <adurbin@google.com>, Diego Rivas <diegorivas@google.com>, Duncan Laurie <dlaurie@google.com>, Furquan Shaikh <furquan@google.com>, Jesse Barnes <jsbarnes@google.com>, Christian Kellner <christian@kellner.me>, Alex Williamson <alex.williamson@redhat.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, oohall@gmail.com, Suzuki K Poulose <suzuki.poulose@arm.com>, Arnd Bergmann <arnd@arndb.de>, Heikki Krogerus <heikki.krogerus@linux.intel.com> Subject: Re: [PATCH v2 5/7] driver core: Add device location to "struct device" and expose it in sysfs Date: Tue, 30 Jun 2020 10:43:37 -0700 [thread overview] Message-ID: <CAGETcx9hgV70DVdbOvCF+tO4b-6+1JzN1_OmPmnWjj9qJhB_dw@mail.gmail.com> (raw) In-Reply-To: <20200630044943.3425049-6-rajatja@google.com> On Mon, Jun 29, 2020 at 9:49 PM Rajat Jain <rajatja@google.com> wrote: > > Add a new (optional) field to denote the physical location of a device > in the system, and expose it in sysfs. This was discussed here: > https://lore.kernel.org/linux-acpi/20200618184621.GA446639@kroah.com/ > > (The primary choice for attribute name i.e. "location" is already > exposed as an ABI elsewhere, so settled for "site"). Individual buses > that want to support this new attribute can opt-in by setting a flag in > bus_type, and then populating the location of device while enumerating > it. > > Signed-off-by: Rajat Jain <rajatja@google.com> > --- > v2: (Initial version) > > drivers/base/core.c | 35 +++++++++++++++++++++++++++++++ > include/linux/device.h | 42 ++++++++++++++++++++++++++++++++++++++ > include/linux/device/bus.h | 8 ++++++++ > 3 files changed, 85 insertions(+) > <snip> I'm not CC'ed in 4/7, so just replying > diff --git a/include/linux/device.h b/include/linux/device.h > index 15460a5ac024a..a4143735ae712 100644 > --- a/include/linux/device.h > +++ b/include/linux/device.h > @@ -428,6 +428,31 @@ enum dl_dev_state { > DL_DEV_UNBINDING, > }; > > +/** > + * enum device_site - Physical location of the device in the system. > + * The semantics of values depend on subsystem / bus: > + * > + * @SITE_UNKNOWN: Location is Unknown (default) > + * > + * @SITE_INTERNAL: Device is internal to the system, and cannot be (easily) > + * removed. E.g. SoC internal devices, onboard soldered > + * devices, internal M.2 cards (that cannot be removed > + * without opening the chassis). > + * @SITE_EXTENDED: Device sits an extension of the system. E.g. devices > + * on external PCIe trays, docking stations etc. These > + * devices may be removable, but are generally housed > + * internally on an extension board, so they are removed > + * only when that whole extension board is removed. > + * @SITE_EXTERNAL: Devices truly external to the system (i.e. plugged on > + * an external port) that may be removed or added frequently. > + */ > +enum device_site { > + SITE_UNKNOWN = 0, > + SITE_INTERNAL, > + SITE_EXTENDED, > + SITE_EXTERNAL, > +}; > + > /** > * struct dev_links_info - Device data related to device links. > * @suppliers: List of links to supplier devices. > @@ -513,6 +538,7 @@ struct dev_links_info { > * device (i.e. the bus driver that discovered the device). > * @iommu_group: IOMMU group the device belongs to. > * @iommu: Per device generic IOMMU runtime data > + * @site: Physical location of the device w.r.t. the system > * > * @offline_disabled: If set, the device is permanently online. > * @offline: Set after successful invocation of bus type's .offline(). > @@ -613,6 +639,8 @@ struct device { > struct iommu_group *iommu_group; > struct dev_iommu *iommu; > > + enum device_site site; /* Device physical location */ > + > bool offline_disabled:1; > bool offline:1; > bool of_node_reused:1; > @@ -806,6 +834,20 @@ static inline bool dev_has_sync_state(struct device *dev) > return false; > } > > +static inline int dev_set_site(struct device *dev, enum device_site site) > +{ > + if (site < SITE_UNKNOWN || site > SITE_EXTERNAL) > + return -EINVAL; > + > + dev->site = site; > + return 0; > +} > + > +static inline bool dev_is_external(struct device *dev) > +{ > + return dev->site == SITE_EXTERNAL; > +} I'm not CC'ed in the rest of the patches in this series, so just responding here. I see you use this function in patch 6/7 to decide if the PCI device is trusted. Anything other than EXTERNAL is being treated as trusted. I'd argue that anything that's not internal should be distrusted. For example, I can have a hacked up laptop dock that I can share with you when you visit my home/office and now you are trusting it when you shouldn't be. Also, "UNKNOWN" is treated as trusted in patch 6/7. I'm guessing this is because some of the devices might not have the info in their firmware? At which point, this feature isn't even protecting all the PCI ports properly? This adds to Greg point that this should be a userspace policy so that it can override whatever is wrong/missing in the firmware. -Saravana
WARNING: multiple messages have this Message-ID (diff)
From: Saravana Kannan via iommu <iommu@lists.linux-foundation.org> To: Rajat Jain <rajatja@google.com> Cc: Todd Broch <tbroch@google.com>, Linux PCI <linux-pci@vger.kernel.org>, lalithambika.krishnakumar@intel.com, Heikki Krogerus <heikki.krogerus@linux.intel.com>, Diego Rivas <diegorivas@google.com>, Jean-Philippe Brucker <jean-philippe@linaro.org>, Furquan Shaikh <furquan@google.com>, Raj Ashok <ashok.raj@intel.com>, ACPI Devel Maling List <linux-acpi@vger.kernel.org>, Christian Kellner <christian@kellner.me>, Mattias Nissler <mnissler@google.com>, Jesse Barnes <jsbarnes@google.com>, Len Brown <lenb@kernel.org>, Rajat Jain <rajatxjain@gmail.com>, Prashant Malani <pmalani@google.com>, Suzuki K Poulose <suzuki.poulose@arm.com>, Aaron Durbin <adurbin@google.com>, Alex Williamson <alex.williamson@redhat.com>, Bjorn Helgaas <bhelgaas@google.com>, Mika Westerberg <mika.westerberg@linux.intel.com>, Bernie Keany <bernie.keany@intel.com>, Duncan Laurie <dlaurie@google.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, "Rafael J. Wysocki" <rjw@rjwysocki.net>, LKML <linux-kernel@vger.kernel.org>, iommu@lists.linux-foundation.org, Arnd Bergmann <arnd@arndb.de>, oohall@gmail.com, Benson Leung <bleung@google.com>, David Woodhouse <dwmw2@infradead.org>, Alex Levin <levinale@google.com> Subject: Re: [PATCH v2 5/7] driver core: Add device location to "struct device" and expose it in sysfs Date: Tue, 30 Jun 2020 10:43:37 -0700 [thread overview] Message-ID: <CAGETcx9hgV70DVdbOvCF+tO4b-6+1JzN1_OmPmnWjj9qJhB_dw@mail.gmail.com> (raw) In-Reply-To: <20200630044943.3425049-6-rajatja@google.com> On Mon, Jun 29, 2020 at 9:49 PM Rajat Jain <rajatja@google.com> wrote: > > Add a new (optional) field to denote the physical location of a device > in the system, and expose it in sysfs. This was discussed here: > https://lore.kernel.org/linux-acpi/20200618184621.GA446639@kroah.com/ > > (The primary choice for attribute name i.e. "location" is already > exposed as an ABI elsewhere, so settled for "site"). Individual buses > that want to support this new attribute can opt-in by setting a flag in > bus_type, and then populating the location of device while enumerating > it. > > Signed-off-by: Rajat Jain <rajatja@google.com> > --- > v2: (Initial version) > > drivers/base/core.c | 35 +++++++++++++++++++++++++++++++ > include/linux/device.h | 42 ++++++++++++++++++++++++++++++++++++++ > include/linux/device/bus.h | 8 ++++++++ > 3 files changed, 85 insertions(+) > <snip> I'm not CC'ed in 4/7, so just replying > diff --git a/include/linux/device.h b/include/linux/device.h > index 15460a5ac024a..a4143735ae712 100644 > --- a/include/linux/device.h > +++ b/include/linux/device.h > @@ -428,6 +428,31 @@ enum dl_dev_state { > DL_DEV_UNBINDING, > }; > > +/** > + * enum device_site - Physical location of the device in the system. > + * The semantics of values depend on subsystem / bus: > + * > + * @SITE_UNKNOWN: Location is Unknown (default) > + * > + * @SITE_INTERNAL: Device is internal to the system, and cannot be (easily) > + * removed. E.g. SoC internal devices, onboard soldered > + * devices, internal M.2 cards (that cannot be removed > + * without opening the chassis). > + * @SITE_EXTENDED: Device sits an extension of the system. E.g. devices > + * on external PCIe trays, docking stations etc. These > + * devices may be removable, but are generally housed > + * internally on an extension board, so they are removed > + * only when that whole extension board is removed. > + * @SITE_EXTERNAL: Devices truly external to the system (i.e. plugged on > + * an external port) that may be removed or added frequently. > + */ > +enum device_site { > + SITE_UNKNOWN = 0, > + SITE_INTERNAL, > + SITE_EXTENDED, > + SITE_EXTERNAL, > +}; > + > /** > * struct dev_links_info - Device data related to device links. > * @suppliers: List of links to supplier devices. > @@ -513,6 +538,7 @@ struct dev_links_info { > * device (i.e. the bus driver that discovered the device). > * @iommu_group: IOMMU group the device belongs to. > * @iommu: Per device generic IOMMU runtime data > + * @site: Physical location of the device w.r.t. the system > * > * @offline_disabled: If set, the device is permanently online. > * @offline: Set after successful invocation of bus type's .offline(). > @@ -613,6 +639,8 @@ struct device { > struct iommu_group *iommu_group; > struct dev_iommu *iommu; > > + enum device_site site; /* Device physical location */ > + > bool offline_disabled:1; > bool offline:1; > bool of_node_reused:1; > @@ -806,6 +834,20 @@ static inline bool dev_has_sync_state(struct device *dev) > return false; > } > > +static inline int dev_set_site(struct device *dev, enum device_site site) > +{ > + if (site < SITE_UNKNOWN || site > SITE_EXTERNAL) > + return -EINVAL; > + > + dev->site = site; > + return 0; > +} > + > +static inline bool dev_is_external(struct device *dev) > +{ > + return dev->site == SITE_EXTERNAL; > +} I'm not CC'ed in the rest of the patches in this series, so just responding here. I see you use this function in patch 6/7 to decide if the PCI device is trusted. Anything other than EXTERNAL is being treated as trusted. I'd argue that anything that's not internal should be distrusted. For example, I can have a hacked up laptop dock that I can share with you when you visit my home/office and now you are trusting it when you shouldn't be. Also, "UNKNOWN" is treated as trusted in patch 6/7. I'm guessing this is because some of the devices might not have the info in their firmware? At which point, this feature isn't even protecting all the PCI ports properly? This adds to Greg point that this should be a userspace policy so that it can override whatever is wrong/missing in the firmware. -Saravana _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
next prev parent reply other threads:[~2020-06-30 17:44 UTC|newest] Thread overview: 86+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-06-30 4:49 [PATCH v2 0/7] Tighten PCI security, expose dev location in sysfs Rajat Jain 2020-06-30 4:49 ` Rajat Jain via iommu 2020-06-30 4:49 ` [PATCH v2 1/7] PCI: Keep the ACS capability offset in device Rajat Jain 2020-06-30 4:49 ` Rajat Jain via iommu 2020-07-06 15:58 ` Bjorn Helgaas 2020-07-06 15:58 ` Bjorn Helgaas 2020-07-06 22:16 ` Rajat Jain 2020-07-06 22:16 ` Rajat Jain via iommu 2020-07-06 23:18 ` Bjorn Helgaas 2020-07-06 23:18 ` Bjorn Helgaas 2020-06-30 4:49 ` [PATCH v2 2/7] PCI: Set "untrusted" flag for truly external devices only Rajat Jain 2020-06-30 4:49 ` Rajat Jain via iommu 2020-06-30 7:38 ` Lu Baolu 2020-06-30 7:38 ` Lu Baolu 2020-06-30 7:55 ` Greg Kroah-Hartman 2020-06-30 7:55 ` Greg Kroah-Hartman 2020-07-06 16:41 ` Bjorn Helgaas 2020-07-06 16:41 ` Bjorn Helgaas 2020-07-06 18:48 ` Greg Kroah-Hartman 2020-07-06 18:48 ` Greg Kroah-Hartman 2020-07-06 16:38 ` Bjorn Helgaas 2020-07-06 16:38 ` Bjorn Helgaas 2020-07-06 22:31 ` Rajat Jain 2020-07-06 22:31 ` Rajat Jain via iommu 2020-07-06 23:30 ` Bjorn Helgaas 2020-07-06 23:30 ` Bjorn Helgaas 2020-07-06 23:40 ` Rajat Jain 2020-07-06 23:40 ` Rajat Jain via iommu 2020-06-30 4:49 ` [PATCH v2 3/7] PCI/ACS: Enable PCI_ACS_TB for untrusted/external-facing devices Rajat Jain 2020-06-30 4:49 ` Rajat Jain via iommu 2020-07-06 16:45 ` Bjorn Helgaas 2020-07-06 16:45 ` Bjorn Helgaas 2020-07-06 23:12 ` Rajat Jain 2020-07-06 23:12 ` Rajat Jain via iommu 2020-07-06 17:07 ` Bjorn Helgaas 2020-07-06 17:07 ` Bjorn Helgaas 2020-07-06 23:19 ` Rajat Jain 2020-07-06 23:19 ` Rajat Jain via iommu 2020-06-30 4:49 ` [PATCH v2 4/7] PCI: Add device even if driver attach failed Rajat Jain 2020-06-30 4:49 ` Rajat Jain via iommu 2020-06-30 8:02 ` Greg Kroah-Hartman 2020-06-30 8:02 ` Greg Kroah-Hartman 2020-07-06 23:35 ` Rajat Jain 2020-07-06 23:35 ` Rajat Jain via iommu 2020-06-30 4:49 ` [PATCH v2 5/7] driver core: Add device location to "struct device" and expose it in sysfs Rajat Jain 2020-06-30 4:49 ` Rajat Jain via iommu 2020-06-30 8:01 ` Greg Kroah-Hartman 2020-06-30 8:01 ` Greg Kroah-Hartman 2020-06-30 10:49 ` Heikki Krogerus 2020-06-30 10:49 ` Heikki Krogerus 2020-06-30 12:52 ` Greg Kroah-Hartman 2020-06-30 12:52 ` Greg Kroah-Hartman 2020-06-30 13:00 ` Rafael J. Wysocki 2020-06-30 13:00 ` Rafael J. Wysocki 2020-06-30 15:38 ` Greg Kroah-Hartman 2020-06-30 15:38 ` Greg Kroah-Hartman 2020-06-30 16:08 ` Rafael J. Wysocki 2020-06-30 16:08 ` Rafael J. Wysocki 2020-06-30 17:00 ` Greg Kroah-Hartman 2020-06-30 17:00 ` Greg Kroah-Hartman 2020-07-01 18:06 ` Rajat Jain 2020-07-01 18:06 ` Rajat Jain via iommu 2020-07-02 5:23 ` Oliver O'Halloran 2020-07-02 5:23 ` Oliver O'Halloran 2020-07-02 7:32 ` Greg Kroah-Hartman 2020-07-02 7:32 ` Greg Kroah-Hartman 2020-07-02 8:40 ` Oliver O'Halloran 2020-07-02 8:40 ` Oliver O'Halloran 2020-07-02 8:52 ` Greg Kroah-Hartman 2020-07-02 8:52 ` Greg Kroah-Hartman 2020-07-02 8:53 ` Greg Kroah-Hartman 2020-07-02 8:53 ` Greg Kroah-Hartman 2020-07-07 6:03 ` Rajat Jain 2020-07-07 6:03 ` Rajat Jain via iommu 2020-06-30 17:43 ` Saravana Kannan [this message] 2020-06-30 17:43 ` Saravana Kannan via iommu 2020-06-30 4:49 ` [PATCH v2 6/7] PCI: Move pci_dev->untrusted logic to use device location instead Rajat Jain 2020-06-30 4:49 ` Rajat Jain via iommu 2020-06-30 7:39 ` Lu Baolu 2020-06-30 7:39 ` Lu Baolu 2020-06-30 4:49 ` [PATCH v2 7/7] PCI: Add parameter to disable attaching external devices Rajat Jain 2020-06-30 4:49 ` Rajat Jain via iommu 2020-07-04 11:44 ` [PATCH v2 0/7] Tighten PCI security, expose dev location in sysfs Pavel Machek 2020-07-04 11:44 ` Pavel Machek 2020-07-06 22:18 ` Rajat Jain 2020-07-06 22:18 ` Rajat Jain via iommu
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAGETcx9hgV70DVdbOvCF+tO4b-6+1JzN1_OmPmnWjj9qJhB_dw@mail.gmail.com \ --to=saravanak@google.com \ --cc=adurbin@google.com \ --cc=alex.williamson@redhat.com \ --cc=arnd@arndb.de \ --cc=ashok.raj@intel.com \ --cc=baolu.lu@linux.intel.com \ --cc=bernie.keany@intel.com \ --cc=bhelgaas@google.com \ --cc=bleung@google.com \ --cc=christian@kellner.me \ --cc=diegorivas@google.com \ --cc=dlaurie@google.com \ --cc=dwmw2@infradead.org \ --cc=furquan@google.com \ --cc=gregkh@linuxfoundation.org \ --cc=heikki.krogerus@linux.intel.com \ --cc=iommu@lists.linux-foundation.org \ --cc=jean-philippe@linaro.org \ --cc=joro@8bytes.org \ --cc=jsbarnes@google.com \ --cc=lalithambika.krishnakumar@intel.com \ --cc=lenb@kernel.org \ --cc=levinale@google.com \ --cc=linux-acpi@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pci@vger.kernel.org \ --cc=mika.westerberg@linux.intel.com \ --cc=mnissler@google.com \ --cc=oohall@gmail.com \ --cc=pmalani@google.com \ --cc=rajatja@google.com \ --cc=rajatxjain@gmail.com \ --cc=rjw@rjwysocki.net \ --cc=suzuki.poulose@arm.com \ --cc=tbroch@google.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.