* [PATCH v4 1/4] checkpatch: add sub routine get_stat_real()
2018-02-27 3:04 [PATCH v4 0/4] checkpatch: warn for use of %px Tobin C. Harding
@ 2018-02-27 3:04 ` Tobin C. Harding
2018-02-27 3:04 ` [PATCH v4 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 3:04 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 v4 2/4] checkpatch: remove unused variable declarations
2018-02-27 3:04 [PATCH v4 0/4] checkpatch: warn for use of %px Tobin C. Harding
2018-02-27 3:04 ` [PATCH v4 1/4] checkpatch: add sub routine get_stat_real() Tobin C. Harding
@ 2018-02-27 3:04 ` Tobin C. Harding
2018-02-27 3:04 ` [PATCH v4 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 3:04 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 v4 3/4] checkpatch: add sub routine get_stat_here()
2018-02-27 3:04 [PATCH v4 0/4] checkpatch: warn for use of %px Tobin C. Harding
2018-02-27 3:04 ` [PATCH v4 1/4] checkpatch: add sub routine get_stat_real() Tobin C. Harding
2018-02-27 3:04 ` [PATCH v4 2/4] checkpatch: remove unused variable declarations Tobin C. Harding
@ 2018-02-27 3:04 ` Tobin C. Harding
2018-02-27 3:04 ` [PATCH v4 4/4] checkpatch: warn for use of %px Tobin C. Harding
2018-03-02 23:46 ` [PATCH v4 0/4] " Andrew Morton
4 siblings, 0 replies; 6+ messages in thread
From: Tobin C. Harding @ 2018-02-27 3:04 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 v4 4/4] checkpatch: warn for use of %px
2018-02-27 3:04 [PATCH v4 0/4] checkpatch: warn for use of %px Tobin C. Harding
` (2 preceding siblings ...)
2018-02-27 3:04 ` [PATCH v4 3/4] checkpatch: add sub routine get_stat_here() Tobin C. Harding
@ 2018-02-27 3:04 ` Tobin C. Harding
2018-03-02 23:46 ` [PATCH v4 0/4] " Andrew Morton
4 siblings, 0 replies; 6+ messages in thread
From: Tobin C. Harding @ 2018-02-27 3:04 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>
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v4 0/4] checkpatch: warn for use of %px
2018-02-27 3:04 [PATCH v4 0/4] checkpatch: warn for use of %px Tobin C. Harding
` (3 preceding siblings ...)
2018-02-27 3:04 ` [PATCH v4 4/4] checkpatch: warn for use of %px Tobin C. Harding
@ 2018-03-02 23:46 ` Andrew Morton
4 siblings, 0 replies; 6+ messages in thread
From: Andrew Morton @ 2018-03-02 23:46 UTC (permalink / raw)
To: Tobin C. Harding; +Cc: Joe Perches, Andy Whitcroft, linux-kernel
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...
^ permalink raw reply [flat|nested] 6+ messages in thread