[PATCH] checkpatch: warn for use of %px

[PATCH] 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.

Suggested-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Tobin C. Harding <me@tobin.cc>
Co-Developed-by: Joe Perches <joe@perches.com>

---

Joe,

Are you happy with this tagging? Needs your signed-off-by still.


Andrew,

Is it okay to add your Suggested-by tag here?

I'm not entirely sure when one is supposed to add someones signed-off-by
tag since the docs state that it should not be added without
permission. I am also unsure where/when is the best time to request this
permission.

thanks,
Tobin.


 scripts/checkpatch.pl | 42 ++++++++++++++++++++++++++++++++----------
 1 file changed, 32 insertions(+), 10 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 040aa79e1d9d..c63466f86b18 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1612,6 +1612,17 @@ sub raw_line {
 	return $line;
 }
 
+sub 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);
@@ -5747,24 +5758,35 @@ sub process {
 		    defined $stat &&
 		    $stat =~ /^\+(?![^\{]*\{\s*).*\b(\w+)\s*\(.*$String\s*,/s &&
 		    $1 !~ /^_*volatile_*$/) {
-			my $bad_extension = "";
+			my ($specifier, $extension, $stat_real);
+			my $bad_specifier = "";
 			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(?![\WFfSsBKRraEhMmIiUDdgVCbGNOx]).)/) {
-					$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 = stat_real($linenr, $lc);
+						}
+						WARN("VSPRINTF_SPECIFIER_PX",
+						     "Using vsprintf specifier '\%px' potentially exposes the kernel layout in memory, if you don't _realy_ need the address please consider using '\%p'.\n" . "$here\n$stat_real\n");
+					}
 				}
+
 			}
-			if ($bad_extension ne "") {
-				my $stat_real = raw_line($linenr, 0);
-				for (my $count = $linenr + 1; $count <= $lc; $count++) {
-					$stat_real = $stat_real . "\n" . raw_line($count, 0);
-				}
+			if ($bad_specifier ne "") {
+				$stat_real = stat_real($linenr, $lc);
 				WARN("VSPRINTF_POINTER_EXTENSION",
-				     "Invalid vsprintf pointer extension '$bad_extension'\n" . "$here\n$stat_real\n");
+				     "Invalid vsprintf pointer extension '$bad_specifier'\n" . "$here\n$stat_real\n");
 			}
 		}
 
-- 
2.7.4

Re: [PATCH] checkpatch: warn for use of %px

[Joe Perches]

On Tue, 2017-12-05 at 08:17 +1100, Tobin C. Harding wrote:
> Usage of the new %px specifier potentially leaks sensitive
> inforamtion. Printing kernel addresses exposes the kernel layout in

information

> 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.
> 
> Suggested-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Tobin C. Harding <me@tobin.cc>
> Co-Developed-by: Joe Perches <joe@perches.com>
> 
> ---
> 
> Joe,
> 
> Are you happy with this tagging? Needs your signed-off-by still.

Maybe with a few corrections (below)
> 
> Andrew,
> 
> Is it okay to add your Suggested-by tag here?
> 
> I'm not entirely sure when one is supposed to add someones signed-off-by
> tag since the docs state that it should not be added without
> permission. I am also unsure where/when is the best time to request this
> permission.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -1612,6 +1612,17 @@ sub raw_line {
>  	return $line;
>  }
>  
> +sub 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;
> +}

If you are going to make a subroutine of this
there are some other places it could be used too.

