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

[PATCH v4 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.)

v3 was an Epic fail, not testing final patch series before submission.

Joe, I removed your 'Acked-by' tag because the patch you originally
acked is different after rebasing.  I kept the Co-Developed-by tag
because the code you wrote is still there I just had to massage it a bit
since the check for deprecated %p[Ff] has been added since we did v2.

(Sorry about the noise with v3, I hacked that version together as a
RESEND, a v1, and then finally decided on a v3 and failed to test it
after rebase :( And now I am going to violate the 1 version per day
rule to boot. 


thanks for your patience,
Tobin.


Tested on a bunch of old patches and a hand rolled printk test module
to stress test printk %p extensions.

v4:
 - actually rebase the patch set properly
 - remove 'Acked-by' tag for Joe

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 | 130 +++++++++++++++++++++++++-------------------------
 1 file changed, 66 insertions(+), 64 deletions(-)

-- 
2.7.4

[PATCH v4 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 v4 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 v4 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 v4 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>
Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 scripts/checkpatch.pl | 48 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 16 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 679a10298cb5..5628e847c6fc 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5797,29 +5797,45 @@ 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;
-				}
-			}
-			if ($bad_extension ne "") {
-				my $stat_real = get_stat_real($linenr, $lc);
-				my $ext_type = "Invalid";
-				my $use = "";
-				if ($bad_extension =~ /p[Ff]/) {
-					$ext_type = "Deprecated";
-					$use = " - use %pS instead";
-					$use =~ s/pS/ps/ if ($bad_extension =~ /pf/);
+
+				while ($fmt =~ /(\%[\*\d\.]*p(\w))/g) {
+					$specifier = $1;
+					$extension = $2;
+					if ($extension !~ /[SsBKRraEhMmIiUDdgVCbGNOx]/) {
+						$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_specifier ne "") {
+					my $stat_real = get_stat_real($linenr, $lc);
+					my $ext_type = "Invalid";
+					my $use = "";
+					if ($bad_specifier =~ /p[Ff]/) {
+						$ext_type = "Deprecated";
+						$use = " - use %pS instead";
+						$use =~ s/pS/ps/ if ($bad_specifier =~ /pf/);
+					}
 
-				WARN("VSPRINTF_POINTER_EXTENSION",
-				     "$ext_type vsprintf pointer extension '$bad_extension'$use\n" . "$here\n$stat_real\n");
+					WARN("VSPRINTF_POINTER_EXTENSION",
+					     "$ext_type vsprintf pointer extension '$bad_specifier'$use\n" . "$here\n$stat_real\n");
+				}
 			}
 		}
 
-- 
2.7.4

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

[Andrew Morton]

On Tue, 27 Feb 2018 14:04:04 +1100 "Tobin C. Harding" <me@tobin.cc> wrote:

> 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.)
> 
> v3 was an Epic fail, not testing final patch series before submission.
> 
> Joe, I removed your 'Acked-by' tag because the patch you originally
> acked is different after rebasing.  I kept the Co-Developed-by tag
> because the code you wrote is still there I just had to massage it a bit
> since the check for deprecated %p[Ff] has been added since we did v2.

I prefer not to include tags which aren't listed in
Documentation/process/submitting-patches.rst, but I now see that some
bright spark added Co-Developed-by: to
Documentation/process/5.Posting.rst, so the two files are a)
duplicative and b) out of sync.

Co-Developed-by is a little more specific than signed-off-by, but not
usefully so, I suggest...