[bug report] PCI: Check BAR index for validity

[bug report] PCI: Check BAR index for validity

[Dan Carpenter]

Hello Philipp Stanner,

Commit ba10e5011d05 ("PCI: Check BAR index for validity") from Mar 4,
2025 (linux-next), leads to the following Smatch static checker
warning:

	drivers/pci/devres.c:632 pcim_remove_bar_from_legacy_table()
	error: buffer overflow 'legacy_iomap_table' 6 <= 15

drivers/pci/devres.c
    621 static void pcim_remove_bar_from_legacy_table(struct pci_dev *pdev, int bar)
    622 {
    623         void __iomem **legacy_iomap_table;
    624 
    625         if (!pci_bar_index_is_valid(bar))

This line used to check PCI_STD_NUM_BARS (6) but now it's checking
PCI_NUM_RESOURCES (15).

    626                 return;
    627 
    628         legacy_iomap_table = (void __iomem **)pcim_iomap_table(pdev);
    629         if (!legacy_iomap_table)
    630                 return;
    631 
--> 632         legacy_iomap_table[bar] = NULL;
                ^^^^^^^^^^^^^^^^^^^^^^^
Leading to a buffer overflow.

    633 }

regards,
dan carpenter

Re: [bug report] PCI: Check BAR index for validity

[Bjorn Helgaas]

On Sat, Mar 08, 2025 at 01:23:28PM +0300, Dan Carpenter wrote:
> Hello Philipp Stanner,
> 
> Commit ba10e5011d05 ("PCI: Check BAR index for validity") from Mar 4,
> 2025 (linux-next), leads to the following Smatch static checker
> warning:
> 
> 	drivers/pci/devres.c:632 pcim_remove_bar_from_legacy_table()
> 	error: buffer overflow 'legacy_iomap_table' 6 <= 15

Thanks, I dropped this patch for now.

> drivers/pci/devres.c
>     621 static void pcim_remove_bar_from_legacy_table(struct pci_dev *pdev, int bar)
>     622 {
>     623         void __iomem **legacy_iomap_table;
>     624 
>     625         if (!pci_bar_index_is_valid(bar))
> 
> This line used to check PCI_STD_NUM_BARS (6) but now it's checking
> PCI_NUM_RESOURCES (15).
> 
>     626                 return;
>     627 
>     628         legacy_iomap_table = (void __iomem **)pcim_iomap_table(pdev);
>     629         if (!legacy_iomap_table)
>     630                 return;
>     631 
> --> 632         legacy_iomap_table[bar] = NULL;
>                 ^^^^^^^^^^^^^^^^^^^^^^^
> Leading to a buffer overflow.
> 
>     633 }
> 
> regards,
> dan carpenter

Re: [bug report] PCI: Check BAR index for validity

[Philipp Stanner]