> +
>  sub cat_vet {
>  	my ($vet) = @_;
>  	my ($res, $coded);
> @@ -5747,24 +5758,35 @@ sub process {
>  		    defined $stat &&
>  		    $stat =~ /^\+(?![^\{]*\{\s*).*\b(\w+)\s*\(.*$String\s*,/s &&
>  		    $1 !~ /^_*volatile_*$/) {
> -			my $bad_extension = "";
> +			my ($specifier, $extension, $stat_real);

My preference is not to define multiple variables on a single line.
I'd rather have:
			my $specifier;
			my $extension;
			my $stat_real;

> +			my $bad_specifier = "";
>  			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(?![\WFfSsBKRraEhMmIiUDdgVCbGNOx]).)/) {
> -					$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 = stat_real($linenr, $lc);
> +						}
> +						WARN("VSPRINTF_SPECIFIER_PX",
> +						     "Using vsprintf specifier '\%px' potentially exposes the kernel layout in memory, if you don't _realy_ need the address please consider using '\%p'.\n" . "$here\n$stat_real\n");	

"kernel memory layout" not "kernel layout in memory"
"really" not "_realy_"

Re: [PATCH] checkpatch: warn for use of %px

[Tobin C. Harding]

On Mon, Dec 04, 2017 at 11:24:24PM -0800, Joe Perches wrote:
> On Tue, 2017-12-05 at 08:17 +1100, Tobin C. Harding wrote:
> > Usage of the new %px specifier potentially leaks sensitive
> > inforamtion. Printing kernel addresses exposes the kernel layout in
> 
> information

I don't understand this comment? Do you mean the wording is wrong? I'll
re-word as suggested below.

> > 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.
> > 
> > Suggested-by: Andrew Morton <akpm@linux-foundation.org>
> > Signed-off-by: Tobin C. Harding <me@tobin.cc>
> > Co-Developed-by: Joe Perches <joe@perches.com>
> > 
> > ---
> > 
> > Joe,
> > 
> > Are you happy with this tagging? Needs your signed-off-by still.
> 
> Maybe with a few corrections (below)

thanks for the tips.

> > Andrew,
> > 
> > Is it okay to add your Suggested-by tag here?
> > 
> > I'm not entirely sure when one is supposed to add someones signed-off-by
> > tag since the docs state that it should not be added without
> > permission. I am also unsure where/when is the best time to request this
> > permission.
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -1612,6 +1612,17 @@ sub raw_line {
> >  	return $line;
> >  }
> >  
> > +sub 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;
> > +}
> 
> If you are going to make a subroutine of this
> there are some other places it could be used too.

Ok, I'm not super happy with sub routine name. Have you a better suggestion?

> > +
> >  sub cat_vet {
> >  	my ($vet) = @_;
> >  	my ($res, $coded);
> > @@ -5747,24 +5758,35 @@ sub process {
> >  		    defined $stat &&
> >  		    $stat =~ /^\+(?![^\{]*\{\s*).*\b(\w+)\s*\(.*$String\s*,/s &&
> >  		    $1 !~ /^_*volatile_*$/) {
> > -			my $bad_extension = "";
> > +			my ($specifier, $extension, $stat_real);
> 
> My preference is not to define multiple variables on a single line.
> I'd rather have:
> 			my $specifier;
> 			my $extension;
> 			my $stat_real;

No problem, is this a kernel wide thing or just a checkpatch thing (so I
can follow your lead if need be in leaking_addresses.pl). Or is it the
same as we do in C, in which case $extension and $specifier could be on
a single line but not $stat_real?

> > +			my $bad_specifier = "";
> >  			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(?![\WFfSsBKRraEhMmIiUDdgVCbGNOx]).)/) {
> > -					$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 = stat_real($linenr, $lc);
> > +						}
> > +						WARN("VSPRINTF_SPECIFIER_PX",
> > +						     "Using vsprintf specifier '\%px' potentially exposes the kernel layout in memory, if you don't _realy_ need the address please consider using '\%p'.\n" . "$here\n$stat_real\n");	
> 
> "kernel memory layout" not "kernel layout in memory"
> "really" not "_realy_"

Got it.

thanks,
Tobin.

Re: [PATCH] checkpatch: warn for use of %px

[Joe Perches]

On Tue, 2017-12-05 at 20:44 +1100, Tobin C. Harding wrote:
> On Mon, Dec 04, 2017 at 11:24:24PM -0800, Joe Perches wrote:
> > On Tue, 2017-12-05 at 08:17 +1100, Tobin C. Harding wrote:
> > > Usage of the new %px specifier potentially leaks sensitive
> > > inforamtion. Printing kernel addresses exposes the kernel layout in
> > 
> > information

> I don't understand this comment? Do you mean the wording is wrong?
> I'll re-word as suggested below.

It's just a spelling typo correction.

[]
> > > Suggested-by: Andrew Morton <akpm@linux-foundation.org>
> > > Signed-off-by: Tobin C. Harding <me@tobin.cc>
> > > Co-Developed-by: Joe Perches <joe@perches.com>
> > > Are you happy with this tagging? Needs your signed-off-by still.

I think signatures tags are pretty freeform and
I'm not particularly concerned about them.

I think Andrew Morton may object and change it
or remove it.  Have an:

Acked-by: Joe Perches <joe@perches.com>

if the spellos and style bits are changed.

> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > 
> > []
> > > @@ -1612,6 +1612,17 @@ sub raw_line {
> > >  	return $line;
> > >  }
> > >  
> > > +sub 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;
> > > +}
> > 
> > If you are going to make a subroutine of this
> > there are some other places it could be used too.
>
> Ok, I'm not super happy with sub routine name. Have you a better suggestion?

Maybe get_stat_real?

> > > +
> > >  sub cat_vet {
> > >  	my ($vet) = @_;
> > >  	my ($res, $coded);
> > > @@ -5747,24 +5758,35 @@ sub process {
> > >  		    defined $stat &&
> > >  		    $stat =~ /^\+(?![^\{]*\{\s*).*\b(\w+)\s*\(.*$String\s*,/s &&
> > >  		    $1 !~ /^_*volatile_*$/) {
> > > -			my $bad_extension = "";
> > > +			my ($specifier, $extension, $stat_real);
> > 
> > My preference is not to define multiple variables on a single line.
> > I'd rather have:
> > 			my $specifier;
> > 			my $extension;
> > 			my $stat_real;
> 
> No problem, is this a kernel wide thing or just a checkpatch thing (so I
> can follow your lead if need be in leaking_addresses.pl). Or is it the
> same as we do in C, in which case $extension and $specifier could be on
> a single line but not $stat_real?

It's a personal preference.

Perl can have pretty cryptic constructs and I prefer
reading it like C.

Re: [PATCH] checkpatch: warn for use of %px

[Tobin C. Harding]

On Tue, Dec 05, 2017 at 07:27:18AM -0800, Joe Perches wrote:
> On Tue, 2017-12-05 at 20:44 +1100, Tobin C. Harding wrote:
> > On Mon, Dec 04, 2017 at 11:24:24PM -0800, Joe Perches wrote:
> > > On Tue, 2017-12-05 at 08:17 +1100, Tobin C. Harding wrote:
> > > > Usage of the new %px specifier potentially leaks sensitive
> > > > inforamtion. Printing kernel addresses exposes the kernel layout in
> > > 
> > > information
> 
> > I don't understand this comment? Do you mean the wording is wrong?
> > I'll re-word as suggested below.
> 
> It's just a spelling typo correction.

ha ha, note to self - be careful doing kernel email late at night :)

> []
> > > > Suggested-by: Andrew Morton <akpm@linux-foundation.org>
> > > > Signed-off-by: Tobin C. Harding <me@tobin.cc>
> > > > Co-Developed-by: Joe Perches <joe@perches.com>
> > > > Are you happy with this tagging? Needs your signed-off-by still.
> 
> I think signatures tags are pretty freeform and
> I'm not particularly concerned about them.
> 
> I think Andrew Morton may object and change it
> or remove it.  Have an:
> 
> Acked-by: Joe Perches <joe@perches.com>

Cool, will add and re-spin

thanks,
Tobin.