From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753482AbdK3D6f (ORCPT ); Wed, 29 Nov 2017 22:58:35 -0500 Received: from smtprelay0227.hostedemail.com ([216.40.44.227]:53824 "EHLO smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752241AbdK3D6d (ORCPT ); Wed, 29 Nov 2017 22:58:33 -0500 X-Session-Marker: 6A6F6540706572636865732E636F6D X-Spam-Summary: X-HE-Tag: bit89_2cd0b7418d854 X-Filterd-Recvd-Size: 6044 Message-ID: <1512014306.19952.80.camel@perches.com> Subject: Re: [PATCH V11 4/5] vsprintf: add printk specifier %px From: Joe Perches To: "Tobin C. Harding" , Andrew Morton Cc: kernel-hardening@lists.openwall.com, Linus Torvalds , "Jason A. Donenfeld" , "Theodore Ts'o" , Kees Cook , Paolo Bonzini , Tycho Andersen , "Roberts, William C" , Tejun Heo , Jordan Glover , Greg KH , Petr Mladek , Ian Campbell , Sergey Senozhatsky , Catalin Marinas , Will Deacon , Steven Rostedt , Chris Fries , Dave Weinstein , Daniel Micay , Djalal Harouni , Radim =?UTF-8?Q?Kr=C4=8Dm=C3=A1=C5=99?= , linux-kernel@vger.kernel.org, Network Development , David Miller , Stephen Rothwell , Andrey Ryabinin , Alexander Potapenko , Dmitry Vyukov Date: Wed, 29 Nov 2017 19:58:26 -0800 In-Reply-To: <20171129232603.GO6217@eros> References: <1511921105-3647-1-git-send-email-me@tobin.cc> <1511921105-3647-5-git-send-email-me@tobin.cc> <20171129152058.389361d3576e784f47326cac@linux-foundation.org> <20171129232603.GO6217@eros> Content-Type: text/plain; charset="ISO-8859-1" X-Mailer: Evolution 3.26.1-1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2017-11-30 at 10:26 +1100, Tobin C. Harding wrote: > On Wed, Nov 29, 2017 at 03:20:58PM -0800, Andrew Morton wrote: > > On Wed, 29 Nov 2017 13:05:04 +1100 "Tobin C. Harding" wrote: > > > > > printk specifier %p now hashes all addresses before printing. Sometimes > > > we need to see the actual unmodified address. This can be achieved using > > > %lx but then we face the risk that if in future we want to change the > > > way the Kernel handles printing of pointers we will have to grep through > > > the already existent 50 000 %lx call sites. Let's add specifier %px as a > > > clear, opt-in, way to print a pointer and maintain some level of > > > isolation from all the other hex integer output within the Kernel. > > > > > > Add printk specifier %px to print the actual unmodified address. > > > > > > ... > > > > > > +Unmodified Addresses > > > +==================== > > > + > > > +:: > > > + > > > + %px 01234567 or 0123456789abcdef > > > + > > > +For printing pointers when you _really_ want to print the address. Please > > > +consider whether or not you are leaking sensitive information about the > > > +Kernel layout in memory before printing pointers with %px. %px is > > > +functionally equivalent to %lx. %px is preferred to %lx because it is more > > > +uniquely grep'able. If, in the future, we need to modify the way the Kernel > > > +handles printing pointers it will be nice to be able to find the call > > > +sites. > > > + > > > > You might want to add a checkpatch rule which emits a stern > > do-you-really-want-to-do-this warning when someone uses %px. > > > > Oh, nice idea. It has to be a CHECK but right? No, it has to be something that's not --strict so a WARN would probably be best. > By stern, you mean use stern language? I hope he doesn't mean tweet. Something like: --- scripts/checkpatch.pl | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 0ce249f157a1..9d789cbe7df5 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -5758,21 +5758,40 @@ sub process { defined $stat && $stat =~ /^\+(?![^\{]*\{\s*).*\b(\w+)\s*\(.*$String\s*,/s && $1 !~ /^_*volatile_*$/) { + my $complete_extension = ""; + my $extension = ""; my $bad_extension = ""; my $lc = $stat =~ tr@\n@@; $lc = $lc + $linenr; + my $stat_real; 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(?![\WFfSsBKRraEhMmIiUDdgVCbGNO]).)/) { - $bad_extension = $1; - last; + while ($fmt =~ /(\%[\*\d\.]*p(\w))/g) { + $complete_extension = $1; + $extension = $2; + if ($extension !~ /[FfSsBKRraEhMmIiUDdgVCbGNOx]/) { + $bad_extension = $complete_extension; + last; + } + if ($extension eq "x") { + if (!defined($stat_real)) { + $stat_real = raw_line($linenr, 0); + for (my $count = $linenr + 1; $count <= $lc; $count++) { + $stat_real = $stat_real . "\n" . raw_line($count, 0); + } + } + WARN("VSPRINTF_POINTER_PX", + "Using vsprintf pointer extension '$complete_extension' exposes kernel address for possible hacking\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 (!defined($stat_real)) { + $stat_real = raw_line($linenr, 0); + for (my $count = $linenr + 1; $count <= $lc; $count++) { + $stat_real = $stat_real . "\n" . raw_line($count, 0); + } } WARN("VSPRINTF_POINTER_EXTENSION", "Invalid vsprintf pointer extension '$bad_extension'\n" . "$here\n$stat_real\n");