On Sat, 2025-03-08 at 15:07 -0600, Bjorn Helgaas wrote:
> On Sat, Mar 08, 2025 at 01:23:28PM +0300, Dan Carpenter wrote:
> > Hello Philipp Stanner,
> > 
> > Commit ba10e5011d05 ("PCI: Check BAR index for validity") from Mar
> > 4,
> > 2025 (linux-next), leads to the following Smatch static checker
> > warning:
> > 
> > 	drivers/pci/devres.c:632
> > pcim_remove_bar_from_legacy_table()
> > 	error: buffer overflow 'legacy_iomap_table' 6 <= 15
> 
> Thanks, I dropped this patch for now.
> 
> > drivers/pci/devres.c
> >     621 static void pcim_remove_bar_from_legacy_table(struct
> > pci_dev *pdev, int bar)
> >     622 {
> >     623         void __iomem **legacy_iomap_table;
> >     624 
> >     625         if (!pci_bar_index_is_valid(bar))
> > 
> > This line used to check PCI_STD_NUM_BARS (6) but now it's checking
> > PCI_NUM_RESOURCES (15).

What is even going on here. Why are thos different values? Does a PCI
device now have at most 6, or 15 BARs?

Or is a BAR different from a "resource"?

And why would it be 15? I haven't read the standard, but I would
suspect it should be 16.

And which of those two here should be used?
https://elixir.bootlin.com/linux/v6.14-rc4/source/include/linux/pci.h#L133

The comment doesn't say *which one* is "preserved for backwards
compatibility".

So many questions…

But granted, the check is wrong for the devres resource array, and I
suppose it should be made the same size as pci_dev.resource.


> > 
> >     626                 return;
> >     627 
> >     628         legacy_iomap_table = (void __iomem
> > **)pcim_iomap_table(pdev);
> >     629         if (!legacy_iomap_table)
> >     630                 return;
> >     631 
> > --> 632         legacy_iomap_table[bar] = NULL;
> >                 ^^^^^^^^^^^^^^^^^^^^^^^
> > Leading to a buffer overflow.

Leading to a *potential* buffer overflow.

Anyways, thanks for reporting.

P.


> > 
> >     633 }
> > 
> > regards,
> > dan carpenter

Re: [bug report] PCI: Check BAR index for validity

[Philipp Stanner]

On Mon, 2025-03-10 at 08:54 +0100, Philipp Stanner wrote:
> On Sat, 2025-03-08 at 15:07 -0600, Bjorn Helgaas wrote:
> > On Sat, Mar 08, 2025 at 01:23:28PM +0300, Dan Carpenter wrote:
> > > Hello Philipp Stanner,
> > > 
> > > Commit ba10e5011d05 ("PCI: Check BAR index for validity") from
> > > Mar
> > > 4,
> > > 2025 (linux-next), leads to the following Smatch static checker
> > > warning:
> > > 
> > > 	drivers/pci/devres.c:632
> > > pcim_remove_bar_from_legacy_table()
> > > 	error: buffer overflow 'legacy_iomap_table' 6 <= 15
> > 
> > Thanks, I dropped this patch for now.
> > 
> > > drivers/pci/devres.c
> > >     621 static void pcim_remove_bar_from_legacy_table(struct
> > > pci_dev *pdev, int bar)
> > >     622 {
> > >     623         void __iomem **legacy_iomap_table;
> > >     624 
> > >     625         if (!pci_bar_index_is_valid(bar))
> > > 
> > > This line used to check PCI_STD_NUM_BARS (6) but now it's
> > > checking
> > > PCI_NUM_RESOURCES (15).
> 
> What is even going on here. Why are thos different values? Does a PCI
> device now have at most 6, or 15 BARs?
> 
> Or is a BAR different from a "resource"?
> 
> And why would it be 15? I haven't read the standard, but I would
> suspect it should be 16.
> 
> And which of those two here should be used?
> https://elixir.bootlin.com/linux/v6.14-rc4/source/include/linux/pci.h#L133
> 
> The comment doesn't say *which one* is "preserved for backwards
> compatibility".

Furthermore, I just saw that the old pcim_ code would then also be
half-broken, because it also uses PCI_STD_NUM_BARS, whereas the pci_
functions use PCI_NUM_RESOURCES:

https://elixir.bootlin.com/linux/v6.8.9/source/drivers/pci/pci.c#L6555


P.


> 
> So many questions…
> 
> But granted, the check is wrong for the devres resource array, and I
> suppose it should be made the same size as pci_dev.resource.
> 
> 
> > > 
> > >     626                 return;
> > >     627 
> > >     628         legacy_iomap_table = (void __iomem
> > > **)pcim_iomap_table(pdev);
> > >     629         if (!legacy_iomap_table)
> > >     630                 return;
> > >     631 
> > > --> 632         legacy_iomap_table[bar] = NULL;
> > >                 ^^^^^^^^^^^^^^^^^^^^^^^
> > > Leading to a buffer overflow.
> 
> Leading to a *potential* buffer overflow.
> 
> Anyways, thanks for reporting.
> 
> P.
> 
> 
> > > 
> > >     633 }
> > > 
> > > regards,
> > > dan carpenter
> 

