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

[PATCH 0/1] 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 not allocating the memory region of the
"chamelon table" that is only needed for parsing. Just do an ioremap instead.

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

Javier Rodriguez (1):
  mcb: Remove requesting memory region to avoid memory overlapping

 drivers/mcb/mcb-pci.c | 10 ----------
 1 file changed, 10 deletions(-)

-- 
2.34.1

[PATCH 1/1] mcb: Remove requesting 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 overlaps with the IP Cores' memory regions.

There is no need to request the memory region for only parsing the
chameleon table as it is read-only. It is sufficient to only remaps
it when parsing.

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 | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/mcb/mcb-pci.c b/drivers/mcb/mcb-pci.c
index dc88232d9af8..e98f626020a7 100644
--- a/drivers/mcb/mcb-pci.c
+++ b/drivers/mcb/mcb-pci.c
@@ -29,7 +29,6 @@ static int mcb_pci_get_irq(struct mcb_device *mdev)
 
 static int mcb_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
-	struct resource *res;
 	struct priv *priv;
 	int ret;
 	unsigned long flags;
@@ -52,15 +51,6 @@ static int mcb_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		goto out_disable;
 	}
 
-	res = devm_request_mem_region(&pdev->dev, priv->mapbase,
-				      CHAM_HEADER_SIZE,
-				      KBUILD_MODNAME);
-	if (!res) {
-		dev_err(&pdev->dev, "Failed to request PCI memory\n");
-		ret = -EBUSY;
-		goto out_disable;
-	}
-
 	priv->base = devm_ioremap(&pdev->dev, priv->mapbase, CHAM_HEADER_SIZE);
 	if (!priv->base) {
 		dev_err(&pdev->dev, "Cannot ioremap\n");
-- 
2.34.1

Re: [PATCH 1/1] mcb: Remove requesting memory region to avoid memory overlapping

[Johannes Thumshirn]

On 23.03.23 13:49, Rodríguez Barbarin, José Javier wrote:
> 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 overlaps with the IP Cores' memory regions.
>
> There is no need to request the memory region for only parsing the
> chameleon table as it is read-only. It is sufficient to only remaps
> it when parsing.
>
> 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 | 10 ----------
>  1 file changed, 10 deletions(-)
>
> diff --git a/drivers/mcb/mcb-pci.c b/drivers/mcb/mcb-pci.c
> index dc88232d9af8..e98f626020a7 100644
> --- a/drivers/mcb/mcb-pci.c
> +++ b/drivers/mcb/mcb-pci.c
> @@ -29,7 +29,6 @@ static int mcb_pci_get_irq(struct mcb_device *mdev)
>  
>  static int mcb_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  {
> -	struct resource *res;
>  	struct priv *priv;
>  	int ret;
>  	unsigned long flags;
> @@ -52,15 +51,6 @@ static int mcb_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		goto out_disable;
>  	}
>  
> -	res = devm_request_mem_region(&pdev->dev, priv->mapbase,
> -				      CHAM_HEADER_SIZE,
> -				      KBUILD_MODNAME);
> -	if (!res) {
> -		dev_err(&pdev->dev, "Failed to request PCI memory\n");
> -		ret = -EBUSY;
> -		goto out_disable;
> -	}
> -
>  	priv->base = devm_ioremap(&pdev->dev, priv->mapbase, CHAM_HEADER_SIZE);
>  	if (!priv->base) {
>  		dev_err(&pdev->dev, "Cannot ioremap\n");

TBH I'm not sure I like this approach.

One thing that could be done (and probably the easiest and most correct)
is resizing
the resource used for the header. Aka checking the header size and
dropping/reallocating
it once we have the correct size. It's not a fastpath anyways so we
don't really care about
reallocations here.

Another way would be flagging these ressources as IORESOURCE_MUXED but
that would need
quite some plumbing, especially for the 16z125 as it uses 8250_port for
requesting memory.

If these are specific FPGA versions/revisions, maybe they could be
quirked as well so we only
allocate as much space as we need.

I'm not too deep into resource manage any more, but
Documentation/PCI/pci.rst explicitly
states, that a driver must request the region to verify no other device
is alreay using the
resource.

So until you (or someone else) are proofing me wrong on these I'd say
NACK to this.

Byte,
    Johannes

Re: [PATCH 1/1] mcb: Remove requesting memory region to avoid memory overlapping

[Rodríguez Barbarin, José Javier]

On Thu, 2023-03-23 at 15:25 +0100, Johannes Thumshirn wrote:
> On 23.03.23 13:49, Rodríguez Barbarin, José Javier wrote:
> > 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 overlaps with the IP Cores' memory regions.
> > 
> > There is no need to request the memory region for only parsing the
> > chameleon table as it is read-only. It is sufficient to only remaps
> > it when parsing.
> > 
> > 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 | 10 ----------
> >  1 file changed, 10 deletions(-)
> > 
> > diff --git a/drivers/mcb/mcb-pci.c b/drivers/mcb/mcb-pci.c
> > index dc88232d9af8..e98f626020a7 100644
> > --- a/drivers/mcb/mcb-pci.c
> > +++ b/drivers/mcb/mcb-pci.c
> > @@ -29,7 +29,6 @@ static int mcb_pci_get_irq(struct mcb_device
> > *mdev)
> >  
> >  static int mcb_pci_probe(struct pci_dev *pdev, const struct
> > pci_device_id *id)
> >  {
> > -       struct resource *res;
> >         struct priv *priv;
> >         int ret;
> >         unsigned long flags;
> > @@ -52,15 +51,6 @@ static int mcb_pci_probe(struct pci_dev *pdev,
> > const struct pci_device_id *id)
> >                 goto out_disable;
> >         }
> >  
> > -       res = devm_request_mem_region(&pdev->dev, priv->mapbase,
> > -                                     CHAM_HEADER_SIZE,
> > -                                     KBUILD_MODNAME);
> > -       if (!res) {
> > -               dev_err(&pdev->dev, "Failed to request PCI
> > memory\n");
> > -               ret = -EBUSY;
> > -               goto out_disable;
> > -       }
> > -
> >         priv->base = devm_ioremap(&pdev->dev, priv->mapbase,
> > CHAM_HEADER_SIZE);
> >         if (!priv->base) {
> >                 dev_err(&pdev->dev, "Cannot ioremap\n");
> 
> TBH I'm not sure I like this approach.
> 
> One thing that could be done (and probably the easiest and most
> correct)
> is resizing
> the resource used for the header. Aka checking the header size and
> dropping/reallocating
> it once we have the correct size. It's not a fastpath anyways so we
> don't really care about
> reallocations here.
> 
> Another way would be flagging these ressources as IORESOURCE_MUXED
> but
> that would need
> quite some plumbing, especially for the 16z125 as it uses 8250_port
> for
> requesting memory.
> 
> If these are specific FPGA versions/revisions, maybe they could be
> quirked as well so we only
> allocate as much space as we need.
> 
> I'm not too deep into resource manage any more, but
> Documentation/PCI/pci.rst explicitly
> states, that a driver must request the region to verify no other
> device
> is alreay using the
> resource.
> 
> So until you (or someone else) are proofing me wrong on these I'd say
> NACK to this.
> 
> Byte,
>     Johannes

Hi Johannes,

One thing we can do is modify the chameleon_parse_cells prototype to
return the actual "chameleon table" size instead of the number of cells
which can have an undetermined size. At the moment, the return value is
only used for error checking but the number of cells is never used.

After that, we can check the actual "chameleon table" size and
drop/reallocate if needed.

Regards,

Javier

Re: [PATCH 1/1] mcb: Remove requesting memory region to avoid memory overlapping

[Johannes Thumshirn]

On 23.03.23 16:11, Rodríguez Barbarin, José Javier wrote:
> One thing we can do is modify the chameleon_parse_cells prototype to
> return the actual "chameleon table" size instead of the number of cells
> which can have an undetermined size. At the moment, the return value is
> only used for error checking but the number of cells is never used.
>
> After that, we can check the actual "chameleon table" size and
> drop/reallocate if needed.
Hi Javier,

Yeah that sounds reasonable. mcb_bus_add_devices() calls device_attach()
for the respective IP cores, so they individual driver's ->probe() function
shouldn't be called before mcb_bus_add_devices(). So you could shrink
the resource between chameleon_parse_cells() and mcb_add_devices().

Btw, mcb-lpc.c needs the same fix as well.

Byte,
    Johannes