[PATCH v1 1/1] PCI: Avoid potential out-of-bounds read in pci_dev_for_each_resource()

[PATCH v1 1/1] PCI: Avoid potential out-of-bounds read in pci_dev_for_each_resource()

[Andy Shevchenko]

Coverity complains that pointer in the pci_dev_for_each_resource()
may be wrong, i.e. mighe be used for the out-of-bounds read.

There is no actual issue right now, because we have another check
afterwards and the out-of-bounds read is not being performed. In any
case it's better code with this get fixed, hence the proposed change.

As Jonas pointed out "It probably makes the code slightly less
performant as res will now be checked for being not NULL (which will
always be true), but I doubt it will be significant (or in any hot
paths)."

Fixes: 09cc90063240 ("PCI: Introduce pci_dev_for_each_resource()")
Reported-by: Bjorn Helgaas <helgaas@kernel.org>
Closes: https://lore.kernel.org/r/20230509182122.GA1259567@bhelgaas
Suggested-by: Jonas Gorski <jonas.gorski@gmail.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/pci.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 60ca768bc867..19adad23a204 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2127,14 +2127,14 @@ int pci_iobar_pfn(struct pci_dev *pdev, int bar, struct vm_area_struct *vma);
 	(pci_resource_end((dev), (bar)) ? 				\
 	 resource_size(pci_resource_n((dev), (bar))) : 0)
 
-#define __pci_dev_for_each_res0(dev, res, ...)				\
-	for (unsigned int __b = 0;					\
-	     res = pci_resource_n(dev, __b), __b < PCI_NUM_RESOURCES;	\
+#define __pci_dev_for_each_res0(dev, res, ...)					\
+	for (unsigned int __b = 0;						\
+	     __b < PCI_NUM_RESOURCES && (res = pci_resource_n(dev, __b));	\
 	     __b++)
 
-#define __pci_dev_for_each_res1(dev, res, __b)				\
-	for (__b = 0;							\
-	     res = pci_resource_n(dev, __b), __b < PCI_NUM_RESOURCES;	\
+#define __pci_dev_for_each_res1(dev, res, __b)					\
+	for (__b = 0;								\
+	     __b < PCI_NUM_RESOURCES && (res = pci_resource_n(dev, __b));	\
 	     __b++)
 
 #define pci_dev_for_each_resource(dev, res, ...)			\
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v1 1/1] PCI: Avoid potential out-of-bounds read in pci_dev_for_each_resource()

[Bjorn Helgaas]

On Mon, Oct 30, 2023 at 01:42:18PM +0200, Andy Shevchenko wrote:
> Coverity complains that pointer in the pci_dev_for_each_resource()
> may be wrong, i.e. mighe be used for the out-of-bounds read.
> 
> There is no actual issue right now, because we have another check
> afterwards and the out-of-bounds read is not being performed. In any
> case it's better code with this get fixed, hence the proposed change.
> 
> As Jonas pointed out "It probably makes the code slightly less
> performant as res will now be checked for being not NULL (which will
> always be true), but I doubt it will be significant (or in any hot
> paths)."
> 
> Fixes: 09cc90063240 ("PCI: Introduce pci_dev_for_each_resource()")
> Reported-by: Bjorn Helgaas <helgaas@kernel.org>
> Closes: https://lore.kernel.org/r/20230509182122.GA1259567@bhelgaas
> Suggested-by: Jonas Gorski <jonas.gorski@gmail.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks, Andy, I'll look at this soon after v6.7-rc1 (probably Nov 12).

> ---
>  include/linux/pci.h | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 60ca768bc867..19adad23a204 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -2127,14 +2127,14 @@ int pci_iobar_pfn(struct pci_dev *pdev, int bar, struct vm_area_struct *vma);
>  	(pci_resource_end((dev), (bar)) ? 				\
>  	 resource_size(pci_resource_n((dev), (bar))) : 0)
>  
> -#define __pci_dev_for_each_res0(dev, res, ...)				\
> -	for (unsigned int __b = 0;					\
> -	     res = pci_resource_n(dev, __b), __b < PCI_NUM_RESOURCES;	\
> +#define __pci_dev_for_each_res0(dev, res, ...)					\
> +	for (unsigned int __b = 0;						\
> +	     __b < PCI_NUM_RESOURCES && (res = pci_resource_n(dev, __b));	\
>  	     __b++)
>  
> -#define __pci_dev_for_each_res1(dev, res, __b)				\
> -	for (__b = 0;							\
> -	     res = pci_resource_n(dev, __b), __b < PCI_NUM_RESOURCES;	\
> +#define __pci_dev_for_each_res1(dev, res, __b)					\
> +	for (__b = 0;								\
> +	     __b < PCI_NUM_RESOURCES && (res = pci_resource_n(dev, __b));	\
>  	     __b++)
>  
>  #define pci_dev_for_each_resource(dev, res, ...)			\
> -- 
> 2.40.0.1.gaa8946217a0b
> 

Re: [PATCH v1 1/1] PCI: Avoid potential out-of-bounds read in pci_dev_for_each_resource()

[Andy Shevchenko]

On Fri, Nov 03, 2023 at 01:46:14PM -0500, Bjorn Helgaas wrote:
> On Mon, Oct 30, 2023 at 01:42:18PM +0200, Andy Shevchenko wrote:
> > Coverity complains that pointer in the pci_dev_for_each_resource()
> > may be wrong, i.e. mighe be used for the out-of-bounds read.
> > 
> > There is no actual issue right now, because we have another check
> > afterwards and the out-of-bounds read is not being performed. In any
> > case it's better code with this get fixed, hence the proposed change.
> > 
> > As Jonas pointed out "It probably makes the code slightly less
> > performant as res will now be checked for being not NULL (which will
> > always be true), but I doubt it will be significant (or in any hot
> > paths)."
> > 
> > Fixes: 09cc90063240 ("PCI: Introduce pci_dev_for_each_resource()")
> > Reported-by: Bjorn Helgaas <helgaas@kernel.org>
> > Closes: https://lore.kernel.org/r/20230509182122.GA1259567@bhelgaas
> > Suggested-by: Jonas Gorski <jonas.gorski@gmail.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Thanks, Andy, I'll look at this soon after v6.7-rc1 (probably Nov 12).

Since it's anyway a non-critical issue, take your time. Based on the tags
it may be backported if needed, so business as usual.

Thank you!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] PCI: Avoid potential out-of-bounds read in pci_dev_for_each_resource()

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>


On Mon, 30 Oct 2023 13:42:18 +0200, Andy Shevchenko wrote:
> Coverity complains that pointer in the pci_dev_for_each_resource()
> may be wrong, i.e. mighe be used for the out-of-bounds read.
> 
> There is no actual issue right now, because we have another check
> afterwards and the out-of-bounds read is not being performed. In any
> case it's better code with this get fixed, hence the proposed change.
> 
> [...]

Applied to "resource" for v6.8, thanks!

[1/1] PCI: Avoid potential out-of-bounds read in pci_dev_for_each_resource()
      commit: 3171e46d677a668eed3086da78671f1e4f5b8405

Best regards,
-- 
Bjorn Helgaas <bhelgaas@google.com>