| Message ID | 20231030114218.2752236-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v1,1/1] PCI: Avoid potential out-of-bounds read in pci_dev_for_each_resource() | expand |
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 >
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!
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,
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, ...) \
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(-)