[PATCH v2 0/3] mcb-pci: fix memory overlapping on MCB devices

[PATCH v2 0/3] mcb-pci: fix memory overlapping on MCB devices

[Rodríguez Barbarin, José Javier]

mcb-pci driver is allocating the memory region for the "chameleon table"
with a fixed size of 0x200. This region is only use to do a initial
parsing to discover the devices implemented as IP Cores.

If the "chameleon table" is actually smalled than 0x200 and the first
device offset happen to be within 0x200, a memory overlapping can ocurr.

Here an extract of the memory overlapping when registering a 16z125 IP Core:

[   31.016972] 8250_men_mcb mcb0-16z125-0:0:0: can't request region for resource [mem 0xa8200100-0xa820010f]
[   31.016994] 8250_men_mcb: probe of mcb0-16z125-0:0:0 failed with error -16
[   31.017010] 8250_men_mcb mcb0-16z125-1:0:0: can't request region for resource [mem 0xa8200110-0xa820011f]

And here, the memory allocated for the chameleon table parsing:

user@host:$ sudo /proc/iomem
...
a8200000-a82001ff : mcb_pci
...

This patch solves this problem by dropping/reallocating the memory region of the
"chamelon table" with the actual size once it has been parsed.

This patch is based on linux-next (next-20230323)

Changes for V2:
 * make parsing function return the size of "chameleon table".
 * reallocate instead of not requesting the memory region.

Javier Rodriguez (3):
  mcb: Return actual parsed size when reading chameleon table
  mcb-pci: Reallocate memory region to avoid memory overlapping
  mcb-lpc: Reallocate memory region to avoid memory overlapping

 drivers/mcb/mcb-lpc.c   | 35 +++++++++++++++++++++++++++++++----
 drivers/mcb/mcb-parse.c | 10 +++++++---
 drivers/mcb/mcb-pci.c   | 27 +++++++++++++++++++++++++--
 3 files changed, 63 insertions(+), 9 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] mcb: Return actual parsed size when reading chameleon table

[Rodríguez Barbarin, José Javier]

Function chameleon_parse_cells() returns the number of cells parsed
which has an undetermined size. This return value is only used for
error checking but the number of cells is never used.

Change return value to be number of bytes parsed to allow for memory
management improvements.

Co-developed-by: Jorge Sanjuan Garcia <jorge.sanjuangarcia@duagon.com>
Signed-off-by: Jorge Sanjuan Garcia <jorge.sanjuangarcia@duagon.com>
Signed-off-by: Javier Rodriguez <josejavier.rodriguez@duagon.com>
---
 drivers/mcb/mcb-parse.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/mcb/mcb-parse.c b/drivers/mcb/mcb-parse.c
index aa6938da0db8..5c2746e1d4cf 100644
--- a/drivers/mcb/mcb-parse.c
+++ b/drivers/mcb/mcb-parse.c
@@ -130,7 +130,7 @@ static void chameleon_parse_bar(void __iomem *base,
 	}
 }
 
