* [PATCH v3 0/4] checkpatch: warn for use of %px
@ 2018-02-27 2:24 Tobin C. Harding
2018-02-27 2:24 ` [PATCH v3 1/4] checkpatch: add sub routine get_stat_real() Tobin C. Harding
` (4 more replies)
0 siblings, 5 replies; 6+ messages in thread
From: Tobin C. Harding @ 2018-02-27 2:24 UTC (permalink / raw)
To: Andrew Morton; +Cc: Tobin C. Harding, Joe Perches, Andy Whitcroft, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 1/4] checkpatch: add sub routine get_stat_real()
2018-02-27 2:24 [PATCH v3 0/4] checkpatch: warn for use of %px Tobin C. Harding
@ 2018-02-27 2:24 ` Tobin C. Harding
2018-02-27 2:24 ` [PATCH v3 2/4] checkpatch: remove unused variable declarations Tobin C. Harding
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Tobin C. Harding @ 2018-02-27 2:24 UTC (permalink / raw)
To: Andrew Morton; +Cc: Tobin C. Harding, Joe Perches, Andy Whitcroft, linux-kernel
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v3 2/4] checkpatch: remove unused variable declarations
2018-02-27 2:24 [PATCH v3 0/4] checkpatch: warn for use of %px Tobin C. Harding
2018-02-27 2:24 ` [PATCH v3 1/4] checkpatch: add sub routine get_stat_real() Tobin C. Harding
@ 2018-02-27 2:24 ` Tobin C. Harding
2018-02-27 2:24 ` [PATCH v3 3/4] checkpatch: add sub routine get_stat_here() Tobin C. Harding
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Tobin C. Harding @ 2018-02-27 2:24 UTC (permalink / raw)
To: Andrew Morton; +Cc: Tobin C. Harding, Joe Perches, Andy Whitcroft, linux-kernel
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v3 3/4] checkpatch: add sub routine get_stat_here()
2018-02-27 2:24 [PATCH v3 0/4] checkpatch: warn for use of %px Tobin C. Harding
2018-02-27 2:24 ` [PATCH v3 1/4] checkpatch: add sub routine get_stat_real() Tobin C. Harding
2018-02-27 2:24 ` [PATCH v3 2/4] checkpatch: remove unused variable declarations Tobin C. Harding
@ 2018-02-27 2:24 ` Tobin C. Harding
2018-02-27 2:24 ` [PATCH v3 4/4] checkpatch: warn for use of %px Tobin C. Harding
2018-02-27 2:37 ` [PATCH v3 0/4] " Tobin C. Harding
4 siblings, 0 replies; 6+ messages in thread
From: Tobin C. Harding @ 2018-02-27 2:24 UTC (permalink / raw)
To: Andrew Morton; +Cc: Tobin C. Harding, Joe Perches, Andy Whitcroft, linux-kernel
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v3 4/4] checkpatch: warn for use of %px
2018-02-27 2:24 [PATCH v3 0/4] checkpatch: warn for use of %px Tobin C. Harding
` (2 preceding siblings ...)
2018-02-27 2:24 ` [PATCH v3 3/4] checkpatch: add sub routine get_stat_here() Tobin C. Harding
@ 2018-02-27 2:24 ` Tobin C. Harding
2018-02-27 2:37 ` [PATCH v3 0/4] " Tobin C. Harding
4 siblings, 0 replies; 6+ messages in thread
From: Tobin C. Harding @ 2018-02-27 2:24 UTC (permalink / raw)
To: Andrew Morton; +Cc: Tobin C. Harding, Joe Perches, Andy Whitcroft, linux-kernel
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 0/4] checkpatch: warn for use of %px
2018-02-27 2:24 [PATCH v3 0/4] checkpatch: warn for use of %px Tobin C. Harding
` (3 preceding siblings ...)
2018-02-27 2:24 ` [PATCH v3 4/4] checkpatch: warn for use of %px Tobin C. Harding
@ 2018-02-27 2:37 ` Tobin C. Harding
4 siblings, 0 replies; 6+ messages in thread
From: Tobin C. Harding @ 2018-02-27 2:37 UTC (permalink / raw)
To: Andrew Morton; +Cc: Joe Perches, Andy Whitcroft, linux-kernel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-02-27 2:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-27 2:24 [PATCH v3 0/4] checkpatch: warn for use of %px Tobin C. Harding
2018-02-27 2:24 ` [PATCH v3 1/4] checkpatch: add sub routine get_stat_real() Tobin C. Harding
2018-02-27 2:24 ` [PATCH v3 2/4] checkpatch: remove unused variable declarations Tobin C. Harding
2018-02-27 2:24 ` [PATCH v3 3/4] checkpatch: add sub routine get_stat_here() Tobin C. Harding
2018-02-27 2:24 ` [PATCH v3 4/4] checkpatch: warn for use of %px Tobin C. Harding
2018-02-27 2:37 ` [PATCH v3 0/4] " Tobin C. Harding
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).