From: Joe Perches <joe@perches.com> To: "Roberts, William C" <william.c.roberts@intel.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "apw@canonical.com" <apw@canonical.com> Cc: "kernel-hardening@lists.openwall.com" <kernel-hardening@lists.openwall.com> Subject: Re: [PATCH] checkpatch: add warning on %pk instead of %pK usage Date: Fri, 10 Feb 2017 19:23:48 -0800 [thread overview] Message-ID: <1486783428.2192.44.camel@perches.com> (raw) In-Reply-To: <476DC76E7D1DF2438D32BFADF679FC562305DC7B@ORSMSX103.amr.corp.intel.com> [-- Attachment #1: Type: text/plain, Size: 3012 bytes --] On Sat, 2017-02-11 at 01:32 +0000, Roberts, William C wrote: > <snip> > > > By "normal" I'm referring to things that call into pointer(), just > > > casually looking I see bstr_printf vsnprintf kvasprintf, which would > > > be easy enough to add > > > > > > > What do you think is missing? sn?printf ? That's easy to add. > > > > > > The problem starts to get hairy when we think of how often folks roll > > > their own logging macros (see some small sampling at the end). > > > > > > I think we would want to add DEBUG DBG and sn?printf and maybe > > > consider dropping the \b on the regex so it's a bit more matchy but > > > still shouldn't end up matching on any ASM as you pointed out in the V2 nack. > > > > > > Ill break this down into: > > > 1. the patch as I know you'll take it, as you wrote it :-P 2. Adding > > > to the logging macros 3. exploring making it less matchy > > -Kees and Andrew they likely don't care about the rest of this... > > I have been working up a regex (I suck at these) to match C functions that have an invalid > %p format string and take arguments: > http://www.regexr.com/3f92k > > This could be a way to get better coverage in a more generic approach, thoughts? Maybe this: (attached too because Evolution is a bad email client) It's still kind of hacky, but it does find multiple line statements like: + printf(KERN_INFO + "a %pX", + foo); --- Subject: [PATCH] checkpatch: Add ability to find bad uses of vsprintf %p extensions %pK was at least once misused at %pk in an out-of-tree module. This lead to some security concerns. Add the ability to track single and multiple line statements for misuses of %p. Signed-off-by: Joe Perches --- scripts/checkpatch.pl | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index ad5ea5c545b2..0eaf6b8580d6 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -5676,6 +5676,32 @@ sub process { } } + # check for vsprintf extension %p misuses + if ($^V && $^V ge 5.10.0 && + defined $stat && + $stat =~ /^\+(?![^\{]*\{\s*).*\b(\w+)\s*\(.*$String\s*,/s && + $1 !~ /^_*volatile_*$/) { + my $bad_extension = ""; + 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(?![\WFfSsBKRraEhMmIiUDdgVCbGN]).)/) { + $bad_extension = $1; + last; + } + } + 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); + } + WARN("VSPRINTF_POINTER_EXTENSION", + "Invalid vsprintf pointer extension '$bad_extension'\n" . "$here\n$stat_real\n"); + } + } + # Check for misused memsets if ($^V && $^V ge 5.10.0 && defined $stat && -- [-- Attachment #2: 0001-checkpatch-Add-ability-to-find-bad-uses-of-vsprintf-.patch --] [-- Type: text/x-patch, Size: 1886 bytes --] From 3bd6868711efeb587c5c48e060c415a150fccaca Mon Sep 17 00:00:00 2001 Message-Id: <3bd6868711efeb587c5c48e060c415a150fccaca.1486783224.git.joe@perches.com> From: Joe Perches <joe@perches.com> Date: Fri, 10 Feb 2017 19:17:42 -0800 Subject: [PATCH] checkpatch: Add ability to find bad uses of vsprintf %p<foo> extensions %pK was at least once misused at %pk in an out-of-tree module. This lead to some security concerns. Add the ability to track single and multiple line statements for misuses of %p<foo>. Signed-off-by: Joe Perches <joe@perches.com> --- scripts/checkpatch.pl | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index ad5ea5c545b2..0eaf6b8580d6 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -5676,7 +5676,32 @@ sub process { } } + # check for vsprintf extension %p<foo> misuses + if ($^V && $^V ge 5.10.0 && + defined $stat && + $stat =~ /^\+(?![^\{]*\{\s*).*\b(\w+)\s*\(.*$String\s*,/s && + $1 !~ /^_*volatile_*$/) { + my $bad_extension = ""; + 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(?![\WFfSsBKRraEhMmIiUDdgVCbGN]).)/) { + $bad_extension = $1; + last; + } + } + 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); + } + WARN("VSPRINTF_POINTER_EXTENSION", + "Invalid vsprintf pointer extension '$bad_extension'\n" . "$here\n$stat_real\n"); + } + } + # Check for misused memsets if ($^V && $^V ge 5.10.0 && defined $stat && -- 2.10.0.rc2.1.g053435c
WARNING: multiple messages have this Message-ID (diff)
From: Joe Perches <joe@perches.com> To: "Roberts, William C" <william.c.roberts@intel.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "apw@canonical.com" <apw@canonical.com> Cc: "kernel-hardening@lists.openwall.com" <kernel-hardening@lists.openwall.com> Subject: [kernel-hardening] Re: [PATCH] checkpatch: add warning on %pk instead of %pK usage Date: Fri, 10 Feb 2017 19:23:48 -0800 [thread overview] Message-ID: <1486783428.2192.44.camel@perches.com> (raw) In-Reply-To: <476DC76E7D1DF2438D32BFADF679FC562305DC7B@ORSMSX103.amr.corp.intel.com> [-- Attachment #1: Type: text/plain, Size: 3012 bytes --] On Sat, 2017-02-11 at 01:32 +0000, Roberts, William C wrote: > <snip> > > > By "normal" I'm referring to things that call into pointer(), just > > > casually looking I see bstr_printf vsnprintf kvasprintf, which would > > > be easy enough to add > > > > > > > What do you think is missing? sn?printf ? That's easy to add. > > > > > > The problem starts to get hairy when we think of how often folks roll > > > their own logging macros (see some small sampling at the end). > > > > > > I think we would want to add DEBUG DBG and sn?printf and maybe > > > consider dropping the \b on the regex so it's a bit more matchy but > > > still shouldn't end up matching on any ASM as you pointed out in the V2 nack. > > > > > > Ill break this down into: > > > 1. the patch as I know you'll take it, as you wrote it :-P 2. Adding > > > to the logging macros 3. exploring making it less matchy > > -Kees and Andrew they likely don't care about the rest of this... > > I have been working up a regex (I suck at these) to match C functions that have an invalid > %p format string and take arguments: > http://www.regexr.com/3f92k > > This could be a way to get better coverage in a more generic approach, thoughts? Maybe this: (attached too because Evolution is a bad email client) It's still kind of hacky, but it does find multiple line statements like: + printf(KERN_INFO + "a %pX", + foo); --- Subject: [PATCH] checkpatch: Add ability to find bad uses of vsprintf %p extensions %pK was at least once misused at %pk in an out-of-tree module. This lead to some security concerns. Add the ability to track single and multiple line statements for misuses of %p. Signed-off-by: Joe Perches --- scripts/checkpatch.pl | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index ad5ea5c545b2..0eaf6b8580d6 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -5676,6 +5676,32 @@ sub process { } } + # check for vsprintf extension %p misuses + if ($^V && $^V ge 5.10.0 && + defined $stat && + $stat =~ /^\+(?![^\{]*\{\s*).*\b(\w+)\s*\(.*$String\s*,/s && + $1 !~ /^_*volatile_*$/) { + my $bad_extension = ""; + 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(?![\WFfSsBKRraEhMmIiUDdgVCbGN]).)/) { + $bad_extension = $1; + last; + } + } + 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); + } + WARN("VSPRINTF_POINTER_EXTENSION", + "Invalid vsprintf pointer extension '$bad_extension'\n" . "$here\n$stat_real\n"); + } + } + # Check for misused memsets if ($^V && $^V ge 5.10.0 && defined $stat && -- [-- Attachment #2: 0001-checkpatch-Add-ability-to-find-bad-uses-of-vsprintf-.patch --] [-- Type: text/x-patch, Size: 1886 bytes --] From 3bd6868711efeb587c5c48e060c415a150fccaca Mon Sep 17 00:00:00 2001 Message-Id: <3bd6868711efeb587c5c48e060c415a150fccaca.1486783224.git.joe@perches.com> From: Joe Perches <joe@perches.com> Date: Fri, 10 Feb 2017 19:17:42 -0800 Subject: [PATCH] checkpatch: Add ability to find bad uses of vsprintf %p<foo> extensions %pK was at least once misused at %pk in an out-of-tree module. This lead to some security concerns. Add the ability to track single and multiple line statements for misuses of %p<foo>. Signed-off-by: Joe Perches <joe@perches.com> --- scripts/checkpatch.pl | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index ad5ea5c545b2..0eaf6b8580d6 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -5676,7 +5676,32 @@ sub process { } } + # check for vsprintf extension %p<foo> misuses + if ($^V && $^V ge 5.10.0 && + defined $stat && + $stat =~ /^\+(?![^\{]*\{\s*).*\b(\w+)\s*\(.*$String\s*,/s && + $1 !~ /^_*volatile_*$/) { + my $bad_extension = ""; + 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(?![\WFfSsBKRraEhMmIiUDdgVCbGN]).)/) { + $bad_extension = $1; + last; + } + } + 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); + } + WARN("VSPRINTF_POINTER_EXTENSION", + "Invalid vsprintf pointer extension '$bad_extension'\n" . "$here\n$stat_real\n"); + } + } + # Check for misused memsets if ($^V && $^V ge 5.10.0 && defined $stat && -- 2.10.0.rc2.1.g053435c
next prev parent reply other threads:[~2017-02-11 3:23 UTC|newest] Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-02-10 19:37 [PATCH] checkpatch: add warning on %pk instead of %pK usage william.c.roberts 2017-02-10 19:37 ` [kernel-hardening] " william.c.roberts 2017-02-10 20:12 ` Joe Perches 2017-02-10 20:12 ` [kernel-hardening] " Joe Perches 2017-02-10 22:14 ` Roberts, William C 2017-02-10 22:14 ` [kernel-hardening] " Roberts, William C 2017-02-10 22:26 ` Roberts, William C 2017-02-10 22:26 ` [kernel-hardening] " Roberts, William C 2017-02-10 22:49 ` Joe Perches 2017-02-10 22:49 ` [kernel-hardening] " Joe Perches 2017-02-10 22:59 ` Joe Perches 2017-02-10 22:59 ` [kernel-hardening] " Joe Perches 2017-02-10 23:31 ` Roberts, William C 2017-02-10 23:31 ` [kernel-hardening] " Roberts, William C 2017-02-10 23:49 ` Joe Perches 2017-02-10 23:49 ` [kernel-hardening] " Joe Perches 2017-02-10 23:54 ` Roberts, William C 2017-02-10 23:54 ` [kernel-hardening] " Roberts, William C 2017-02-11 0:01 ` Joe Perches 2017-02-11 0:01 ` [kernel-hardening] " Joe Perches 2017-02-11 1:32 ` Roberts, William C 2017-02-11 1:32 ` [kernel-hardening] " Roberts, William C 2017-02-11 3:23 ` Joe Perches [this message] 2017-02-11 3:23 ` [kernel-hardening] " Joe Perches 2017-02-13 19:46 ` Roberts, William C 2017-02-13 19:46 ` [kernel-hardening] " Roberts, William C 2017-02-13 20:14 ` Joe Perches 2017-02-13 20:14 ` [kernel-hardening] " Joe Perches [not found] ` <1487016251.6214.6.camel@perches.com> [not found] ` <476DC76E7D1DF2438D32BFADF679FC562305F62F@ORSMSX103.amr.corp.intel.com> 2017-02-13 22:20 ` Joe Perches 2017-02-13 22:20 ` [kernel-hardening] " Joe Perches 2017-02-15 23:49 ` Roberts, William C 2017-02-15 23:49 ` [kernel-hardening] " Roberts, William C 2017-02-16 0:19 ` Joe Perches 2017-02-16 0:19 ` [kernel-hardening] " Joe Perches 2017-02-27 16:26 ` Roberts, William C 2017-02-27 16:26 ` [kernel-hardening] " Roberts, William C 2017-02-27 20:54 ` [PATCH] checkpatch: Add ability to find bad uses of vsprintf %p<foo> extensions Joe Perches 2017-02-27 20:54 ` [kernel-hardening] " Joe Perches 2017-02-27 21:18 ` Kees Cook 2017-02-27 21:18 ` Kees Cook 2017-02-28 15:34 ` Roberts, William C 2017-02-28 15:34 ` [kernel-hardening] " Roberts, William C 2017-03-01 0:06 ` Andrew Morton 2017-03-01 0:06 ` [kernel-hardening] " Andrew Morton 2017-03-01 0:11 ` Kees Cook 2017-03-01 0:11 ` Kees Cook 2017-03-01 1:14 ` Joe Perches 2017-03-01 1:14 ` Joe Perches 2017-03-01 0:12 ` Joe Perches 2017-03-01 0:12 ` [kernel-hardening] " Joe Perches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=1486783428.2192.44.camel@perches.com \ --to=joe@perches.com \ --cc=apw@canonical.com \ --cc=kernel-hardening@lists.openwall.com \ --cc=linux-kernel@vger.kernel.org \ --cc=william.c.roberts@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.