| Message ID | 20200616011742.138975-4-rajatja@google.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [1/4] pci: Keep the ACS capability offset in device | expand |
On Mon, Jun 15, 2020 at 06:17:42PM -0700, Rajat Jain wrote: > This is needed to allow the userspace to determine when an untrusted > device has been added, and thus allowing it to bind the driver manually > to it, if it so wishes. This is being done as part of the approach > discussed at https://lkml.org/lkml/2020/6/9/1331 > > Signed-off-by: Rajat Jain <rajatja@google.com> > --- > drivers/pci/pci-sysfs.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > index 6d78df981d41a..574e9c613ba26 100644 > --- a/drivers/pci/pci-sysfs.c > +++ b/drivers/pci/pci-sysfs.c > @@ -50,6 +50,7 @@ pci_config_attr(subsystem_device, "0x%04x\n"); > pci_config_attr(revision, "0x%02x\n"); > pci_config_attr(class, "0x%06x\n"); > pci_config_attr(irq, "%u\n"); > +pci_config_attr(untrusted, "%u\n"); > > static ssize_t broken_parity_status_show(struct device *dev, > struct device_attribute *attr, > @@ -608,6 +609,7 @@ static struct attribute *pci_dev_attrs[] = { > #endif > &dev_attr_driver_override.attr, > &dev_attr_ari_enabled.attr, > + &dev_attr_untrusted.attr, > NULL, > }; You also need a Documentation/ABI/ update for this new file. thanks, greg k-h
On Mon, Jun 15, 2020 at 06:17:42PM -0700, Rajat Jain via iommu wrote: > This is needed to allow the userspace to determine when an untrusted > device has been added, and thus allowing it to bind the driver manually > to it, if it so wishes. This is being done as part of the approach > discussed at https://lkml.org/lkml/2020/6/9/1331 Please move the attribute to struct device instead of further entrenching it in PCIe. I'm starting to grow tired of saying this every other week while you guys are all moving into the entirely wrong direction.
On Tue, Jun 16, 2020 at 12:32 AM Christoph Hellwig <hch@infradead.org> wrote: > > On Mon, Jun 15, 2020 at 06:17:42PM -0700, Rajat Jain via iommu wrote: > > This is needed to allow the userspace to determine when an untrusted > > device has been added, and thus allowing it to bind the driver manually > > to it, if it so wishes. This is being done as part of the approach > > discussed at https://lkml.org/lkml/2020/6/9/1331 > > Please move the attribute to struct device instead of further > entrenching it in PCIe. Need clarification. The flag "untrusted" is currently a part of pci_dev struct, and is populated within the PCI subsystem. 1) Is your suggestion to move this flag as well as the attribute to device core (in "struct device")? This would allow other buses to populate/use this flag if they want. By default it'll be set to 0 for all devices (PCI subsystem will populate it based on platform info, like it does today). OR 2) Are you suggesting to keep the "untrusted" flag within PCI, but attach the sysfs attribute to the base device? (&pci_dev->dev)? Thanks, Rajat > I'm starting to grow tired of saying this > every other week while you guys are all moving into the entirely > wrong direction.
On Tue, Jun 16, 2020 at 12:27:35PM -0700, Rajat Jain wrote: > Need clarification. The flag "untrusted" is currently a part of > pci_dev struct, and is populated within the PCI subsystem. Yes, and that is the problem. > > 1) Is your suggestion to move this flag as well as the attribute to > device core (in "struct device")? This would allow other buses to > populate/use this flag if they want. By default it'll be set to 0 for > all devices (PCI subsystem will populate it based on platform info, > like it does today). > > OR > > 2) Are you suggesting to keep the "untrusted" flag within PCI, but > attach the sysfs attribute to the base device? (&pci_dev->dev)? (1). As for IOMMUs and userspace policy it really should not matter what bus a device is on if it is external and not trustworthy.
Hi Greg, Christoph, On Wed, Jun 17, 2020 at 12:31 AM Christoph Hellwig <hch@infradead.org> wrote: > > On Tue, Jun 16, 2020 at 12:27:35PM -0700, Rajat Jain wrote: > > Need clarification. The flag "untrusted" is currently a part of > > pci_dev struct, and is populated within the PCI subsystem. > > Yes, and that is the problem. > > > > > 1) Is your suggestion to move this flag as well as the attribute to > > device core (in "struct device")? This would allow other buses to > > populate/use this flag if they want. By default it'll be set to 0 for > > all devices (PCI subsystem will populate it based on platform info, > > like it does today). > > > > OR > > > > 2) Are you suggesting to keep the "untrusted" flag within PCI, but > > attach the sysfs attribute to the base device? (&pci_dev->dev)? > > (1). As for IOMMUs and userspace policy it really should not matter > what bus a device is on if it is external and not trustworthy. Sure. I can move the flag to the "struct device" (and likely call it "external" instead of "untrusted" so as to make it suitable for more use cases later). The buses can fill this up if they know which devices are external and which ones are not (otherwise it will be 0 by default). The PCI can fill this up like it does today, from platform info (ACPI / Device tree). Greg, how does this sound? Thanks, Rajat
On Wed, Jun 17, 2020 at 12:53:03PM -0700, Rajat Jain wrote: > Hi Greg, Christoph, > > On Wed, Jun 17, 2020 at 12:31 AM Christoph Hellwig <hch@infradead.org> wrote: > > > > On Tue, Jun 16, 2020 at 12:27:35PM -0700, Rajat Jain wrote: > > > Need clarification. The flag "untrusted" is currently a part of > > > pci_dev struct, and is populated within the PCI subsystem. > > > > Yes, and that is the problem. > > > > > > > > 1) Is your suggestion to move this flag as well as the attribute to > > > device core (in "struct device")? This would allow other buses to > > > populate/use this flag if they want. By default it'll be set to 0 for > > > all devices (PCI subsystem will populate it based on platform info, > > > like it does today). > > > > > > OR > > > > > > 2) Are you suggesting to keep the "untrusted" flag within PCI, but > > > attach the sysfs attribute to the base device? (&pci_dev->dev)? > > > > (1). As for IOMMUs and userspace policy it really should not matter > > what bus a device is on if it is external and not trustworthy. > > Sure. I can move the flag to the "struct device" (and likely call > it "external" instead of "untrusted" so as to make it suitable for > more use cases later). The buses can fill this up if they know which > devices are external and which ones are not (otherwise it will be 0 by > default). The PCI can fill this up like it does today, from platform > info (ACPI / Device tree). Greg, how does this sound? That's fine, convert USB over to use it at the same time if you get the chance :) thanks, greg k-h
On Wed, Jun 17, 2020 at 10:56 PM Rajat Jain <rajatja@google.com> wrote: > On Wed, Jun 17, 2020 at 12:31 AM Christoph Hellwig <hch@infradead.org> wrote: ... > (and likely call it "external" instead of "untrusted". Which is not okay. 'External' to what? 'untrusted' has been carefully chosen by the meaning of it. What external does mean for M.2. WWAN card in my laptop? It's in ACPI tables, but I can replace it. This is only one example. Or if firmware of some device is altered, and it's internal (whatever it means) is it trusted or not? So, please leave it as is (I mean name).
On Thu, Jun 18, 2020 at 11:12:56AM +0300, Andy Shevchenko wrote: > On Wed, Jun 17, 2020 at 10:56 PM Rajat Jain <rajatja@google.com> wrote: > > On Wed, Jun 17, 2020 at 12:31 AM Christoph Hellwig <hch@infradead.org> wrote: > > ... > > > (and likely call it "external" instead of "untrusted". > > Which is not okay. 'External' to what? 'untrusted' has been carefully > chosen by the meaning of it. > What external does mean for M.2. WWAN card in my laptop? It's in ACPI > tables, but I can replace it. Then your ACPI tables should show this, there is an attribute for it, right? > This is only one example. Or if firmware of some device is altered, > and it's internal (whatever it means) is it trusted or not? That is what people are using policy for today, if you object to this, please bring it up to those developers :) > So, please leave it as is (I mean name). firmware today exports this attribute, why do you not want userspace to also know it? Trust is different, yes, don't get the two mixed up please. That should be a different sysfs attribute for obvious reasons. thanks, greg k-h
On Thu, Jun 18, 2020 at 11:36 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Thu, Jun 18, 2020 at 11:12:56AM +0300, Andy Shevchenko wrote: > > On Wed, Jun 17, 2020 at 10:56 PM Rajat Jain <rajatja@google.com> wrote: > > > On Wed, Jun 17, 2020 at 12:31 AM Christoph Hellwig <hch@infradead.org> wrote: > > > > ... > > > > > (and likely call it "external" instead of "untrusted". > > > > Which is not okay. 'External' to what? 'untrusted' has been carefully > > chosen by the meaning of it. > > What external does mean for M.2. WWAN card in my laptop? It's in ACPI > > tables, but I can replace it. > > Then your ACPI tables should show this, there is an attribute for it, > right? There is a _PLD() method, but it's for the USB devices (or optional for others, I don't remember by heart). So, most of the ACPI tables, alas, don't show this. > > This is only one example. Or if firmware of some device is altered, > > and it's internal (whatever it means) is it trusted or not? > > That is what people are using policy for today, if you object to this, > please bring it up to those developers :) > > So, please leave it as is (I mean name). > > firmware today exports this attribute, why do you not want userspace to > also know it? > > Trust is different, yes, don't get the two mixed up please. That should > be a different sysfs attribute for obvious reasons. Yes, as a bottom line that's what I meant as well.
On Thu, Jun 18, 2020 at 12:14:41PM +0300, Andy Shevchenko wrote: > On Thu, Jun 18, 2020 at 11:36 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Thu, Jun 18, 2020 at 11:12:56AM +0300, Andy Shevchenko wrote: > > > On Wed, Jun 17, 2020 at 10:56 PM Rajat Jain <rajatja@google.com> wrote: > > > > On Wed, Jun 17, 2020 at 12:31 AM Christoph Hellwig <hch@infradead.org> wrote: > > > > > > ... > > > > > > > (and likely call it "external" instead of "untrusted". > > > > > > Which is not okay. 'External' to what? 'untrusted' has been carefully > > > chosen by the meaning of it. > > > What external does mean for M.2. WWAN card in my laptop? It's in ACPI > > > tables, but I can replace it. > > > > Then your ACPI tables should show this, there is an attribute for it, > > right? > > There is a _PLD() method, but it's for the USB devices (or optional > for others, I don't remember by heart). So, most of the ACPI tables, > alas, don't show this. There is something like this for PCI as well, otherwise they wouldn't be getting this info from "the ether" :) thanks, greg k-h
Hello, On Thu, Jun 18, 2020 at 2:14 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Thu, Jun 18, 2020 at 11:36 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Thu, Jun 18, 2020 at 11:12:56AM +0300, Andy Shevchenko wrote: > > > On Wed, Jun 17, 2020 at 10:56 PM Rajat Jain <rajatja@google.com> wrote: > > > > On Wed, Jun 17, 2020 at 12:31 AM Christoph Hellwig <hch@infradead.org> wrote: > > > > > > ... > > > > > > > (and likely call it "external" instead of "untrusted". > > > > > > Which is not okay. 'External' to what? 'untrusted' has been carefully > > > chosen by the meaning of it. > > > What external does mean for M.2. WWAN card in my laptop? It's in ACPI > > > tables, but I can replace it. > > > > Then your ACPI tables should show this, there is an attribute for it, > > right? > > There is a _PLD() method, but it's for the USB devices (or optional > for others, I don't remember by heart). So, most of the ACPI tables, > alas, don't show this. > > > > This is only one example. Or if firmware of some device is altered, > > > and it's internal (whatever it means) is it trusted or not? > > > > That is what people are using policy for today, if you object to this, > > please bring it up to those developers :) > > > > So, please leave it as is (I mean name). > > > > firmware today exports this attribute, why do you not want userspace to > > also know it? To clarify, the attribute exposed by the firmware today is "ExternalFacingPort" and "external-facing" respectively: 617654aae50e ("PCI / ACPI: Identify untrusted PCI devices") 9cb30a71ac45d("PCI: OF: Support "external-facing" property") The kernel flag was named "untrusted" though, hence the assumption that "external=untrusted" is currently baked into the kernel today. IMHO, using "external" would fix that (The assumption can thus be contained in the IOMMU drivers) and at the same time allow more use of this attribute. > > > > Trust is different, yes, don't get the two mixed up please. That should > > be a different sysfs attribute for obvious reasons. > > Yes, as a bottom line that's what I meant as well. So what is the consensus here? I don't have a strong opinion - but it seemed to me Greg is saying "external" and Andy is saying "untrusted"? Thanks, Rajat > > -- > With Best Regards, > Andy Shevchenko
On Thu, Jun 18, 2020 at 6:04 PM Rajat Jain <rajatja@google.com> wrote: > On Thu, Jun 18, 2020 at 2:14 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: ... > To clarify, the attribute exposed by the firmware today is > "ExternalFacingPort" and "external-facing" respectively: > > 617654aae50e ("PCI / ACPI: Identify untrusted PCI devices") > 9cb30a71ac45d("PCI: OF: Support "external-facing" property") > > The kernel flag was named "untrusted" though, hence the assumption > that "external=untrusted" is currently baked into the kernel today. > IMHO, using "external" would fix that (The assumption can thus be > contained in the IOMMU drivers) and at the same time allow more use of > this attribute. That discussion had been held, IIRC, during introduction of the untrusted member in struct pci_dev... > > > Trust is different, yes, don't get the two mixed up please. That should > > > be a different sysfs attribute for obvious reasons. > > > > Yes, as a bottom line that's what I meant as well. > > So what is the consensus here? I don't have a strong opinion - but it > seemed to me Greg is saying "external" and Andy is saying "untrusted"? ...and a conclusion has been made as you may see. So, I would highly recommend to speak to the author(s) of the patch that introduced / adopted 'untrusted' member.
On Thu, Jun 18, 2020 at 08:03:49AM -0700, Rajat Jain wrote: > Hello, > > On Thu, Jun 18, 2020 at 2:14 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > > > On Thu, Jun 18, 2020 at 11:36 AM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > On Thu, Jun 18, 2020 at 11:12:56AM +0300, Andy Shevchenko wrote: > > > > On Wed, Jun 17, 2020 at 10:56 PM Rajat Jain <rajatja@google.com> wrote: > > > > > On Wed, Jun 17, 2020 at 12:31 AM Christoph Hellwig <hch@infradead.org> wrote: > > > > > > > > ... > > > > > > > > > (and likely call it "external" instead of "untrusted". > > > > > > > > Which is not okay. 'External' to what? 'untrusted' has been carefully > > > > chosen by the meaning of it. > > > > What external does mean for M.2. WWAN card in my laptop? It's in ACPI > > > > tables, but I can replace it. > > > > > > Then your ACPI tables should show this, there is an attribute for it, > > > right? > > > > There is a _PLD() method, but it's for the USB devices (or optional > > for others, I don't remember by heart). So, most of the ACPI tables, > > alas, don't show this. > > > > > > This is only one example. Or if firmware of some device is altered, > > > > and it's internal (whatever it means) is it trusted or not? > > > > > > That is what people are using policy for today, if you object to this, > > > please bring it up to those developers :) > > > > > > So, please leave it as is (I mean name). > > > > > > firmware today exports this attribute, why do you not want userspace to > > > also know it? > > To clarify, the attribute exposed by the firmware today is > "ExternalFacingPort" and "external-facing" respectively: > > 617654aae50e ("PCI / ACPI: Identify untrusted PCI devices") > 9cb30a71ac45d("PCI: OF: Support "external-facing" property") > > The kernel flag was named "untrusted" though, hence the assumption > that "external=untrusted" is currently baked into the kernel today. > IMHO, using "external" would fix that (The assumption can thus be > contained in the IOMMU drivers) and at the same time allow more use of > this attribute. > > > > > > > Trust is different, yes, don't get the two mixed up please. That should > > > be a different sysfs attribute for obvious reasons. > > > > Yes, as a bottom line that's what I meant as well. > > So what is the consensus here? I don't have a strong opinion - but it > seemed to me Greg is saying "external" and Andy is saying "untrusted"? Those two things are totally separate things when it comes to a device. One (external) describes the location of the device in the system. The other (untrusted) describes what you want the kernel to do with this device (trust or not trust it). One you can change (from trust to untrusted or back), the other you can not, it is a fixed read-only property that describes the hardware device as defined by the firmware. Depending on the policy you wish to define, you can use the location of the device to determine if you want to trust the device or not. Again, this is what USB does, but I'm getting really tired of saying this, so I'm going to stop now... greg k-h
Hi Greg, On Thu, Jun 18, 2020 at 06:02:12PM +0200, Greg Kroah-Hartman wrote: > On Thu, Jun 18, 2020 at 08:03:49AM -0700, Rajat Jain wrote: > > Hello, > > > > On Thu, Jun 18, 2020 at 2:14 AM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > > > > On Thu, Jun 18, 2020 at 11:36 AM Greg Kroah-Hartman > > > <gregkh@linuxfoundation.org> wrote: > > > > > > > > On Thu, Jun 18, 2020 at 11:12:56AM +0300, Andy Shevchenko wrote: > > > > > On Wed, Jun 17, 2020 at 10:56 PM Rajat Jain <rajatja@google.com> wrote: > > > > > > On Wed, Jun 17, 2020 at 12:31 AM Christoph Hellwig <hch@infradead.org> wrote: > > > > > > > > > > ... > > > > > > > > > > > (and likely call it "external" instead of "untrusted". > > > > > > > > > > Which is not okay. 'External' to what? 'untrusted' has been carefully > > > > > chosen by the meaning of it. > > > > > What external does mean for M.2. WWAN card in my laptop? It's in ACPI > > > > > tables, but I can replace it. > > > > > > > > Then your ACPI tables should show this, there is an attribute for it, > > > > right? > > > > > > There is a _PLD() method, but it's for the USB devices (or optional > > > for others, I don't remember by heart). So, most of the ACPI tables, > > > alas, don't show this. > > > > > > > > This is only one example. Or if firmware of some device is altered, > > > > > and it's internal (whatever it means) is it trusted or not? > > > > > > > > That is what people are using policy for today, if you object to this, > > > > please bring it up to those developers :) > > > > > > > > So, please leave it as is (I mean name). > > > > > > > > firmware today exports this attribute, why do you not want userspace to > > > > also know it? > > > > To clarify, the attribute exposed by the firmware today is > > "ExternalFacingPort" and "external-facing" respectively: > > > > 617654aae50e ("PCI / ACPI: Identify untrusted PCI devices") > > 9cb30a71ac45d("PCI: OF: Support "external-facing" property") > > > > The kernel flag was named "untrusted" though, hence the assumption > > that "external=untrusted" is currently baked into the kernel today. > > IMHO, using "external" would fix that (The assumption can thus be > > contained in the IOMMU drivers) and at the same time allow more use of > > this attribute. > > > > > > > > > > Trust is different, yes, don't get the two mixed up please. That should > > > > be a different sysfs attribute for obvious reasons. > > > > > > Yes, as a bottom line that's what I meant as well. > > > > So what is the consensus here? I don't have a strong opinion - but it > > seemed to me Greg is saying "external" and Andy is saying "untrusted"? > > Those two things are totally separate things when it comes to a device. Agree that these are two separate attributes, and better not mixed. > > One (external) describes the location of the device in the system. > > The other (untrusted) describes what you want the kernel to do with this > device (trust or not trust it). > > One you can change (from trust to untrusted or back), the other you can > not, it is a fixed read-only property that describes the hardware device > as defined by the firmware. The genesis is due to lack of a mechanism to establish if the device is trusted or not was the due lack of some specs and implementation around Component Measurement And Authentication (CMA). Treating external as untrusted was the best first effort. i.e trust internal devices and don't trust external devices for enabling ATS. But that said external is just describing topology, and if Linux wants to use that in the policy that's different. Some day external device may also use CMA to estabilish trust. FWIW even internal devices aren't trust worthy, except maybe RCIEP's. > > Depending on the policy you wish to define, you can use the location of > the device to determine if you want to trust the device or not. > Cheers, Ashok
Thanks Greg and Andy for your continued inputs, and thanks Ashok for chiming in. On Thu, Jun 18, 2020 at 9:23 AM Raj, Ashok <ashok.raj@intel.com> wrote: > > Hi Greg, > > > On Thu, Jun 18, 2020 at 06:02:12PM +0200, Greg Kroah-Hartman wrote: > > On Thu, Jun 18, 2020 at 08:03:49AM -0700, Rajat Jain wrote: > > > Hello, > > > > > > On Thu, Jun 18, 2020 at 2:14 AM Andy Shevchenko > > > <andy.shevchenko@gmail.com> wrote: > > > > > > > > On Thu, Jun 18, 2020 at 11:36 AM Greg Kroah-Hartman > > > > <gregkh@linuxfoundation.org> wrote: > > > > > > > > > > On Thu, Jun 18, 2020 at 11:12:56AM +0300, Andy Shevchenko wrote: > > > > > > On Wed, Jun 17, 2020 at 10:56 PM Rajat Jain <rajatja@google.com> wrote: > > > > > > > On Wed, Jun 17, 2020 at 12:31 AM Christoph Hellwig <hch@infradead.org> wrote: > > > > > > > > > > > > ... > > > > > > > > > > > > > (and likely call it "external" instead of "untrusted". > > > > > > > > > > > > Which is not okay. 'External' to what? 'untrusted' has been carefully > > > > > > chosen by the meaning of it. > > > > > > What external does mean for M.2. WWAN card in my laptop? It's in ACPI > > > > > > tables, but I can replace it. > > > > > > > > > > Then your ACPI tables should show this, there is an attribute for it, > > > > > right? > > > > > > > > There is a _PLD() method, but it's for the USB devices (or optional > > > > for others, I don't remember by heart). So, most of the ACPI tables, > > > > alas, don't show this. > > > > > > > > > > This is only one example. Or if firmware of some device is altered, > > > > > > and it's internal (whatever it means) is it trusted or not? > > > > > > > > > > That is what people are using policy for today, if you object to this, > > > > > please bring it up to those developers :) > > > > > > > > > > So, please leave it as is (I mean name). > > > > > > > > > > firmware today exports this attribute, why do you not want userspace to > > > > > also know it? > > > > > > To clarify, the attribute exposed by the firmware today is > > > "ExternalFacingPort" and "external-facing" respectively: > > > > > > 617654aae50e ("PCI / ACPI: Identify untrusted PCI devices") > > > 9cb30a71ac45d("PCI: OF: Support "external-facing" property") > > > > > > The kernel flag was named "untrusted" though, hence the assumption > > > that "external=untrusted" is currently baked into the kernel today. > > > IMHO, using "external" would fix that (The assumption can thus be > > > contained in the IOMMU drivers) and at the same time allow more use of > > > this attribute. > > > > > > > > > > > > > Trust is different, yes, don't get the two mixed up please. That should > > > > > be a different sysfs attribute for obvious reasons. > > > > > > > > Yes, as a bottom line that's what I meant as well. > > > > > > So what is the consensus here? I don't have a strong opinion - but it > > > seemed to me Greg is saying "external" and Andy is saying "untrusted"? > > > > Those two things are totally separate things when it comes to a device. > > Agree that these are two separate attributes, and better not mixed. +1. > > > > > One (external) describes the location of the device in the system. > > > > The other (untrusted) describes what you want the kernel to do with this > > device (trust or not trust it). > > > > One you can change (from trust to untrusted or back), the other you can > > not, it is a fixed read-only property that describes the hardware device > > as defined by the firmware. Correct. I believe what is being described by the firmware is a fixed read-only property describing the location of the device ("external") - not what to do with it ("untrusted"). > > The genesis is due to lack of a mechanism to establish if the device > is trusted or not was the due lack of some specs and implementation around > Component Measurement And Authentication (CMA). Treating external as > untrusted was the best first effort. i.e trust internal > devices and don't trust external devices for enabling ATS. > > But that said external is just describing topology, and if Linux wants to > use that in the policy that's different. Some day external device may also > use CMA to estabilish trust. FWIW even internal devices aren't trust > worthy, except maybe RCIEP's. Correct. Since the firmware is actually describing the unchangeable topology (and not the policy), the takeaway I am taking from this discussion is that the flag should be called "external". Like I said, I don't have any hard opinions on this. So if you feel that my conclusion is wrong and consensus was the other way around ("untrusted"), let me know and I'll be happy to change this. Thanks, Rajat > > > > > Depending on the policy you wish to define, you can use the location of > > the device to determine if you want to trust the device or not. > > > > Cheers, > Ashok
On Thu, Jun 18, 2020 at 10:23:38AM -0700, Rajat Jain wrote: > Thanks Greg and Andy for your continued inputs, and thanks Ashok for chiming in. > > On Thu, Jun 18, 2020 at 9:23 AM Raj, Ashok <ashok.raj@intel.com> wrote: > > > > Hi Greg, > > > > > > On Thu, Jun 18, 2020 at 06:02:12PM +0200, Greg Kroah-Hartman wrote: > > > On Thu, Jun 18, 2020 at 08:03:49AM -0700, Rajat Jain wrote: > > > > Hello, > > > > > > > > On Thu, Jun 18, 2020 at 2:14 AM Andy Shevchenko > > > > <andy.shevchenko@gmail.com> wrote: > > > > > > > > > > On Thu, Jun 18, 2020 at 11:36 AM Greg Kroah-Hartman > > > > > <gregkh@linuxfoundation.org> wrote: > > > > > > > > > > > > On Thu, Jun 18, 2020 at 11:12:56AM +0300, Andy Shevchenko wrote: > > > > > > > On Wed, Jun 17, 2020 at 10:56 PM Rajat Jain <rajatja@google.com> wrote: > > > > > > > > On Wed, Jun 17, 2020 at 12:31 AM Christoph Hellwig <hch@infradead.org> wrote: > > > > > > > > > > > > > > ... > > > > > > > > > > > > > > > (and likely call it "external" instead of "untrusted". > > > > > > > > > > > > > > Which is not okay. 'External' to what? 'untrusted' has been carefully > > > > > > > chosen by the meaning of it. > > > > > > > What external does mean for M.2. WWAN card in my laptop? It's in ACPI > > > > > > > tables, but I can replace it. > > > > > > > > > > > > Then your ACPI tables should show this, there is an attribute for it, > > > > > > right? > > > > > > > > > > There is a _PLD() method, but it's for the USB devices (or optional > > > > > for others, I don't remember by heart). So, most of the ACPI tables, > > > > > alas, don't show this. > > > > > > > > > > > > This is only one example. Or if firmware of some device is altered, > > > > > > > and it's internal (whatever it means) is it trusted or not? > > > > > > > > > > > > That is what people are using policy for today, if you object to this, > > > > > > please bring it up to those developers :) > > > > > > > > > > > > So, please leave it as is (I mean name). > > > > > > > > > > > > firmware today exports this attribute, why do you not want userspace to > > > > > > also know it? > > > > > > > > To clarify, the attribute exposed by the firmware today is > > > > "ExternalFacingPort" and "external-facing" respectively: > > > > > > > > 617654aae50e ("PCI / ACPI: Identify untrusted PCI devices") > > > > 9cb30a71ac45d("PCI: OF: Support "external-facing" property") > > > > > > > > The kernel flag was named "untrusted" though, hence the assumption > > > > that "external=untrusted" is currently baked into the kernel today. > > > > IMHO, using "external" would fix that (The assumption can thus be > > > > contained in the IOMMU drivers) and at the same time allow more use of > > > > this attribute. > > > > > > > > > > > > > > > > Trust is different, yes, don't get the two mixed up please. That should > > > > > > be a different sysfs attribute for obvious reasons. > > > > > > > > > > Yes, as a bottom line that's what I meant as well. > > > > > > > > So what is the consensus here? I don't have a strong opinion - but it > > > > seemed to me Greg is saying "external" and Andy is saying "untrusted"? > > > > > > Those two things are totally separate things when it comes to a device. > > > > Agree that these are two separate attributes, and better not mixed. > > +1. > > > > > > > > > One (external) describes the location of the device in the system. > > > > > > The other (untrusted) describes what you want the kernel to do with this > > > device (trust or not trust it). > > > > > > One you can change (from trust to untrusted or back), the other you can > > > not, it is a fixed read-only property that describes the hardware device > > > as defined by the firmware. > > Correct. I believe what is being described by the firmware is a fixed > read-only property describing the location of the device ("external") > - not what to do with it ("untrusted"). > > > > > The genesis is due to lack of a mechanism to establish if the device > > is trusted or not was the due lack of some specs and implementation around > > Component Measurement And Authentication (CMA). Treating external as > > untrusted was the best first effort. i.e trust internal > > devices and don't trust external devices for enabling ATS. > > > > But that said external is just describing topology, and if Linux wants to > > use that in the policy that's different. Some day external device may also > > use CMA to estabilish trust. FWIW even internal devices aren't trust > > worthy, except maybe RCIEP's. > > Correct. Since the firmware is actually describing the unchangeable > topology (and not the policy), the takeaway I am taking from this > discussion is that the flag should be called "external". The attribute should be called something like "location" or something like that (naming is hard), as you don't always know if something is external or not (it could be internal, it could be unknown, it could be internal to an external device that you trust (think PCI drawers for "super" computers that are hot pluggable but yet really part of the internal bus). > Like I said, I don't have any hard opinions on this. So if you feel > that my conclusion is wrong and consensus was the other way around > ("untrusted"), let me know and I'll be happy to change this. "trust" has no direct relation to the location, except in a policy of what you wish to do with that device, so as long as you keep them separate that way, I am fine with it. thanks, greg k-h
Hi Bjorn, All, Thank you so much for your helpful review and inputs. On Mon, Jun 15, 2020 at 6:17 PM Rajat Jain <rajatja@google.com> wrote: > > This is needed to allow the userspace to determine when an untrusted > device has been added, and thus allowing it to bind the driver manually > to it, if it so wishes. This is being done as part of the approach > discussed at https://lkml.org/lkml/2020/6/9/1331 > > Signed-off-by: Rajat Jain <rajatja@google.com> Considering the suggestions obtained on this patch to move this attribute to device core, please consider this particular patch rescinded. I'll submit this as a separate patch since I think that will take more bake time and more discussion, than the other patches in this patchset. Please consider applying the other 3 patches in this patchset though, and I'd appreciate your review and comments. Thanks & Best Regards, Rajat > --- > drivers/pci/pci-sysfs.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > index 6d78df981d41a..574e9c613ba26 100644 > --- a/drivers/pci/pci-sysfs.c > +++ b/drivers/pci/pci-sysfs.c > @@ -50,6 +50,7 @@ pci_config_attr(subsystem_device, "0x%04x\n"); > pci_config_attr(revision, "0x%02x\n"); > pci_config_attr(class, "0x%06x\n"); > pci_config_attr(irq, "%u\n"); > +pci_config_attr(untrusted, "%u\n"); > > static ssize_t broken_parity_status_show(struct device *dev, > struct device_attribute *attr, > @@ -608,6 +609,7 @@ static struct attribute *pci_dev_attrs[] = { > #endif > &dev_attr_driver_override.attr, > &dev_attr_ari_enabled.attr, > + &dev_attr_untrusted.attr, > NULL, > }; > > -- > 2.27.0.290.gba653c62da-goog >
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c index 6d78df981d41a..574e9c613ba26 100644 --- a/drivers/pci/pci-sysfs.c +++ b/drivers/pci/pci-sysfs.c @@ -50,6 +50,7 @@ pci_config_attr(subsystem_device, "0x%04x\n"); pci_config_attr(revision, "0x%02x\n"); pci_config_attr(class, "0x%06x\n"); pci_config_attr(irq, "%u\n"); +pci_config_attr(untrusted, "%u\n"); static ssize_t broken_parity_status_show(struct device *dev, struct device_attribute *attr, @@ -608,6 +609,7 @@ static struct attribute *pci_dev_attrs[] = { #endif &dev_attr_driver_override.attr, &dev_attr_ari_enabled.attr, + &dev_attr_untrusted.attr, NULL, };
This is needed to allow the userspace to determine when an untrusted device has been added, and thus allowing it to bind the driver manually to it, if it so wishes. This is being done as part of the approach discussed at https://lkml.org/lkml/2020/6/9/1331 Signed-off-by: Rajat Jain <rajatja@google.com> --- drivers/pci/pci-sysfs.c | 2 ++ 1 file changed, 2 insertions(+)