netdev - Re: [PATCH v2] net: ethernet: Drop unnecessary continue

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] [day] [month] [year] [list]

Message-Id: <20180307.103901.1771176275743006915.davem@davemloft.net>
Date:   Wed, 07 Mar 2018 10:39:01 -0500 (EST)
From:   David Miller <davem@...emloft.net>
To:     arushisinghal19971997@...il.com
Cc:     jdmason@...zu.us, jakub.kicinski@...ronome.com,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        oss-drivers@...ronome.com, outreachy-kernel@...glegroups.com
Subject: Re: [PATCH v2] net: ethernet: Drop unnecessary continue

From: Arushi Singhal <arushisinghal19971997@...il.com>
Date: Sat, 3 Mar 2018 21:44:56 +0530

> Continue at the bottom of a loop are removed.
> Issue found using drop_continue.cocci Coccinelle script.
> 
> Signed-off-by: Arushi Singhal <arushisinghal19971997@...il.com>
> ---
> Changes in v2:
> - Braces is dropped from if with single statement.

Sorry there is no way I am applying this.

Just blindly removing continue statements that some static checker
or compiler warning mentions is completely unacceptable.

Actually _LOOK_ at this code:

>  		if(cards[i].vendor_id) {
>  			for(j=0;j<3;j++)
> -				if(inb(ioaddr+cards[i].addr_offset+j) != cards[i].vendor_id[j]) {
> +				if(inb(ioaddr+cards[i].addr_offset+j) != cards[i].vendor_id[j])
>  					release_region(ioaddr, cards[i].total_size);
> -					continue;
> -			  }
>  		}

The extraneous continue statement is the _least_ of the problems here.

This code, if the if() statement triggers, will release the ioaddr
region and then _CONTINUE_ the for(j) loop, and try to access the
registers using the same 'ioaddr' again.

That's actually a _REAL_ bug, and you're making it seem like
the behavior is intentional by editing it like this.

And the bug probably exists because this entire sequence has
misaligned closing curly braces.  It makes it look like
the continue is for the outer loop, which would be fine.

But it's not.  It's for the inner loop, and this causes the "use
ioaddr after releasing it" bug.

Please do not submit patches like this one, it makes for a lot of
wasted auditing and review for people.  The onus is on you to read
and understand the code you are editing before submitting your
changes.

Thank you.