linux-kernel - Re: [PATCH v8 0/7] Add pci_dev_for_each

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOiHx=nTgtnfUqRDJR0yFP0du3Yvs73PkEUR_1eb+1gtbDBM-g@mail.gmail.com>
Date:   Thu, 1 Jun 2023 13:17:21 +0200
From:   Jonas Gorski <jonas.gorski@...il.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Krzysztof Wilczyński <kw@...ux.com>,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
        Mickaël Salaün <mic@...ikod.net>,
        Rich Felker <dalias@...c.org>, linux-sh@...r.kernel.org,
        Dominik Brodowski <linux@...inikbrodowski.net>,
        Andrew Lunn <andrew@...n.ch>, sparclinux@...r.kernel.org,
        Stefano Stabellini <sstabellini@...nel.org>,
        Yoshinori Sato <ysato@...rs.sourceforge.jp>,
        Gregory Clement <gregory.clement@...tlin.com>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        Russell King <linux@...linux.org.uk>,
        linux-acpi@...r.kernel.org, Miguel Ojeda <ojeda@...nel.org>,
        xen-devel@...ts.xenproject.org, Matt Turner <mattst88@...il.com>,
        Anatolij Gustschin <agust@...x.de>,
        Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
        Arnd Bergmann <arnd@...db.de>,
        Niklas Schnelle <schnelle@...ux.ibm.com>,
        Richard Henderson <richard.henderson@...aro.org>,
        Nicholas Piggin <npiggin@...il.com>,
        Ivan Kokshaysky <ink@...assic.park.msu.ru>,
        John Paul Adrian Glaubitz <glaubitz@...sik.fu-berlin.de>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        linux-arm-kernel@...ts.infradead.org,
        Juergen Gross <jgross@...e.com>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Philippe Mathieu-Daudé <philmd@...aro.org>,
        linuxppc-dev@...ts.ozlabs.org,
        Randy Dunlap <rdunlap@...radead.org>,
        linux-mips@...r.kernel.org,
        Oleksandr Tyshchenko <oleksandr_tyshchenko@...m.com>,
        linux-alpha@...r.kernel.org,
        Pali Rohár <pali@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        "Maciej W. Rozycki" <macro@...am.me.uk>
Subject: Re: [PATCH v8 0/7] Add pci_dev_for_each_resource() helper and update users

On Wed, 31 May 2023 at 23:30, Bjorn Helgaas <helgaas@...nel.org> wrote:
>
> On Wed, May 31, 2023 at 08:48:35PM +0200, Jonas Gorski wrote:
> > ...
>
> > Looking at the code I understand where coverity is coming from:
> >
> > #define __pci_dev_for_each_res0(dev, res, ...)                         \
> >        for (unsigned int __b = 0;                                      \
> >             res = pci_resource_n(dev, __b), __b < PCI_NUM_RESOURCES;   \
> >             __b++)
> >
> >  res will be assigned before __b is checked for being less than
> > PCI_NUM_RESOURCES, making it point to behind the array at the end of
> > the last loop iteration.
> >
> > Rewriting the test expression as
> >
> > __b < PCI_NUM_RESOURCES && (res = pci_resource_n(dev, __b));
> >
> > should avoid the (coverity) warning by making use of lazy evaluation.
> >
> > 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).
>
> Thanks a lot for looking into this!  I think you're right, and I think
> the rewritten expression is more logical as well.  Do you want to post
> a patch for it?

Not sure when I'll come around to, so I have no strong feeling here.
So feel free to just borrow my suggestion, especially since I won't be
able to test it (don't have a kernel tree ready I can build and boot).

Also looking more closely at the Coverity output, I think it might not
handle the comma operator well in the loop condition:

>          11. incr: Incrementing __b. The value of __b may now be up to 17.
>          12. alias: Assigning: r = &pdev->resource[__b]. r may now point to as high as element 17 of pdev->resource (which consists of 17 64-byte elements).
>          13. Condition __b < PCI_NUM_RESOURCES, taking true branch.
>          14. Condition (r = &pdev->resource[__b]) , (__b < PCI_NUM_RESOURCES), taking true branch.

13 If __b is 17 ( = PCI_NUM_RESOURCES) we wouldn't taking the true
branch, but somehow Coverity thinks that we do. No idea if it is worth
reporting to Coverity.

The changed condition statement should hopefully silence the warning though.

Regards
Jonas