| Message ID | 20200630044943.3425049-4-rajatja@google.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Tighten PCI security, expose dev location in sysfs | expand |
On Mon, Jun 29, 2020 at 09:49:39PM -0700, Rajat Jain wrote: > When enabling ACS, enable translation blocking for external facing ports > and untrusted devices. > > Signed-off-by: Rajat Jain <rajatja@google.com> > --- > v2: Commit log change > > drivers/pci/pci.c | 4 ++++ > drivers/pci/quirks.c | 11 +++++++++++ > 2 files changed, 15 insertions(+) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index d2ff987585855..79853b52658a2 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -3330,6 +3330,10 @@ static void pci_std_enable_acs(struct pci_dev *dev) > /* Upstream Forwarding */ > ctrl |= (cap & PCI_ACS_UF); > > + if (dev->external_facing || dev->untrusted) > + /* Translation Blocking */ > + ctrl |= (cap & PCI_ACS_TB); > + > pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl); > } > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index b341628e47527..6294adeac4049 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -4934,6 +4934,13 @@ static void pci_quirk_enable_intel_rp_mpc_acs(struct pci_dev *dev) > } > } > > +/* > + * Currently this quirk does the equivalent of > + * PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_SV > + * > + * Currently missing, it also needs to do equivalent of PCI_ACS_TB, > + * if dev->external_facing || dev->untrusted I don't understand this comment. Is this a "TODO"? Is there something more that needs to be done here? After a patch is applied, a comment should describe the code as it is. > + */ > static int pci_quirk_enable_intel_pch_acs(struct pci_dev *dev) > { > if (!pci_quirk_intel_pch_acs_match(dev)) > @@ -4973,6 +4980,10 @@ static int pci_quirk_enable_intel_spt_pch_acs(struct pci_dev *dev) > ctrl |= (cap & PCI_ACS_CR); > ctrl |= (cap & PCI_ACS_UF); > > + if (dev->external_facing || dev->untrusted) > + /* Translation Blocking */ > + ctrl |= (cap & PCI_ACS_TB); > + > pci_write_config_dword(dev, pos + INTEL_SPT_ACS_CTRL, ctrl); > > pci_info(dev, "Intel SPT PCH root port ACS workaround enabled\n"); > -- > 2.27.0.212.ge8ba1cc988-goog >
On Mon, Jun 29, 2020 at 09:49:39PM -0700, Rajat Jain wrote: > When enabling ACS, enable translation blocking for external facing ports > and untrusted devices. > > Signed-off-by: Rajat Jain <rajatja@google.com> > --- > v2: Commit log change > > drivers/pci/pci.c | 4 ++++ > drivers/pci/quirks.c | 11 +++++++++++ > 2 files changed, 15 insertions(+) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index d2ff987585855..79853b52658a2 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -3330,6 +3330,10 @@ static void pci_std_enable_acs(struct pci_dev *dev) > /* Upstream Forwarding */ > ctrl |= (cap & PCI_ACS_UF); > > + if (dev->external_facing || dev->untrusted) > + /* Translation Blocking */ > + ctrl |= (cap & PCI_ACS_TB); > + > pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl); > } > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index b341628e47527..6294adeac4049 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -4934,6 +4934,13 @@ static void pci_quirk_enable_intel_rp_mpc_acs(struct pci_dev *dev) > } > } > > +/* > + * Currently this quirk does the equivalent of > + * PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_SV Nit: Reorder these as in c8de8ed2dcaa ("PCI: Make ACS quirk implementations more uniform") so they match other similar lists in the code. But more to the point: we have a bunch of other quirks for devices that do not have an ACS capability but *do* provide some ACS-like features. Most of them support PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF because that's what we usually want. But I bet some of them also actually provide the equivalent of PCI_ACS_TB. REQ_ACS_FLAGS doesn't include PCI_ACS_TB. Is there anything we need to do on the pci_acs_enabled() side to check for PCI_ACS_TB, and consequently, to update any of the quirks for devices that provide it? > + * > + * Currently missing, it also needs to do equivalent of PCI_ACS_TB, > + * if dev->external_facing || dev->untrusted > + */ > static int pci_quirk_enable_intel_pch_acs(struct pci_dev *dev) > { > if (!pci_quirk_intel_pch_acs_match(dev)) > @@ -4973,6 +4980,10 @@ static int pci_quirk_enable_intel_spt_pch_acs(struct pci_dev *dev) > ctrl |= (cap & PCI_ACS_CR); > ctrl |= (cap & PCI_ACS_UF); > > + if (dev->external_facing || dev->untrusted) > + /* Translation Blocking */ > + ctrl |= (cap & PCI_ACS_TB); > + > pci_write_config_dword(dev, pos + INTEL_SPT_ACS_CTRL, ctrl); > > pci_info(dev, "Intel SPT PCH root port ACS workaround enabled\n"); > -- > 2.27.0.212.ge8ba1cc988-goog >
On Mon, Jul 6, 2020 at 9:45 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Mon, Jun 29, 2020 at 09:49:39PM -0700, Rajat Jain wrote: > > When enabling ACS, enable translation blocking for external facing ports > > and untrusted devices. > > > > Signed-off-by: Rajat Jain <rajatja@google.com> > > --- > > v2: Commit log change > > > > drivers/pci/pci.c | 4 ++++ > > drivers/pci/quirks.c | 11 +++++++++++ > > 2 files changed, 15 insertions(+) > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > index d2ff987585855..79853b52658a2 100644 > > --- a/drivers/pci/pci.c > > +++ b/drivers/pci/pci.c > > @@ -3330,6 +3330,10 @@ static void pci_std_enable_acs(struct pci_dev *dev) > > /* Upstream Forwarding */ > > ctrl |= (cap & PCI_ACS_UF); > > > > + if (dev->external_facing || dev->untrusted) > > + /* Translation Blocking */ > > + ctrl |= (cap & PCI_ACS_TB); > > + > > pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl); > > } > > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > > index b341628e47527..6294adeac4049 100644 > > --- a/drivers/pci/quirks.c > > +++ b/drivers/pci/quirks.c > > @@ -4934,6 +4934,13 @@ static void pci_quirk_enable_intel_rp_mpc_acs(struct pci_dev *dev) > > } > > } > > > > +/* > > + * Currently this quirk does the equivalent of > > + * PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_SV > > + * > > + * Currently missing, it also needs to do equivalent of PCI_ACS_TB, > > + * if dev->external_facing || dev->untrusted > > I don't understand this comment. Is this a "TODO"? Is there > something more that needs to be done here? Yes. I'll mark it as a TODO to make it more clear. > > After a patch is applied, a comment should describe the code as it is. > > > + */ > > static int pci_quirk_enable_intel_pch_acs(struct pci_dev *dev) > > { > > if (!pci_quirk_intel_pch_acs_match(dev)) > > @@ -4973,6 +4980,10 @@ static int pci_quirk_enable_intel_spt_pch_acs(struct pci_dev *dev) > > ctrl |= (cap & PCI_ACS_CR); > > ctrl |= (cap & PCI_ACS_UF); > > > > + if (dev->external_facing || dev->untrusted) > > + /* Translation Blocking */ > > + ctrl |= (cap & PCI_ACS_TB); > > + > > pci_write_config_dword(dev, pos + INTEL_SPT_ACS_CTRL, ctrl); > > > > pci_info(dev, "Intel SPT PCH root port ACS workaround enabled\n"); > > -- > > 2.27.0.212.ge8ba1cc988-goog > >
On Mon, Jul 6, 2020 at 10:07 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Mon, Jun 29, 2020 at 09:49:39PM -0700, Rajat Jain wrote: > > When enabling ACS, enable translation blocking for external facing ports > > and untrusted devices. > > > > Signed-off-by: Rajat Jain <rajatja@google.com> > > --- > > v2: Commit log change > > > > drivers/pci/pci.c | 4 ++++ > > drivers/pci/quirks.c | 11 +++++++++++ > > 2 files changed, 15 insertions(+) > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > index d2ff987585855..79853b52658a2 100644 > > --- a/drivers/pci/pci.c > > +++ b/drivers/pci/pci.c > > @@ -3330,6 +3330,10 @@ static void pci_std_enable_acs(struct pci_dev *dev) > > /* Upstream Forwarding */ > > ctrl |= (cap & PCI_ACS_UF); > > > > + if (dev->external_facing || dev->untrusted) > > + /* Translation Blocking */ > > + ctrl |= (cap & PCI_ACS_TB); > > + > > pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl); > > } > > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > > index b341628e47527..6294adeac4049 100644 > > --- a/drivers/pci/quirks.c > > +++ b/drivers/pci/quirks.c > > @@ -4934,6 +4934,13 @@ static void pci_quirk_enable_intel_rp_mpc_acs(struct pci_dev *dev) > > } > > } > > > > +/* > > + * Currently this quirk does the equivalent of > > + * PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_SV > > Nit: Reorder these as in c8de8ed2dcaa ("PCI: Make ACS quirk > implementations more uniform") so they match other similar lists in > the code. Will do. > > But more to the point: we have a bunch of other quirks for devices > that do not have an ACS capability but *do* provide some ACS-like > features. Most of them support > > PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF > > because that's what we usually want. But I bet some of them also > actually provide the equivalent of PCI_ACS_TB. > > REQ_ACS_FLAGS doesn't include PCI_ACS_TB. Is there anything we need > to do on the pci_acs_enabled() side to check for PCI_ACS_TB, and > consequently, to update any of the quirks for devices that provide it? I'm actually not sure. +Alex Williamson , do you have any comments here? Thanks, Rajat > > > + * > > + * Currently missing, it also needs to do equivalent of PCI_ACS_TB, > > + * if dev->external_facing || dev->untrusted > > + */ > > static int pci_quirk_enable_intel_pch_acs(struct pci_dev *dev) > > { > > if (!pci_quirk_intel_pch_acs_match(dev)) > > @@ -4973,6 +4980,10 @@ static int pci_quirk_enable_intel_spt_pch_acs(struct pci_dev *dev) > > ctrl |= (cap & PCI_ACS_CR); > > ctrl |= (cap & PCI_ACS_UF); > > > > + if (dev->external_facing || dev->untrusted) > > + /* Translation Blocking */ > > + ctrl |= (cap & PCI_ACS_TB); > > + > > pci_write_config_dword(dev, pos + INTEL_SPT_ACS_CTRL, ctrl); > > > > pci_info(dev, "Intel SPT PCH root port ACS workaround enabled\n"); > > -- > > 2.27.0.212.ge8ba1cc988-goog > >
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index d2ff987585855..79853b52658a2 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -3330,6 +3330,10 @@ static void pci_std_enable_acs(struct pci_dev *dev) /* Upstream Forwarding */ ctrl |= (cap & PCI_ACS_UF); + if (dev->external_facing || dev->untrusted) + /* Translation Blocking */ + ctrl |= (cap & PCI_ACS_TB); + pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl); } diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index b341628e47527..6294adeac4049 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -4934,6 +4934,13 @@ static void pci_quirk_enable_intel_rp_mpc_acs(struct pci_dev *dev) } } +/* + * Currently this quirk does the equivalent of + * PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_SV + * + * Currently missing, it also needs to do equivalent of PCI_ACS_TB, + * if dev->external_facing || dev->untrusted + */ static int pci_quirk_enable_intel_pch_acs(struct pci_dev *dev) { if (!pci_quirk_intel_pch_acs_match(dev)) @@ -4973,6 +4980,10 @@ static int pci_quirk_enable_intel_spt_pch_acs(struct pci_dev *dev) ctrl |= (cap & PCI_ACS_CR); ctrl |= (cap & PCI_ACS_UF); + if (dev->external_facing || dev->untrusted) + /* Translation Blocking */ + ctrl |= (cap & PCI_ACS_TB); + pci_write_config_dword(dev, pos + INTEL_SPT_ACS_CTRL, ctrl); pci_info(dev, "Intel SPT PCH root port ACS workaround enabled\n");
When enabling ACS, enable translation blocking for external facing ports and untrusted devices. Signed-off-by: Rajat Jain <rajatja@google.com> --- v2: Commit log change drivers/pci/pci.c | 4 ++++ drivers/pci/quirks.c | 11 +++++++++++ 2 files changed, 15 insertions(+)