netdev - Re: [Kgdb-bugreport] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr

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]

Date:   Thu, 3 Mar 2022 12:18:24 +0000
From:   Daniel Thompson <daniel.thompson@...aro.org>
To:     Xiaomeng Tong <xiam0nd.tong@...il.com>
Cc:     david.laight@...lab.com, alsa-devel@...a-project.org,
        kvm@...r.kernel.org, gustavo@...eddedor.com,
        linux-iio@...r.kernel.org, kgdb-bugreport@...ts.sourceforge.net,
        linux@...musvillemoes.dk, dri-devel@...ts.freedesktop.org,
        c.giuffrida@...nl, amd-gfx@...ts.freedesktop.org,
        torvalds@...ux-foundation.org, samba-technical@...ts.samba.org,
        linux1394-devel@...ts.sourceforge.net, drbd-dev@...ts.linbit.com,
        linux-arch@...r.kernel.org, linux-cifs@...r.kernel.org,
        linux-aspeed@...ts.ozlabs.org, linux-scsi@...r.kernel.org,
        linux-rdma@...r.kernel.org, linux-staging@...ts.linux.dev,
        h.j.bos@...nl, jgg@...pe.ca, intel-wired-lan@...ts.osuosl.org,
        nouveau@...ts.freedesktop.org,
        bcm-kernel-feedback-list@...adcom.com, dan.carpenter@...cle.com,
        linux-media@...r.kernel.org, keescook@...omium.org, arnd@...db.de,
        linux-pm@...r.kernel.org, intel-gfx@...ts.freedesktop.org,
        bjohannesmeyer@...il.com, linux-block@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, christophe.jaillet@...adoo.fr,
        jakobkoschel@...il.com, v9fs-developer@...ts.sourceforge.net,
        linux-tegra@...r.kernel.org, tglx@...utronix.de,
        andriy.shevchenko@...ux.intel.com,
        linux-arm-kernel@...ts.infradead.org, linux-sgx@...r.kernel.org,
        nathan@...nel.org, netdev@...r.kernel.org,
        linux-usb@...r.kernel.org, linux-wireless@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        linux-f2fs-devel@...ts.sourceforge.net,
        tipc-discussion@...ts.sourceforge.net,
        linux-crypto@...r.kernel.org, dmaengine@...r.kernel.org,
        linux-mediatek@...ts.infradead.org, akpm@...ux-foundation.org,
        linuxppc-dev@...ts.ozlabs.org, christian.koenig@....com,
        rppt@...nel.org
Subject: Re: [Kgdb-bugreport] [PATCH 2/6] treewide: remove using list
 iterator after loop body as a ptr

On Thu, Mar 03, 2022 at 03:26:57PM +0800, Xiaomeng Tong wrote:
> On Thu, 3 Mar 2022 04:58:23 +0000, David Laight wrote:
> > on 3 Mar 2022 10:27:29 +0800, Xiaomeng Tong wrote:
> > > The problem is the mis-use of iterator outside the loop on exit, and
> > > the iterator will be the HEAD's container_of pointer which pointers
> > > to a type-confused struct. Sidenote: The *mis-use* here refers to
> > > mistakely access to other members of the struct, instead of the
> > > list_head member which acutally is the valid HEAD.
> >
> > The problem is that the HEAD's container_of pointer should never
> > be calculated at all.
> > This is what is fundamentally broken about the current definition.
> 
> Yes, the rule is "the HEAD's container_of pointer should never be
> calculated at all outside the loop", but how do you make sure everyone
> follows this rule?

Your formulation of the rule is correct: never run container_of() on HEAD
pointer.

However the rule that is introduced by list_for_each_entry_inside() is
*not* this rule. The rule it introduces is: never access the iterator
variable outside the loop.

Making the iterator NULL on loop exit does follow the rule you proposed
but using a different technique: do not allow HEAD to be stored in the
iterator variable after loop exit. This also makes it impossible to run
container_of() on the HEAD pointer.


> Everyone makes mistakes, but we can eliminate them all from the beginning
> with the help of compiler which can catch such use-after-loop things.

Indeed but if we introduce new interfaces then we don't have to worry
about existing usages and silent regressions. Code will have been
written knowing the loop can exit with the iterator set to NULL.

Sure it is still possible for programmers to make mistakes and
dereference the NULL pointer but C programmers are well training w.r.t.
NULL pointer checking so such mistakes are much less likely than with
the current list_for_each_entry() macro. This risk must be offset
against the way a NULLify approach can lead to more elegant code when we
are doing a list search.


Daniel.