* [PATCH] checkpatch: add warning on %pk instead of %pK usage
@ 2017-02-10 19:37 ` william.c.roberts
0 siblings, 0 replies; 50+ messages in thread
From: william.c.roberts @ 2017-02-10 19:37 UTC (permalink / raw)
To: linux-kernel, joe, apw; +Cc: keescook, kernel-hardening, William Roberts
From: William Roberts <william.c.roberts@intel.com>
Sample output:
WARNING: %pk is close to %pK, did you mean %pK?.
\#20: FILE: drivers/char/applicom.c:230:
+ printk(KERN_INFO "Could not allocate IRQ %d for PCI Applicom device. %pk\n", dev->irq, pci_get_class);
Signed-off-by: William Roberts <william.c.roberts@intel.com>
---
scripts/checkpatch.pl | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 982c52c..f20f5c5 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6096,6 +6096,12 @@ sub process {
"recursive locking is bad, do not use this ever.\n" . $herecurr);
}
+# check for bad %pK usage
+ if ($rawline =~ /\%pk/) {
+ WARN("FORMAT SPECIFIER",
+ "%pk is close to %pK, did you mean %pK?.\n" . $herecurr);
+ }
+
# check for lockdep_set_novalidate_class
if ($line =~ /^.\s*lockdep_set_novalidate_class\s*\(/ ||
$line =~ /__lockdep_no_validate__\s*\)/ ) {
--
2.7.4
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [kernel-hardening] [PATCH] checkpatch: add warning on %pk instead of %pK usage
@ 2017-02-10 19:37 ` william.c.roberts
0 siblings, 0 replies; 50+ messages in thread
From: william.c.roberts @ 2017-02-10 19:37 UTC (permalink / raw)
To: linux-kernel, joe, apw; +Cc: keescook, kernel-hardening, William Roberts
From: William Roberts <william.c.roberts@intel.com>
Sample output:
WARNING: %pk is close to %pK, did you mean %pK?.
\#20: FILE: drivers/char/applicom.c:230:
+ printk(KERN_INFO "Could not allocate IRQ %d for PCI Applicom device. %pk\n", dev->irq, pci_get_class);
Signed-off-by: William Roberts <william.c.roberts@intel.com>
---
scripts/checkpatch.pl | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 982c52c..f20f5c5 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6096,6 +6096,12 @@ sub process {
"recursive locking is bad, do not use this ever.\n" . $herecurr);
}
+# check for bad %pK usage
+ if ($rawline =~ /\%pk/) {
+ WARN("FORMAT SPECIFIER",
+ "%pk is close to %pK, did you mean %pK?.\n" . $herecurr);
+ }
+
# check for lockdep_set_novalidate_class
if ($line =~ /^.\s*lockdep_set_novalidate_class\s*\(/ ||
$line =~ /__lockdep_no_validate__\s*\)/ ) {
--
2.7.4
^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [PATCH] checkpatch: add warning on %pk instead of %pK usage
2017-02-10 19:37 ` [kernel-hardening] " william.c.roberts
@ 2017-02-10 20:12 ` Joe Perches
-1 siblings, 0 replies; 50+ messages in thread
From: Joe Perches @ 2017-02-10 20:12 UTC (permalink / raw)
To: william.c.roberts, linux-kernel, apw, Andew Morton
Cc: keescook, kernel-hardening
On Fri, 2017-02-10 at 11:37 -0800, william.c.roberts@intel.com wrote:
> From: William Roberts <william.c.roberts@intel.com>
>
> Sample output:
> WARNING: %pk is close to %pK, did you mean %pK?.
> \#20: FILE: drivers/char/applicom.c:230:
> + printk(KERN_INFO "Could not allocate IRQ %d for PCI Applicom device. %pk\n", dev->irq, pci_get_class);
There isn't a single instance of this in the kernel tree.
Maybe if this is really useful, then all the %p<foo> extensions
should be enumerated and all unknown uses should have warnings.
Something like:
---
scripts/checkpatch.pl | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index ad5ea5c545b2..8a90b457e8b5 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5305,6 +5305,15 @@ sub process {
}
}
+# check for vsprintf extension %p<foo> misuses
+ if ($line =~ /\b$logFunctions\s*\(.*$String/) {
+ my $format = get_quoted_string($line, $rawline);
+ if ($format =~ /(\%[\*\d\.]*p(?![\WFfSsBKRraEhMmIiUDdgVCbGN]).)/) {
+ WARN("VSPRINTF_POINTER_EXTENSION",
+ "Invalid vsprintf pointer extension '$1'\n" . $herecurr);
+ }
+ }
+
# check for logging continuations
if ($line =~ /\bprintk\s*\(\s*KERN_CONT\b|\bpr_cont\s*\(/) {
WARN("LOGGING_CONTINUATION",
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [kernel-hardening] Re: [PATCH] checkpatch: add warning on %pk instead of %pK usage
@ 2017-02-10 20:12 ` Joe Perches
0 siblings, 0 replies; 50+ messages in thread
From: Joe Perches @ 2017-02-10 20:12 UTC (permalink / raw)
To: william.c.roberts, linux-kernel, apw, Andew Morton
Cc: keescook, kernel-hardening
On Fri, 2017-02-10 at 11:37 -0800, william.c.roberts@intel.com wrote:
> From: William Roberts <william.c.roberts@intel.com>
>
> Sample output:
> WARNING: %pk is close to %pK, did you mean %pK?.
> \#20: FILE: drivers/char/applicom.c:230:
> + printk(KERN_INFO "Could not allocate IRQ %d for PCI Applicom device. %pk\n", dev->irq, pci_get_class);
There isn't a single instance of this in the kernel tree.
Maybe if this is really useful, then all the %p<foo> extensions
should be enumerated and all unknown uses should have warnings.
Something like:
---
scripts/checkpatch.pl | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index ad5ea5c545b2..8a90b457e8b5 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5305,6 +5305,15 @@ sub process {
}
}
+# check for vsprintf extension %p<foo> misuses
+ if ($line =~ /\b$logFunctions\s*\(.*$String/) {
+ my $format = get_quoted_string($line, $rawline);
+ if ($format =~ /(\%[\*\d\.]*p(?![\WFfSsBKRraEhMmIiUDdgVCbGN]).)/) {
+ WARN("VSPRINTF_POINTER_EXTENSION",
+ "Invalid vsprintf pointer extension '$1'\n" . $herecurr);
+ }
+ }
+
# check for logging continuations
if ($line =~ /\bprintk\s*\(\s*KERN_CONT\b|\bpr_cont\s*\(/) {
WARN("LOGGING_CONTINUATION",
^ permalink raw reply related [flat|nested] 50+ messages in thread
* RE: [PATCH] checkpatch: add warning on %pk instead of %pK usage
2017-02-10 20:12 ` [kernel-hardening] " Joe Perches
@ 2017-02-10 22:14 ` Roberts, William C
-1 siblings, 0 replies; 50+ messages in thread
From: Roberts, William C @ 2017-02-10 22:14 UTC (permalink / raw)
To: Joe Perches, linux-kernel, apw, Andew Morton; +Cc: keescook, kernel-hardening
> -----Original Message-----
> From: Joe Perches [mailto:joe@perches.com]
> Sent: Friday, February 10, 2017 12:12 PM
> To: Roberts, William C <william.c.roberts@intel.com>; linux-
> kernel@vger.kernel.org; apw@canonical.com; Andew Morton <akpm@linux-
> foundation.org>
> Cc: keescook@chromium.org; kernel-hardening@lists.openwall.com
> Subject: Re: [PATCH] checkpatch: add warning on %pk instead of %pK usage
>
> On Fri, 2017-02-10 at 11:37 -0800, william.c.roberts@intel.com wrote:
> > From: William Roberts <william.c.roberts@intel.com>
> >
> > Sample output:
> > WARNING: %pk is close to %pK, did you mean %pK?.
> > \#20: FILE: drivers/char/applicom.c:230:
> > + printk(KERN_INFO "Could not allocate IRQ %d for PCI
> Applicom
> > +device. %pk\n", dev->irq, pci_get_class);
>
> There isn't a single instance of this in the kernel tree.
>
> Maybe if this is really useful, then all the %p<foo> extensions should be
> enumerated and all unknown uses should have warnings.
I was thinking of doing that, but I figured I would start with the bare minimum patch.
>
> Something like:
>
> ---
> scripts/checkpatch.pl | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index
> ad5ea5c545b2..8a90b457e8b5 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -5305,6 +5305,15 @@ sub process {
> }
> }
>
> +# check for vsprintf extension %p<foo> misuses
> + if ($line =~ /\b$logFunctions\s*\(.*$String/) {
> + my $format = get_quoted_string($line, $rawline);
> + if ($format =~
> /(\%[\*\d\.]*p(?![\WFfSsBKRraEhMmIiUDdgVCbGN]).)/) {
> + WARN("VSPRINTF_POINTER_EXTENSION",
> + "Invalid vsprintf pointer extension '$1'\n" .
> $herecurr);
> + }
> + }
> +
> # check for logging continuations
> if ($line =~ /\bprintk\s*\(\s*KERN_CONT\b|\bpr_cont\s*\(/) {
> WARN("LOGGING_CONTINUATION",
^ permalink raw reply [flat|nested] 50+ messages in thread
* [kernel-hardening] RE: [PATCH] checkpatch: add warning on %pk instead of %pK usage
@ 2017-02-10 22:14 ` Roberts, William C
0 siblings, 0 replies; 50+ messages in thread
From: Roberts, William C @ 2017-02-10 22:14 UTC (permalink / raw)
To: Joe Perches, linux-kernel, apw, Andew Morton; +Cc: keescook, kernel-hardening
> -----Original Message-----
> From: Joe Perches [mailto:joe@perches.com]
> Sent: Friday, February 10, 2017 12:12 PM
> To: Roberts, William C <william.c.roberts@intel.com>; linux-
> kernel@vger.kernel.org; apw@canonical.com; Andew Morton <akpm@linux-
> foundation.org>
> Cc: keescook@chromium.org; kernel-hardening@lists.openwall.com
> Subject: Re: [PATCH] checkpatch: add warning on %pk instead of %pK usage
>
> On Fri, 2017-02-10 at 11:37 -0800, william.c.roberts@intel.com wrote:
> > From: William Roberts <william.c.roberts@intel.com>
> >
> > Sample output:
> > WARNING: %pk is close to %pK, did you mean %pK?.
> > \#20: FILE: drivers/char/applicom.c:230:
> > + printk(KERN_INFO "Could not allocate IRQ %d for PCI
> Applicom
> > +device. %pk\n", dev->irq, pci_get_class);
>
> There isn't a single instance of this in the kernel tree.
>
> Maybe if this is really useful, then all the %p<foo> extensions should be
> enumerated and all unknown uses should have warnings.
I was thinking of doing that, but I figured I would start with the bare minimum patch.
>
> Something like:
>
> ---
> scripts/checkpatch.pl | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index
> ad5ea5c545b2..8a90b457e8b5 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -5305,6 +5305,15 @@ sub process {
> }
> }
>
> +# check for vsprintf extension %p<foo> misuses
> + if ($line =~ /\b$logFunctions\s*\(.*$String/) {
> + my $format = get_quoted_string($line, $rawline);
> + if ($format =~
> /(\%[\*\d\.]*p(?![\WFfSsBKRraEhMmIiUDdgVCbGN]).)/) {
> + WARN("VSPRINTF_POINTER_EXTENSION",
> + "Invalid vsprintf pointer extension '$1'\n" .
> $herecurr);
> + }
> + }
> +
> # check for logging continuations
> if ($line =~ /\bprintk\s*\(\s*KERN_CONT\b|\bpr_cont\s*\(/) {
> WARN("LOGGING_CONTINUATION",
^ permalink raw reply [flat|nested] 50+ messages in thread
* RE: [PATCH] checkpatch: add warning on %pk instead of %pK usage
2017-02-10 22:14 ` [kernel-hardening] " Roberts, William C
@ 2017-02-10 22:26 ` Roberts, William C
-1 siblings, 0 replies; 50+ messages in thread
From: Roberts, William C @ 2017-02-10 22:26 UTC (permalink / raw)
To: Roberts, William C, Joe Perches, linux-kernel, apw, Andew Morton
Cc: keescook, kernel-hardening
<snip>
> >
> > On Fri, 2017-02-10 at 11:37 -0800, william.c.roberts@intel.com wrote:
> > > From: William Roberts <william.c.roberts@intel.com>
> > >
> > > Sample output:
> > > WARNING: %pk is close to %pK, did you mean %pK?.
> > > \#20: FILE: drivers/char/applicom.c:230:
> > > + printk(KERN_INFO "Could not allocate IRQ %d for PCI
> > Applicom
> > > +device. %pk\n", dev->irq, pci_get_class);
> >
> > There isn't a single instance of this in the kernel tree.
> >
> > Maybe if this is really useful, then all the %p<foo> extensions should
> > be enumerated and all unknown uses should have warnings.
>
> I was thinking of doing that, but I figured I would start with the bare minimum
> patch.
>
> >
> > Something like:
> >
> > ---
> > scripts/checkpatch.pl | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index
> > ad5ea5c545b2..8a90b457e8b5 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -5305,6 +5305,15 @@ sub process {
> > }
> > }
> >
> > +# check for vsprintf extension %p<foo> misuses
> > + if ($line =~ /\b$logFunctions\s*\(.*$String/) {
I don't see the normal string formatting routines in that list... I think this is too restrictive.
> > + my $format = get_quoted_string($line, $rawline);
Ahh thanks for that get_quoted_string().
> > + if ($format =~
> > /(\%[\*\d\.]*p(?![\WFfSsBKRraEhMmIiUDdgVCbGN]).)/) {
> > + WARN("VSPRINTF_POINTER_EXTENSION",
> > + "Invalid vsprintf pointer extension '$1'\n" .
> > $herecurr);
I think I'll send out a V2 with this part of the addition. I like that, and your wording.
> > + }
> > + }
> > +
> > # check for logging continuations
> > if ($line =~ /\bprintk\s*\(\s*KERN_CONT\b|\bpr_cont\s*\(/) {
> > WARN("LOGGING_CONTINUATION",
I did a grep on some of the patters to see what it would match against
^ permalink raw reply [flat|nested] 50+ messages in thread
* [kernel-hardening] RE: [PATCH] checkpatch: add warning on %pk instead of %pK usage
@ 2017-02-10 22:26 ` Roberts, William C
0 siblings, 0 replies; 50+ messages in thread
From: Roberts, William C @ 2017-02-10 22:26 UTC (permalink / raw)
To: Roberts, William C, Joe Perches, linux-kernel, apw, Andew Morton
Cc: keescook, kernel-hardening
<snip>
> >
> > On Fri, 2017-02-10 at 11:37 -0800, william.c.roberts@intel.com wrote:
> > > From: William Roberts <william.c.roberts@intel.com>
> > >
> > > Sample output:
> > > WARNING: %pk is close to %pK, did you mean %pK?.
> > > \#20: FILE: drivers/char/applicom.c:230:
> > > + printk(KERN_INFO "Could not allocate IRQ %d for PCI
> > Applicom
> > > +device. %pk\n", dev->irq, pci_get_class);
> >
> > There isn't a single instance of this in the kernel tree.
> >
> > Maybe if this is really useful, then all the %p<foo> extensions should
> > be enumerated and all unknown uses should have warnings.
>
> I was thinking of doing that, but I figured I would start with the bare minimum
> patch.
>
> >
> > Something like:
> >
> > ---
> > scripts/checkpatch.pl | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index
> > ad5ea5c545b2..8a90b457e8b5 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -5305,6 +5305,15 @@ sub process {
> > }
> > }
> >
> > +# check for vsprintf extension %p<foo> misuses
> > + if ($line =~ /\b$logFunctions\s*\(.*$String/) {
I don't see the normal string formatting routines in that list... I think this is too restrictive.
> > + my $format = get_quoted_string($line, $rawline);
Ahh thanks for that get_quoted_string().
> > + if ($format =~
> > /(\%[\*\d\.]*p(?![\WFfSsBKRraEhMmIiUDdgVCbGN]).)/) {
> > + WARN("VSPRINTF_POINTER_EXTENSION",
> > + "Invalid vsprintf pointer extension '$1'\n" .
> > $herecurr);
I think I'll send out a V2 with this part of the addition. I like that, and your wording.
> > + }
> > + }
> > +
> > # check for logging continuations
> > if ($line =~ /\bprintk\s*\(\s*KERN_CONT\b|\bpr_cont\s*\(/) {
> > WARN("LOGGING_CONTINUATION",
I did a grep on some of the patters to see what it would match against
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH] checkpatch: add warning on %pk instead of %pK usage
2017-02-10 22:26 ` [kernel-hardening] " Roberts, William C
@ 2017-02-10 22:49 ` Joe Perches
-1 siblings, 0 replies; 50+ messages in thread
From: Joe Perches @ 2017-02-10 22:49 UTC (permalink / raw)
To: Roberts, William C, linux-kernel, apw, Andew Morton
Cc: keescook, kernel-hardening
On Fri, 2017-02-10 at 22:26 +0000, Roberts, William C wrote:
> <snip>
>
> > >
> > > On Fri, 2017-02-10 at 11:37 -0800, william.c.roberts@intel.com wrote:
> > > > From: William Roberts <william.c.roberts@intel.com>
> > > >
> > > > Sample output:
> > > > WARNING: %pk is close to %pK, did you mean %pK?.
> > > > \#20: FILE: drivers/char/applicom.c:230:
> > > > + printk(KERN_INFO "Could not allocate IRQ %d for PCI
> > >
> > > Applicom
> > > > +device. %pk\n", dev->irq, pci_get_class);
> > >
> > > There isn't a single instance of this in the kernel tree.
> > >
> > > Maybe if this is really useful, then all the %p<foo> extensions should
> > > be enumerated and all unknown uses should have warnings.
> >
> > I was thinking of doing that, but I figured I would start with the bare minimum
> > patch.
> >
> > >
> > > Something like:
> > >
> > > ---
> > > scripts/checkpatch.pl | 9 +++++++++
> > > 1 file changed, 9 insertions(+)
> > >
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index
> > > ad5ea5c545b2..8a90b457e8b5 100755
> > > --- a/scripts/checkpatch.pl
> > > +++ b/scripts/checkpatch.pl
> > > @@ -5305,6 +5305,15 @@ sub process {
> > > }
> > > }
> > >
> > > +# check for vsprintf extension %p<foo> misuses
> > > + if ($line =~ /\b$logFunctions\s*\(.*$String/) {
>
> I don't see the normal string formatting routines in that list... I think this is too restrictive.
I don't. There are no "normal" string formatting routines.
What do you think is missing? sn?printf ? That's easy to add.
^ permalink raw reply [flat|nested] 50+ messages in thread
* [kernel-hardening] Re: [PATCH] checkpatch: add warning on %pk instead of %pK usage
@ 2017-02-10 22:49 ` Joe Perches
0 siblings, 0 replies; 50+ messages in thread
From: Joe Perches @ 2017-02-10 22:49 UTC (permalink / raw)
To: Roberts, William C, linux-kernel, apw, Andew Morton
Cc: keescook, kernel-hardening
On Fri, 2017-02-10 at 22:26 +0000, Roberts, William C wrote:
> <snip>
>
> > >
> > > On Fri, 2017-02-10 at 11:37 -0800, william.c.roberts@intel.com wrote:
> > > > From: William Roberts <william.c.roberts@intel.com>
> > > >
> > > > Sample output:
> > > > WARNING: %pk is close to %pK, did you mean %pK?.
> > > > \#20: FILE: drivers/char/applicom.c:230:
> > > > + printk(KERN_INFO "Could not allocate IRQ %d for PCI
> > >
> > > Applicom
> > > > +device. %pk\n", dev->irq, pci_get_class);
> > >
> > > There isn't a single instance of this in the kernel tree.
> > >
> > > Maybe if this is really useful, then all the %p<foo> extensions should
> > > be enumerated and all unknown uses should have warnings.
> >
> > I was thinking of doing that, but I figured I would start with the bare minimum
> > patch.
> >
> > >
> > > Something like:
> > >
> > > ---
> > > scripts/checkpatch.pl | 9 +++++++++
> > > 1 file changed, 9 insertions(+)
> > >
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index
> > > ad5ea5c545b2..8a90b457e8b5 100755
> > > --- a/scripts/checkpatch.pl
> > > +++ b/scripts/checkpatch.pl
> > > @@ -5305,6 +5305,15 @@ sub process {
> > > }
> > > }
> > >
> > > +# check for vsprintf extension %p<foo> misuses
> > > + if ($line =~ /\b$logFunctions\s*\(.*$String/) {
>
> I don't see the normal string formatting routines in that list... I think this is too restrictive.
I don't. There are no "normal" string formatting routines.
What do you think is missing? sn?printf ? That's easy to add.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH] checkpatch: add warning on %pk instead of %pK usage
2017-02-10 22:49 ` [kernel-hardening] " Joe Perches
@ 2017-02-10 22:59 ` Joe Perches
-1 siblings, 0 replies; 50+ messages in thread
From: Joe Perches @ 2017-02-10 22:59 UTC (permalink / raw)
To: Roberts, William C, linux-kernel, apw, Andew Morton
Cc: keescook, kernel-hardening
On Fri, 2017-02-10 at 14:49 -0800, Joe Perches wrote:
> On Fri, 2017-02-10 at 22:26 +0000, Roberts, William C wrote:
> > <snip>
> >
> > > >
> > > > On Fri, 2017-02-10 at 11:37 -0800, william.c.roberts@intel.com wrote:
> > > > > From: William Roberts <william.c.roberts@intel.com>
> > > > >
> > > > > Sample output:
> > > > > WARNING: %pk is close to %pK, did you mean %pK?.
> > > > > \#20: FILE: drivers/char/applicom.c:230:
> > > > > + printk(KERN_INFO "Could not allocate IRQ %d for PCI
> > > >
> > > > Applicom
> > > > > +device. %pk\n", dev->irq, pci_get_class);
> > > >
> > > > There isn't a single instance of this in the kernel tree.
Just in case anyone else wondered why this came up.
https://googleprojectzero.blogspot.com/2017/02/lifting-hyper-visor-bypassing-samsungs.html
^ permalink raw reply [flat|nested] 50+ messages in thread
* [kernel-hardening] Re: [PATCH] checkpatch: add warning on %pk instead of %pK usage
@ 2017-02-10 22:59 ` Joe Perches
0 siblings, 0 replies; 50+ messages in thread
From: Joe Perches @ 2017-02-10 22:59 UTC (permalink / raw)
To: Roberts, William C, linux-kernel, apw, Andew Morton
Cc: keescook, kernel-hardening
On Fri, 2017-02-10 at 14:49 -0800, Joe Perches wrote:
> On Fri, 2017-02-10 at 22:26 +0000, Roberts, William C wrote:
> > <snip>
> >
> > > >
> > > > On Fri, 2017-02-10 at 11:37 -0800, william.c.roberts@intel.com wrote:
> > > > > From: William Roberts <william.c.roberts@intel.com>
> > > > >
> > > > > Sample output:
> > > > > WARNING: %pk is close to %pK, did you mean %pK?.
> > > > > \#20: FILE: drivers/char/applicom.c:230:
> > > > > + printk(KERN_INFO "Could not allocate IRQ %d for PCI
> > > >
> > > > Applicom
> > > > > +device. %pk\n", dev->irq, pci_get_class);
> > > >
> > > > There isn't a single instance of this in the kernel tree.
Just in case anyone else wondered why this came up.
https://googleprojectzero.blogspot.com/2017/02/lifting-hyper-visor-bypassing-samsungs.html
^ permalink raw reply [flat|nested] 50+ messages in thread
* RE: [PATCH] checkpatch: add warning on %pk instead of %pK usage
2017-02-10 22:49 ` [kernel-hardening] " Joe Perches
@ 2017-02-10 23:31 ` Roberts, William C
-1 siblings, 0 replies; 50+ messages in thread
From: Roberts, William C @ 2017-02-10 23:31 UTC (permalink / raw)
To: Joe Perches, linux-kernel, apw, Andew Morton; +Cc: keescook, kernel-hardening
> -----Original Message-----
> From: Joe Perches [mailto:joe@perches.com]
> Sent: Friday, February 10, 2017 2:50 PM
> To: Roberts, William C <william.c.roberts@intel.com>; linux-
> kernel@vger.kernel.org; apw@canonical.com; Andew Morton <akpm@linux-
> foundation.org>
> Cc: keescook@chromium.org; kernel-hardening@lists.openwall.com
> Subject: Re: [PATCH] checkpatch: add warning on %pk instead of %pK usage
>
> On Fri, 2017-02-10 at 22:26 +0000, Roberts, William C wrote:
> > <snip>
> >
> > > >
> > > > On Fri, 2017-02-10 at 11:37 -0800, william.c.roberts@intel.com wrote:
> > > > > From: William Roberts <william.c.roberts@intel.com>
> > > > >
> > > > > Sample output:
> > > > > WARNING: %pk is close to %pK, did you mean %pK?.
> > > > > \#20: FILE: drivers/char/applicom.c:230:
> > > > > + printk(KERN_INFO "Could not allocate IRQ %d for
> PCI
> > > >
> > > > Applicom
> > > > > +device. %pk\n", dev->irq, pci_get_class);
> > > >
> > > > There isn't a single instance of this in the kernel tree.
> > > >
> > > > Maybe if this is really useful, then all the %p<foo> extensions
> > > > should be enumerated and all unknown uses should have warnings.
> > >
> > > I was thinking of doing that, but I figured I would start with the
> > > bare minimum patch.
> > >
> > > >
> > > > Something like:
> > > >
> > > > ---
> > > > scripts/checkpatch.pl | 9 +++++++++
> > > > 1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index
> > > > ad5ea5c545b2..8a90b457e8b5 100755
> > > > --- a/scripts/checkpatch.pl
> > > > +++ b/scripts/checkpatch.pl
> > > > @@ -5305,6 +5305,15 @@ sub process {
> > > > }
> > > > }
> > > >
> > > > +# check for vsprintf extension %p<foo> misuses
> > > > + if ($line =~ /\b$logFunctions\s*\(.*$String/) {
> >
> > I don't see the normal string formatting routines in that list... I think this is too
> restrictive.
>
> I don't. There are no "normal" string formatting routines.
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
Data:
arch/alpha/kernel/pci_iommu.c:25:# define DBGA(args...) printk(KERN_DEBUG args)
arch/alpha/kernel/pci_iommu.c:30:# define DBGA2(args...) printk(KERN_DEBUG args)
arch/alpha/kernel/core_tsunami.c:50:# define DBG_CFG(args) printk args
arch/alpha/kernel/core_titan.c:50:# define DBG_CFG(args) printk args
arch/alpha/kernel/ptrace.c:34:#define DBG(fac,args) {if ((fac) & DEBUG) printk args;}
arch/alpha/kernel/core_apecs.c:42:# define DBGC(args) printk args
arch/alpha/kernel/core_irongate.c:38:# define DBG_CFG(args) printk args
arch/alpha/kernel/core_wildfire.c:30:# define DBG_CFG(args) printk args
arch/alpha/kernel/smc37c93x.c:18:# define DBG_DEVS(args) printk args
arch/alpha/boot/misc.c:27:#define puts srm_printk
arch/alpha/mm/numa.c:27:#define DBGDCONT(args...) printk(args)
arch/powerpc/sysdev/tsi108_pci.c:43:#define DBG(x...) printk(x)
arch/powerpc/sysdev/ge/ge_pic.c:31:#define DBG(fmt...) do { printk(KERN_DEBUG "gef_pic: " fmt); } while (0)
arch/powerpc/sysdev/tsi108_dev.c:34:#define DBG(fmt...) do { printk(fmt); } while(0)
arch/powerpc/sysdev/mpic.c:45:#define DBG(fmt...) printk(fmt)
arch/powerpc/kernel/process.c:69:#define TM_DEBUG(x...) printk(KERN_INFO x)
arch/powerpc/kernel/vdso.c:42:#define DBG(fmt...) printk(fmt)
arch/powerpc/kernel/legacy_serial.c:21:#define DBG(fmt...) do { printk(fmt); } while(0)
arch/powerpc/kernel/traps.c:89:#define TM_DEBUG(x...) printk(KERN_INFO x)
arch/powerpc/kernel/prom.c:65:#define DBG(fmt...) printk(KERN_ERR fmt)
arch/powerpc/kvm/book3s_paired_singles.c:33:#define dprintk printk
^ permalink raw reply [flat|nested] 50+ messages in thread
* [kernel-hardening] RE: [PATCH] checkpatch: add warning on %pk instead of %pK usage
@ 2017-02-10 23:31 ` Roberts, William C
0 siblings, 0 replies; 50+ messages in thread
From: Roberts, William C @ 2017-02-10 23:31 UTC (permalink / raw)
To: Joe Perches, linux-kernel, apw, Andew Morton; +Cc: keescook, kernel-hardening
> -----Original Message-----
> From: Joe Perches [mailto:joe@perches.com]
> Sent: Friday, February 10, 2017 2:50 PM
> To: Roberts, William C <william.c.roberts@intel.com>; linux-
> kernel@vger.kernel.org; apw@canonical.com; Andew Morton <akpm@linux-
> foundation.org>
> Cc: keescook@chromium.org; kernel-hardening@lists.openwall.com
> Subject: Re: [PATCH] checkpatch: add warning on %pk instead of %pK usage
>
> On Fri, 2017-02-10 at 22:26 +0000, Roberts, William C wrote:
> > <snip>
> >
> > > >
> > > > On Fri, 2017-02-10 at 11:37 -0800, william.c.roberts@intel.com wrote:
> > > > > From: William Roberts <william.c.roberts@intel.com>
> > > > >
> > > > > Sample output:
> > > > > WARNING: %pk is close to %pK, did you mean %pK?.
> > > > > \#20: FILE: drivers/char/applicom.c:230:
> > > > > + printk(KERN_INFO "Could not allocate IRQ %d for
> PCI
> > > >
> > > > Applicom
> > > > > +device. %pk\n", dev->irq, pci_get_class);
> > > >
> > > > There isn't a single instance of this in the kernel tree.
> > > >
> > > > Maybe if this is really useful, then all the %p<foo> extensions
> > > > should be enumerated and all unknown uses should have warnings.
> > >
> > > I was thinking of doing that, but I figured I would start with the
> > > bare minimum patch.
> > >
> > > >
> > > > Something like:
> > > >
> > > > ---
> > > > scripts/checkpatch.pl | 9 +++++++++
> > > > 1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index
> > > > ad5ea5c545b2..8a90b457e8b5 100755
> > > > --- a/scripts/checkpatch.pl
> > > > +++ b/scripts/checkpatch.pl
> > > > @@ -5305,6 +5305,15 @@ sub process {
> > > > }
> > > > }
> > > >
> > > > +# check for vsprintf extension %p<foo> misuses
> > > > + if ($line =~ /\b$logFunctions\s*\(.*$String/) {
> >
> > I don't see the normal string formatting routines in that list... I think this is too
> restrictive.
>
> I don't. There are no "normal" string formatting routines.
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
Data:
arch/alpha/kernel/pci_iommu.c:25:# define DBGA(args...) printk(KERN_DEBUG args)
arch/alpha/kernel/pci_iommu.c:30:# define DBGA2(args...) printk(KERN_DEBUG args)
arch/alpha/kernel/core_tsunami.c:50:# define DBG_CFG(args) printk args
arch/alpha/kernel/core_titan.c:50:# define DBG_CFG(args) printk args
arch/alpha/kernel/ptrace.c:34:#define DBG(fac,args) {if ((fac) & DEBUG) printk args;}
arch/alpha/kernel/core_apecs.c:42:# define DBGC(args) printk args
arch/alpha/kernel/core_irongate.c:38:# define DBG_CFG(args) printk args
arch/alpha/kernel/core_wildfire.c:30:# define DBG_CFG(args) printk args
arch/alpha/kernel/smc37c93x.c:18:# define DBG_DEVS(args) printk args
arch/alpha/boot/misc.c:27:#define puts srm_printk
arch/alpha/mm/numa.c:27:#define DBGDCONT(args...) printk(args)
arch/powerpc/sysdev/tsi108_pci.c:43:#define DBG(x...) printk(x)
arch/powerpc/sysdev/ge/ge_pic.c:31:#define DBG(fmt...) do { printk(KERN_DEBUG "gef_pic: " fmt); } while (0)
arch/powerpc/sysdev/tsi108_dev.c:34:#define DBG(fmt...) do { printk(fmt); } while(0)
arch/powerpc/sysdev/mpic.c:45:#define DBG(fmt...) printk(fmt)
arch/powerpc/kernel/process.c:69:#define TM_DEBUG(x...) printk(KERN_INFO x)
arch/powerpc/kernel/vdso.c:42:#define DBG(fmt...) printk(fmt)
arch/powerpc/kernel/legacy_serial.c:21:#define DBG(fmt...) do { printk(fmt); } while(0)
arch/powerpc/kernel/traps.c:89:#define TM_DEBUG(x...) printk(KERN_INFO x)
arch/powerpc/kernel/prom.c:65:#define DBG(fmt...) printk(KERN_ERR fmt)
arch/powerpc/kvm/book3s_paired_singles.c:33:#define dprintk printk
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH] checkpatch: add warning on %pk instead of %pK usage
2017-02-10 23:31 ` [kernel-hardening] " Roberts, William C
@ 2017-02-10 23:49 ` Joe Perches
-1 siblings, 0 replies; 50+ messages in thread
From: Joe Perches @ 2017-02-10 23:49 UTC (permalink / raw)
To: Roberts, William C, linux-kernel, apw, Andew Morton
Cc: keescook, kernel-hardening, Julia Lawall, Emese Revfy
(adding Emese Revfy and Julia Lawall)
On Fri, 2017-02-10 at 23:31 +0000, Roberts, William C wrote:
> 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
checkpatch is a line-oriented bunch of regexes
and doesn't know what is a __printf format.
It won't ever be "perfect" for this sort of
format verification checking.
Another way to do this is to write a gcc compiler
plugin that verifies the %p<foo> format types and
emits a warning/error.
That's probably the "best" solution.
Maybe coccinelle could help too.
^ permalink raw reply [flat|nested] 50+ messages in thread
* [kernel-hardening] Re: [PATCH] checkpatch: add warning on %pk instead of %pK usage
@ 2017-02-10 23:49 ` Joe Perches
0 siblings, 0 replies; 50+ messages in thread
From: Joe Perches @ 2017-02-10 23:49 UTC (permalink / raw)
To: Roberts, William C, linux-kernel, apw, Andew Morton
Cc: keescook, kernel-hardening, Julia Lawall, Emese Revfy
(adding Emese Revfy and Julia Lawall)
On Fri, 2017-02-10 at 23:31 +0000, Roberts, William C wrote:
> 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
checkpatch is a line-oriented bunch of regexes
and doesn't know what is a __printf format.
It won't ever be "perfect" for this sort of
format verification checking.
Another way to do this is to write a gcc compiler
plugin that verifies the %p<foo> format types and
emits a warning/error.
That's probably the "best" solution.
Maybe coccinelle could help too.
^ permalink raw reply [flat|nested] 50+ messages in thread
* RE: [PATCH] checkpatch: add warning on %pk instead of %pK usage
2017-02-10 22:49 ` [kernel-hardening] " Joe Perches
@ 2017-02-10 23:54 ` Roberts, William C
-1 siblings, 0 replies; 50+ messages in thread
From: Roberts, William C @ 2017-02-10 23:54 UTC (permalink / raw)
To: Joe Perches, linux-kernel, apw, Andew Morton; +Cc: keescook, kernel-hardening
> -----Original Message-----
> From: Roberts, William C
> Sent: Friday, February 10, 2017 3:32 PM
> To: 'Joe Perches' <joe@perches.com>; linux-kernel@vger.kernel.org;
> apw@canonical.com; Andew Morton <akpm@linux-foundation.org>
> Cc: keescook@chromium.org; kernel-hardening@lists.openwall.com
> Subject: RE: [PATCH] checkpatch: add warning on %pk instead of %pK usage
>
>
>
> > -----Original Message-----
> > From: Joe Perches [mailto:joe@perches.com]
> > Sent: Friday, February 10, 2017 2:50 PM
> > To: Roberts, William C <william.c.roberts@intel.com>; linux-
> > kernel@vger.kernel.org; apw@canonical.com; Andew Morton <akpm@linux-
> > foundation.org>
> > Cc: keescook@chromium.org; kernel-hardening@lists.openwall.com
> > Subject: Re: [PATCH] checkpatch: add warning on %pk instead of %pK
> > usage
> >
> > On Fri, 2017-02-10 at 22:26 +0000, Roberts, William C wrote:
> > > <snip>
> > >
> > > > >
> > > > > On Fri, 2017-02-10 at 11:37 -0800, william.c.roberts@intel.com wrote:
> > > > > > From: William Roberts <william.c.roberts@intel.com>
> > > > > >
> > > > > > Sample output:
> > > > > > WARNING: %pk is close to %pK, did you mean %pK?.
> > > > > > \#20: FILE: drivers/char/applicom.c:230:
> > > > > > + printk(KERN_INFO "Could not allocate IRQ %d for
> > PCI
> > > > >
> > > > > Applicom
> > > > > > +device. %pk\n", dev->irq, pci_get_class);
> > > > >
> > > > > There isn't a single instance of this in the kernel tree.
> > > > >
> > > > > Maybe if this is really useful, then all the %p<foo> extensions
> > > > > should be enumerated and all unknown uses should have warnings.
> > > >
> > > > I was thinking of doing that, but I figured I would start with the
> > > > bare minimum patch.
> > > >
> > > > >
> > > > > Something like:
> > > > >
> > > > > ---
> > > > > scripts/checkpatch.pl | 9 +++++++++
> > > > > 1 file changed, 9 insertions(+)
> > > > >
> > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index
> > > > > ad5ea5c545b2..8a90b457e8b5 100755
> > > > > --- a/scripts/checkpatch.pl
> > > > > +++ b/scripts/checkpatch.pl
> > > > > @@ -5305,6 +5305,15 @@ sub process {
> > > > > }
> > > > > }
> > > > >
> > > > > +# check for vsprintf extension %p<foo> misuses
> > > > > + if ($line =~ /\b$logFunctions\s*\(.*$String/) {
> > >
> > > I don't see the normal string formatting routines in that list... I
> > > think this is too
> > restrictive.
> >
> > I don't. There are no "normal" string formatting routines.
>
> 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
Sent v3 --> Let me think on something better than items 2 and 3. We really want to
Know if were looking at at a string that is in a function or something there about.
Everyone has their own print routines... which is why I am in favor of neutering %p
within vsprintf itself.
>
> Data:
> arch/alpha/kernel/pci_iommu.c:25:# define DBGA(args...)
> printk(KERN_DEBUG args)
> arch/alpha/kernel/pci_iommu.c:30:# define DBGA2(args...)
> printk(KERN_DEBUG args)
> arch/alpha/kernel/core_tsunami.c:50:# define DBG_CFG(args) printk args
> arch/alpha/kernel/core_titan.c:50:# define DBG_CFG(args) printk args
> arch/alpha/kernel/ptrace.c:34:#define DBG(fac,args) {if ((fac) & DEBUG) printk
> args;}
> arch/alpha/kernel/core_apecs.c:42:# define DBGC(args) printk args
> arch/alpha/kernel/core_irongate.c:38:# define DBG_CFG(args) printk args
> arch/alpha/kernel/core_wildfire.c:30:# define DBG_CFG(args) printk args
> arch/alpha/kernel/smc37c93x.c:18:# define DBG_DEVS(args) printk args
> arch/alpha/boot/misc.c:27:#define puts srm_printk
> arch/alpha/mm/numa.c:27:#define DBGDCONT(args...) printk(args)
> arch/powerpc/sysdev/tsi108_pci.c:43:#define DBG(x...) printk(x)
> arch/powerpc/sysdev/ge/ge_pic.c:31:#define DBG(fmt...) do {
> printk(KERN_DEBUG "gef_pic: " fmt); } while (0)
> arch/powerpc/sysdev/tsi108_dev.c:34:#define DBG(fmt...) do { printk(fmt); }
> while(0) arch/powerpc/sysdev/mpic.c:45:#define DBG(fmt...) printk(fmt)
> arch/powerpc/kernel/process.c:69:#define TM_DEBUG(x...) printk(KERN_INFO
> x) arch/powerpc/kernel/vdso.c:42:#define DBG(fmt...) printk(fmt)
> arch/powerpc/kernel/legacy_serial.c:21:#define DBG(fmt...) do { printk(fmt); }
> while(0) arch/powerpc/kernel/traps.c:89:#define TM_DEBUG(x...)
> printk(KERN_INFO x) arch/powerpc/kernel/prom.c:65:#define DBG(fmt...)
> printk(KERN_ERR fmt) arch/powerpc/kvm/book3s_paired_singles.c:33:#define
> dprintk printk
>
^ permalink raw reply [flat|nested] 50+ messages in thread
* [kernel-hardening] RE: [PATCH] checkpatch: add warning on %pk instead of %pK usage
@ 2017-02-10 23:54 ` Roberts, William C
0 siblings, 0 replies; 50+ messages in thread
From: Roberts, William C @ 2017-02-10 23:54 UTC (permalink / raw)
To: Joe Perches, linux-kernel, apw, Andew Morton; +Cc: keescook, kernel-hardening
> -----Original Message-----
> From: Roberts, William C
> Sent: Friday, February 10, 2017 3:32 PM
> To: 'Joe Perches' <joe@perches.com>; linux-kernel@vger.kernel.org;
> apw@canonical.com; Andew Morton <akpm@linux-foundation.org>
> Cc: keescook@chromium.org; kernel-hardening@lists.openwall.com
> Subject: RE: [PATCH] checkpatch: add warning on %pk instead of %pK usage
>
>
>
> > -----Original Message-----
> > From: Joe Perches [mailto:joe@perches.com]
> > Sent: Friday, February 10, 2017 2:50 PM
> > To: Roberts, William C <william.c.roberts@intel.com>; linux-
> > kernel@vger.kernel.org; apw@canonical.com; Andew Morton <akpm@linux-
> > foundation.org>
> > Cc: keescook@chromium.org; kernel-hardening@lists.openwall.com
> > Subject: Re: [PATCH] checkpatch: add warning on %pk instead of %pK
> > usage
> >
> > On Fri, 2017-02-10 at 22:26 +0000, Roberts, William C wrote:
> > > <snip>
> > >
> > > > >
> > > > > On Fri, 2017-02-10 at 11:37 -0800, william.c.roberts@intel.com wrote:
> > > > > > From: William Roberts <william.c.roberts@intel.com>
> > > > > >
> > > > > > Sample output:
> > > > > > WARNING: %pk is close to %pK, did you mean %pK?.
> > > > > > \#20: FILE: drivers/char/applicom.c:230:
> > > > > > + printk(KERN_INFO "Could not allocate IRQ %d for
> > PCI
> > > > >
> > > > > Applicom
> > > > > > +device. %pk\n", dev->irq, pci_get_class);
> > > > >
> > > > > There isn't a single instance of this in the kernel tree.
> > > > >
> > > > > Maybe if this is really useful, then all the %p<foo> extensions
> > > > > should be enumerated and all unknown uses should have warnings.
> > > >
> > > > I was thinking of doing that, but I figured I would start with the
> > > > bare minimum patch.
> > > >
> > > > >
> > > > > Something like:
> > > > >
> > > > > ---
> > > > > scripts/checkpatch.pl | 9 +++++++++
> > > > > 1 file changed, 9 insertions(+)
> > > > >
> > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index
> > > > > ad5ea5c545b2..8a90b457e8b5 100755
> > > > > --- a/scripts/checkpatch.pl
> > > > > +++ b/scripts/checkpatch.pl
> > > > > @@ -5305,6 +5305,15 @@ sub process {
> > > > > }
> > > > > }
> > > > >
> > > > > +# check for vsprintf extension %p<foo> misuses
> > > > > + if ($line =~ /\b$logFunctions\s*\(.*$String/) {
> > >
> > > I don't see the normal string formatting routines in that list... I
> > > think this is too
> > restrictive.
> >
> > I don't. There are no "normal" string formatting routines.
>
> 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
Sent v3 --> Let me think on something better than items 2 and 3. We really want to
Know if were looking at at a string that is in a function or something there about.
Everyone has their own print routines... which is why I am in favor of neutering %p
within vsprintf itself.
>
> Data:
> arch/alpha/kernel/pci_iommu.c:25:# define DBGA(args...)
> printk(KERN_DEBUG args)
> arch/alpha/kernel/pci_iommu.c:30:# define DBGA2(args...)
> printk(KERN_DEBUG args)
> arch/alpha/kernel/core_tsunami.c:50:# define DBG_CFG(args) printk args
> arch/alpha/kernel/core_titan.c:50:# define DBG_CFG(args) printk args
> arch/alpha/kernel/ptrace.c:34:#define DBG(fac,args) {if ((fac) & DEBUG) printk
> args;}
> arch/alpha/kernel/core_apecs.c:42:# define DBGC(args) printk args
> arch/alpha/kernel/core_irongate.c:38:# define DBG_CFG(args) printk args
> arch/alpha/kernel/core_wildfire.c:30:# define DBG_CFG(args) printk args
> arch/alpha/kernel/smc37c93x.c:18:# define DBG_DEVS(args) printk args
> arch/alpha/boot/misc.c:27:#define puts srm_printk
> arch/alpha/mm/numa.c:27:#define DBGDCONT(args...) printk(args)
> arch/powerpc/sysdev/tsi108_pci.c:43:#define DBG(x...) printk(x)
> arch/powerpc/sysdev/ge/ge_pic.c:31:#define DBG(fmt...) do {
> printk(KERN_DEBUG "gef_pic: " fmt); } while (0)
> arch/powerpc/sysdev/tsi108_dev.c:34:#define DBG(fmt...) do { printk(fmt); }
> while(0) arch/powerpc/sysdev/mpic.c:45:#define DBG(fmt...) printk(fmt)
> arch/powerpc/kernel/process.c:69:#define TM_DEBUG(x...) printk(KERN_INFO
> x) arch/powerpc/kernel/vdso.c:42:#define DBG(fmt...) printk(fmt)
> arch/powerpc/kernel/legacy_serial.c:21:#define DBG(fmt...) do { printk(fmt); }
> while(0) arch/powerpc/kernel/traps.c:89:#define TM_DEBUG(x...)
> printk(KERN_INFO x) arch/powerpc/kernel/prom.c:65:#define DBG(fmt...)
> printk(KERN_ERR fmt) arch/powerpc/kvm/book3s_paired_singles.c:33:#define
> dprintk printk
>
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH] checkpatch: add warning on %pk instead of %pK usage
2017-02-10 23:54 ` [kernel-hardening] " Roberts, William C
@ 2017-02-11 0:01 ` Joe Perches
-1 siblings, 0 replies; 50+ messages in thread
From: Joe Perches @ 2017-02-11 0:01 UTC (permalink / raw)
To: Roberts, William C, linux-kernel, apw, Andew Morton
Cc: keescook, kernel-hardening
On Fri, 2017-02-10 at 23:54 +0000, Roberts, William C wrote:
> > 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).
It's not just the "hairy" local macros.
In its current form, checkpatch could not find uses like:
netif_<foo>(x, y, z,
"some string with %pk",
args);
and
some_logging_function(arg, "string 1" CONSTANT "string 2", etc...)
if string 2 or CONSTANT had the "%pk" use.
and a bunch of other styles.
This really needs to be verified by the compiler.
^ permalink raw reply [flat|nested] 50+ messages in thread
* [kernel-hardening] Re: [PATCH] checkpatch: add warning on %pk instead of %pK usage
@ 2017-02-11 0:01 ` Joe Perches
0 siblings, 0 replies; 50+ messages in thread
From: Joe Perches @ 2017-02-11 0:01 UTC (permalink / raw)
To: Roberts, William C, linux-kernel, apw, Andew Morton
Cc: keescook, kernel-hardening
On Fri, 2017-02-10 at 23:54 +0000, Roberts, William C wrote:
> > 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).
It's not just the "hairy" local macros.
In its current form, checkpatch could not find uses like:
netif_<foo>(x, y, z,
"some string with %pk",
args);
and
some_logging_function(arg, "string 1" CONSTANT "string 2", etc...)
if string 2 or CONSTANT had the "%pk" use.
and a bunch of other styles.
This really needs to be verified by the compiler.
^ permalink raw reply [flat|nested] 50+ messages in thread
* RE: [PATCH] checkpatch: add warning on %pk instead of %pK usage
2017-02-10 22:49 ` [kernel-hardening] " Joe Perches
@ 2017-02-11 1:32 ` Roberts, William C
-1 siblings, 0 replies; 50+ messages in thread
From: Roberts, William C @ 2017-02-11 1:32 UTC (permalink / raw)
To: Joe Perches, linux-kernel, apw; +Cc: kernel-hardening
<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?
^ permalink raw reply [flat|nested] 50+ messages in thread
* [kernel-hardening] RE: [PATCH] checkpatch: add warning on %pk instead of %pK usage
@ 2017-02-11 1:32 ` Roberts, William C
0 siblings, 0 replies; 50+ messages in thread
From: Roberts, William C @ 2017-02-11 1:32 UTC (permalink / raw)
To: Joe Perches, linux-kernel, apw; +Cc: kernel-hardening
<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?
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH] checkpatch: add warning on %pk instead of %pK usage
2017-02-11 1:32 ` [kernel-hardening] " Roberts, William C
@ 2017-02-11 3:23 ` Joe Perches
-1 siblings, 0 replies; 50+ messages in thread
From: Joe Perches @ 2017-02-11 3:23 UTC (permalink / raw)
To: Roberts, William C, linux-kernel, apw; +Cc: kernel-hardening
[-- 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
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [kernel-hardening] Re: [PATCH] checkpatch: add warning on %pk instead of %pK usage
@ 2017-02-11 3:23 ` Joe Perches
0 siblings, 0 replies; 50+ messages in thread
From: Joe Perches @ 2017-02-11 3:23 UTC (permalink / raw)
To: Roberts, William C, linux-kernel, apw; +Cc: kernel-hardening
[-- 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
^ permalink raw reply related [flat|nested] 50+ messages in thread
* RE: [PATCH] checkpatch: add warning on %pk instead of %pK usage
2017-02-11 3:23 ` [kernel-hardening] " Joe Perches
@ 2017-02-13 19:46 ` Roberts, William C
-1 siblings, 0 replies; 50+ messages in thread
From: Roberts, William C @ 2017-02-13 19:46 UTC (permalink / raw)
To: Joe Perches, linux-kernel, apw; +Cc: kernel-hardening
> -----Original Message-----
> From: Joe Perches [mailto:joe@perches.com]
> Sent: Friday, February 10, 2017 7:24 PM
> To: Roberts, William C <william.c.roberts@intel.com>; linux-
> kernel@vger.kernel.org; apw@canonical.com
> Cc: kernel-hardening@lists.openwall.com
> Subject: Re: [PATCH] checkpatch: add warning on %pk instead of %pK usage
>
> 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);
>
I downloaded your checkpatch.pl patch wouldn't apply for some reason... I applied it by hand and
couldn't get it to trigger on either the case you show above or below:
+ MY_DEBUG(drv->foo,
+ "%pk",
+ foo->boo);
+
> ---
> 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 &&
> --
Mixed tabs/spaces issues. But I like the concept of matching across multiple lines. My tree was set to:
commit 7089db84e356562f8ba737c29e472cc42d530dbc
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date: Sun Feb 12 13:03:20 2017 -0800
Linux 4.10-rc8
$ git apply --check ~/Downloads/0001-checkpatch-Add-ability-to-find-bad-uses-of-vsprintf-.patch
error: patch failed: scripts/checkpatch.pl:5676
error: scripts/checkpatch.pl: patch does not apply
^ permalink raw reply [flat|nested] 50+ messages in thread
* [kernel-hardening] RE: [PATCH] checkpatch: add warning on %pk instead of %pK usage
@ 2017-02-13 19:46 ` Roberts, William C
0 siblings, 0 replies; 50+ messages in thread
From: Roberts, William C @ 2017-02-13 19:46 UTC (permalink / raw)
To: Joe Perches, linux-kernel, apw; +Cc: kernel-hardening
> -----Original Message-----
> From: Joe Perches [mailto:joe@perches.com]
> Sent: Friday, February 10, 2017 7:24 PM
> To: Roberts, William C <william.c.roberts@intel.com>; linux-
> kernel@vger.kernel.org; apw@canonical.com
> Cc: kernel-hardening@lists.openwall.com
> Subject: Re: [PATCH] checkpatch: add warning on %pk instead of %pK usage
>
> 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);
>
I downloaded your checkpatch.pl patch wouldn't apply for some reason... I applied it by hand and
couldn't get it to trigger on either the case you show above or below:
+ MY_DEBUG(drv->foo,
+ "%pk",
+ foo->boo);
+
> ---
> 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 &&
> --
Mixed tabs/spaces issues. But I like the concept of matching across multiple lines. My tree was set to:
commit 7089db84e356562f8ba737c29e472cc42d530dbc
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date: Sun Feb 12 13:03:20 2017 -0800
Linux 4.10-rc8
$ git apply --check ~/Downloads/0001-checkpatch-Add-ability-to-find-bad-uses-of-vsprintf-.patch
error: patch failed: scripts/checkpatch.pl:5676
error: scripts/checkpatch.pl: patch does not apply
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH] checkpatch: add warning on %pk instead of %pK usage
2017-02-13 19:46 ` [kernel-hardening] " Roberts, William C
@ 2017-02-13 20:14 ` Joe Perches
-1 siblings, 0 replies; 50+ messages in thread
From: Joe Perches @ 2017-02-13 20:14 UTC (permalink / raw)
To: Roberts, William C, linux-kernel, apw; +Cc: kernel-hardening
[-- Attachment #1: Type: text/plain, Size: 5463 bytes --]
(resending including cc's)
On Mon, 2017-02-13 at 19:46 +0000, Roberts, William C wrote:
> > -----Original Message-----
> > From: Joe Perches [mailto:joe@perches.com]
> > Sent: Friday, February 10, 2017 7:24 PM
> > To: Roberts, William C <william.c.roberts@intel.com>; linux-
> > kernel@vger.kernel.org; apw@canonical.com
> > Cc: kernel-hardening@lists.openwall.com
> > Subject: Re: [PATCH] checkpatch: add warning on %pk instead of %pK usage
> >
> > 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);
> >
>
> I downloaded your checkpatch.pl patch wouldn't apply for some reason... I applied it by hand and
> couldn't get it to trigger on either the case you show above or below:
>
> + MY_DEBUG(drv->foo,
> + "%pk",
> + foo->boo);
> +
>
> > ---
> > 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 &&
> > --
>
> Mixed tabs/spaces issues. But I like the concept of matching across multiple lines. My tree was set to:
>
> commit 7089db84e356562f8ba737c29e472cc42d530dbc
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Sun Feb 12 13:03:20 2017 -0800
>
> Linux 4.10-rc8
>
> $ git apply --check ~/Downloads/0001-checkpatch-Add-ability-to-find-bad-uses-of-vsprintf-.patch
> error: patch failed: scripts/checkpatch.pl:5676
> error: scripts/checkpatch.pl: patch does not apply
>
No worries.
No idea why it doesn't work for you.
Maybe the hand applying was somehow
faulty?
The attached is on top of -next so it does have offsets
on Linus' tree, but it seems to work.
(on -linux)
$ patch -p1 < cp_vsp.diff
patching file scripts/checkpatch.pl
Hunk #1 succeeded at 5634 (offset -36 lines).
$ cat t_block.c
{
MY_DEBUG(drv->foo,
"%pk",
foo->boo);
}
$ ./scripts/checkpatch.pl -f t_block.c
WARNING: Invalid vsprintf pointer extension '%pk'
#2: FILE: t_block.c:2:
+ MY_DEBUG(drv->foo,
+ "%pk",
+ foo->boo);
total: 0 errors, 1 warnings, 5 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
t_block.c has style problems, please review.
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
[-- Attachment #2: cp_vsp.diff --]
[-- Type: text/x-patch, Size: 1301 bytes --]
scripts/checkpatch.pl | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 8e96af53611c..4cb90d5f04ce 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5670,6 +5670,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 &&
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [kernel-hardening] Re: [PATCH] checkpatch: add warning on %pk instead of %pK usage
@ 2017-02-13 20:14 ` Joe Perches
0 siblings, 0 replies; 50+ messages in thread
From: Joe Perches @ 2017-02-13 20:14 UTC (permalink / raw)
To: Roberts, William C, linux-kernel, apw; +Cc: kernel-hardening
[-- Attachment #1: Type: text/plain, Size: 5463 bytes --]
(resending including cc's)
On Mon, 2017-02-13 at 19:46 +0000, Roberts, William C wrote:
> > -----Original Message-----
> > From: Joe Perches [mailto:joe@perches.com]
> > Sent: Friday, February 10, 2017 7:24 PM
> > To: Roberts, William C <william.c.roberts@intel.com>; linux-
> > kernel@vger.kernel.org; apw@canonical.com
> > Cc: kernel-hardening@lists.openwall.com
> > Subject: Re: [PATCH] checkpatch: add warning on %pk instead of %pK usage
> >
> > 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);
> >
>
> I downloaded your checkpatch.pl patch wouldn't apply for some reason... I applied it by hand and
> couldn't get it to trigger on either the case you show above or below:
>
> + MY_DEBUG(drv->foo,
> + "%pk",
> + foo->boo);
> +
>
> > ---
> > 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 &&
> > --
>
> Mixed tabs/spaces issues. But I like the concept of matching across multiple lines. My tree was set to:
>
> commit 7089db84e356562f8ba737c29e472cc42d530dbc
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Sun Feb 12 13:03:20 2017 -0800
>
> Linux 4.10-rc8
>
> $ git apply --check ~/Downloads/0001-checkpatch-Add-ability-to-find-bad-uses-of-vsprintf-.patch
> error: patch failed: scripts/checkpatch.pl:5676
> error: scripts/checkpatch.pl: patch does not apply
>
No worries.
No idea why it doesn't work for you.
Maybe the hand applying was somehow
faulty?
The attached is on top of -next so it does have offsets
on Linus' tree, but it seems to work.
(on -linux)
$ patch -p1 < cp_vsp.diff
patching file scripts/checkpatch.pl
Hunk #1 succeeded at 5634 (offset -36 lines).
$ cat t_block.c
{
MY_DEBUG(drv->foo,
"%pk",
foo->boo);
}
$ ./scripts/checkpatch.pl -f t_block.c
WARNING: Invalid vsprintf pointer extension '%pk'
#2: FILE: t_block.c:2:
+ MY_DEBUG(drv->foo,
+ "%pk",
+ foo->boo);
total: 0 errors, 1 warnings, 5 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
t_block.c has style problems, please review.
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
[-- Attachment #2: cp_vsp.diff --]
[-- Type: text/x-patch, Size: 1301 bytes --]
scripts/checkpatch.pl | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 8e96af53611c..4cb90d5f04ce 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5670,6 +5670,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 &&
^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [PATCH] checkpatch: add warning on %pk instead of %pK usage
[not found] ` <476DC76E7D1DF2438D32BFADF679FC562305F62F@ORSMSX103.amr.corp.intel.com>
@ 2017-02-13 22:20 ` Joe Perches
0 siblings, 0 replies; 50+ messages in thread
From: Joe Perches @ 2017-02-13 22:20 UTC (permalink / raw)
To: Roberts, William C; +Cc: linux-kernel, apw, kernel-hardening
[-- Attachment #1: Type: text/plain, Size: 2383 bytes --]
(Adding back the cc's)
On Mon, 2017-02-13 at 21:28 +0000, Roberts, William C wrote:
> <snip>
> > No worries.
> > No idea why it doesn't work for you.
> > Maybe the hand applying was somehow
> > faulty?
> >
> > The attached is on top of -next so it does have offsets on Linus' tree, but it seems
> > to work.
> >
> > (on -linux)
> >
> > $ patch -p1 < cp_vsp.diff
> > patching file scripts/checkpatch.pl
> > Hunk #1 succeeded at 5634 (offset -36 lines).
> >
> > $ cat t_block.c
> > {
> > MY_DEBUG(drv->foo,
> > "%pk",
> > foo->boo);
> > }
> > $ ./scripts/checkpatch.pl -f t_block.c
> > WARNING: Invalid vsprintf pointer extension '%pk'
> > #2: FILE: t_block.c:2:
> > + MY_DEBUG(drv->foo,
> > + "%pk",
> > + foo->boo);
> >
> > total: 0 errors, 1 warnings, 5 lines checked
> >
> > NOTE: For some of the reported defects, checkpatch may be able to
> > mechanically convert to the typical style using --fix or --fix-inplace.
> >
> > t_block.c has style problems, please review.
> >
> > NOTE: If any of the errors are false positives, please report
> > them to the maintainer, see CHECKPATCH in MAINTAINERS.
>
>
> Applied. It works fine with your example (see attached 0001-tblock.patch) but it doesn't provide
> Output for me with 0002-drv-hack.patch (attached as well)
>
> $ ./scripts/checkpatch.pl 0002-drv-hack.patch
> total: 0 errors, 0 warnings, 10 lines checked
>
> 0002-drv-hack.patch has no obvious style problems and is ready for submission.
>
> ./scripts/checkpatch.pl 0001-tblock.patch
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #13:
> new file mode 100644
>
> WARNING: Invalid vsprintf pointer extension '%pk'
> #19: FILE: t_block.c:2:
> + MY_DEBUG(drv->foo,
> + "%pk",
> + foo->boo);
>
> total: 0 errors, 2 warnings, 6 lines checked
>
> NOTE: For some of the reported defects, checkpatch may be able to
> mechanically convert to the typical style using --fix or --fix-inplace.
>
> 0001-tblock.patch has style problems, please review.
>
> NOTE: If any of the errors are false positives, please report
> them to the maintainer, see CHECKPATCH in MAINTAINERS.
This means _all_ the $stat checks aren't being done
on patches that add just a single multi-line statement.
Andrew? Any thoughts on how to enable $stat appropriately
for patch contexts with a single multi-line statement?
[-- Attachment #2: 1.patch --]
[-- Type: text/x-patch, Size: 695 bytes --]
From 00191661141fb11abac22efe98ee58d37a9d9391 Mon Sep 17 00:00:00 2001
From: William Roberts <william.c.roberts@intel.com>
Date: Mon, 13 Feb 2017 11:35:03 -0800
Subject: [PATCH 2/2] drv hack
Signed-off-by: William Roberts <william.c.roberts@intel.com>
---
drivers/char/applicom.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/char/applicom.c b/drivers/char/applicom.c
index e5c62dc..4f6934d 100644
--- a/drivers/char/applicom.c
+++ b/drivers/char/applicom.c
@@ -153,6 +153,10 @@ static int ac_register_board(unsigned long physloc, void __iomem *loc,
return 0;
}
+ MY_DEBUG(drv->foo,
+ "%pk",
+ foo->boo);
+
boardno--;
apbs[boardno].PhysIO = physloc;
--
2.7.4
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [kernel-hardening] Re: [PATCH] checkpatch: add warning on %pk instead of %pK usage
@ 2017-02-13 22:20 ` Joe Perches
0 siblings, 0 replies; 50+ messages in thread
From: Joe Perches @ 2017-02-13 22:20 UTC (permalink / raw)
To: Roberts, William C; +Cc: linux-kernel, apw, kernel-hardening
[-- Attachment #1: Type: text/plain, Size: 2383 bytes --]
(Adding back the cc's)
On Mon, 2017-02-13 at 21:28 +0000, Roberts, William C wrote:
> <snip>
> > No worries.
> > No idea why it doesn't work for you.
> > Maybe the hand applying was somehow
> > faulty?
> >
> > The attached is on top of -next so it does have offsets on Linus' tree, but it seems
> > to work.
> >
> > (on -linux)
> >
> > $ patch -p1 < cp_vsp.diff
> > patching file scripts/checkpatch.pl
> > Hunk #1 succeeded at 5634 (offset -36 lines).
> >
> > $ cat t_block.c
> > {
> > MY_DEBUG(drv->foo,
> > "%pk",
> > foo->boo);
> > }
> > $ ./scripts/checkpatch.pl -f t_block.c
> > WARNING: Invalid vsprintf pointer extension '%pk'
> > #2: FILE: t_block.c:2:
> > + MY_DEBUG(drv->foo,
> > + "%pk",
> > + foo->boo);
> >
> > total: 0 errors, 1 warnings, 5 lines checked
> >
> > NOTE: For some of the reported defects, checkpatch may be able to
> > mechanically convert to the typical style using --fix or --fix-inplace.
> >
> > t_block.c has style problems, please review.
> >
> > NOTE: If any of the errors are false positives, please report
> > them to the maintainer, see CHECKPATCH in MAINTAINERS.
>
>
> Applied. It works fine with your example (see attached 0001-tblock.patch) but it doesn't provide
> Output for me with 0002-drv-hack.patch (attached as well)
>
> $ ./scripts/checkpatch.pl 0002-drv-hack.patch
> total: 0 errors, 0 warnings, 10 lines checked
>
> 0002-drv-hack.patch has no obvious style problems and is ready for submission.
>
> ./scripts/checkpatch.pl 0001-tblock.patch
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #13:
> new file mode 100644
>
> WARNING: Invalid vsprintf pointer extension '%pk'
> #19: FILE: t_block.c:2:
> + MY_DEBUG(drv->foo,
> + "%pk",
> + foo->boo);
>
> total: 0 errors, 2 warnings, 6 lines checked
>
> NOTE: For some of the reported defects, checkpatch may be able to
> mechanically convert to the typical style using --fix or --fix-inplace.
>
> 0001-tblock.patch has style problems, please review.
>
> NOTE: If any of the errors are false positives, please report
> them to the maintainer, see CHECKPATCH in MAINTAINERS.
This means _all_ the $stat checks aren't being done
on patches that add just a single multi-line statement.
Andrew? Any thoughts on how to enable $stat appropriately
for patch contexts with a single multi-line statement?
[-- Attachment #2: 1.patch --]
[-- Type: text/x-patch, Size: 695 bytes --]
From 00191661141fb11abac22efe98ee58d37a9d9391 Mon Sep 17 00:00:00 2001
From: William Roberts <william.c.roberts@intel.com>
Date: Mon, 13 Feb 2017 11:35:03 -0800
Subject: [PATCH 2/2] drv hack
Signed-off-by: William Roberts <william.c.roberts@intel.com>
---
drivers/char/applicom.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/char/applicom.c b/drivers/char/applicom.c
index e5c62dc..4f6934d 100644
--- a/drivers/char/applicom.c
+++ b/drivers/char/applicom.c
@@ -153,6 +153,10 @@ static int ac_register_board(unsigned long physloc, void __iomem *loc,
return 0;
}
+ MY_DEBUG(drv->foo,
+ "%pk",
+ foo->boo);
+
boardno--;
apbs[boardno].PhysIO = physloc;
--
2.7.4
^ permalink raw reply related [flat|nested] 50+ messages in thread
* RE: [PATCH] checkpatch: add warning on %pk instead of %pK usage
2017-02-13 22:20 ` [kernel-hardening] " Joe Perches
@ 2017-02-15 23:49 ` Roberts, William C
-1 siblings, 0 replies; 50+ messages in thread
From: Roberts, William C @ 2017-02-15 23:49 UTC (permalink / raw)
To: Joe Perches; +Cc: linux-kernel, apw, kernel-hardening
> -----Original Message-----
> From: Joe Perches [mailto:joe@perches.com]
> Sent: Monday, February 13, 2017 2:21 PM
> To: Roberts, William C <william.c.roberts@intel.com>
> Cc: linux-kernel@vger.kernel.org; apw@canonical.com; kernel-
> hardening@lists.openwall.com
> Subject: Re: [PATCH] checkpatch: add warning on %pk instead of %pK usage
>
> (Adding back the cc's)
>
> On Mon, 2017-02-13 at 21:28 +0000, Roberts, William C wrote:
> > <snip>
> > > No worries.
> > > No idea why it doesn't work for you.
> > > Maybe the hand applying was somehow
> > > faulty?
> > >
> > > The attached is on top of -next so it does have offsets on Linus'
> > > tree, but it seems to work.
> > >
> > > (on -linux)
> > >
> > > $ patch -p1 < cp_vsp.diff
> > > patching file scripts/checkpatch.pl
> > > Hunk #1 succeeded at 5634 (offset -36 lines).
> > >
> > > $ cat t_block.c
> > > {
> > > MY_DEBUG(drv->foo,
> > > "%pk",
> > > foo->boo);
> > > }
> > > $ ./scripts/checkpatch.pl -f t_block.c
> > > WARNING: Invalid vsprintf pointer extension '%pk'
> > > #2: FILE: t_block.c:2:
> > > + MY_DEBUG(drv->foo,
> > > + "%pk",
> > > + foo->boo);
> > >
> > > total: 0 errors, 1 warnings, 5 lines checked
> > >
> > > NOTE: For some of the reported defects, checkpatch may be able to
> > > mechanically convert to the typical style using --fix or --fix-inplace.
> > >
> > > t_block.c has style problems, please review.
> > >
> > > NOTE: If any of the errors are false positives, please report
> > > them to the maintainer, see CHECKPATCH in MAINTAINERS.
> >
> >
> > Applied. It works fine with your example (see attached
> > 0001-tblock.patch) but it doesn't provide Output for me with
> > 0002-drv-hack.patch (attached as well)
> >
> > $ ./scripts/checkpatch.pl 0002-drv-hack.patch
> > total: 0 errors, 0 warnings, 10 lines checked
> >
> > 0002-drv-hack.patch has no obvious style problems and is ready for submission.
> >
> > ./scripts/checkpatch.pl 0001-tblock.patch
> > WARNING: added, moved or deleted file(s), does MAINTAINERS need
> updating?
> > #13:
> > new file mode 100644
> >
> > WARNING: Invalid vsprintf pointer extension '%pk'
> > #19: FILE: t_block.c:2:
> > + MY_DEBUG(drv->foo,
> > + "%pk",
> > + foo->boo);
> >
> > total: 0 errors, 2 warnings, 6 lines checked
> >
> > NOTE: For some of the reported defects, checkpatch may be able to
> > mechanically convert to the typical style using --fix or --fix-inplace.
> >
> > 0001-tblock.patch has style problems, please review.
> >
> > NOTE: If any of the errors are false positives, please report
> > them to the maintainer, see CHECKPATCH in MAINTAINERS.
>
> This means _all_ the $stat checks aren't being done on patches that add just a
> single multi-line statement.
>
> Andrew? Any thoughts on how to enable $stat appropriately for patch contexts
> with a single multi-line statement?
I'm for merging your patch as is, and then take up the fact that $stat is not working correctly
as a separate change, does that seem reasonable?
^ permalink raw reply [flat|nested] 50+ messages in thread
* [kernel-hardening] RE: [PATCH] checkpatch: add warning on %pk instead of %pK usage
@ 2017-02-15 23:49 ` Roberts, William C
0 siblings, 0 replies; 50+ messages in thread
From: Roberts, William C @ 2017-02-15 23:49 UTC (permalink / raw)
To: Joe Perches; +Cc: linux-kernel, apw, kernel-hardening
> -----Original Message-----
> From: Joe Perches [mailto:joe@perches.com]
> Sent: Monday, February 13, 2017 2:21 PM
> To: Roberts, William C <william.c.roberts@intel.com>
> Cc: linux-kernel@vger.kernel.org; apw@canonical.com; kernel-
> hardening@lists.openwall.com
> Subject: Re: [PATCH] checkpatch: add warning on %pk instead of %pK usage
>
> (Adding back the cc's)
>
> On Mon, 2017-02-13 at 21:28 +0000, Roberts, William C wrote:
> > <snip>
> > > No worries.
> > > No idea why it doesn't work for you.
> > > Maybe the hand applying was somehow
> > > faulty?
> > >
> > > The attached is on top of -next so it does have offsets on Linus'
> > > tree, but it seems to work.
> > >
> > > (on -linux)
> > >
> > > $ patch -p1 < cp_vsp.diff
> > > patching file scripts/checkpatch.pl
> > > Hunk #1 succeeded at 5634 (offset -36 lines).
> > >
> > > $ cat t_block.c
> > > {
> > > MY_DEBUG(drv->foo,
> > > "%pk",
> > > foo->boo);
> > > }
> > > $ ./scripts/checkpatch.pl -f t_block.c
> > > WARNING: Invalid vsprintf pointer extension '%pk'
> > > #2: FILE: t_block.c:2:
> > > + MY_DEBUG(drv->foo,
> > > + "%pk",
> > > + foo->boo);
> > >
> > > total: 0 errors, 1 warnings, 5 lines checked
> > >
> > > NOTE: For some of the reported defects, checkpatch may be able to
> > > mechanically convert to the typical style using --fix or --fix-inplace.
> > >
> > > t_block.c has style problems, please review.
> > >
> > > NOTE: If any of the errors are false positives, please report
> > > them to the maintainer, see CHECKPATCH in MAINTAINERS.
> >
> >
> > Applied. It works fine with your example (see attached
> > 0001-tblock.patch) but it doesn't provide Output for me with
> > 0002-drv-hack.patch (attached as well)
> >
> > $ ./scripts/checkpatch.pl 0002-drv-hack.patch
> > total: 0 errors, 0 warnings, 10 lines checked
> >
> > 0002-drv-hack.patch has no obvious style problems and is ready for submission.
> >
> > ./scripts/checkpatch.pl 0001-tblock.patch
> > WARNING: added, moved or deleted file(s), does MAINTAINERS need
> updating?
> > #13:
> > new file mode 100644
> >
> > WARNING: Invalid vsprintf pointer extension '%pk'
> > #19: FILE: t_block.c:2:
> > + MY_DEBUG(drv->foo,
> > + "%pk",
> > + foo->boo);
> >
> > total: 0 errors, 2 warnings, 6 lines checked
> >
> > NOTE: For some of the reported defects, checkpatch may be able to
> > mechanically convert to the typical style using --fix or --fix-inplace.
> >
> > 0001-tblock.patch has style problems, please review.
> >
> > NOTE: If any of the errors are false positives, please report
> > them to the maintainer, see CHECKPATCH in MAINTAINERS.
>
> This means _all_ the $stat checks aren't being done on patches that add just a
> single multi-line statement.
>
> Andrew? Any thoughts on how to enable $stat appropriately for patch contexts
> with a single multi-line statement?
I'm for merging your patch as is, and then take up the fact that $stat is not working correctly
as a separate change, does that seem reasonable?
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH] checkpatch: add warning on %pk instead of %pK usage
2017-02-15 23:49 ` [kernel-hardening] " Roberts, William C
@ 2017-02-16 0:19 ` Joe Perches
-1 siblings, 0 replies; 50+ messages in thread
From: Joe Perches @ 2017-02-16 0:19 UTC (permalink / raw)
To: Roberts, William C, Andrew Morton; +Cc: linux-kernel, apw, kernel-hardening
On Wed, 2017-02-15 at 23:49 +0000, Roberts, William C wrote:
> >
> > This means _all_ the $stat checks aren't being done on patches that add just a
> > single multi-line statement.
> >
> > Andrew? Any thoughts on how to enable $stat appropriately for patch contexts
> > with a single multi-line statement?
>
> I'm for merging your patch as is, and then take up the fact that $stat is not working correctly
> as a separate change, does that seem reasonable?
Sure, Andrew Morton is the typical upstream path for checkpatch.
(cc'd)
Andy Whitcroft? Any chance to look at this?
^ permalink raw reply [flat|nested] 50+ messages in thread
* [kernel-hardening] Re: [PATCH] checkpatch: add warning on %pk instead of %pK usage
@ 2017-02-16 0:19 ` Joe Perches
0 siblings, 0 replies; 50+ messages in thread
From: Joe Perches @ 2017-02-16 0:19 UTC (permalink / raw)
To: Roberts, William C, Andrew Morton; +Cc: linux-kernel, apw, kernel-hardening
On Wed, 2017-02-15 at 23:49 +0000, Roberts, William C wrote:
> >
> > This means _all_ the $stat checks aren't being done on patches that add just a
> > single multi-line statement.
> >
> > Andrew? Any thoughts on how to enable $stat appropriately for patch contexts
> > with a single multi-line statement?
>
> I'm for merging your patch as is, and then take up the fact that $stat is not working correctly
> as a separate change, does that seem reasonable?
Sure, Andrew Morton is the typical upstream path for checkpatch.
(cc'd)
Andy Whitcroft? Any chance to look at this?
^ permalink raw reply [flat|nested] 50+ messages in thread
* RE: [PATCH] checkpatch: add warning on %pk instead of %pK usage
2017-02-15 23:49 ` [kernel-hardening] " Roberts, William C
@ 2017-02-27 16:26 ` Roberts, William C
-1 siblings, 0 replies; 50+ messages in thread
From: Roberts, William C @ 2017-02-27 16:26 UTC (permalink / raw)
To: Roberts, William C, Joe Perches; +Cc: linux-kernel, apw, kernel-hardening
> -----Original Message-----
> From: Roberts, William C [mailto:william.c.roberts@intel.com]
> Sent: Wednesday, February 15, 2017 3:49 PM
> To: Joe Perches <joe@perches.com>
> Cc: linux-kernel@vger.kernel.org; apw@canonical.com; kernel-
> hardening@lists.openwall.com
> Subject: [kernel-hardening] RE: [PATCH] checkpatch: add warning on %pk instead
> of %pK usage
>
>
>
> > -----Original Message-----
> > From: Joe Perches [mailto:joe@perches.com]
> > Sent: Monday, February 13, 2017 2:21 PM
> > To: Roberts, William C <william.c.roberts@intel.com>
> > Cc: linux-kernel@vger.kernel.org; apw@canonical.com; kernel-
> > hardening@lists.openwall.com
> > Subject: Re: [PATCH] checkpatch: add warning on %pk instead of %pK
> > usage
> >
> > (Adding back the cc's)
> >
> > On Mon, 2017-02-13 at 21:28 +0000, Roberts, William C wrote:
> > > <snip>
> > > > No worries.
> > > > No idea why it doesn't work for you.
> > > > Maybe the hand applying was somehow faulty?
> > > >
> > > > The attached is on top of -next so it does have offsets on Linus'
> > > > tree, but it seems to work.
> > > >
> > > > (on -linux)
> > > >
> > > > $ patch -p1 < cp_vsp.diff
> > > > patching file scripts/checkpatch.pl Hunk #1 succeeded at 5634
> > > > (offset -36 lines).
> > > >
> > > > $ cat t_block.c
> > > > {
> > > > MY_DEBUG(drv->foo,
> > > > "%pk",
> > > > foo->boo);
> > > > }
> > > > $ ./scripts/checkpatch.pl -f t_block.c
> > > > WARNING: Invalid vsprintf pointer extension '%pk'
> > > > #2: FILE: t_block.c:2:
> > > > + MY_DEBUG(drv->foo,
> > > > + "%pk",
> > > > + foo->boo);
> > > >
> > > > total: 0 errors, 1 warnings, 5 lines checked
> > > >
> > > > NOTE: For some of the reported defects, checkpatch may be able to
> > > > mechanically convert to the typical style using --fix or --fix-inplace.
> > > >
> > > > t_block.c has style problems, please review.
> > > >
> > > > NOTE: If any of the errors are false positives, please report
> > > > them to the maintainer, see CHECKPATCH in MAINTAINERS.
> > >
> > >
> > > Applied. It works fine with your example (see attached
> > > 0001-tblock.patch) but it doesn't provide Output for me with
> > > 0002-drv-hack.patch (attached as well)
> > >
> > > $ ./scripts/checkpatch.pl 0002-drv-hack.patch
> > > total: 0 errors, 0 warnings, 10 lines checked
> > >
> > > 0002-drv-hack.patch has no obvious style problems and is ready for
> submission.
> > >
> > > ./scripts/checkpatch.pl 0001-tblock.patch
> > > WARNING: added, moved or deleted file(s), does MAINTAINERS need
> > updating?
> > > #13:
> > > new file mode 100644
> > >
> > > WARNING: Invalid vsprintf pointer extension '%pk'
> > > #19: FILE: t_block.c:2:
> > > + MY_DEBUG(drv->foo,
> > > + "%pk",
> > > + foo->boo);
> > >
> > > total: 0 errors, 2 warnings, 6 lines checked
> > >
> > > NOTE: For some of the reported defects, checkpatch may be able to
> > > mechanically convert to the typical style using --fix or --fix-inplace.
> > >
> > > 0001-tblock.patch has style problems, please review.
> > >
> > > NOTE: If any of the errors are false positives, please report
> > > them to the maintainer, see CHECKPATCH in MAINTAINERS.
> >
> > This means _all_ the $stat checks aren't being done on patches that
> > add just a single multi-line statement.
> >
> > Andrew? Any thoughts on how to enable $stat appropriately for patch
> > contexts with a single multi-line statement?
>
> I'm for merging your patch as is, and then take up the fact that $stat is not
> working correctly as a separate change, does that seem reasonable?
I haven't seen anything on list about your patch, are we kind of stuck or do you
have some plan on adding your stat patch in the future?
^ permalink raw reply [flat|nested] 50+ messages in thread
* [kernel-hardening] RE: [PATCH] checkpatch: add warning on %pk instead of %pK usage
@ 2017-02-27 16:26 ` Roberts, William C
0 siblings, 0 replies; 50+ messages in thread
From: Roberts, William C @ 2017-02-27 16:26 UTC (permalink / raw)
To: Roberts, William C, Joe Perches; +Cc: linux-kernel, apw, kernel-hardening
> -----Original Message-----
> From: Roberts, William C [mailto:william.c.roberts@intel.com]
> Sent: Wednesday, February 15, 2017 3:49 PM
> To: Joe Perches <joe@perches.com>
> Cc: linux-kernel@vger.kernel.org; apw@canonical.com; kernel-
> hardening@lists.openwall.com
> Subject: [kernel-hardening] RE: [PATCH] checkpatch: add warning on %pk instead
> of %pK usage
>
>
>
> > -----Original Message-----
> > From: Joe Perches [mailto:joe@perches.com]
> > Sent: Monday, February 13, 2017 2:21 PM
> > To: Roberts, William C <william.c.roberts@intel.com>
> > Cc: linux-kernel@vger.kernel.org; apw@canonical.com; kernel-
> > hardening@lists.openwall.com
> > Subject: Re: [PATCH] checkpatch: add warning on %pk instead of %pK
> > usage
> >
> > (Adding back the cc's)
> >
> > On Mon, 2017-02-13 at 21:28 +0000, Roberts, William C wrote:
> > > <snip>
> > > > No worries.
> > > > No idea why it doesn't work for you.
> > > > Maybe the hand applying was somehow faulty?
> > > >
> > > > The attached is on top of -next so it does have offsets on Linus'
> > > > tree, but it seems to work.
> > > >
> > > > (on -linux)
> > > >
> > > > $ patch -p1 < cp_vsp.diff
> > > > patching file scripts/checkpatch.pl Hunk #1 succeeded at 5634
> > > > (offset -36 lines).
> > > >
> > > > $ cat t_block.c
> > > > {
> > > > MY_DEBUG(drv->foo,
> > > > "%pk",
> > > > foo->boo);
> > > > }
> > > > $ ./scripts/checkpatch.pl -f t_block.c
> > > > WARNING: Invalid vsprintf pointer extension '%pk'
> > > > #2: FILE: t_block.c:2:
> > > > + MY_DEBUG(drv->foo,
> > > > + "%pk",
> > > > + foo->boo);
> > > >
> > > > total: 0 errors, 1 warnings, 5 lines checked
> > > >
> > > > NOTE: For some of the reported defects, checkpatch may be able to
> > > > mechanically convert to the typical style using --fix or --fix-inplace.
> > > >
> > > > t_block.c has style problems, please review.
> > > >
> > > > NOTE: If any of the errors are false positives, please report
> > > > them to the maintainer, see CHECKPATCH in MAINTAINERS.
> > >
> > >
> > > Applied. It works fine with your example (see attached
> > > 0001-tblock.patch) but it doesn't provide Output for me with
> > > 0002-drv-hack.patch (attached as well)
> > >
> > > $ ./scripts/checkpatch.pl 0002-drv-hack.patch
> > > total: 0 errors, 0 warnings, 10 lines checked
> > >
> > > 0002-drv-hack.patch has no obvious style problems and is ready for
> submission.
> > >
> > > ./scripts/checkpatch.pl 0001-tblock.patch
> > > WARNING: added, moved or deleted file(s), does MAINTAINERS need
> > updating?
> > > #13:
> > > new file mode 100644
> > >
> > > WARNING: Invalid vsprintf pointer extension '%pk'
> > > #19: FILE: t_block.c:2:
> > > + MY_DEBUG(drv->foo,
> > > + "%pk",
> > > + foo->boo);
> > >
> > > total: 0 errors, 2 warnings, 6 lines checked
> > >
> > > NOTE: For some of the reported defects, checkpatch may be able to
> > > mechanically convert to the typical style using --fix or --fix-inplace.
> > >
> > > 0001-tblock.patch has style problems, please review.
> > >
> > > NOTE: If any of the errors are false positives, please report
> > > them to the maintainer, see CHECKPATCH in MAINTAINERS.
> >
> > This means _all_ the $stat checks aren't being done on patches that
> > add just a single multi-line statement.
> >
> > Andrew? Any thoughts on how to enable $stat appropriately for patch
> > contexts with a single multi-line statement?
>
> I'm for merging your patch as is, and then take up the fact that $stat is not
> working correctly as a separate change, does that seem reasonable?
I haven't seen anything on list about your patch, are we kind of stuck or do you
have some plan on adding your stat patch in the future?
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH] checkpatch: Add ability to find bad uses of vsprintf %p<foo> extensions
2017-02-27 16:26 ` [kernel-hardening] " Roberts, William C
@ 2017-02-27 20:54 ` Joe Perches
-1 siblings, 0 replies; 50+ messages in thread
From: Joe Perches @ 2017-02-27 20:54 UTC (permalink / raw)
To: Andrew Morton, Andy Whitcroft
Cc: Roberts, William C, kernel-hardening, linux-kernel
%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>
---
Andrew, this has gone back and forth a few times.
It's imperfect as a patch context with just a single
function addition can be missed, but that's not new
with $stat tests and just this patch. Perhaps one day
the $stat identification mechanism can be improved.
Until then, can you please apply this? Thanks.
scripts/checkpatch.pl | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index ad5ea5c545b2..9293b8a1c121 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5676,6 +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
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [kernel-hardening] [PATCH] checkpatch: Add ability to find bad uses of vsprintf %p<foo> extensions
@ 2017-02-27 20:54 ` Joe Perches
0 siblings, 0 replies; 50+ messages in thread
From: Joe Perches @ 2017-02-27 20:54 UTC (permalink / raw)
To: Andrew Morton, Andy Whitcroft
Cc: Roberts, William C, kernel-hardening, linux-kernel
%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>
---
Andrew, this has gone back and forth a few times.
It's imperfect as a patch context with just a single
function addition can be missed, but that's not new
with $stat tests and just this patch. Perhaps one day
the $stat identification mechanism can be improved.
Until then, can you please apply this? Thanks.
scripts/checkpatch.pl | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index ad5ea5c545b2..9293b8a1c121 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5676,6 +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
^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [kernel-hardening] [PATCH] checkpatch: Add ability to find bad uses of vsprintf %p<foo> extensions
2017-02-27 20:54 ` [kernel-hardening] " Joe Perches
@ 2017-02-27 21:18 ` Kees Cook
-1 siblings, 0 replies; 50+ messages in thread
From: Kees Cook @ 2017-02-27 21:18 UTC (permalink / raw)
To: Joe Perches
Cc: Andrew Morton, Andy Whitcroft, Roberts, William C,
kernel-hardening, LKML
On Mon, Feb 27, 2017 at 12:54 PM, Joe Perches <joe@perches.com> wrote:
> %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>
Acked-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
>
> Andrew, this has gone back and forth a few times.
>
> It's imperfect as a patch context with just a single
> function addition can be missed, but that's not new
> with $stat tests and just this patch. Perhaps one day
> the $stat identification mechanism can be improved.
>
> Until then, can you please apply this? Thanks.
>
> scripts/checkpatch.pl | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index ad5ea5c545b2..9293b8a1c121 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -5676,6 +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
>
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [kernel-hardening] [PATCH] checkpatch: Add ability to find bad uses of vsprintf %p<foo> extensions
@ 2017-02-27 21:18 ` Kees Cook
0 siblings, 0 replies; 50+ messages in thread
From: Kees Cook @ 2017-02-27 21:18 UTC (permalink / raw)
To: Joe Perches
Cc: Andrew Morton, Andy Whitcroft, Roberts, William C,
kernel-hardening, LKML
On Mon, Feb 27, 2017 at 12:54 PM, Joe Perches <joe@perches.com> wrote:
> %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>
Acked-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
>
> Andrew, this has gone back and forth a few times.
>
> It's imperfect as a patch context with just a single
> function addition can be missed, but that's not new
> with $stat tests and just this patch. Perhaps one day
> the $stat identification mechanism can be improved.
>
> Until then, can you please apply this? Thanks.
>
> scripts/checkpatch.pl | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index ad5ea5c545b2..9293b8a1c121 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -5676,6 +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
>
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 50+ messages in thread
* RE: [PATCH] checkpatch: Add ability to find bad uses of vsprintf %p<foo> extensions
2017-02-27 20:54 ` [kernel-hardening] " Joe Perches
@ 2017-02-28 15:34 ` Roberts, William C
-1 siblings, 0 replies; 50+ messages in thread
From: Roberts, William C @ 2017-02-28 15:34 UTC (permalink / raw)
To: Joe Perches, Andrew Morton, Andy Whitcroft; +Cc: kernel-hardening, linux-kernel
> -----Original Message-----
> From: Joe Perches [mailto:joe@perches.com]
> Sent: Monday, February 27, 2017 12:55 PM
> To: Andrew Morton <akpm@linux-foundation.org>; Andy Whitcroft
> <apw@canonical.com>
> Cc: Roberts, William C <william.c.roberts@intel.com>; kernel-
> hardening@lists.openwall.com; linux-kernel@vger.kernel.org
> 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>
Acked-By: William Roberts <william.c.roberts@intel.com>
> ---
>
> Andrew, this has gone back and forth a few times.
>
> It's imperfect as a patch context with just a single function addition can be
> missed, but that's not new with $stat tests and just this patch. Perhaps one day
> the $stat identification mechanism can be improved.
>
> Until then, can you please apply this? Thanks.
>
> scripts/checkpatch.pl | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index
> ad5ea5c545b2..9293b8a1c121 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -5676,6 +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
^ permalink raw reply [flat|nested] 50+ messages in thread
* [kernel-hardening] RE: [PATCH] checkpatch: Add ability to find bad uses of vsprintf %p<foo> extensions
@ 2017-02-28 15:34 ` Roberts, William C
0 siblings, 0 replies; 50+ messages in thread
From: Roberts, William C @ 2017-02-28 15:34 UTC (permalink / raw)
To: Joe Perches, Andrew Morton, Andy Whitcroft; +Cc: kernel-hardening, linux-kernel
> -----Original Message-----
> From: Joe Perches [mailto:joe@perches.com]
> Sent: Monday, February 27, 2017 12:55 PM
> To: Andrew Morton <akpm@linux-foundation.org>; Andy Whitcroft
> <apw@canonical.com>
> Cc: Roberts, William C <william.c.roberts@intel.com>; kernel-
> hardening@lists.openwall.com; linux-kernel@vger.kernel.org
> 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>
Acked-By: William Roberts <william.c.roberts@intel.com>
> ---
>
> Andrew, this has gone back and forth a few times.
>
> It's imperfect as a patch context with just a single function addition can be
> missed, but that's not new with $stat tests and just this patch. Perhaps one day
> the $stat identification mechanism can be improved.
>
> Until then, can you please apply this? Thanks.
>
> scripts/checkpatch.pl | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index
> ad5ea5c545b2..9293b8a1c121 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -5676,6 +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
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH] checkpatch: Add ability to find bad uses of vsprintf %p<foo> extensions
2017-02-27 20:54 ` [kernel-hardening] " Joe Perches
@ 2017-03-01 0:06 ` Andrew Morton
-1 siblings, 0 replies; 50+ messages in thread
From: Andrew Morton @ 2017-03-01 0:06 UTC (permalink / raw)
To: Joe Perches
Cc: Andy Whitcroft, Roberts, William C, kernel-hardening, linux-kernel
On Mon, 27 Feb 2017 12:54:55 -0800 Joe Perches <joe@perches.com> wrote:
> %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>.
Should we also do this?
--- a/lib/vsprintf.c~checkpatch-add-ability-to-find-bad-uses-of-vsprintf-%pfoo-extensions-fix
+++ a/lib/vsprintf.c
@@ -1477,6 +1477,9 @@ int kptr_restrict __read_mostly;
* by an extra set of alphanumeric characters that are extended format
* specifiers.
*
+ * Please update scripts/checkpatch.pl when adding new conversion characters.
+ * (search for "check for vsprintf extension").
+ *
* Right now we handle:
*
* - 'F' For symbolic function descriptor pointers with offset
_
^ permalink raw reply [flat|nested] 50+ messages in thread
* [kernel-hardening] Re: [PATCH] checkpatch: Add ability to find bad uses of vsprintf %p<foo> extensions
@ 2017-03-01 0:06 ` Andrew Morton
0 siblings, 0 replies; 50+ messages in thread
From: Andrew Morton @ 2017-03-01 0:06 UTC (permalink / raw)
To: Joe Perches
Cc: Andy Whitcroft, Roberts, William C, kernel-hardening, linux-kernel
On Mon, 27 Feb 2017 12:54:55 -0800 Joe Perches <joe@perches.com> wrote:
> %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>.
Should we also do this?
--- a/lib/vsprintf.c~checkpatch-add-ability-to-find-bad-uses-of-vsprintf-%pfoo-extensions-fix
+++ a/lib/vsprintf.c
@@ -1477,6 +1477,9 @@ int kptr_restrict __read_mostly;
* by an extra set of alphanumeric characters that are extended format
* specifiers.
*
+ * Please update scripts/checkpatch.pl when adding new conversion characters.
+ * (search for "check for vsprintf extension").
+ *
* Right now we handle:
*
* - 'F' For symbolic function descriptor pointers with offset
_
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [kernel-hardening] Re: [PATCH] checkpatch: Add ability to find bad uses of vsprintf %p<foo> extensions
2017-03-01 0:06 ` [kernel-hardening] " Andrew Morton
@ 2017-03-01 0:11 ` Kees Cook
-1 siblings, 0 replies; 50+ messages in thread
From: Kees Cook @ 2017-03-01 0:11 UTC (permalink / raw)
To: Andrew Morton
Cc: Joe Perches, Andy Whitcroft, Roberts, William C, kernel-hardening, LKML
On Tue, Feb 28, 2017 at 4:06 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Mon, 27 Feb 2017 12:54:55 -0800 Joe Perches <joe@perches.com> wrote:
>
>> %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>.
>
> Should we also do this?
Ah yes, good idea. Maybe "...when adding/removing new conversion..." ?
-Kees
>
> --- a/lib/vsprintf.c~checkpatch-add-ability-to-find-bad-uses-of-vsprintf-%pfoo-extensions-fix
> +++ a/lib/vsprintf.c
> @@ -1477,6 +1477,9 @@ int kptr_restrict __read_mostly;
> * by an extra set of alphanumeric characters that are extended format
> * specifiers.
> *
> + * Please update scripts/checkpatch.pl when adding new conversion characters.
> + * (search for "check for vsprintf extension").
> + *
> * Right now we handle:
> *
> * - 'F' For symbolic function descriptor pointers with offset
> _
>
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [kernel-hardening] Re: [PATCH] checkpatch: Add ability to find bad uses of vsprintf %p<foo> extensions
@ 2017-03-01 0:11 ` Kees Cook
0 siblings, 0 replies; 50+ messages in thread
From: Kees Cook @ 2017-03-01 0:11 UTC (permalink / raw)
To: Andrew Morton
Cc: Joe Perches, Andy Whitcroft, Roberts, William C, kernel-hardening, LKML
On Tue, Feb 28, 2017 at 4:06 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Mon, 27 Feb 2017 12:54:55 -0800 Joe Perches <joe@perches.com> wrote:
>
>> %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>.
>
> Should we also do this?
Ah yes, good idea. Maybe "...when adding/removing new conversion..." ?
-Kees
>
> --- a/lib/vsprintf.c~checkpatch-add-ability-to-find-bad-uses-of-vsprintf-%pfoo-extensions-fix
> +++ a/lib/vsprintf.c
> @@ -1477,6 +1477,9 @@ int kptr_restrict __read_mostly;
> * by an extra set of alphanumeric characters that are extended format
> * specifiers.
> *
> + * Please update scripts/checkpatch.pl when adding new conversion characters.
> + * (search for "check for vsprintf extension").
> + *
> * Right now we handle:
> *
> * - 'F' For symbolic function descriptor pointers with offset
> _
>
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH] checkpatch: Add ability to find bad uses of vsprintf %p<foo> extensions
2017-03-01 0:06 ` [kernel-hardening] " Andrew Morton
@ 2017-03-01 0:12 ` Joe Perches
-1 siblings, 0 replies; 50+ messages in thread
From: Joe Perches @ 2017-03-01 0:12 UTC (permalink / raw)
To: Andrew Morton
Cc: Andy Whitcroft, Roberts, William C, kernel-hardening, linux-kernel
On Tue, 2017-02-28 at 16:06 -0800, Andrew Morton wrote:
> On Mon, 27 Feb 2017 12:54:55 -0800 Joe Perches <joe@perches.com> wrote:
>
> > %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>.
>
> Should we also do this?
>
> --- a/lib/vsprintf.c~checkpatch-add-ability-to-find-bad-uses-of-vsprintf-%pfoo-extensions-fix
> +++ a/lib/vsprintf.c
> @@ -1477,6 +1477,9 @@ int kptr_restrict __read_mostly;
> * by an extra set of alphanumeric characters that are extended format
> * specifiers.
> *
> + * Please update scripts/checkpatch.pl when adding new conversion characters.
> + * (search for "check for vsprintf extension").
> + *
Seems sensible, thanks.
^ permalink raw reply [flat|nested] 50+ messages in thread
* [kernel-hardening] Re: [PATCH] checkpatch: Add ability to find bad uses of vsprintf %p<foo> extensions
@ 2017-03-01 0:12 ` Joe Perches
0 siblings, 0 replies; 50+ messages in thread
From: Joe Perches @ 2017-03-01 0:12 UTC (permalink / raw)
To: Andrew Morton
Cc: Andy Whitcroft, Roberts, William C, kernel-hardening, linux-kernel
On Tue, 2017-02-28 at 16:06 -0800, Andrew Morton wrote:
> On Mon, 27 Feb 2017 12:54:55 -0800 Joe Perches <joe@perches.com> wrote:
>
> > %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>.
>
> Should we also do this?
>
> --- a/lib/vsprintf.c~checkpatch-add-ability-to-find-bad-uses-of-vsprintf-%pfoo-extensions-fix
> +++ a/lib/vsprintf.c
> @@ -1477,6 +1477,9 @@ int kptr_restrict __read_mostly;
> * by an extra set of alphanumeric characters that are extended format
> * specifiers.
> *
> + * Please update scripts/checkpatch.pl when adding new conversion characters.
> + * (search for "check for vsprintf extension").
> + *
Seems sensible, thanks.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [kernel-hardening] Re: [PATCH] checkpatch: Add ability to find bad uses of vsprintf %p<foo> extensions
2017-03-01 0:11 ` Kees Cook
@ 2017-03-01 1:14 ` Joe Perches
-1 siblings, 0 replies; 50+ messages in thread
From: Joe Perches @ 2017-03-01 1:14 UTC (permalink / raw)
To: Kees Cook, Andrew Morton
Cc: Andy Whitcroft, Roberts, William C, kernel-hardening, LKML
On Tue, 2017-02-28 at 16:11 -0800, Kees Cook wrote:
> On Tue, Feb 28, 2017 at 4:06 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> > On Mon, 27 Feb 2017 12:54:55 -0800 Joe Perches <joe@perches.com> wrote:
> >
> > > %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>.
> >
> > Should we also do this?
>
> Ah yes, good idea. Maybe "...when adding/removing new conversion..." ?
Deleting conversions seems unlikely.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [kernel-hardening] Re: [PATCH] checkpatch: Add ability to find bad uses of vsprintf %p<foo> extensions
@ 2017-03-01 1:14 ` Joe Perches
0 siblings, 0 replies; 50+ messages in thread
From: Joe Perches @ 2017-03-01 1:14 UTC (permalink / raw)
To: Kees Cook, Andrew Morton
Cc: Andy Whitcroft, Roberts, William C, kernel-hardening, LKML
On Tue, 2017-02-28 at 16:11 -0800, Kees Cook wrote:
> On Tue, Feb 28, 2017 at 4:06 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> > On Mon, 27 Feb 2017 12:54:55 -0800 Joe Perches <joe@perches.com> wrote:
> >
> > > %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>.
> >
> > Should we also do this?
>
> Ah yes, good idea. Maybe "...when adding/removing new conversion..." ?
Deleting conversions seems unlikely.
^ permalink raw reply [flat|nested] 50+ messages in thread
end of thread, other threads:[~2017-03-01 2:01 UTC | newest]
Thread overview: 50+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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
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.