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(-)
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;
+
+ 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);
--
2.48.1
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!
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
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
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;
© 2016 - 2026 Red Hat, Inc.