[PATCH v6] PCI: imx6: limit DBI register length

[PATCH v6] PCI: imx6: limit DBI register length

[Stefan Agner]

Define the length of the DBI registers and limit config space to its
length. This makes sure that the kernel does not access registers
beyond that point, avoiding the following abort on a i.MX 6Quad:
  # cat /sys/devices/soc0/soc/1ffc000.pcie/pci0000\:00/0000\:00\:00.0/config
  [  100.021433] Unhandled fault: imprecise external abort (0x1406) at 0xb6ea7000
  ...
  [  100.056423] PC is at dw_pcie_read+0x50/0x84
  [  100.060790] LR is at dw_pcie_rd_own_conf+0x44/0x48
  ...

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
Changes in v3:
- Rebase on pci/dwc
Changes in v4:
- Rebase on pci/dwc
Changes in v5:
- Rebased ontop of pci/dwc
- Use DBI length of 0x200
Changes in v6:
- Use pci_dev.cfg_size mechanism to limit config space (this made patch 1
  of previous versions of this patchset obsolete).

(Lucas, I dropped your Reviewed-by since changes are substantial...)

 drivers/pci/controller/dwc/pci-imx6.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index aaa9489e2140..8bd335c52414 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -56,6 +56,7 @@ enum imx6_pcie_variants {
 struct imx6_pcie_drvdata {
 	enum imx6_pcie_variants variant;
 	u32 flags;
+	int dbi_length;
 };
 
 struct imx6_pcie {
@@ -912,6 +913,24 @@ static const struct dw_pcie_ops dw_pcie_ops = {
 	/* No special ops needed, but pcie-designware still expects this struct */
 };
 
+static void imx6_pcie_quirk(struct pci_dev *dev)
+{
+	struct pci_bus *bus = dev->bus;
+	struct pcie_port *pp = bus->sysdata;
+
+	if (bus->number == pp->root_bus_nr) {
+		struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+		struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
+
+		/*
+		 * Limit config length to avoid the kernel reading beyond
+		 * the register set and causing an abort on i.MX 6Quad
+		 */
+		dev->cfg_size = imx6_pcie->drvdata->dbi_length;
+	}
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, imx6_pcie_quirk);
+
 #ifdef CONFIG_PM_SLEEP
 static void imx6_pcie_ltssm_disable(struct device *dev)
 {
@@ -1242,6 +1261,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
 		.variant = IMX6Q,
 		.flags = IMX6_PCIE_FLAG_IMX6_PHY |
 			 IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE,
+		.dbi_length = 0x200,
 	},
 	[IMX6SX] = {
 		.variant = IMX6SX,
-- 
2.20.1

Re: [PATCH v6] PCI: imx6: limit DBI register length

[Leonard Crestez]

On Mon, 2019-02-25 at 15:25 +0100, Stefan Agner wrote:
> Define the length of the DBI registers and limit config space to its
> length. This makes sure that the kernel does not access registers
> beyond that point, avoiding the following abort on a i.MX 6Quad:
>   # cat /sys/devices/soc0/soc/1ffc000.pcie/pci0000\:00/0000\:00\:00.0/config
>   [  100.021433] Unhandled fault: imprecise external abort (0x1406) at 0xb6ea7000
>   ...
>   [  100.056423] PC is at dw_pcie_read+0x50/0x84
>   [  100.060790] LR is at dw_pcie_rd_own_conf+0x44/0x48
>  
> +static void imx6_pcie_quirk(struct pci_dev *dev)
> +{
> +	struct pci_bus *bus = dev->bus;
> +	struct pcie_port *pp = bus->sysdata;
> +
> +	if (bus->number == pp->root_bus_nr) {
> +		struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +		struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
> +
> +		/*
> +		 * Limit config length to avoid the kernel reading beyond
> +		 * the register set and causing an abort on i.MX 6Quad
> +		 */
> +		dev->cfg_size = imx6_pcie->drvdata->dbi_length;
> +	}
> +}
> +DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, imx6_pcie_quirk);

Are you sure that this quirk is only applied to imx6-pcie?
Superficially it looks like it would apply to all PCI roots and this
could be a problem on configs which enable all drivers, like arm64.

Maybe the linker magic inside DECLARE_PCI_FIXUP deals with that, in
that case I apologize.

--
Regards,
Leonard

Re: [PATCH v6] PCI: imx6: limit DBI register length

[Stefan Agner]

On 25.02.2019 15:47, Leonard Crestez wrote:
> On Mon, 2019-02-25 at 15:25 +0100, Stefan Agner wrote:
>> Define the length of the DBI registers and limit config space to its
>> length. This makes sure that the kernel does not access registers
>> beyond that point, avoiding the following abort on a i.MX 6Quad:
>>   # cat /sys/devices/soc0/soc/1ffc000.pcie/pci0000\:00/0000\:00\:00.0/config
>>   [  100.021433] Unhandled fault: imprecise external abort (0x1406) at 0xb6ea7000
>>   ...
>>   [  100.056423] PC is at dw_pcie_read+0x50/0x84
>>   [  100.060790] LR is at dw_pcie_rd_own_conf+0x44/0x48
>>
>> +static void imx6_pcie_quirk(struct pci_dev *dev)
>> +{
>> +	struct pci_bus *bus = dev->bus;
>> +	struct pcie_port *pp = bus->sysdata;
>> +
>> +	if (bus->number == pp->root_bus_nr) {
>> +		struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> +		struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
>> +
>> +		/*
>> +		 * Limit config length to avoid the kernel reading beyond
>> +		 * the register set and causing an abort on i.MX 6Quad
>> +		 */
>> +		dev->cfg_size = imx6_pcie->drvdata->dbi_length;
>> +	}
>> +}
>> +DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, imx6_pcie_quirk);
> 
> Are you sure that this quirk is only applied to imx6-pcie?
> Superficially it looks like it would apply to all PCI roots and this
> could be a problem on configs which enable all drivers, like arm64.

That was inspired by the FIXUP in keystone, which uses
PCI_ANY_ID/PCI_ANY_ID too.

However, keystone also does not have arm64 variants it seems.
Furthermore, the keystone fixup checks PCI ids in code.

> 
> Maybe the linker magic inside DECLARE_PCI_FIXUP deals with that, in
> that case I apologize.

I don't think there is special linker magic, if CONFIG_PCI_IMX6 is
enabled this will get called.

I agree, I should use PCI_VENDOR_ID_SYNOPSYS, 0xabcd here.


Btw, while looking at the patch again I realized that I need to check
imx6_pcie->drvdata->dbi_length for 0 and not apply in this case to avoid
breaking other i.MX designs.

I will send a v7.

--
Stefan