[PATCH v3 0/4] checkpatch: warn for use of %px

[PATCH v3 0/4] checkpatch: warn for use of %px

[Tobin C. Harding]

Hi Andrew,

This is a resurrection of a patch set from last December.  There was
some confusion (on my behalf) as to how patches to checkpatch got into
the mainline.  Are you willing (and able) to take patches to
checkpatch.pl?

Patch 1 through 3 are cleanup/refactoring patches.

Patch 3 makes checkpatch emit a warning for usage of specifier %px.

You may remember that the initial idea for this was from yourself, v1
requested permission to use 'Suggested-by' tag.  I didn't get comment on
that so v2 removed the tag.  (I'm not totally across when one should add
the 'Suggested-by' tag.)


thanks,
Tobin.


v3:
 - rebase onto 4.16-rc3
 - separate 'remove unused variable' into it's own patch

v2:
 - change new sub name stat_real() -> get_stat_real()
 - add new sub get_stat_here()
 - move the addition of new sub routines into separate patches
 - add 'Acked-by' tag for Joe
 - remove 'Suggested-by' tag for Andrew Morton

Tobin C. Harding (4):
  checkpatch: add sub routine get_stat_real()
  checkpatch: remove unused variable declarations
  checkpatch: add sub routine get_stat_here()
  checkpatch: warn for use of %px

 scripts/checkpatch.pl | 113 ++++++++++++++++++++++++++------------------------
 1 file changed, 58 insertions(+), 55 deletions(-)

-- 
2.7.4

[PATCH v3 1/4] checkpatch: add sub routine get_stat_real()

[Tobin C. Harding]

checkpatch currently contains duplicate code. We can define a sub
routine and call that instead. This reduces code duplication and line
count.

Add subroutine get_stat_real()

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 scripts/checkpatch.pl | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 3d4040322ae1..235a02f4f4e1 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1644,6 +1644,17 @@ sub raw_line {
 	return $line;
 }
 
+sub get_stat_real {
+	my ($linenr, $lc) = @_;
+
+	my $stat_real = raw_line($linenr, 0);
+	for (my $count = $linenr + 1; $count <= $lc; $count++) {
+		$stat_real = $stat_real . "\n" . raw_line($count, 0);
+	}
+
+	return $stat_real;
+}
+
 sub cat_vet {
 	my ($vet) = @_;
 	my ($res, $coded);
@@ -5806,17 +5817,15 @@ sub process {
 				}
 			}
 			if ($bad_extension ne "") {
-				my $stat_real = raw_line($linenr, 0);
+				my $stat_real = get_stat_real($linenr, $lc);
 				my $ext_type = "Invalid";
 				my $use = "";
-				for (my $count = $linenr + 1; $count <= $lc; $count++) {
-					$stat_real = $stat_real . "\n" . raw_line($count, 0);
-				}
 				if ($bad_extension =~ /p[Ff]/) {
 					$ext_type = "Deprecated";
 					$use = " - use %pS instead";
 					$use =~ s/pS/ps/ if ($bad_extension =~ /pf/);
 				}
+
 				WARN("VSPRINTF_POINTER_EXTENSION",
 				     "$ext_type vsprintf pointer extension '$bad_extension'$use\n" . "$here\n$stat_real\n");
 			}
@@ -5931,10 +5940,7 @@ sub process {
 		     $stat !~ /(?:$Compare)\s*\bsscanf\s*$balanced_parens/)) {
 			my $lc = $stat =~ tr@\n@@;
 			$lc = $lc + $linenr;
-			my $stat_real = raw_line($linenr, 0);
-		        for (my $count = $linenr + 1; $count <= $lc; $count++) {
-				$stat_real = $stat_real . "\n" . raw_line($count, 0);
-			}
+			my $stat_real = get_stat_real($linenr, $lc);
 			WARN("NAKED_SSCANF",
 			     "unchecked sscanf return value\n" . "$here\n$stat_real\n");
 		}
