| Message ID | 20250304143112.104190-2-phasta@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: Check BAR index for validity | expand |
On Tue, Mar 04, 2025 at 03:31:13PM +0100, Philipp Stanner wrote: > Many functions in PCI use accessor macros such as pci_resource_len(), > which take a BAR index. That index, however, is never checked for > validity, potentially resulting in undefined behavior by overflowing the > array pci_dev.resource in the macro pci_resource_n(). > > Since many users of those macros directly assign the accessed value to > an unsigned integer, the macros cannot be changed easily anymore to > return -EINVAL for invalid indexes. Consequently, the problem has to be > mitigated in higher layers. > > Add pci_bar_index_valid(). Use it where appropriate. > > Reported-by: Bingbu Cao <bingbu.cao@linux.intel.com> > Closes: https://lore.kernel.org/all/adb53b1f-29e1-3d14-0e61-351fd2d3ff0d@linux.intel.com/ > Signed-off-by: Philipp Stanner <phasta@kernel.org> Applied to pci/devres for v6.15, thanks. I reversed this: > +static inline bool pci_bar_index_is_valid(int bar) > +{ > + if (bar < 0 || bar >= PCI_NUM_RESOURCES) > + return false; > + > + return true; > +} so the test describes valid indexes, not invalid ones: if (bar >= 0 && bar < PCI_NUM_RESOURCES) return true; return false;
On Tue, 2025-03-04 at 15:31 +0100, Philipp Stanner wrote: > Many functions in PCI use accessor macros such as pci_resource_len(), > which take a BAR index. That index, however, is never checked for > validity, potentially resulting in undefined behavior by overflowing > the > array pci_dev.resource in the macro pci_resource_n(). > > Since many users of those macros directly assign the accessed value > to > an unsigned integer, the macros cannot be changed easily anymore to > return -EINVAL for invalid indexes. Consequently, the problem has to > be > mitigated in higher layers. > > Add pci_bar_index_valid(). Use it where appropriate. > > Reported-by: Bingbu Cao <bingbu.cao@linux.intel.com> > Closes: > https://lore.kernel.org/all/adb53b1f-29e1-3d14-0e61-351fd2d3ff0d@linux.intel.com/ > Signed-off-by: Philipp Stanner <phasta@kernel.org> > --- > drivers/pci/devres.c | 16 ++++++++++++++-- > drivers/pci/iomap.c | 30 +++++++++++++++++++++--------- > drivers/pci/pci.c | 6 ++++++ > drivers/pci/pci.h | 8 ++++++++ > 4 files changed, 49 insertions(+), 11 deletions(-) > > diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c > index 3431a7df3e0d..d2c09589c537 100644 > --- a/drivers/pci/devres.c > +++ b/drivers/pci/devres.c > @@ -577,7 +577,7 @@ static int > pcim_add_mapping_to_legacy_table(struct pci_dev *pdev, > { > void __iomem **legacy_iomap_table; > > - if (bar >= PCI_STD_NUM_BARS) > + if (!pci_bar_index_is_valid(bar)) > return -EINVAL; > > legacy_iomap_table = (void __iomem > **)pcim_iomap_table(pdev); > @@ -622,7 +622,7 @@ static void > pcim_remove_bar_from_legacy_table(struct pci_dev *pdev, int bar) > { > void __iomem **legacy_iomap_table; > > - if (bar >= PCI_STD_NUM_BARS) > + if (!pci_bar_index_is_valid(bar)) > return; > > legacy_iomap_table = (void __iomem > **)pcim_iomap_table(pdev); > @@ -655,6 +655,9 @@ void __iomem *pcim_iomap(struct pci_dev *pdev, > int bar, unsigned long maxlen) > void __iomem *mapping; > struct pcim_addr_devres *res; > > + if (!pci_bar_index_is_valid(bar)) > + return NULL; > + > res = pcim_addr_devres_alloc(pdev); > if (!res) > return NULL; > @@ -722,6 +725,9 @@ void __iomem *pcim_iomap_region(struct pci_dev > *pdev, int bar, > int ret; > struct pcim_addr_devres *res; > > + if (!pci_bar_index_is_valid(bar)) > + return IOMEM_ERR_PTR(-EINVAL); > + > res = pcim_addr_devres_alloc(pdev); > if (!res) > return IOMEM_ERR_PTR(-ENOMEM); > @@ -823,6 +829,9 @@ static int _pcim_request_region(struct pci_dev > *pdev, int bar, const char *name, > int ret; > struct pcim_addr_devres *res; > > + if (!pci_bar_index_is_valid(bar)) > + return -EINVAL; > + > res = pcim_addr_devres_alloc(pdev); > if (!res) > return -ENOMEM; > @@ -991,6 +1000,9 @@ void __iomem *pcim_iomap_range(struct pci_dev > *pdev, int bar, > void __iomem *mapping; > struct pcim_addr_devres *res; > > + if (!pci_bar_index_is_valid(bar)) > + return IOMEM_ERR_PTR(-EINVAL); > + > res = pcim_addr_devres_alloc(pdev); > if (!res) > return IOMEM_ERR_PTR(-ENOMEM); > diff --git a/drivers/pci/iomap.c b/drivers/pci/iomap.c > index 9fb7cacc15cd..0cab82cbcc99 100644 > --- a/drivers/pci/iomap.c > +++ b/drivers/pci/iomap.c > @@ -9,6 +9,8 @@ > > #include <linux/export.h> > > +#include "pci.h" /* for pci_bar_index_is_valid() */ > + > /** > * pci_iomap_range - create a virtual mapping cookie for a PCI BAR > * @dev: PCI device that owns the BAR > @@ -33,12 +35,18 @@ void __iomem *pci_iomap_range(struct pci_dev > *dev, > unsigned long offset, > unsigned long maxlen) > { > - resource_size_t start = pci_resource_start(dev, bar); > - resource_size_t len = pci_resource_len(dev, bar); > - unsigned long flags = pci_resource_flags(dev, bar); > + resource_size_t start, len; > + unsigned long flags; > > + if (!pci_bar_index_is_valid(bar)) > + return NULL; > if (len <= offset || !start) > return NULL; Argh, damn, this is a bug of course. 'start' is undefined at this point, as is 'len'. > + > + start = pci_resource_start(dev, bar); > + len = pci_resource_len(dev, bar); > + flags = pci_resource_flags(dev, bar); > + > len -= offset; > start += offset; > if (maxlen && len > maxlen) > @@ -77,17 +85,21 @@ void __iomem *pci_iomap_wc_range(struct pci_dev > *dev, > unsigned long offset, > unsigned long maxlen) > { > - resource_size_t start = pci_resource_start(dev, bar); > - resource_size_t len = pci_resource_len(dev, bar); > - unsigned long flags = pci_resource_flags(dev, bar); > - > + resource_size_t start, len; > + unsigned long flags; > > - if (flags & IORESOURCE_IO) > + if (!pci_bar_index_is_valid(bar)) > return NULL; > - > if (len <= offset || !start) > return NULL; Same here. Shall I send a v2 or do you want to rebase, Bjorn? sry bout that. P. > > + start = pci_resource_start(dev, bar); > + len = pci_resource_len(dev, bar); > + flags = pci_resource_flags(dev, bar); > + > + if (flags & IORESOURCE_IO) > + return NULL; > + > len -= offset; > start += offset; > if (maxlen && len > maxlen) > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 869d204a70a3..da82d734d09c 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -3921,6 +3921,9 @@ EXPORT_SYMBOL(pci_enable_atomic_ops_to_root); > */ > void pci_release_region(struct pci_dev *pdev, int bar) > { > + if (!pci_bar_index_is_valid(bar)) > + return; > + > /* > * This is done for backwards compatibility, because the old > PCI devres > * API had a mode in which the function became managed if it > had been > @@ -3965,6 +3968,9 @@ EXPORT_SYMBOL(pci_release_region); > static int __pci_request_region(struct pci_dev *pdev, int bar, > const char *name, int exclusive) > { > + if (!pci_bar_index_is_valid(bar)) > + return -EINVAL; > + > if (pci_is_managed(pdev)) { > if (exclusive == IORESOURCE_EXCLUSIVE) > return pcim_request_region_exclusive(pdev, > bar, name); > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index 01e51db8d285..ae8b2f5c118b 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -167,6 +167,14 @@ static inline void pci_wakeup_event(struct > pci_dev *dev) > pm_wakeup_event(&dev->dev, 100); > } > > +static inline bool pci_bar_index_is_valid(int bar) > +{ > + if (bar < 0 || bar >= PCI_NUM_RESOURCES) > + return false; > + > + return true; > +} > + > static inline bool pci_has_subordinate(struct pci_dev *pci_dev) > { > return !!(pci_dev->subordinate);
Hello, [...] > Shall I send a v2 or do you want to rebase, Bjorn? If the changes are relatively small, then you can send a patch top be applied atop the existing branch. I will squash things as needed. Otherwise, a v2. > sry bout that. No worries. Things happen. We can fix everything. :) Krzysztof
Hello,
kernel test robot noticed "last_state.load_disk_fail" on:
commit: 21ad75d75a0d6b1a30159d17f355120912a92a46 ("[PATCH] PCI: Check BAR index for validity")
url: https://github.com/intel-lab-lkp/linux/commits/Philipp-Stanner/PCI-Check-BAR-index-for-validity/20250304-224213
base: https://git.kernel.org/cgit/linux/kernel/git/pci/pci.git next
patch link: https://lore.kernel.org/all/20250304143112.104190-2-phasta@kernel.org/
patch subject: [PATCH] PCI: Check BAR index for validity
in testcase: hwsim
version: hwsim-x86_64-53303bb3e-1_20250303
with following parameters:
test: group-35
config: x86_64-rhel-9.4-func
compiler: gcc-12
test machine: 8 threads 1 sockets Intel(R) Core(TM) i7-4790 v3 @ 3.60GHz (Haswell) with 6G memory
(please refer to attached dmesg/kmsg for entire log/backtrace)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202503101027.bf280c9-lkp@intel.com
The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20250310/202503101027.bf280c9-lkp@intel.com
by this commit, we noticed persistent error while loading disks:
[ 50.323405][ T314]
LKP: ttyS0: 286: can't load the [ 183.977924][ T314] LKP: stdout: 286: can't load the disk /dev/disk/by-id/ata-INTEL_SSDSC2BB800G4_PHWL4171000P800RGN-part2, skip testing...
which doesn't show on parent 12c8c13635385a which bot chose as base for this
commit:
* 21ad75d75a0d6b (linux-review/Philipp-Stanner/PCI-Check-BAR-index-for-validity/20250304-224213) PCI: Check BAR index for validity
* 12c8c13635385a Merge branch 'pci/misc'
if need a different base, please let us know. Thanks!
diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c index 3431a7df3e0d..d2c09589c537 100644 --- a/drivers/pci/devres.c +++ b/drivers/pci/devres.c @@ -577,7 +577,7 @@ static int pcim_add_mapping_to_legacy_table(struct pci_dev *pdev, { void __iomem **legacy_iomap_table; - if (bar >= PCI_STD_NUM_BARS) + if (!pci_bar_index_is_valid(bar)) return -EINVAL; legacy_iomap_table = (void __iomem **)pcim_iomap_table(pdev); @@ -622,7 +622,7 @@ static void pcim_remove_bar_from_legacy_table(struct pci_dev *pdev, int bar) { void __iomem **legacy_iomap_table; - if (bar >= PCI_STD_NUM_BARS) + if (!pci_bar_index_is_valid(bar)) return; legacy_iomap_table = (void __iomem **)pcim_iomap_table(pdev); @@ -655,6 +655,9 @@ void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, unsigned long maxlen) void __iomem *mapping; struct pcim_addr_devres *res; + if (!pci_bar_index_is_valid(bar)) + return NULL; + res = pcim_addr_devres_alloc(pdev); if (!res) return NULL; @@ -722,6 +725,9 @@ void __iomem *pcim_iomap_region(struct pci_dev *pdev, int bar, int ret; struct pcim_addr_devres *res; + if (!pci_bar_index_is_valid(bar)) + return IOMEM_ERR_PTR(-EINVAL); + res = pcim_addr_devres_alloc(pdev); if (!res) return IOMEM_ERR_PTR(-ENOMEM); @@ -823,6 +829,9 @@ static int _pcim_request_region(struct pci_dev *pdev, int bar, const char *name, int ret; struct pcim_addr_devres *res; + if (!pci_bar_index_is_valid(bar)) + return -EINVAL; + res = pcim_addr_devres_alloc(pdev); if (!res) return -ENOMEM; @@ -991,6 +1000,9 @@ void __iomem *pcim_iomap_range(struct pci_dev *pdev, int bar, void __iomem *mapping; struct pcim_addr_devres *res; + if (!pci_bar_index_is_valid(bar)) + return IOMEM_ERR_PTR(-EINVAL); + res = pcim_addr_devres_alloc(pdev); if (!res) return IOMEM_ERR_PTR(-ENOMEM); diff --git a/drivers/pci/iomap.c b/drivers/pci/iomap.c index 9fb7cacc15cd..0cab82cbcc99 100644 --- a/drivers/pci/iomap.c +++ b/drivers/pci/iomap.c @@ -9,6 +9,8 @@ #include <linux/export.h> +#include "pci.h" /* for pci_bar_index_is_valid() */ + /** * pci_iomap_range - create a virtual mapping cookie for a PCI BAR * @dev: PCI device that owns the BAR @@ -33,12 +35,18 @@ void __iomem *pci_iomap_range(struct pci_dev *dev, unsigned long offset, unsigned long maxlen) { - resource_size_t start = pci_resource_start(dev, bar); - resource_size_t len = pci_resource_len(dev, bar); - unsigned long flags = pci_resource_flags(dev, bar); + resource_size_t start, len; + unsigned long flags; + if (!pci_bar_index_is_valid(bar)) + return NULL; if (len <= offset || !start) return NULL; + + start = pci_resource_start(dev, bar); + len = pci_resource_len(dev, bar); + flags = pci_resource_flags(dev, bar); + len -= offset; start += offset; if (maxlen && len > maxlen) @@ -77,17 +85,21 @@ void __iomem *pci_iomap_wc_range(struct pci_dev *dev, unsigned long offset, unsigned long maxlen) { - resource_size_t start = pci_resource_start(dev, bar); - resource_size_t len = pci_resource_len(dev, bar); - unsigned long flags = pci_resource_flags(dev, bar); - + resource_size_t start, len; + unsigned long flags; - if (flags & IORESOURCE_IO) + if (!pci_bar_index_is_valid(bar)) return NULL; - if (len <= offset || !start) return NULL; + start = pci_resource_start(dev, bar); + len = pci_resource_len(dev, bar); + flags = pci_resource_flags(dev, bar); + + if (flags & IORESOURCE_IO) + return NULL; + len -= offset; start += offset; if (maxlen && len > maxlen) diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 869d204a70a3..da82d734d09c 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -3921,6 +3921,9 @@ EXPORT_SYMBOL(pci_enable_atomic_ops_to_root); */ void pci_release_region(struct pci_dev *pdev, int bar) { + if (!pci_bar_index_is_valid(bar)) + return; + /* * This is done for backwards compatibility, because the old PCI devres * API had a mode in which the function became managed if it had been @@ -3965,6 +3968,9 @@ EXPORT_SYMBOL(pci_release_region); static int __pci_request_region(struct pci_dev *pdev, int bar, const char *name, int exclusive) { + if (!pci_bar_index_is_valid(bar)) + return -EINVAL; + if (pci_is_managed(pdev)) { if (exclusive == IORESOURCE_EXCLUSIVE) return pcim_request_region_exclusive(pdev, bar, name); diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 01e51db8d285..ae8b2f5c118b 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -167,6 +167,14 @@ static inline void pci_wakeup_event(struct pci_dev *dev) pm_wakeup_event(&dev->dev, 100); } +static inline bool pci_bar_index_is_valid(int bar) +{ + if (bar < 0 || bar >= PCI_NUM_RESOURCES) + return false; + + return true; +} + static inline bool pci_has_subordinate(struct pci_dev *pci_dev) { return !!(pci_dev->subordinate);
Many functions in PCI use accessor macros such as pci_resource_len(), which take a BAR index. That index, however, is never checked for validity, potentially resulting in undefined behavior by overflowing the array pci_dev.resource in the macro pci_resource_n(). Since many users of those macros directly assign the accessed value to an unsigned integer, the macros cannot be changed easily anymore to return -EINVAL for invalid indexes. Consequently, the problem has to be mitigated in higher layers. Add pci_bar_index_valid(). Use it where appropriate. Reported-by: Bingbu Cao <bingbu.cao@linux.intel.com> Closes: https://lore.kernel.org/all/adb53b1f-29e1-3d14-0e61-351fd2d3ff0d@linux.intel.com/ Signed-off-by: Philipp Stanner <phasta@kernel.org> --- drivers/pci/devres.c | 16 ++++++++++++++-- drivers/pci/iomap.c | 30 +++++++++++++++++++++--------- drivers/pci/pci.c | 6 ++++++ drivers/pci/pci.h | 8 ++++++++ 4 files changed, 49 insertions(+), 11 deletions(-)