Re: [PATCH] PCI: Check BAR index for validity

[Philipp Stanner]

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@xxxxxxxxxxxxxxx>
> Closes:
> https://lore.kernel.org/all/adb53b1f-29e1-3d14-0e61-351fd2d3ff0d@xxxxxxxxxxxxxxx/
> Signed-off-by: Philipp Stanner <phasta@xxxxxxxxxx>
> ---
>  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);