@@ -5945,10 +5951,7 @@ sub process {
 		    $line =~ /\bsscanf\b/) {
 			my $lc = $stat =~ tr@\n@@;
 			$lc = $lc + $linenr;
-			my $stat_real = raw_line($linenr, 0);
-		        for (my $count = $linenr + 1; $count <= $lc; $count++) {
-				$stat_real = $stat_real . "\n" . raw_line($count, 0);
-			}
+			my $stat_real = get_stat_real($linenr, $lc);
 			if ($stat_real =~ /\bsscanf\b\s*\(\s*$FuncArg\s*,\s*("[^"]+")/) {
 				my $format = $6;
 				my $count = $format =~ tr@%@%@;
@@ -6382,10 +6385,7 @@ sub process {
 
 				my $lc = $stat =~ tr@\n@@;
 				$lc = $lc + $linenr;
-				my $stat_real = raw_line($linenr, 0);
-				for (my $count = $linenr + 1; $count <= $lc; $count++) {
-					$stat_real = $stat_real . "\n" . raw_line($count, 0);
-				}
+				my $stat_real = get_stat_real($linenr, $lc);
 
 				my $skip_args = "";
 				if ($arg_pos > 1) {
-- 
2.7.4

[PATCH v3 2/4] checkpatch: remove unused variable declarations

[Tobin C. Harding]

Variables are declared and not used, we should remove them.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 scripts/checkpatch.pl | 2 --
 1 file changed, 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 235a02f4f4e1..d505fa4ec20c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6081,7 +6081,6 @@ sub process {
 			}
 			if ($r1 !~ /^sizeof\b/ && $r2 =~ /^sizeof\s*\S/ &&
 			    !($r1 =~ /^$Constant$/ || $r1 =~ /^[A-Z_][A-Z0-9_]*$/)) {
-				my $ctx = '';
 				my $herectx = $here . "\n";
 				my $cnt = statement_rawlines($stat);
 				for (my $n = 0; $n < $cnt; $n++) {
@@ -6169,7 +6168,6 @@ sub process {
 		if ($^V && $^V ge 5.10.0 &&
 		    defined $stat &&
 		    $stat =~ /^\+[$;\s]*(?:case[$;\s]+\w+[$;\s]*:[$;\s]*|)*[$;\s]*\bdefault[$;\s]*:[$;\s]*;/g) {
-			my $ctx = '';
 			my $herectx = $here . "\n";
 			my $cnt = statement_rawlines($stat);
 			for (my $n = 0; $n < $cnt; $n++) {
-- 
2.7.4

[PATCH v3 3/4] checkpatch: add sub routine get_stat_here()

[Tobin C. Harding]

checkpatch currently contains duplicate code. We can define a sub
routine and call that instead. This reduces code duplication and line
count.

Add subroutine get_stat_here()

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 scripts/checkpatch.pl | 52 ++++++++++++++++++++-------------------------------
 1 file changed, 20 insertions(+), 32 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d505fa4ec20c..679a10298cb5 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1655,6 +1655,17 @@ sub get_stat_real {
 	return $stat_real;
 }
 
+sub get_stat_here {
+	my ($linenr, $cnt, $here) = @_;
+
+	my $herectx = $here . "\n";
+	for (my $n = 0; $n < $cnt; $n++) {
+		$herectx .= raw_line($linenr, $n) . "\n";
+	}
+
+	return $herectx;
+}
+
 sub cat_vet {
 	my ($vet) = @_;
 	my ($res, $coded);
@@ -4952,12 +4963,8 @@ sub process {
 			#print "REST<$rest> dstat<$dstat> ctx<$ctx>\n";
 
 			$ctx =~ s/\n*$//;
-			my $herectx = $here . "\n";
 			my $stmt_cnt = statement_rawlines($ctx);
-
-			for (my $n = 0; $n < $stmt_cnt; $n++) {
-				$herectx .= raw_line($linenr, $n) . "\n";
-			}
+			my $herectx = get_stat_here($linenr, $stmt_cnt, $here);
 
 			if ($dstat ne '' &&
 			    $dstat !~ /^(?:$Ident|-?$Constant),$/ &&			# 10, // foo(),
@@ -5029,12 +5036,9 @@ sub process {
 # check for macros with flow control, but without ## concatenation
 # ## concatenation is commonly a macro that defines a function so ignore those
 			if ($has_flow_statement && !$has_arg_concat) {
-				my $herectx = $here . "\n";
 				my $cnt = statement_rawlines($ctx);
+				my $herectx = get_stat_here($linenr, $cnt, $here);
 
-				for (my $n = 0; $n < $cnt; $n++) {
-					$herectx .= raw_line($linenr, $n) . "\n";
-				}
 				WARN("MACRO_WITH_FLOW_CONTROL",
 				     "Macros with flow control statements should be avoided\n" . "$herectx");
 			}
@@ -5074,11 +5078,7 @@ sub process {
 
 				$ctx =~ s/\n*$//;
 				my $cnt = statement_rawlines($ctx);
-				my $herectx = $here . "\n";
-
-				for (my $n = 0; $n < $cnt; $n++) {
-					$herectx .= raw_line($linenr, $n) . "\n";
-				}
+				my $herectx = get_stat_here($linenr, $cnt, $here);
 
 				if (($stmts =~ tr/;/;/) == 1 &&
 				    $stmts !~ /^\s*(if|while|for|switch)\b/) {
@@ -5092,11 +5092,7 @@ sub process {
 			} elsif ($dstat =~ /^\+\s*#\s*define\s+$Ident.*;\s*$/) {
 				$ctx =~ s/\n*$//;
 				my $cnt = statement_rawlines($ctx);
-				my $herectx = $here . "\n";
-
-				for (my $n = 0; $n < $cnt; $n++) {
-					$herectx .= raw_line($linenr, $n) . "\n";
-				}
+				my $herectx = get_stat_here($linenr, $cnt, $here);
 
 				WARN("TRAILING_SEMICOLON",
 				     "macros should not use a trailing semicolon\n" . "$herectx");
@@ -5219,12 +5215,8 @@ sub process {
 				}
 			}
 			if ($level == 0 && $block =~ /^\s*\{/ && !$allowed) {
-				my $herectx = $here . "\n";
 				my $cnt = statement_rawlines($block);
-
-				for (my $n = 0; $n < $cnt; $n++) {
-					$herectx .= raw_line($linenr, $n) . "\n";
-				}
+				my $herectx = get_stat_here($linenr, $cnt, $here);
 
 				WARN("BRACES",
 				     "braces {} are not necessary for single statement blocks\n" . $herectx);
@@ -6081,11 +6073,9 @@ sub process {
 			}
 			if ($r1 !~ /^sizeof\b/ && $r2 =~ /^sizeof\s*\S/ &&
 			    !($r1 =~ /^$Constant$/ || $r1 =~ /^[A-Z_][A-Z0-9_]*$/)) {
-				my $herectx = $here . "\n";
 				my $cnt = statement_rawlines($stat);
-				for (my $n = 0; $n < $cnt; $n++) {
-					$herectx .= raw_line($linenr, $n) . "\n";
-				}
+				my $herectx = get_stat_here($linenr, $cnt, $here);
+
 				if (WARN("ALLOC_WITH_MULTIPLY",
 					 "Prefer $newfunc over $oldfunc with multiply\n" . $herectx) &&
 				    $cnt == 1 &&
@@ -6168,11 +6158,9 @@ sub process {
 		if ($^V && $^V ge 5.10.0 &&
 		    defined $stat &&
 		    $stat =~ /^\+[$;\s]*(?:case[$;\s]+\w+[$;\s]*:[$;\s]*|)*[$;\s]*\bdefault[$;\s]*:[$;\s]*;/g) {
-			my $herectx = $here . "\n";
 			my $cnt = statement_rawlines($stat);
-			for (my $n = 0; $n < $cnt; $n++) {
-				$herectx .= raw_line($linenr, $n) . "\n";
-			}
+			my $herectx = get_stat_here($linenr, $cnt, $here);
+
 			WARN("DEFAULT_NO_BREAK",
 			     "switch default: should use break\n" . $herectx);
 		}
-- 
2.7.4

[PATCH v3 4/4] checkpatch: warn for use of %px

[Tobin C. Harding]

Usage of the new %px specifier potentially leaks sensitive
inforamtion. Printing kernel addresses exposes the kernel layout in
memory, this is potentially exploitable. We have tools in the kernel to
help us do the right thing. We can have checkpatch warn developers of
potential dangers of using %px.

Have checkpatch emit a warning for usage of specifier %px.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
Co-Developed-by: Joe Perches <joe@perches.com>
Acked-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 679a10298cb5..cf1a1070fc4a 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5797,16 +5797,33 @@ sub process {
 		    defined $stat &&
 		    $stat =~ /^\+(?![^\{]*\{\s*).*\b(\w+)\s*\(.*$String\s*,/s &&
 		    $1 !~ /^_*volatile_*$/) {
-			my $bad_extension = "";
+			my $specifier;
+			my $extension;
+			my $bad_specifier = "";
+			my $stat_real;
+
 			my $lc = $stat =~ tr@\n@@;
 			$lc = $lc + $linenr;
 		        for (my $count = $linenr; $count <= $lc; $count++) {
 				my $fmt = get_quoted_string($lines[$count - 1], raw_line($count, 0));
 				$fmt =~ s/%%//g;
-				if ($fmt =~ /(\%[\*\d\.]*p(?![\WSsBKRraEhMmIiUDdgVCbGNOx]).)/) {
-					$bad_extension = $1;
-					last;
+
+				while ($fmt =~ /(\%[\*\d\.]*p(\w))/g) {
+					$specifier = $1;
+					$extension = $2;
+					if ($extension !~ /[FfSsBKRraEhMmIiUDdgVCbGNOx]/) {
+						$bad_specifier = $specifier;
+						last;
+					}
+					if ($extension eq "x" && !defined($stat_real)) {
+						if (!defined($stat_real)) {
+							$stat_real = get_stat_real($linenr, $lc);
+						}
+						WARN("VSPRINTF_SPECIFIER_PX",
+						     "Using vsprintf specifier '\%px' potentially exposes the kernel memory layout, if you don't really need the address please consider using '\%p'.\n" . "$here\n$stat_real\n");
+					}
 				}
+
 			}
 			if ($bad_extension ne "") {
 				my $stat_real = get_stat_real($linenr, $lc);
@@ -5819,7 +5836,7 @@ sub process {
 				}
 
 				WARN("VSPRINTF_POINTER_EXTENSION",
-				     "$ext_type vsprintf pointer extension '$bad_extension'$use\n" . "$here\n$stat_real\n");
+				     "Invalid vsprintf pointer extension '$bad_specifier'\n" . "$here\n$stat_real\n");
 			}
 		}
 
-- 
2.7.4

Re: [PATCH v3 0/4] checkpatch: warn for use of %px

[Tobin C. Harding]

On Tue, Feb 27, 2018 at 01:24:19PM +1100, Tobin C. Harding wrote:
...

Please drop this set

Epic fail: did not test final patch series before submission.

Sorry for the noise,
Tobin.