linux-kernel - Re: [RFC PATCH 07/13] udp_tunnel: remove the usage of the list iterator after the loop

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: <20220224062022.GH3943@kadam>
Date:   Thu, 24 Feb 2022 09:20:22 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc:     Jakob Koschel <jakobkoschel@...il.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Arnd Bergman <arnd@...db.de>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Kees Cook <keescook@...omium.org>,
        Mike Rapoport <rppt@...nel.org>,
        "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
        Brian Johannesmeyer <bjohannesmeyer@...il.com>,
        Cristiano Giuffrida <c.giuffrida@...nl>,
        "Bos, H.J." <h.j.bos@...nl>
Subject: Re: [RFC PATCH 07/13] udp_tunnel: remove the usage of the list
 iterator after the loop

On Wed, Feb 23, 2022 at 09:00:36PM +0100, Christophe JAILLET wrote:
> Le 17/02/2022 à 19:48, Jakob Koschel a écrit :
> > The usage of node->dev after the loop body is a legitimate type
> > confusion if the break was not hit. It will compare an undefined
> > memory location with dev that could potentially be equal. The value
> > of node->dev in this case could either be a random struct member of the
> > head element or an out-of-bounds value.
> > 
> > Therefore it is more safe to use the found variable. With the
> > introduction of speculative safe list iterator this check could be
> > replaced with if (!node).
> > 
> > Signed-off-by: Jakob Koschel <jakobkoschel@...il.com>
> > ---
> >   net/ipv4/udp_tunnel_nic.c | 7 +++++--
> >   1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/ipv4/udp_tunnel_nic.c b/net/ipv4/udp_tunnel_nic.c
> > index b91003538d87..c47f9fb36d29 100644
> > --- a/net/ipv4/udp_tunnel_nic.c
> > +++ b/net/ipv4/udp_tunnel_nic.c
> > @@ -842,11 +842,14 @@ udp_tunnel_nic_unregister(struct net_device *dev, struct udp_tunnel_nic *utn)
> >   	 */
> >   	if (info->shared) {
> >   		struct udp_tunnel_nic_shared_node *node, *first;
> > +		bool found = false;
> >   		list_for_each_entry(node, &info->shared->devices, list)
> > -			if (node->dev == dev)
> > +			if (node->dev == dev) {
> > +				found = true;
> >   				break;
> > -		if (node->dev != dev)
> > +			}
> > +		if (!found)
> >   			return;
> >   		list_del(&node->list);
> 
> Hi,
> 
> just in case, see Dan Carpeter's patch for the same issue with another fix
> at:
> https://lore.kernel.org/kernel-janitors/20220222134251.GA2271@kili/

Yeah.  My patch was already applied.

I've had an unpublished Smatch check for this for a while but I've been
re-writing it recently to make it more generic so that it worked for
all the different list_for_each type macros.  I'm going to publish it
soon.

Of course, all the real bugs are fixed so the remaining warnings are
false positives.

regards,
dan carpenter