-static int chameleon_get_bar(char __iomem **base, phys_addr_t mapbase,
+static int chameleon_get_bar(void __iomem **base, phys_addr_t mapbase,
 			     struct chameleon_bar **cb)
 {
 	struct chameleon_bar *c;
@@ -179,12 +179,13 @@ int chameleon_parse_cells(struct mcb_bus *bus, phys_addr_t mapbase,
 {
 	struct chameleon_fpga_header *header;
 	struct chameleon_bar *cb;
-	char __iomem *p = base;
+	void __iomem *p = base;
 	int num_cells = 0;
 	uint32_t dtype;
 	int bar_count;
 	int ret;
 	u32 hsize;
+	u32 table_size;
 
 	hsize = sizeof(struct chameleon_fpga_header);
 
@@ -239,12 +240,15 @@ int chameleon_parse_cells(struct mcb_bus *bus, phys_addr_t mapbase,
 		num_cells++;
 	}
 
+
 	if (num_cells == 0)
 		num_cells = -EINVAL;
 
+	table_size = p - base;
+	pr_debug("%d cell(s) found. Chameleon table size: 0x%04x bytes\n", num_cells, table_size);
 	kfree(cb);
 	kfree(header);
-	return num_cells;
+	return table_size;
 
 free_bar:
 	kfree(cb);
-- 
2.34.1

[PATCH v2 2/3] mcb-pci: Reallocate memory region to avoid memory overlapping

[Rodríguez Barbarin, José Javier]

mcb-pci requests a fixed-size memory region to parse the chameleon
table, however, if the chameleon table is smaller that the allocated
region, it could overlap with the IP Cores' memory regions.

After parsing the chameleon table, drop/reallocate the memory region
with the actual chameleon table size.

Co-developed-by: Jorge Sanjuan Garcia <jorge.sanjuangarcia@duagon.com>
Signed-off-by: Jorge Sanjuan Garcia <jorge.sanjuangarcia@duagon.com>
Signed-off-by: Javier Rodriguez <josejavier.rodriguez@duagon.com>
---
 drivers/mcb/mcb-pci.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/mcb/mcb-pci.c b/drivers/mcb/mcb-pci.c
index dc88232d9af8..53d9202ff9a7 100644
--- a/drivers/mcb/mcb-pci.c
+++ b/drivers/mcb/mcb-pci.c
@@ -31,7 +31,7 @@ static int mcb_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	struct resource *res;
 	struct priv *priv;
-	int ret;
+	int ret, table_size;
 	unsigned long flags;
 
 	priv = devm_kzalloc(&pdev->dev, sizeof(struct priv), GFP_KERNEL);
@@ -90,7 +90,30 @@ static int mcb_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (ret < 0)
 		goto out_mcb_bus;
 
-	dev_dbg(&pdev->dev, "Found %d cells\n", ret);
+	table_size = ret;
+
+	if (table_size < CHAM_HEADER_SIZE) {
+		/* Release the previous resources */
+		devm_iounmap(&pdev->dev, priv->base);
+		devm_release_mem_region(&pdev->dev, priv->mapbase, CHAM_HEADER_SIZE);
+
+		/* Then, allocate it again with the actual chameleon table size */
+		res = devm_request_mem_region(&pdev->dev, priv->mapbase,
+						table_size,
+						KBUILD_MODNAME);
+		if (!res) {
+			dev_err(&pdev->dev, "Failed to request PCI memory\n");
+			ret = -EBUSY;
+			goto out_mcb_bus;
+		}
+
+		priv->base = devm_ioremap(&pdev->dev, priv->mapbase, table_size);
+		if (!priv->base) {
+			dev_err(&pdev->dev, "Cannot ioremap\n");
+			ret = -ENOMEM;
+			goto out_mcb_bus;
+		}
+	}
 
 	mcb_bus_add_devices(priv->bus);
 
-- 
2.34.1

[PATCH v2 3/3] mcb-lpc: Reallocate memory region to avoid memory overlapping

[Rodríguez Barbarin, José Javier]

mcb-lpc requests a fixed-size memory region to parse the chameleon
table, however, if the chameleon table is smaller that the allocated
region, it could overlap with the IP Cores' memory regions.

After parsing the chameleon table, drop/reallocate the memory region
with the actual chameleon table size.

Co-developed-by: Jorge Sanjuan Garcia <jorge.sanjuangarcia@duagon.com>
Signed-off-by: Jorge Sanjuan Garcia <jorge.sanjuangarcia@duagon.com>
Signed-off-by: Javier Rodriguez <josejavier.rodriguez@duagon.com>
---
 drivers/mcb/mcb-lpc.c | 35 +++++++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/mcb/mcb-lpc.c b/drivers/mcb/mcb-lpc.c
index 53decd89876e..a851e0236464 100644
--- a/drivers/mcb/mcb-lpc.c
+++ b/drivers/mcb/mcb-lpc.c
@@ -23,7 +23,7 @@ static int mcb_lpc_probe(struct platform_device *pdev)
 {
 	struct resource *res;
 	struct priv *priv;
-	int ret = 0;
+	int ret = 0, table_size;
 
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -58,16 +58,43 @@ static int mcb_lpc_probe(struct platform_device *pdev)
 
 	ret = chameleon_parse_cells(priv->bus, priv->mem->start, priv->base);
 	if (ret < 0) {
-		mcb_release_bus(priv->bus);
-		return ret;
+		goto out_mcb_bus;
 	}
 
-	dev_dbg(&pdev->dev, "Found %d cells\n", ret);
+	table_size = ret;
+
+	if (table_size < CHAM_HEADER_SIZE) {
+		/* Release the previous resources */
+		devm_iounmap(&pdev->dev, priv->base);
+		devm_release_mem_region(&pdev->dev, priv->mem->start, resource_size(priv->mem));
+
+		/* Then, allocate it again with the actual chameleon table size */
+		res = devm_request_mem_region(&pdev->dev, priv->mem->start,
+					      table_size,
+					      KBUILD_MODNAME);
+		if (!res) {
+			dev_err(&pdev->dev, "Failed to request PCI memory\n");
+			ret = -EBUSY;
+			goto out_mcb_bus;
+		}
+
+		priv->base = devm_ioremap(&pdev->dev, priv->mem->start, table_size);
+		if (!priv->base) {
+			dev_err(&pdev->dev, "Cannot ioremap\n");
+			ret = -ENOMEM;
+			goto out_mcb_bus;
+		}
+
+		platform_set_drvdata(pdev, priv);
+	}
 
 	mcb_bus_add_devices(priv->bus);
 
 	return 0;
 
+out_mcb_bus:
+	mcb_release_bus(priv->bus);
+	return ret;
 }
 
 static int mcb_lpc_remove(struct platform_device *pdev)
-- 
2.34.1

Re: [PATCH v2 1/3] mcb: Return actual parsed size when reading chameleon table

[Johannes Thumshirn]

Am 28.03.23 um 16:34 schrieb Rodríguez Barbarin, José Javier:
> Function chameleon_parse_cells() returns the number of cells parsed

^ The function?

> @@ -239,12 +240,15 @@ int chameleon_parse_cells(struct mcb_bus *bus, phys_addr_t mapbase,
>   		num_cells++;
>   	}
>   
> +

Stray newline.
>   	if (num_cells == 0)
>   		num_cells = -EINVAL;
>   
> +	table_size = p - base;
> +	pr_debug("%d cell(s) found. Chameleon table size: 0x%04x bytes\n", num_cells, table_size);
>   	kfree(cb);
>   	kfree(header);
> -	return num_cells;
> +	return table_size;

Ahm doesn't that need to be:
             return num_cells < 0 ? num_cells : table_size;


Otherwise we loose the -EINVAL return here.

I could've fixed up the 1st two, but the last one is a functional change 
and I won't fix it
up when applying.

Byte,
     Johannes