| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <mjrcz7qleml7hmwfdwxt2bbbq5qg7ko36l6zwt2snppk7ajrbj@evmvu7tzr4w2>
Date: Tue, 21 Oct 2025 07:55:45 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
Cc: Bjorn Helgaas <helgaas@...nel.org>, Jingoo Han <jingoohan1@...il.com>,
Lorenzo Pieralisi <lpieralisi@...nel.org>, Krzysztof Wilczyński <kwilczynski@...nel.org>,
Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>, linux-pci@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org, Ron Economos <re@...z.net>
Subject: Re: [PATCH 1/2] PCI: dwc: Fix ECAM enablement when used with vendor
drivers
On Sat, Oct 18, 2025 at 03:00:32PM +0530, Krishna Chaitanya Chundru wrote:
>
>
> On 10/18/2025 2:55 PM, Krishna Chaitanya Chundru wrote:
> >
> >
> > On 10/18/2025 8:39 AM, Manivannan Sadhasivam wrote:
> > > On Fri, Oct 17, 2025 at 02:10:05PM -0500, Bjorn Helgaas wrote:
> > > > On Fri, Oct 17, 2025 at 05:10:53PM +0530, Krishna Chaitanya
> > > > Chundru wrote:
> > > > > When the vendor configuration space is 256MB aligned, the DesignWare
> > > > > PCIe host driver enables ECAM access and sets the DBI base
> > > > > to the dw_pcie_ecam_conf_map_busstart
> > > > > of the config space. This causes vendor drivers to incorrectly program
> > > > > iATU regions, as they rely on the DBI address for internal accesses.
> > > > >
> > > > > To fix this, avoid overwriting the DBI base when ECAM is enabled.
> > > > > Instead, introduce a custom ECAM PCI ops implementation that accesses
> > > > > the DBI region directly for bus 0 and uses ECAM for other buses.
> > > > >
> > > > > Fixes: f6fd357f7afb ("PCI: dwc: Prepare the driver for
> > > > > enabling ECAM mechanism using iATU 'CFG Shift Feature'")
> > > > > Reported-by: Ron Economos <re@...z.net>
> > > > > Closes: https://lore.kernel.org/all/eac81c57-1164-4d74-a1b4-6f353c577731@w6rz.net/
> > > > > Suggested-by: Manivannan Sadhasivam <mani@...nel.org>
> > > > > Signed-off-by: Krishna Chaitanya Chundru
> > > > > <krishna.chundru@....qualcomm.com>
> > > > > ---
> > > > > drivers/pci/controller/dwc/pcie-designware-host.c | 28
> > > > > +++++++++++++++++++----
> > > > > 1 file changed, 24 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git
> > > > > a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > index 20c9333bcb1c4812e2fd96047a49944574df1e6f..e92513c5bda51bde3a7157033ddbd73afa370d78
> > > > > 100644
> > > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > @@ -23,6 +23,7 @@
> > > > > #include "pcie-designware.h"
> > > > > static struct pci_ops dw_pcie_ops;
> > > > > +static struct pci_ops dw_pcie_ecam_ops;
> > > > > static struct pci_ops dw_child_pcie_ops;
> > > > > #define DW_PCIE_MSI_FLAGS_REQUIRED
> > > > > (MSI_FLAG_USE_DEF_DOM_OPS | \
> > > > > @@ -471,9 +472,6 @@ static int
> > > > > dw_pcie_create_ecam_window(struct dw_pcie_rp *pp, struct
> > > > > resource *re
> > > > > if (IS_ERR(pp->cfg))
> > > > > return PTR_ERR(pp->cfg);
> > > > > - pci->dbi_base = pp->cfg->win;
> > > > > - pci->dbi_phys_addr = res->start;
> > > > > -
> > > > > return 0;
> > > > > }
> > > > > @@ -529,7 +527,7 @@ static int
> > > > > dw_pcie_host_get_resources(struct dw_pcie_rp *pp)
> > > > > if (ret)
> > > > > return ret;
> > > > > - pp->bridge->ops = (struct pci_ops
> > > > > *)&pci_generic_ecam_ops.pci_ops;
> > > > > + pp->bridge->ops = &dw_pcie_ecam_ops;
> > > > > pp->bridge->sysdata = pp->cfg;
> > > > > pp->cfg->priv = pp;
> > > > > } else {
> > > > > @@ -842,12 +840,34 @@ void __iomem
> > > > > *dw_pcie_own_conf_map_bus(struct pci_bus *bus, unsigned int
> > > > > devfn,
> > > > > }
> > > > > EXPORT_SYMBOL_GPL(dw_pcie_own_conf_map_bus);
> > > > > +static void __iomem *dw_pcie_ecam_conf_map_bus(struct
> > > > > pci_bus *bus, unsigned int devfn, int where)
> > > > > +{
> > > > > + struct pci_config_window *cfg = bus->sysdata;
> > > > > + struct dw_pcie_rp *pp = cfg->priv;
> > > > > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > > + unsigned int busn = bus->number;
> > > > > +
> > > > > + if (busn > 0)
> > > > > + return pci_ecam_map_bus(bus, devfn, where);
> > > >
> > > > Is there a way to avoid the "root bus is bus 00" assumption here? It
> > > > looks like something like this might work (it inverts the condition
> > > > to take care of the root bus special case first):
> > > >
> > > > if (bus == pp->bridge->bus) {
> > > > if (PCI_SLOT(devfn) > 0)
> > > > return NULL;
> > > >
> > > > return pci->dbi_base + where;
> > > > }
> > > >
> > > > return pci_ecam_map_bus(bus, devfn, where);
> > > >
> > >
> > > I guess it will work.
> > >
> > > > > + if (PCI_SLOT(devfn) > 0)
> > > > > + return NULL;
> > > >
> > > > This essentially says only one function (00.0) can be on the root bus.
> > > > I assume that someday that will be relaxed and there may be multiple
> > > > Root Ports and maybe RCiEPs on the root bus, so it would be nice if we
> > > > didn't have to have this check.
> > > >
> > > > What happens without it? Does the IP return the ~0 data that the PCI
> > > > core would interpret as "there's no device here"?
> > > >
> > >
> > > I hope the read returns ~0, but the idea is to catch the invalid
> > > access before
> > > trying to read/write. In case of multi Root Port design, I don't
> > > think we have a
> > > way to identify it. So maybe it is safe to remove this check.
> > >
> > For multi root port we may need to revisit this, currently along with
> > dbi there are some other registers iATU, elbi etc. So there might be
> > chances to read these registers like iATU as part of enumeration and
> > these can return non ~0 values which will have adverse effects.
> > So we should have this check for now.
> >
> One more issue is some controllers may pass only 4k memory as dbi memory
> so we might get unmapped address access issues also.
Oh yes. The single Root Port controllers might supply only 4KiB of the DBI
space, so without the check, the driver could read/write past the mapped space.
Good catch!
- Mani
--
மணிவண்ணன் சதாசிவம்
Powered by blists - more mailing lists