linux-kernel - Re: add memset checks to checkpatch.pl

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: <1282174721.6724.202.camel@Joe-Laptop>
Date:	Wed, 18 Aug 2010 16:38:41 -0700
From:	Joe Perches <joe@...ches.com>
To:	Dave Jones <davej@...hat.com>
Cc:	apw@...onical.com, Linux Kernel <linux-kernel@...r.kernel.org>
Subject: Re: add memset checks to checkpatch.pl

On Wed, 2010-08-18 at 18:17 -0400, Dave Jones wrote:
> On Wed, Aug 18, 2010 at 02:57:59PM -0700, Joe Perches wrote:
>  > > I'm all for improving my shoddy perl where possible, but this doesn't seem to actually
>  > > catch any of the test cases I wrote.  (it's also missing a } )
>  > I intend never to be a perl monk.
>  > I notice the missing { before the elsif after I sent it.
>  > Oh well.
>  > I just typed it in the emailer, so it's not tested at all.
>  > Also it's missing a $ after 1 in the second $memset_size test.
> still didn't catch anything for me.
>  > What are your test cases anyway?
> 
> memset(foo, 0, 10);
> memset(foo, 10, 0);
> memset(foo, 1, 10);
> memset(foo, 10, 1);
> 
>  > Likely $Lval isn't matching things like
>  > sizeof(*foo) so this isn't checked:
>  > 
>  > 	memset(foo, bar, sizeof(*foo));
> 
> I chose to just ignore any non integer arguments to keep things simple.

This seems to work.

I think $FuncArg is not great though.

Because $FuncArg uses $match_balanced_parens, the
match list args are unexpected.

$2, $4, and $6 are the args of any memset argument
that uses style func(args)

ie: memset(addr(foo), val(bar), sizeof(*foo))

Andy, what do you think?

Not-signed-off-by: Joe Perches

---
 scripts/checkpatch.pl |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2039acd..af7a4f9 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -172,6 +172,10 @@ our $Operators	= qr{
 			&&|\|\||,|\^|\+\+|--|&|\||\+|-|\*|\/|%
 		  }x;
 
+our $match_balanced_parens = qr/(\((?:[^\(\)]++|(?-1))*\))/;
+our $Function	= qr{(?:$Ident\s*$match_balanced_parens)};
+our $FuncArg	= qr{(?:$Lval|$Constant|$Function)};
+
 our $NonptrType;
 our $Type;
 our $Declare;
@@ -2655,6 +2659,20 @@ sub process {
 			WARN("sizeof(& should be avoided\n" . $herecurr);
 		}
 
+# Check for misused memsets
+
+		if ($line =~ /^\+.*\bmemset\s*\(\s*($FuncArg)\s*,\s*($FuncArg)\s*,\s*($FuncArg)\s*\)/) {
+			my $memset_addr = $1;
+			my $memset_val = $3;
+			my $memset_size = $5;
+
+			if ($memset_size =~ /^(0x|)0$/i) {
+				ERROR("memset uses second argument as constant byte value, not third.\n" . $herecurr);
+			} elsif ($memset_size =~ /^(0x|)1$/i) {
+				WARN("single byte memset is suspicious. Swapped 2nd/3rd argument?\n" . $herecurr);
+			}
+		}
+
 # check for new externs in .c files.
 		if ($realfile =~ /\.c$/ && defined $stat &&
 		    $stat =~ /^.\s*(?:extern\s+)?$Type\s+($Ident)(\s*)\(/s)


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/