* [PATCH v2] checkpatch: Warn about 0-length and 1-element arrays
@ 2023-05-26 17:39 Kees Cook
2023-05-27 1:38 ` Joe Perches
0 siblings, 1 reply; 3+ messages in thread
From: Kees Cook @ 2023-05-26 17:39 UTC (permalink / raw)
To: Andy Whitcroft
Cc: Kees Cook, Dwaipayan Ray, Lukas Bulwahn, Gustavo A . R . Silva,
Joe Perches, linux-kernel, linux-hardening
Fake flexible arrays have been deprecated since last millennium. Proper
C99 flexible arrays must be used throughout the kernel so
CONFIG_FORTIFY_SOURCE and CONFIG_UBSAN_BOUNDS can provide proper array
bounds checking.
Cc: Andy Whitcroft <apw@canonical.com>
Cc: Dwaipayan Ray <dwaipayanray1@gmail.com>
Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Cc: Gustavo A. R. Silva <gustavoars@kernel.org>
Fixed-by: Joe Perches <joe@perches.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Link: https://lore.kernel.org/r/20230517204530.never.151-kees@kernel.org
---
v2: find using "end of struct" marking instead of $context_function (joe)
v1: https://lore.kernel.org/all/20230517204530.never.151-kees@kernel.org/
---
scripts/checkpatch.pl | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 30b0b4fdb3bf..cfa19bfc857c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -7430,6 +7430,21 @@ sub process {
}
}
+# check for array definition/declarations that should use flexible arrays instead
+ if ($sline =~ /^[\+ ]\s*}\s*;\s*$/ &&
+ $prevline =~ /^\+\s*(?:(?:struct|union|enum)\s+$Ident|\}|$Type)\s*$Ident\s*\[\s*(0|1)\s*\]\s*;\s*$/) {
+ if ($1 == '0') {
+ if (WARN("ZERO_LENGTH_ARRAY",
+ "Use C99 flexible arrays instead of zero-length arrays - see https://github.com/KSPP/linux/issues/78\n" . $hereprev) &&
+ $fix) {
+ $fixed[$fixlinenr - 1] =~ s/\[0\]/[]/g;
+ }
+ } else {
+ WARN("ONE_ELEMENT_ARRAY",
+ "Use C99 flexible arrays instead of one-element arrays - see https://github.com/KSPP/linux/issues/79\n" . $hereprev);
+ }
+ }
+
# nested likely/unlikely calls
if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
WARN("LIKELY_MISUSE",
--
2.34.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] checkpatch: Warn about 0-length and 1-element arrays
2023-05-26 17:39 [PATCH v2] checkpatch: Warn about 0-length and 1-element arrays Kees Cook
@ 2023-05-27 1:38 ` Joe Perches
2023-05-27 2:01 ` Kees Cook
0 siblings, 1 reply; 3+ messages in thread
From: Joe Perches @ 2023-05-27 1:38 UTC (permalink / raw)
To: Kees Cook, Andy Whitcroft
Cc: Dwaipayan Ray, Lukas Bulwahn, Gustavo A . R . Silva,
linux-kernel, linux-hardening
On Fri, 2023-05-26 at 10:39 -0700, Kees Cook wrote:
> Fake flexible arrays have been deprecated since last millennium. Proper
> C99 flexible arrays must be used throughout the kernel so
> CONFIG_FORTIFY_SOURCE and CONFIG_UBSAN_BOUNDS can provide proper array
> bounds checking.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> @@ -7430,6 +7430,21 @@ sub process {
> }
> }
>
> +# check for array definition/declarations that should use flexible arrays instead
> + if ($sline =~ /^[\+ ]\s*}\s*;\s*$/ &&
> + $prevline =~ /^\+\s*(?:(?:struct|union|enum)\s+$Ident|\}|$Type)\s*$Ident\s*\[\s*(0|1)\s*\]\s*;\s*$/) {
I think this is overly complicated and not necessary
$prevline =~ /^\+\s*$Type\s*$Ident\s*\[\s*(0|1)\s*\]\s*;\s*$/) {
should work no?
($Type already includes this from @typeList):
qr{struct\s+$Ident},
qr{union\s+$Ident},
qr{enum\s+$Ident},
> + if ($1 == '0') {
> + if (WARN("ZERO_LENGTH_ARRAY",
> + "Use C99 flexible arrays instead of zero-length arrays - see https://github.com/KSPP/linux/issues/78\n" . $hereprev) &&
> + $fix) {
> + $fixed[$fixlinenr - 1] =~ s/\[0\]/[]/g;
And this $fix doesn't work if the line is struct foo bar[ 0 ];
and the use of /g is odd.
Because the message is a WARN and not an ERR, please use
"Prefer/over" and not "Use/instead of"
$fixed[$fixlinenr - 1] =~ s/\[\s*0\s*\]/[]/;
> + }
> + } else {
> + WARN("ONE_ELEMENT_ARRAY",
> + "Use C99 flexible arrays instead of one-element arrays - see https://github.com/KSPP/linux/issues/79\n" . $hereprev);
> + }
And this could have a $fix change too
if (WARN("ONE_ELEMENT_ARRAY",
...) &&
$fix) {
$fixed[$fixlinenr - 1] =~ s/\[\s*1\s*\]/[]/;
> + }
> +
> # nested likely/unlikely calls
> if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
> WARN("LIKELY_MISUSE",
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] checkpatch: Warn about 0-length and 1-element arrays
2023-05-27 1:38 ` Joe Perches
@ 2023-05-27 2:01 ` Kees Cook
0 siblings, 0 replies; 3+ messages in thread
From: Kees Cook @ 2023-05-27 2:01 UTC (permalink / raw)
To: Joe Perches
Cc: Andy Whitcroft, Dwaipayan Ray, Lukas Bulwahn,
Gustavo A . R . Silva, linux-kernel, linux-hardening
On Fri, May 26, 2023 at 06:38:27PM -0700, Joe Perches wrote:
> On Fri, 2023-05-26 at 10:39 -0700, Kees Cook wrote:
> > Fake flexible arrays have been deprecated since last millennium. Proper
> > C99 flexible arrays must be used throughout the kernel so
> > CONFIG_FORTIFY_SOURCE and CONFIG_UBSAN_BOUNDS can provide proper array
> > bounds checking.
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > []
> > @@ -7430,6 +7430,21 @@ sub process {
> > }
> > }
> >
> > +# check for array definition/declarations that should use flexible arrays instead
> > + if ($sline =~ /^[\+ ]\s*}\s*;\s*$/ &&
> > + $prevline =~ /^\+\s*(?:(?:struct|union|enum)\s+$Ident|\}|$Type)\s*$Ident\s*\[\s*(0|1)\s*\]\s*;\s*$/) {
>
> I think this is overly complicated and not necessary
>
> $prevline =~ /^\+\s*$Type\s*$Ident\s*\[\s*(0|1)\s*\]\s*;\s*$/) {
>
> should work no?
>
> ($Type already includes this from @typeList):
> qr{struct\s+$Ident},
> qr{union\s+$Ident},
> qr{enum\s+$Ident},
Hm, I didn't when I originally tried it. I will double-check.
>
> > + if ($1 == '0') {
> > + if (WARN("ZERO_LENGTH_ARRAY",
> > + "Use C99 flexible arrays instead of zero-length arrays - see https://github.com/KSPP/linux/issues/78\n" . $hereprev) &&
> > + $fix) {
> > + $fixed[$fixlinenr - 1] =~ s/\[0\]/[]/g;
>
> And this $fix doesn't work if the line is struct foo bar[ 0 ];
> and the use of /g is odd.
Thanks!
> Because the message is a WARN and not an ERR, please use
> "Prefer/over" and not "Use/instead of"
I will change to ERR, then. We must not use [0]-sized arrays ever.
>
> $fixed[$fixlinenr - 1] =~ s/\[\s*0\s*\]/[]/;
> > + }
> > + } else {
> > + WARN("ONE_ELEMENT_ARRAY",
> > + "Use C99 flexible arrays instead of one-element arrays - see https://github.com/KSPP/linux/issues/79\n" . $hereprev);
> > + }
>
> And this could have a $fix change too
>
> if (WARN("ONE_ELEMENT_ARRAY",
> ...) &&
> $fix) {
> $fixed[$fixlinenr - 1] =~ s/\[\s*1\s*\]/[]/;
Almost never is it possible to only fix the struct when it's a 1-element
array (due to the impact of the size changes that be present in other
code), so I left it out.
I'll send a v3.
-Kees
--
Kees Cook
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-05-27 2:01 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-26 17:39 [PATCH v2] checkpatch: Warn about 0-length and 1-element arrays Kees Cook
2023-05-27 1:38 ` Joe Perches
2023-05-27 2:01 ` Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).