Re: [bug report] PCI: Check BAR index for validity

[Dan Carpenter]

On Mon, Mar 10, 2025 at 08:54:54AM +0100, Philipp Stanner wrote:
> > > 
> > >     626                 return;
> > >     627 
> > >     628         legacy_iomap_table = (void __iomem
> > > **)pcim_iomap_table(pdev);
> > >     629         if (!legacy_iomap_table)
> > >     630                 return;
> > >     631 
> > > --> 632         legacy_iomap_table[bar] = NULL;
> > >                 ^^^^^^^^^^^^^^^^^^^^^^^
> > > Leading to a buffer overflow.
> 
> Leading to a *potential* buffer overflow.
> 

Smatch is doing cross function analysis in this case.  Smatch knows
that pcim_iounmap_regions() is fine but the bug is when this is
called from pcim_iomap_regions().

drivers/pci/devres.c | pcim_iounmap_regions | pcim_remove_bar_from_legacy_table | PARAM_VALUE | 1 | bar | 0-5
drivers/pci/devres.c |   pcim_iomap_regions | pcim_remove_bar_from_legacy_table | PARAM_VALUE | 1 | bar | 0-15

But, that raises a different question because you would expect
the map and unmap functions to loop over the same bars.

regards,
dan carpenter

Re: [bug report] PCI: Check BAR index for validity

[Bjorn Helgaas]

On Mon, Mar 10, 2025 at 08:54:54AM +0100, Philipp Stanner wrote:
> On Sat, 2025-03-08 at 15:07 -0600, Bjorn Helgaas wrote:
> > On Sat, Mar 08, 2025 at 01:23:28PM +0300, Dan Carpenter wrote:
> > > Hello Philipp Stanner,
> > > 
> > > Commit ba10e5011d05 ("PCI: Check BAR index for validity") from Mar
> > > 4,
> > > 2025 (linux-next), leads to the following Smatch static checker
> > > warning:
> > > 
> > > 	drivers/pci/devres.c:632
> > > pcim_remove_bar_from_legacy_table()
> > > 	error: buffer overflow 'legacy_iomap_table' 6 <= 15
> > 
> > Thanks, I dropped this patch for now.
> > 
> > > drivers/pci/devres.c
> > >     621 static void pcim_remove_bar_from_legacy_table(struct
> > > pci_dev *pdev, int bar)
> > >     622 {
> > >     623         void __iomem **legacy_iomap_table;
> > >     624 
> > >     625         if (!pci_bar_index_is_valid(bar))
> > > 
> > > This line used to check PCI_STD_NUM_BARS (6) but now it's checking
> > > PCI_NUM_RESOURCES (15).
> 
> What is even going on here. Why are thos different values? Does a PCI
> device now have at most 6, or 15 BARs?

You included the link below, so I guess you found the answer to this,
but PCI devices can have:

  - up to six normal BARs (pci_dev.resource[0-5])

  - a ROM BAR (pci_dev.resource[6])

  - up to six SR-IOV BARs (pci_dev.resource[7-12])

  - four windows (for bridges) (pci_dev.resource[13-16])

> Or is a BAR different from a "resource"?

Yes, as above.  Not all can be used at once, e.g., bridges can only
have two BARs and may not implement all four windows, and SR-IOV VFs
can't use the normal BARs at all.

> And why would it be 15? I haven't read the standard, but I would
> suspect it should be 16.

This is an implementation thing, not a PCI spec thing, but I count a
maximum of 17 resources if CONFIG_PCI_IOV is defined.

> And which of those two here should be used?
> https://elixir.bootlin.com/linux/v6.14-rc4/source/include/linux/pci.h#L133