Re: [PATCH 1/2] compiler_types: Ensure __diag_clang() is always available
Hi,
On Tue, Mar 19, 2024 at 09:07:52AM -0700, Nathan Chancellor wrote:
> Attempting to use __diag_clang() and build with GCC results in a build
> error:
>
> include/linux/compiler_types.h:468:38: error: 'ignore' undeclared (first
> use in this function); did you mean 'inode'?
> 468 | __diag_ ## compiler(version, ignore, option)
> | ^~
>
> This error occurs because __diag_clang() is only defined in
> compiler-clang.h, which is only included when using clang as the
> compiler. This error has not been seen before because __diag_clang() has
> only been used in __diag_ignore_all(), which is defined in both
> compiler-clang.h and compiler-gcc.h.
>
> Add an empty stub for __diag_clang() in compiler_types.h, so that it is
> always defined and just becomes a no-op when using GCC.
>
> Fixes: f014a00bbeb0 ("compiler-clang.h: Add __diag infrastructure for clang")
> Signed-off-by: Nathan Chancellor
Reviewed-by: Justin Stitt
> ---
> include/linux/compiler_types.h | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index 3e64ec0f7ac8..fb0c3ff5497d 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -461,6 +461,10 @@ struct ftrace_likely_data {
> #define __diag_GCC(version, severity, string)
> #endif
>
> +#ifndef __diag_clang
> +#define __diag_clang(version, severity, string)
> +#endif
> +
> #define __diag_push()__diag(push)
> #define __diag_pop() __diag(pop)
>
>
> --
> 2.44.0
Thanks
Justin
Re: [PATCH 2/2] tracing: Ignore -Wstring-compare with diagnostic macros
On Tue, Mar 19, 2024 at 9:08 AM Nathan Chancellor wrote:
>
> Commit b1afefa62ca9 ("tracing: Use strcmp() in __assign_str() WARN_ON()
> check") addressed a clang warning, -Wstring-compare, with the use of
> __builtin_constant_p() to dispatch to strcmp() if the source string is a
> string literal and a direct comparison if not. Unfortunately, even with
> this change, the warning is still present because __builtin_constant_p()
> is not evaluated at this stage of the pipeline, so clang still thinks
> the else branch could occur for this situation:
>
> include/trace/events/sunrpc.h:705:4: error: result of comparison against a
> string literal is unspecified (use an explicit string comparison function
> instead) [-Werror,-Wstring-compare]
> ...
> include/trace/stages/stage6_event_callback.h:40:15: note: expanded from
> macro '__assign_str'
> 40 | (src) != __data_offsets.dst##_ptr_);
> \
> |^
> ...
>
> Use the compiler diagnostic macros to disable this warning around the
> WARN_ON_ONCE() expression since a string comparison function, strcmp(),
> will always be used for the comparison of string literals.
>
> Fixes: b1afefa62ca9 ("tracing: Use strcmp() in __assign_str() WARN_ON()
> check")
> Reported-by: Linux Kernel Functional Testing
> Closes:
> https://lore.kernel.org/all/CA+G9fYs=otkazs6g1p1ewadfr0qoe6lgovsohqkxmfxoteo...@mail.gmail.com/
> Signed-off-by: Nathan Chancellor
> ---
> include/trace/stages/stage6_event_callback.h | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/include/trace/stages/stage6_event_callback.h
> b/include/trace/stages/stage6_event_callback.h
> index 83da83a0c14f..56a4eea5a48e 100644
> --- a/include/trace/stages/stage6_event_callback.h
> +++ b/include/trace/stages/stage6_event_callback.h
> @@ -35,9 +35,14 @@
> do {\
> char *__str__ = __get_str(dst); \
> int __len__ = __get_dynamic_array_len(dst) - 1; \
> + __diag_push(); \
> + __diag_ignore(clang, 11, "-Wstring-compare",\
> + "__builtin_constant_p() ensures strcmp()" \
> + "will be used for string literals"); \
> WARN_ON_ONCE(__builtin_constant_p(src) ?\
> strcmp((src), __data_offsets.dst##_ptr_) : \
> (src) != __data_offsets.dst##_ptr_); \
What exactly is the point of the literal string comparison? Why
doesn't strcmp do the trick?
> + __diag_pop(); \
> memcpy(__str__, __data_offsets.dst##_ptr_ ? : \
>EVENT_NULL_STR, __len__);\
> __str__[__len__] = '\0';\
>
> --
> 2.44.0
>
Re: [PATCH v2 0/2] get_maintainer: add patch-only keyword matching
On Fri, Sep 29, 2023 at 11:50 AM Joe Perches wrote: > > On Fri, 2023-09-29 at 11:07 +0900, Justin Stitt wrote: > > On Fri, Sep 29, 2023 at 12:52 AM Nick Desaulniers > > wrote: > > > > > > On Wed, Sep 27, 2023 at 11:09 PM Joe Perches wrote: > > > > > > > > On Thu, 2023-09-28 at 14:31 +0900, Justin Stitt wrote: > > > > > On Thu, Sep 28, 2023 at 2:01 PM Joe Perches wrote: > > > > > > > > > > > > On Thu, 2023-09-28 at 04:23 +, Justin Stitt wrote: > > > > > > > Changes in v2: > > > > > > > - remove formatting pass (thanks Joe) (but seriously the > > > > > > > formatting is > > > > > > > bad, is there opportunity to get a formatting pass in here at > > > > > > > some > > > > > > > point?) > > > > > > > > LG G7 Battery Replacement | Guide | Is it actually a Samsung? I t > > > > > > Why? What is it that makes you believe the formatting is bad? > > > > > > > > > > > > > > > > Investigating further, it looked especially bad in my editor. There is > > > > > a mixture of > > > > > tabs and spaces and my vim tabstop is set to 4 for pl files. Setting > > > > > this to > > > > > 8 is a whole lot better. But I still see some weird spacing > > > > > > > > > > > > > Yes, it's a bit odd indentation. > > > > It's emacs default perl format. > > > > 4 space indent with 8 space tabs, maximal tab fill. > > > > > > > > > > Oh! What?! That's the most surprising convention I've ever heard of > > > (after the GNU C coding style). Yet another thing to hold against > > > perl I guess. :P > > > > > > I have my editor setup to highlight tabs vs spaces via visual cues, so > > > that I don't mess up kernel coding style. (`git clang-format HEAD~` > > > after a commit helps). scripts/get_maintainer.pl has some serious > > > inconsistencies to the point where I'm not sure what it should or was > > > meant to be. Now that you mention it, I see it, and it does seem > > > consistent in that regard. > > > > > > Justin, is your formatter configurable to match that convention? > > > Maybe it's still useful, as long as you configure it to stick to the > > > pre-existing convention. > > > > Negative, all the perl formatters I've tried will convert everything to > > spaces. > > The best I've seen is perltidy. > > > > https://gist.github.com/JustinStitt/347385921c80a5212c2672075aa769b6 > > emacs with cperl mode works fine. > > I don't know much about vim, but when I open this file in vim > it looks perfectly normal and it's apparently properly syntax > highlighted. > I believe a :set tabstop=2 will make it look weird. But really, this whole formatting thing is a non-issue for me personally once I discovered what the problem was. I'm not sure this file attracts nearly enough eyes to warrant an eager formatting attempt as I was previously preaching for. Nick and I using vim with special tab handling are most definitely the exception and for most folks this file probably looks fine.
Re: [PATCH v2 0/2] get_maintainer: add patch-only keyword matching
On Fri, Sep 29, 2023 at 12:52 AM Nick Desaulniers wrote: > > On Wed, Sep 27, 2023 at 11:09 PM Joe Perches wrote: > > > > On Thu, 2023-09-28 at 14:31 +0900, Justin Stitt wrote: > > > On Thu, Sep 28, 2023 at 2:01 PM Joe Perches wrote: > > > > > > > > On Thu, 2023-09-28 at 04:23 +, Justin Stitt wrote: > > > > > Changes in v2: > > > > > - remove formatting pass (thanks Joe) (but seriously the formatting is > > > > > bad, is there opportunity to get a formatting pass in here at some > > > > > point?) > > > > > > > > Why? What is it that makes you believe the formatting is bad? > > > > > > > > > > Investigating further, it looked especially bad in my editor. There is > > > a mixture of > > > tabs and spaces and my vim tabstop is set to 4 for pl files. Setting this > > > to > > > 8 is a whole lot better. But I still see some weird spacing > > > > > > > Yes, it's a bit odd indentation. > > It's emacs default perl format. > > 4 space indent with 8 space tabs, maximal tab fill. > > > > Oh! What?! That's the most surprising convention I've ever heard of > (after the GNU C coding style). Yet another thing to hold against > perl I guess. :P > > I have my editor setup to highlight tabs vs spaces via visual cues, so > that I don't mess up kernel coding style. (`git clang-format HEAD~` > after a commit helps). scripts/get_maintainer.pl has some serious > inconsistencies to the point where I'm not sure what it should or was > meant to be. Now that you mention it, I see it, and it does seem > consistent in that regard. > > Justin, is your formatter configurable to match that convention? > Maybe it's still useful, as long as you configure it to stick to the > pre-existing convention. Negative, all the perl formatters I've tried will convert everything to spaces. The best I've seen is perltidy. https://gist.github.com/JustinStitt/347385921c80a5212c2672075aa769b6 > -- > Thanks, > ~Nick Desaulniers
Re: [PATCH v2 1/2] get_maintainer: add patch-only keyword-matching
On Thu, Sep 28, 2023 at 1:46 PM Joe Perches wrote:
>
> On Thu, 2023-09-28 at 04:23 +, Justin Stitt wrote:
> > Add the "D:" type which behaves the same as "K:" but will only match
> > content present in a patch file.
> >
> > To illustrate:
> >
> > Imagine this entry in MAINTAINERS:
> >
> > NEW REPUBLIC
> > M: Han Solo
> > W: https://www.jointheresistance.org
> > D: \bstrncpy\b
> >
> > Our maintainer, Han, will only be added to the recipients if a patch
> > file is passed to get_maintainer (like what b4 does):
> > $ ./scripts/get_maintainer.pl 0004-some-change.patch
> >
> > If the above patch has a `strncpy` present in the subject, commit log or
> > diff then Han will be to/cc'd.
> >
> > However, in the event of a file from the tree given like:
> > $ ./scripts/get_maintainer.pl ./lib/string.c
> >
> > Han will not be noisily to/cc'd (like a K: type would in this
> > circumstance)
> >
> > Signed-off-by: Justin Stitt
> > ---
> > MAINTAINERS | 5 +
> > scripts/get_maintainer.pl | 12 ++--
> > 2 files changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index b19995690904..94e431daa7c2 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -59,6 +59,11 @@ Descriptions of section entries and preferred order
> > matches patches or files that contain one or more of the words
> > printk, pr_info or pr_err
> > One regex pattern per line. Multiple K: lines acceptable.
> > + D: *Diff content regex* (perl extended) pattern match that applies only
> > to
> > + patches and not entire files (e.g. when using the get_maintainers.pl
> > + script).
> > + Usage same as K:.
> > +
> >
> > Maintainers List
> >
> > diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
> > index ab123b498fd9..a3e697926ddd 100755
> > --- a/scripts/get_maintainer.pl
> > +++ b/scripts/get_maintainer.pl
> > @@ -342,6 +342,7 @@ if ($tree && !top_of_kernel_tree($lk_path)) {
> >
> > my @typevalue = ();
> > my %keyword_hash;
> > +my %patch_keyword_hash;
> > my @mfiles = ();
> > my @self_test_info = ();
> >
> > @@ -369,8 +370,10 @@ sub read_maintainer_file {
> > $value =~ s@([^/])$@$1/@;
> > }
> > } elsif ($type eq "K") {
> > - $keyword_hash{@typevalue} = $value;
> > - }
> > + $keyword_hash{@typevalue} = $value;
> > + } elsif ($type eq "D") {
> > + $patch_keyword_hash{@typevalue} = $value;
> > + }
> > push(@typevalue, "$type:$value");
> > } elsif (!(/^\s*$/ || /^\s*\#/)) {
> > push(@typevalue, $line);
> > @@ -607,6 +610,11 @@ foreach my $file (@ARGV) {
> > push(@keyword_tvi, $line);
> > }
> > }
> > +foreach my $line(keys %patch_keyword_hash) {
> > + if ($patch_line =~ m/${patch_prefix}$patch_keyword_hash{$line}/x) {
> > +push(@keyword_tvi, $line);
> > + }
> > +}
> > }
> > }
> > close($patch);
> >
>
>
> My opinion: Nack.
>
> I think something like this would be better
> as it avoids duplication of K and D content.
If I understand correctly, this puts the onus on the get_maintainer users
to select the right argument whereas adding "D:", albeit with some
duplicate code, allows maintainers themselves to decide in exactly
which context they receive mail.
Adding a command line flag means sometimes K: is treated one
way and sometimes treated a different way depending on
the specific incantation.
This could all be a moot point, though, as I believe Konstantin
is trying to separate out the whole idea of a patch-sender needing
to specify the recipients of a patch. Instead some middleware would
capture mail and delegate automatically based on some queries
set up by maintainers.
Exciting idea, I wonder what the progress is on that?
Cc: Konstantin Ryabitsev
[1]: https://lore.kernel.org/all/20230726-june-mocha-ad6809@meerkat/
> ---
> scripts/get_maintainer.pl | 16 +---
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
> index ab123b498fd9..07e7d744cadb 100755
> --- a/scripts/get_maintainer.pl
> +++ b/scripts/get_maintainer.pl
> @@ -57,6 +57,7 @@ my $subsystem = 0;
> my $st
[PATCH v2 2/2] MAINTAINERS: migrate some K to D
Let's get the ball rolling with some changes from K to D. Ultimately, if it turns out that 100% of K users want to change to D then really the behavior of K could just be changed. Signed-off-by: Justin Stitt Original-author: Kees Cook --- MAINTAINERS | 16 +--- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 94e431daa7c2..80ffdaa8f044 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -5038,7 +5038,7 @@ F:Documentation/kbuild/llvm.rst F: include/linux/compiler-clang.h F: scripts/Makefile.clang F: scripts/clang-tools/ -K: \b(?i:clang|llvm)\b +D: \b(?i:clang|llvm)\b CLK API M: Russell King @@ -8149,7 +8149,7 @@ F:lib/strcat_kunit.c F: lib/strscpy_kunit.c F: lib/test_fortify/* F: scripts/test_fortify.sh -K: \b__NO_FORTIFY\b +D: \b__NO_FORTIFY\b FPGA DFL DRIVERS M: Wu Hao @@ -11405,8 +11405,10 @@ F: Documentation/ABI/testing/sysfs-kernel-warn_count F: include/linux/overflow.h F: include/linux/randomize_kstack.h F: mm/usercopy.c -K: \b(add|choose)_random_kstack_offset\b -K: \b__check_(object_size|heap_object)\b +D: \b(add|choose)_random_kstack_offset\b +D: \b__check_(object_size|heap_object)\b +D: \b__counted_by\b + KERNEL JANITORS L: kernel-janit...@vger.kernel.org @@ -17290,7 +17292,7 @@ F: drivers/acpi/apei/erst.c F: drivers/firmware/efi/efi-pstore.c F: fs/pstore/ F: include/linux/pstore* -K: \b(pstore|ramoops) +D: \b(pstore|ramoops) PTP HARDWARE CLOCK SUPPORT M: Richard Cochran @@ -19231,8 +19233,8 @@ F: include/uapi/linux/seccomp.h F: kernel/seccomp.c F: tools/testing/selftests/kselftest_harness.h F: tools/testing/selftests/seccomp/* -K: \bsecure_computing -K: \bTIF_SECCOMP\b +D: \bsecure_computing +D: \bTIF_SECCOMP\b SECURE DIGITAL HOST CONTROLLER INTERFACE (SDHCI) Broadcom BRCMSTB DRIVER M: Kamal Dasu -- 2.42.0.582.g8ccd20d70d-goog
[PATCH v2 1/2] get_maintainer: add patch-only keyword-matching
Add the "D:" type which behaves the same as "K:" but will only match
content present in a patch file.
To illustrate:
Imagine this entry in MAINTAINERS:
NEW REPUBLIC
M: Han Solo
W: https://www.jointheresistance.org
D: \bstrncpy\b
Our maintainer, Han, will only be added to the recipients if a patch
file is passed to get_maintainer (like what b4 does):
$ ./scripts/get_maintainer.pl 0004-some-change.patch
If the above patch has a `strncpy` present in the subject, commit log or
diff then Han will be to/cc'd.
However, in the event of a file from the tree given like:
$ ./scripts/get_maintainer.pl ./lib/string.c
Han will not be noisily to/cc'd (like a K: type would in this
circumstance)
Signed-off-by: Justin Stitt
---
MAINTAINERS | 5 +
scripts/get_maintainer.pl | 12 ++--
2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index b19995690904..94e431daa7c2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -59,6 +59,11 @@ Descriptions of section entries and preferred order
matches patches or files that contain one or more of the words
printk, pr_info or pr_err
One regex pattern per line. Multiple K: lines acceptable.
+ D: *Diff content regex* (perl extended) pattern match that applies only to
+ patches and not entire files (e.g. when using the get_maintainers.pl
+ script).
+ Usage same as K:.
+
Maintainers List
diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index ab123b498fd9..a3e697926ddd 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -342,6 +342,7 @@ if ($tree && !top_of_kernel_tree($lk_path)) {
my @typevalue = ();
my %keyword_hash;
+my %patch_keyword_hash;
my @mfiles = ();
my @self_test_info = ();
@@ -369,8 +370,10 @@ sub read_maintainer_file {
$value =~ s@([^/])$@$1/@;
}
} elsif ($type eq "K") {
- $keyword_hash{@typevalue} = $value;
- }
+ $keyword_hash{@typevalue} = $value;
+ } elsif ($type eq "D") {
+ $patch_keyword_hash{@typevalue} = $value;
+ }
push(@typevalue, "$type:$value");
} elsif (!(/^\s*$/ || /^\s*\#/)) {
push(@typevalue, $line);
@@ -607,6 +610,11 @@ foreach my $file (@ARGV) {
push(@keyword_tvi, $line);
}
}
+foreach my $line(keys %patch_keyword_hash) {
+ if ($patch_line =~ m/${patch_prefix}$patch_keyword_hash{$line}/x) {
+push(@keyword_tvi, $line);
+ }
+}
}
}
close($patch);
--
2.42.0.582.g8ccd20d70d-goog
[PATCH v2 0/2] get_maintainer: add patch-only keyword matching
This series aims to add "D:" which behaves exactly the same as "K:" but works only on patch files. The goal of this is to reduce noise when folks use get_maintainer on tree files as opposed to patches. "D:" should help maintainers reduce noise in their inboxes, especially when matching omnipresent keywords like [1]. In the event of [1] Kees would be to/cc'd from folks running get_maintainer on _any_ file containing "__counted_by". The number of these files is rising and I fear for his inbox as his goal, as I understand it, is to simply monitor the introduction of new __counted_by annotations to ensure accurate semantics. Joe mentioned in v1 that perhaps K: should be reworked to only consider patch files. I wonder, though, if folks are intentionally using the current behavior of K: and thus would be peeved from a change there. I see this series as, at the very least, a gentle migration in behavior which is purely opt-in and at some point could eagerly be merged with K:. [1]: https://lore.kernel.org/all/20230925172037.work.853-k...@kernel.org/ Signed-off-by: Justin Stitt --- Changes in v2: - remove bits about non-patch usage being bad (thanks Greg, Kees, et al.) - remove formatting pass (thanks Joe) (but seriously the formatting is bad, is there opportunity to get a formatting pass in here at some point?) - add some migration from K to D (thanks Kees, Nick) - Link to v1: https://lore.kernel.org/r/20230927-get_maintainer_add_d-v1-0-28c207229...@google.com --- Justin Stitt (2): get_maintainer: add patch-only keyword-matching MAINTAINERS: migrate some K to D MAINTAINERS | 21 ++--- scripts/get_maintainer.pl | 12 ++-- 2 files changed, 24 insertions(+), 9 deletions(-) --- base-commit: 6465e260f48790807eef06b583b38ca9789b6072 change-id: 20230926-get_maintainer_add_d-07424a814e72 Best regards, -- Justin Stitt
Re: [PATCH 3/3] get_maintainer: add patch-only pattern matching type
On Wed, Sep 27, 2023 at 3:14 PM Greg KH wrote: > > On Wed, Sep 27, 2023 at 03:19:16AM +, Justin Stitt wrote: > > Note that folks really shouldn't be using get_maintainer on tree files > > anyways [1]. > > That's not true, Linus and I use it on a daily basis this way, it's part > of our normal workflow, AND the workflow of the kernel security team. > > So please don't take that valid use-case away from us. Fair. I'm on the side of keeping the "K:'' behavior the way it is and that's why I'm proposing adding "D:" to provide a more granular content matching type operating strictly on patches. It's purely opt-in. The patch I linked mentioned steering folks away from using tree files but not necessarily removing the behavior. > > thanks, > > greg k-h Thanks Justin
[PATCH 3/3] get_maintainer: add patch-only pattern matching type
Add the "D:" type which behaves the same as "K:" but will only match
content present in a patch file.
To illustrate:
Imagine this entry in MAINTAINERS:
NEW REPUBLIC
M: Han Solo
W: https://www.jointheresistance.org
D: \bstrncpy\b
Our maintainer, Han, will only be added to the recipients if a patch
file is passed to get_maintainer (like what b4 does):
$ ./scripts/get_maintainer.pl 0004-some-change.patch
If the above patch has a `strncpy` present in the subject, commit log or
diff then Han will be to/cc'd.
However, in the event of a file from the tree given like:
$ ./scripts/get_maintainer.pl ./lib/string.c
Han will not be noisily to/cc'd (like a K: type would in this
circumstance)
Note that folks really shouldn't be using get_maintainer on tree files
anyways [1].
[1]: https://lore.kernel.org/all/20230726151515.1650519-1-k...@kernel.org/
---
scripts/get_maintainer.pl | 9 +
1 file changed, 9 insertions(+)
diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index e679eac96793..f290bf0948f1 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -309,6 +309,7 @@ if ( $tree && !top_of_kernel_tree($lk_path) ) {
my @typevalue = ();
my %keyword_hash;
+my %patch_keyword_hash;
my @mfiles = ();
my @self_test_info = ();
@@ -339,6 +340,9 @@ sub read_maintainer_file {
elsif ( $type eq "K" ) {
$keyword_hash{@typevalue} = $value;
}
+elsif ( $type eq "D" ) {
+$patch_keyword_hash{@typevalue} = $value;
+}
push( @typevalue, "$type:$value" );
}
elsif ( !( /^\s*$/ || /^\s*\#/ ) ) {
@@ -591,6 +595,11 @@ foreach my $file (@ARGV) {
push( @keyword_tvi, $line );
}
}
+foreach my $line ( keys %patch_keyword_hash ) {
+if ($patch_line =~
m/${patch_prefix}$patch_keyword_hash{$line}/x ) {
+push( @keyword_tvi, $line );
+}
+}
}
}
close($patch);
--
2.42.0.582.g8ccd20d70d-goog
[PATCH 1/3] MAINTAINERS: add documentation for D:
Document what "D:" does. This is more or less the same as what "K:" does but only works for patch files. See [3/3] for more info and an illustrative example. --- MAINTAINERS | 3 +++ 1 file changed, 3 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index b19995690904..de68d2c0cf29 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -59,6 +59,9 @@ Descriptions of section entries and preferred order matches patches or files that contain one or more of the words printk, pr_info or pr_err One regex pattern per line. Multiple K: lines acceptable. + D: *Content regex* (perl extended) pattern match patches only. + Usage same as K:. + Maintainers List -- 2.42.0.582.g8ccd20d70d-goog
Re: [PATCH 1/3] MAINTAINERS: add documentation for D:
On Wed, Sep 27, 2023 at 12:27 PM Joe Perches wrote: > > On Wed, 2023-09-27 at 03:19 +, Justin Stitt wrote: > > Document what "D:" does. > > > > This is more or less the same as what "K:" does but only works for patch > > files. > > Nack. I'd rather just add a !$file test to K: patterns. Are there no legitimate use cases for K:'s current behavior to warrant keeping it around? >
[PATCH 0/3] get_maintainer: add patch-only keyword matching
This series aims to add "D:" which behaves exactly the same as "K:" but works only on patch files. The goal of this is to reduce noise when folks use get_maintainer on tree files as opposed to patches. This use case should be steered away from [1] but "D:" should help maintainers reduce noise in their inboxes regardless, especially when matching omnipresent keywords like [2]. In the event of [2] Kees would be to/cc'd from folks running get_maintainer on _any_ file containing "__counted_by". The number of these files is rising and I fear for his inbox as his goal, as I understand it, is to simply monitor the introduction of new __counted_by annotations to ensure accurate semantics. See [3/3] for an illustrative example. This series also includes a formatting pass over get_maintainer because I personally found it difficult to parse with the human eye. [1]: https://lore.kernel.org/all/20230726151515.1650519-1-k...@kernel.org/ [2]: https://lore.kernel.org/all/20230925172037.work.853-k...@kernel.org/ Signed-off-by: Justin Stitt --- Justin Stitt (3): MAINTAINERS: add documentation for D: get_maintainer: run perltidy get_maintainer: add patch-only pattern matching type MAINTAINERS |3 + scripts/get_maintainer.pl | 3334 +++-- 2 files changed, 1718 insertions(+), 1619 deletions(-) --- base-commit: 6465e260f48790807eef06b583b38ca9789b6072 change-id: 20230926-get_maintainer_add_d-07424a814e72 Best regards, -- Justin Stitt
[PATCH] HID: uhid: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1].
We should prefer more robust and less ambiguous string interfaces.
A suitable replacement is `strscpy` [2] due to the fact that it
guarantees NUL-termination on the destination buffer without
unnecessarily NUL-padding.
Looking at: Commit 4d26d1d1e806 ("Revert "HID: uhid: use strlcpy() instead of
strncpy()"")
we see referenced the fact that many attempts have been made to change
these strncpy's into strlcpy to no success. I think strscpy is an
objectively better interface here as it doesn't unnecessarily NUL-pad
the destination buffer whilst allowing us to drop the `len = min(...)`
pattern as strscpy will implicitly limit the number of bytes copied by
the smaller of its dest and src arguments.
So while the existing code may not have a bug (i.e: overread problems)
we should still favor strscpy due to readability (plus a very slight
performance boost).
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
[1]
Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-harden...@vger.kernel.org
Cc: Kees Cook
Signed-off-by: Justin Stitt
---
drivers/hid/uhid.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 4588d2cd4ea4..00e1566ad37b 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -490,7 +490,7 @@ static int uhid_dev_create2(struct uhid_device *uhid,
const struct uhid_event *ev)
{
struct hid_device *hid;
- size_t rd_size, len;
+ size_t rd_size;
void *rd_data;
int ret;
@@ -514,13 +514,9 @@ static int uhid_dev_create2(struct uhid_device *uhid,
goto err_free;
}
- /* @hid is zero-initialized, strncpy() is correct, strlcpy() not */
- len = min(sizeof(hid->name), sizeof(ev->u.create2.name)) - 1;
- strncpy(hid->name, ev->u.create2.name, len);
- len = min(sizeof(hid->phys), sizeof(ev->u.create2.phys)) - 1;
- strncpy(hid->phys, ev->u.create2.phys, len);
- len = min(sizeof(hid->uniq), sizeof(ev->u.create2.uniq)) - 1;
- strncpy(hid->uniq, ev->u.create2.uniq, len);
+ strscpy(hid->name, ev->u.create2.name, sizeof(hid->name));
+ strscpy(hid->phys, ev->u.create2.phys, sizeof(hid->phys));
+ strscpy(hid->uniq, ev->u.create2.uniq, sizeof(hid->uniq));
hid->ll_driver = _hid_driver;
hid->bus = ev->u.create2.bus;
---
base-commit: 3669558bdf354cd352be955ef2764cde6a9bf5ec
change-id: 20230914-strncpy-drivers-hid-uhid-c-a465f3e784dd
Best regards,
--
Justin Stitt
[PATCH] HID: prodikeys: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. We should prefer more robust and less ambiguous string interfaces. A suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on the destination buffer without unnecessarily NUL-padding. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-harden...@vger.kernel.org Signed-off-by: Justin Stitt --- Note: for some reason if NUL-padding is needed let's opt for `strscpy_pad()` --- drivers/hid/hid-prodikeys.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/hid/hid-prodikeys.c b/drivers/hid/hid-prodikeys.c index e4e9471d0f1e..c16d2ba6ea16 100644 --- a/drivers/hid/hid-prodikeys.c +++ b/drivers/hid/hid-prodikeys.c @@ -639,9 +639,9 @@ static int pcmidi_snd_initialise(struct pcmidi_snd *pm) goto fail; } - strncpy(card->driver, shortname, sizeof(card->driver)); - strncpy(card->shortname, shortname, sizeof(card->shortname)); - strncpy(card->longname, longname, sizeof(card->longname)); + strscpy(card->driver, shortname, sizeof(card->driver)); + strscpy(card->shortname, shortname, sizeof(card->shortname)); + strscpy(card->longname, longname, sizeof(card->longname)); /* Set up rawmidi */ err = snd_rawmidi_new(card, card->shortname, 0, @@ -652,7 +652,7 @@ static int pcmidi_snd_initialise(struct pcmidi_snd *pm) goto fail; } pm->rwmidi = rwmidi; - strncpy(rwmidi->name, card->shortname, sizeof(rwmidi->name)); + strscpy(rwmidi->name, card->shortname, sizeof(rwmidi->name)); rwmidi->info_flags = SNDRV_RAWMIDI_INFO_INPUT; rwmidi->private_data = pm; --- base-commit: 3669558bdf354cd352be955ef2764cde6a9bf5ec change-id: 20230914-strncpy-drivers-hid-hid-prodikeys-c-cf42614a21d4 Best regards, -- Justin Stitt
[PATCH] firmware: ti_sci: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. We should prefer more robust and less ambiguous string interfaces. A suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on the destination buffer. It does not seem like `ver->firmware_description` requires NUL-padding (which is a behavior that strncpy provides) but if it does let's opt for `strscpy_pad()`. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-harden...@vger.kernel.org Signed-off-by: Justin Stitt --- Note: build-tested only. --- drivers/firmware/ti_sci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c index 26a37f47f4ca..ce546f391959 100644 --- a/drivers/firmware/ti_sci.c +++ b/drivers/firmware/ti_sci.c @@ -485,7 +485,7 @@ static int ti_sci_cmd_get_revision(struct ti_sci_info *info) ver->abi_major = rev_info->abi_major; ver->abi_minor = rev_info->abi_minor; ver->firmware_revision = rev_info->firmware_revision; - strncpy(ver->firmware_description, rev_info->firmware_description, + strscpy(ver->firmware_description, rev_info->firmware_description, sizeof(ver->firmware_description)); fail: --- base-commit: 3669558bdf354cd352be955ef2764cde6a9bf5ec change-id: 20230913-strncpy-drivers-firmware-ti_sci-c-22667413c18f Best regards, -- Justin Stitt
[PATCH] firmware: tegra: bpmp: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1].
We should prefer more robust and less ambiguous string interfaces.
It seems like the filename stored at `namevirt` is expected to be
NUL-terminated.
A suitable replacement is `strscpy_pad` due to the fact that it
guarantees NUL-termination on the destination buffer whilst maintaining
the NUL-padding behavior that strncpy provides.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
[1]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-harden...@vger.kernel.org
Signed-off-by: Justin Stitt
---
Note: compile tested only.
---
drivers/firmware/tegra/bpmp-debugfs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/tegra/bpmp-debugfs.c
b/drivers/firmware/tegra/bpmp-debugfs.c
index 6dfe3d34109e..bbcdd9fed3fb 100644
--- a/drivers/firmware/tegra/bpmp-debugfs.c
+++ b/drivers/firmware/tegra/bpmp-debugfs.c
@@ -610,7 +610,7 @@ static int debugfs_show(struct seq_file *m, void *p)
}
len = strlen(filename);
- strncpy(namevirt, filename, namesize);
+ strscpy_pad(namevirt, filename, namesize);
err = mrq_debugfs_read(bpmp, namephys, len, dataphys, datasize,
);
@@ -661,7 +661,7 @@ static ssize_t debugfs_store(struct file *file, const char
__user *buf,
}
len = strlen(filename);
- strncpy(namevirt, filename, namesize);
+ strscpy_pad(namevirt, filename, namesize);
if (copy_from_user(datavirt, buf, count)) {
err = -EFAULT;
---
base-commit: 3669558bdf354cd352be955ef2764cde6a9bf5ec
change-id: 20230913-strncpy-drivers-firmware-tegra-bpmp-debugfs-c-54f7baaf32c0
Best regards,
--
Justin Stitt
[PATCH v3] EDAC/mc_sysfs: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. We should prefer more robust and less ambiguous string interfaces. A suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on the destination buffer without needlessly NUL-padding. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-harden...@vger.kernel.org Signed-off-by: Justin Stitt --- Changes in v3: - prefer strscpy to strscpy_pad (thanks Tony) - Link to v2: https://lore.kernel.org/r/20230913-strncpy-drivers-edac-edac_mc_sysfs-c-v2-1-2d2e6bd43...@google.com Changes in v2: - included refactor of another strncpy in same file - Link to v1: https://lore.kernel.org/r/20230913-strncpy-drivers-edac-edac_mc_sysfs-c-v1-1-d232891b0...@google.com --- Note: build-tested only. --- drivers/edac/edac_mc_sysfs.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c index 15f63452a9be..9a5b4bbd8191 100644 --- a/drivers/edac/edac_mc_sysfs.c +++ b/drivers/edac/edac_mc_sysfs.c @@ -229,8 +229,7 @@ static ssize_t channel_dimm_label_store(struct device *dev, if (copy_count == 0 || copy_count >= sizeof(rank->dimm->label)) return -EINVAL; - strncpy(rank->dimm->label, data, copy_count); - rank->dimm->label[copy_count] = '\0'; + strscpy(rank->dimm->label, data, copy_count); return count; } @@ -535,7 +534,7 @@ static ssize_t dimmdev_label_store(struct device *dev, if (copy_count == 0 || copy_count >= sizeof(dimm->label)) return -EINVAL; - strncpy(dimm->label, data, copy_count); + strscpy(dimm->label, data, copy_count); dimm->label[copy_count] = '\0'; return count; --- base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c change-id: 20230913-strncpy-drivers-edac-edac_mc_sysfs-c-e619b00124a3 Best regards, -- Justin Stitt
Re: [PATCH] EDAC/mc_sysfs: refactor deprecated strncpy
On Wed, Sep 13, 2023 at 8:13 AM Luck, Tony wrote: > > > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > > > We should prefer more robust and less ambiguous string interfaces. > > > > A suitable replacement is `strscpy_pad` [2] due to the fact that it > > guarantees > > NUL-termination on the destination buffer whilst maintaining the > > NUL-padding behavior that `strncpy` provides. This may not be strictly > > necessary but as I couldn't understand what this code does I wanted to > > ensure that the functionality is the same. > > > > Link: > > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > > [1] > > Link: > > https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] > > Link: https://github.com/KSPP/linux/issues/90 > > Cc: linux-harden...@vger.kernel.org > > Signed-off-by: Justin Stitt > > --- > > Note: build-tested only. > > --- > > drivers/edac/edac_mc_sysfs.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c > > index 15f63452a9be..b303309a63cf 100644 > > --- a/drivers/edac/edac_mc_sysfs.c > > +++ b/drivers/edac/edac_mc_sysfs.c > > @@ -229,8 +229,7 @@ static ssize_t channel_dimm_label_store(struct device > > *dev, > > if (copy_count == 0 || copy_count >= sizeof(rank->dimm->label)) > > return -EINVAL; > > > > - strncpy(rank->dimm->label, data, copy_count); > > - rank->dimm->label[copy_count] = '\0'; > > + strscpy_pad(rank->dimm->label, data, copy_count); > > That doc page says the problem with strncpy() is that it doesn't guarantee to > NUL terminate the target string. But this code is aware of that limitation and > zaps a '\0' at the end to be sure. > > So this code doesn't suffer from the potential problems. Right, the original code did not have an existing bug due to the reason you mentioned. However, I'm pretty keen on eliminating uses of this interface treewide as there is always a more robust and less ambiguous option. > > If it is going to be fixed, then some further analysis of the original code > would be wise. Just replacing with strscpy_pad() means the code probably > still suffers from the "needless performance penalty" also mentioned in > the deprecation document. Got it, sending a v2 that prefers `strscpy` to `strscpy_pad` resolving the performance issue. > > -Tony > Thanks for the timely review! Justin
[PATCH v2] ipmi: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1].
In this case, strncpy is being used specifically for its NUL-padding
behavior (and has been commented as such). Moreover, the destination
string is not required to be NUL-terminated [2].
We can use a more robust and less ambiguous interface in
`memcpy_and_pad` which makes the code more readable and even eliminates
the need for that comment.
Let's also use `strnlen` instead of `strlen()` with an upper-bounds
check as this is intrinsically a part of `strnlen`.
Also included in this patch is a simple 1:1 change of `strncpy` to
`strscpy` for ipmi_ssif.c. If NUL-padding is wanted here as well then we
should opt again for `strscpy_pad`.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
[1]
Link: https://lore.kernel.org/all/zqeadybl0uz1n...@mail.minyard.net/ [2]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-harden...@vger.kernel.org
Cc: Kees Cook
Signed-off-by: Justin Stitt
---
Changes in v2:
- use memcpy_and_pad (thanks Corey)
- Link to v1:
https://lore.kernel.org/r/20230912-strncpy-drivers-char-ipmi-ipmi-v1-1-cc43e0d1c...@google.com
---
drivers/char/ipmi/ipmi_msghandler.c | 11 +++
drivers/char/ipmi/ipmi_ssif.c | 2 +-
2 files changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_msghandler.c
b/drivers/char/ipmi/ipmi_msghandler.c
index 186f1fee7534..d6f14279684d 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -5377,20 +5377,15 @@ static void send_panic_events(struct ipmi_smi *intf,
char *str)
j = 0;
while (*p) {
- int size = strlen(p);
+ int size = strnlen(p, 11);
- if (size > 11)
- size = 11;
data[0] = 0;
data[1] = 0;
data[2] = 0xf0; /* OEM event without timestamp. */
data[3] = intf->addrinfo[0].address;
data[4] = j++; /* sequence # */
- /*
-* Always give 11 bytes, so strncpy will fill
-* it with zeroes for me.
-*/
- strncpy(data+5, p, 11);
+
+ memcpy_and_pad(data+5, 11, p, size, '\0');
p += size;
ipmi_panic_request_and_wait(intf, , );
diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
index 3b921c78ba08..edcb83765dce 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -1940,7 +1940,7 @@ static int new_ssif_client(int addr, char *adapter_name,
}
}
- strncpy(addr_info->binfo.type, DEVICE_NAME,
+ strscpy(addr_info->binfo.type, DEVICE_NAME,
sizeof(addr_info->binfo.type));
addr_info->binfo.addr = addr;
addr_info->binfo.platform_data = addr_info;
---
base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
change-id: 20230912-strncpy-drivers-char-ipmi-ipmi-dda47b3773fd
Best regards,
--
Justin Stitt
Re: [PATCH] EDAC/mc_sysfs: refactor deprecated strncpy
On Tue, Sep 12, 2023 at 6:26 PM Justin Stitt wrote: > > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > We should prefer more robust and less ambiguous string interfaces. > > A suitable replacement is `strscpy_pad` [2] due to the fact that it guarantees > NUL-termination on the destination buffer whilst maintaining the > NUL-padding behavior that `strncpy` provides. This may not be strictly > necessary but as I couldn't understand what this code does I wanted to > ensure that the functionality is the same. > > Link: > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-harden...@vger.kernel.org > Signed-off-by: Justin Stitt > --- > Note: build-tested only. > --- > drivers/edac/edac_mc_sysfs.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c > index 15f63452a9be..b303309a63cf 100644 > --- a/drivers/edac/edac_mc_sysfs.c > +++ b/drivers/edac/edac_mc_sysfs.c > @@ -229,8 +229,7 @@ static ssize_t channel_dimm_label_store(struct device > *dev, > if (copy_count == 0 || copy_count >= sizeof(rank->dimm->label)) > return -EINVAL; > > - strncpy(rank->dimm->label, data, copy_count); > - rank->dimm->label[copy_count] = '\0'; > + strscpy_pad(rank->dimm->label, data, copy_count); > > return count; > } > > --- > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c > change-id: 20230913-strncpy-drivers-edac-edac_mc_sysfs-c-e619b00124a3 > > Best regards, > -- > Justin Stitt > I typo'd my grep and initially missed refactoring another instance of strncpy in this same file. v2 [1] resolves this. [1]: https://lore.kernel.org/r/20230913-strncpy-drivers-edac-edac_mc_sysfs-c-v2-1-2d2e6bd43...@google.com
[PATCH v2] EDAC/mc_sysfs: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. We should prefer more robust and less ambiguous string interfaces. A suitable replacement is `strscpy_pad` [2] due to the fact that it guarantees NUL-termination on the destination buffer whilst maintaining the NUL-padding behavior that `strncpy` provides. This may not be strictly necessary but as I couldn't understand what this code does I wanted to ensure that the functionality is the same. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-harden...@vger.kernel.org Signed-off-by: Justin Stitt --- Changes in v2: - included refactor of another strncpy in same file - Link to v1: https://lore.kernel.org/r/20230913-strncpy-drivers-edac-edac_mc_sysfs-c-v1-1-d232891b0...@google.com --- Note: build-tested only. --- drivers/edac/edac_mc_sysfs.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c index 15f63452a9be..ce025a20288c 100644 --- a/drivers/edac/edac_mc_sysfs.c +++ b/drivers/edac/edac_mc_sysfs.c @@ -229,8 +229,7 @@ static ssize_t channel_dimm_label_store(struct device *dev, if (copy_count == 0 || copy_count >= sizeof(rank->dimm->label)) return -EINVAL; - strncpy(rank->dimm->label, data, copy_count); - rank->dimm->label[copy_count] = '\0'; + strscpy_pad(rank->dimm->label, data, copy_count); return count; } @@ -535,7 +534,7 @@ static ssize_t dimmdev_label_store(struct device *dev, if (copy_count == 0 || copy_count >= sizeof(dimm->label)) return -EINVAL; - strncpy(dimm->label, data, copy_count); + strscpy_pad(dimm->label, data, copy_count); dimm->label[copy_count] = '\0'; return count; --- base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c change-id: 20230913-strncpy-drivers-edac-edac_mc_sysfs-c-e619b00124a3 Best regards, -- Justin Stitt
[PATCH] EDAC/mc_sysfs: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. We should prefer more robust and less ambiguous string interfaces. A suitable replacement is `strscpy_pad` [2] due to the fact that it guarantees NUL-termination on the destination buffer whilst maintaining the NUL-padding behavior that `strncpy` provides. This may not be strictly necessary but as I couldn't understand what this code does I wanted to ensure that the functionality is the same. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-harden...@vger.kernel.org Signed-off-by: Justin Stitt --- Note: build-tested only. --- drivers/edac/edac_mc_sysfs.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c index 15f63452a9be..b303309a63cf 100644 --- a/drivers/edac/edac_mc_sysfs.c +++ b/drivers/edac/edac_mc_sysfs.c @@ -229,8 +229,7 @@ static ssize_t channel_dimm_label_store(struct device *dev, if (copy_count == 0 || copy_count >= sizeof(rank->dimm->label)) return -EINVAL; - strncpy(rank->dimm->label, data, copy_count); - rank->dimm->label[copy_count] = '\0'; + strscpy_pad(rank->dimm->label, data, copy_count); return count; } --- base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c change-id: 20230913-strncpy-drivers-edac-edac_mc_sysfs-c-e619b00124a3 Best regards, -- Justin Stitt
[PATCH] dax: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1].
We should prefer more robust and less ambiguous string interfaces.
`dax_id->dev_name` is expected to be NUL-terminated and has been zero-allocated.
A suitable replacement is `strscpy` [2] due to the fact that it
guarantees NUL-termination on the destination buffer. Moreover, due to
`dax_id` being zero-allocated the padding behavior of `strncpy` is not
needed and a simple 1:1 replacement of strncpy -> strscpy should
suffice.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
[1]
Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-harden...@vger.kernel.org
Signed-off-by: Justin Stitt
---
Note: build-tested only.
---
drivers/dax/bus.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
index 0ee96e6fc426..1659b787b65f 100644
--- a/drivers/dax/bus.c
+++ b/drivers/dax/bus.c
@@ -103,7 +103,7 @@ static ssize_t do_id_store(struct device_driver *drv, const
char *buf,
if (action == ID_ADD) {
dax_id = kzalloc(sizeof(*dax_id), GFP_KERNEL);
if (dax_id) {
- strncpy(dax_id->dev_name, buf, DAX_NAME_LEN);
+ strscpy(dax_id->dev_name, buf, DAX_NAME_LEN);
list_add(_id->list, _drv->ids);
} else
rc = -ENOMEM;
---
base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
change-id: 20230913-strncpy-drivers-dax-bus-c-f12e3153e44b
Best regards,
--
Justin Stitt
Re: [PATCH] ipmi: refactor deprecated strncpy
On Tue, Sep 12, 2023 at 5:55 PM Justin Stitt wrote:
>
> On Tue, Sep 12, 2023 at 5:19 PM Corey Minyard wrote:
> >
> > On Tue, Sep 12, 2023 at 11:43:05PM +, Justin Stitt wrote:
> > > `strncpy` is deprecated for use on NUL-terminated destination strings [1].
> > >
> > > In this case, strncpy is being used specifically for its NUL-padding
> > > behavior (and has been commented as such). We can use a more robust and
> > > less ambiguous interface in `strscpy_pad` which makes the code more
> > > readable and even eliminates the need for that comment.
> > >
> > > Let's also use `strnlen` instead of `strlen()` with an upper-bounds
> > > check as this is intrinsically a part of `strnlen`.
> > >
> > > Also included in this patch is a simple 1:1 change of `strncpy` to
> > > `strscpy` for ipmi_ssif.c. If NUL-padding is wanted here as well then we
> > > should opt again for `strscpy_pad`.
> > >
> > > Link:
> > > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> > > [1]
> > > Link: https://github.com/KSPP/linux/issues/90
> > > Cc: linux-harden...@vger.kernel.org
> > > Cc: Kees Cook
> > > Signed-off-by: Justin Stitt
> > > ---
> > > drivers/char/ipmi/ipmi_msghandler.c | 11 +++
> > > drivers/char/ipmi/ipmi_ssif.c | 2 +-
> > > 2 files changed, 4 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/char/ipmi/ipmi_msghandler.c
> > > b/drivers/char/ipmi/ipmi_msghandler.c
> > > index 186f1fee7534..04f7622cb703 100644
> > > --- a/drivers/char/ipmi/ipmi_msghandler.c
> > > +++ b/drivers/char/ipmi/ipmi_msghandler.c
> > > @@ -5377,20 +5377,15 @@ static void send_panic_events(struct ipmi_smi
> > > *intf, char *str)
> > >
> > > j = 0;
> > > while (*p) {
> > > - int size = strlen(p);
> > > + int size = strnlen(p, 11);
> > >
> > > - if (size > 11)
> > > - size = 11;
> > > data[0] = 0;
> > > data[1] = 0;
> > > data[2] = 0xf0; /* OEM event without timestamp. */
> > > data[3] = intf->addrinfo[0].address;
> > > data[4] = j++; /* sequence # */
> > > - /*
> > > - * Always give 11 bytes, so strncpy will fill
> > > - * it with zeroes for me.
> > > - */
> > > - strncpy(data+5, p, 11);
> > > +
> > > + strscpy_pad(data+5, p, 11);
> >
> > This is incorrect, the destination should *not* be nil terminated if the
> > destination is full. strncpy does exactly what is needed here.
>
> Could we use `memcpy_and_pad()` as this matches the behavior of
> strncpy in this case? I understand strncpy works here but I'm really
> keen on snuffing out all its uses -- treewide.
^ I mean something like the following:
|memcpy_and_pad(data+5, 11, p, size, '\0');
as this is explicit in its behavior.
>
> >
> > A comment should be added here, this is not the first time this has been
> > brought up.
> >
> > > p += size;
> > >
> > > ipmi_panic_request_and_wait(intf, , );
> > > diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> > > index 3b921c78ba08..edcb83765dce 100644
> > > --- a/drivers/char/ipmi/ipmi_ssif.c
> > > +++ b/drivers/char/ipmi/ipmi_ssif.c
> > > @@ -1940,7 +1940,7 @@ static int new_ssif_client(int addr, char
> > > *adapter_name,
> > > }
> > > }
> > >
> > > - strncpy(addr_info->binfo.type, DEVICE_NAME,
> > > + strscpy(addr_info->binfo.type, DEVICE_NAME,
> > > sizeof(addr_info->binfo.type));
> >
> > This one is good.
> >
> > -corey
> >
> > > addr_info->binfo.addr = addr;
> > > addr_info->binfo.platform_data = addr_info;
> > >
> > > ---
> > > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
> > > change-id: 20230912-strncpy-drivers-char-ipmi-ipmi-dda47b3773fd
> > >
> > > Best regards,
> > > --
> > > Justin Stitt
> > >
Re: [PATCH] ipmi: refactor deprecated strncpy
On Tue, Sep 12, 2023 at 5:19 PM Corey Minyard wrote:
>
> On Tue, Sep 12, 2023 at 11:43:05PM +, Justin Stitt wrote:
> > `strncpy` is deprecated for use on NUL-terminated destination strings [1].
> >
> > In this case, strncpy is being used specifically for its NUL-padding
> > behavior (and has been commented as such). We can use a more robust and
> > less ambiguous interface in `strscpy_pad` which makes the code more
> > readable and even eliminates the need for that comment.
> >
> > Let's also use `strnlen` instead of `strlen()` with an upper-bounds
> > check as this is intrinsically a part of `strnlen`.
> >
> > Also included in this patch is a simple 1:1 change of `strncpy` to
> > `strscpy` for ipmi_ssif.c. If NUL-padding is wanted here as well then we
> > should opt again for `strscpy_pad`.
> >
> > Link:
> > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> > [1]
> > Link: https://github.com/KSPP/linux/issues/90
> > Cc: linux-harden...@vger.kernel.org
> > Cc: Kees Cook
> > Signed-off-by: Justin Stitt
> > ---
> > drivers/char/ipmi/ipmi_msghandler.c | 11 +++
> > drivers/char/ipmi/ipmi_ssif.c | 2 +-
> > 2 files changed, 4 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/char/ipmi/ipmi_msghandler.c
> > b/drivers/char/ipmi/ipmi_msghandler.c
> > index 186f1fee7534..04f7622cb703 100644
> > --- a/drivers/char/ipmi/ipmi_msghandler.c
> > +++ b/drivers/char/ipmi/ipmi_msghandler.c
> > @@ -5377,20 +5377,15 @@ static void send_panic_events(struct ipmi_smi
> > *intf, char *str)
> >
> > j = 0;
> > while (*p) {
> > - int size = strlen(p);
> > + int size = strnlen(p, 11);
> >
> > - if (size > 11)
> > - size = 11;
> > data[0] = 0;
> > data[1] = 0;
> > data[2] = 0xf0; /* OEM event without timestamp. */
> > data[3] = intf->addrinfo[0].address;
> > data[4] = j++; /* sequence # */
> > - /*
> > - * Always give 11 bytes, so strncpy will fill
> > - * it with zeroes for me.
> > - */
> > - strncpy(data+5, p, 11);
> > +
> > + strscpy_pad(data+5, p, 11);
>
> This is incorrect, the destination should *not* be nil terminated if the
> destination is full. strncpy does exactly what is needed here.
Could we use `memcpy_and_pad()` as this matches the behavior of
strncpy in this case? I understand strncpy works here but I'm really
keen on snuffing out all its uses -- treewide.
>
> A comment should be added here, this is not the first time this has been
> brought up.
>
> > p += size;
> >
> > ipmi_panic_request_and_wait(intf, , );
> > diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> > index 3b921c78ba08..edcb83765dce 100644
> > --- a/drivers/char/ipmi/ipmi_ssif.c
> > +++ b/drivers/char/ipmi/ipmi_ssif.c
> > @@ -1940,7 +1940,7 @@ static int new_ssif_client(int addr, char
> > *adapter_name,
> > }
> > }
> >
> > - strncpy(addr_info->binfo.type, DEVICE_NAME,
> > + strscpy(addr_info->binfo.type, DEVICE_NAME,
> > sizeof(addr_info->binfo.type));
>
> This one is good.
>
> -corey
>
> > addr_info->binfo.addr = addr;
> > addr_info->binfo.platform_data = addr_info;
> >
> > ---
> > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
> > change-id: 20230912-strncpy-drivers-char-ipmi-ipmi-dda47b3773fd
> >
> > Best regards,
> > --
> > Justin Stitt
> >
[PATCH] cpuidle: dt: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. We should prefer more robust and less ambiguous string interfaces. A suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on the destination buffer. With this, we can also drop the now unnecessary `CPUIDLE_(NAME|DESC)_LEN - 1` pieces. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-harden...@vger.kernel.org Signed-off-by: Justin Stitt --- Note: build-tested only --- drivers/cpuidle/dt_idle_states.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/cpuidle/dt_idle_states.c b/drivers/cpuidle/dt_idle_states.c index 12fec92a85fd..97feb7d8fb23 100644 --- a/drivers/cpuidle/dt_idle_states.c +++ b/drivers/cpuidle/dt_idle_states.c @@ -84,8 +84,8 @@ static int init_state_node(struct cpuidle_state *idle_state, * replace with kstrdup and pointer assignment when name * and desc become string pointers */ - strncpy(idle_state->name, state_node->name, CPUIDLE_NAME_LEN - 1); - strncpy(idle_state->desc, desc, CPUIDLE_DESC_LEN - 1); + strscpy(idle_state->name, state_node->name, CPUIDLE_NAME_LEN); + strscpy(idle_state->desc, desc, CPUIDLE_DESC_LEN); return 0; } --- base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c change-id: 20230913-strncpy-drivers-cpuidle-dt_idle_states-c-c84ea03c1379 Best regards, -- Justin Stitt
[PATCH] cpufreq: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. We should prefer more robust and less ambiguous string interfaces. Both `policy->last_governor` and `default_governor` are expected to be NUL-terminated which is shown by their heavy usage with other string apis like `strcmp`. A suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on the destination buffer. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-harden...@vger.kernel.org Signed-off-by: Justin Stitt --- Note: build-tested --- drivers/cpufreq/cpufreq.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 50bbc969ffe5..3eb851a03fce 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1607,7 +1607,7 @@ static void __cpufreq_offline(unsigned int cpu, struct cpufreq_policy *policy) } if (has_target()) - strncpy(policy->last_governor, policy->governor->name, + strscpy(policy->last_governor, policy->governor->name, CPUFREQ_NAME_LEN); else policy->last_policy = policy->policy; @@ -2951,7 +2951,7 @@ static int __init cpufreq_core_init(void) BUG_ON(!cpufreq_global_kobject); if (!strlen(default_governor)) - strncpy(default_governor, gov->name, CPUFREQ_NAME_LEN); + strscpy(default_governor, gov->name, CPUFREQ_NAME_LEN); return 0; } --- base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c change-id: 20230912-strncpy-drivers-cpufreq-cpufreq-c-1d800044b007 Best regards, -- Justin Stitt
[PATCH] ipmi: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1].
In this case, strncpy is being used specifically for its NUL-padding
behavior (and has been commented as such). We can use a more robust and
less ambiguous interface in `strscpy_pad` which makes the code more
readable and even eliminates the need for that comment.
Let's also use `strnlen` instead of `strlen()` with an upper-bounds
check as this is intrinsically a part of `strnlen`.
Also included in this patch is a simple 1:1 change of `strncpy` to
`strscpy` for ipmi_ssif.c. If NUL-padding is wanted here as well then we
should opt again for `strscpy_pad`.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
[1]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-harden...@vger.kernel.org
Cc: Kees Cook
Signed-off-by: Justin Stitt
---
drivers/char/ipmi/ipmi_msghandler.c | 11 +++
drivers/char/ipmi/ipmi_ssif.c | 2 +-
2 files changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_msghandler.c
b/drivers/char/ipmi/ipmi_msghandler.c
index 186f1fee7534..04f7622cb703 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -5377,20 +5377,15 @@ static void send_panic_events(struct ipmi_smi *intf,
char *str)
j = 0;
while (*p) {
- int size = strlen(p);
+ int size = strnlen(p, 11);
- if (size > 11)
- size = 11;
data[0] = 0;
data[1] = 0;
data[2] = 0xf0; /* OEM event without timestamp. */
data[3] = intf->addrinfo[0].address;
data[4] = j++; /* sequence # */
- /*
-* Always give 11 bytes, so strncpy will fill
-* it with zeroes for me.
-*/
- strncpy(data+5, p, 11);
+
+ strscpy_pad(data+5, p, 11);
p += size;
ipmi_panic_request_and_wait(intf, , );
diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
index 3b921c78ba08..edcb83765dce 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -1940,7 +1940,7 @@ static int new_ssif_client(int addr, char *adapter_name,
}
}
- strncpy(addr_info->binfo.type, DEVICE_NAME,
+ strscpy(addr_info->binfo.type, DEVICE_NAME,
sizeof(addr_info->binfo.type));
addr_info->binfo.addr = addr;
addr_info->binfo.platform_data = addr_info;
---
base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
change-id: 20230912-strncpy-drivers-char-ipmi-ipmi-dda47b3773fd
Best regards,
--
Justin Stitt
[PATCH] bus: fsl-mc: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. We need to prefer more robust and less ambiguous string interfaces. `obj_desc->(type|label)` are expected to be NUL-terminated strings as per "include/linux/fsl/mc.h +143" | ... | * struct fsl_mc_obj_desc - Object descriptor | * @type: Type of object: NULL terminated string | ... It seems `cmd_params->obj_type` is also expected to be a NUL-terminated string. A suitable replacement is `strscpy_pad` due to the fact that it guarantees NUL-termination on the destination buffer whilst keeping the NUL-padding behavior that `strncpy` provides. Padding may not strictly be necessary but let's opt to keep it as this ensures no functional change. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-harden...@vger.kernel.org Cc: Kees Cook Signed-off-by: Justin Stitt --- Note: build-tested --- drivers/bus/fsl-mc/dprc.c | 12 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/drivers/bus/fsl-mc/dprc.c b/drivers/bus/fsl-mc/dprc.c index d129338b8bc0..dd1b5c0fb7e2 100644 --- a/drivers/bus/fsl-mc/dprc.c +++ b/drivers/bus/fsl-mc/dprc.c @@ -450,10 +450,8 @@ int dprc_get_obj(struct fsl_mc_io *mc_io, obj_desc->ver_major = le16_to_cpu(rsp_params->version_major); obj_desc->ver_minor = le16_to_cpu(rsp_params->version_minor); obj_desc->flags = le16_to_cpu(rsp_params->flags); - strncpy(obj_desc->type, rsp_params->type, 16); - obj_desc->type[15] = '\0'; - strncpy(obj_desc->label, rsp_params->label, 16); - obj_desc->label[15] = '\0'; + strscpy_pad(obj_desc->type, rsp_params->type, 16); + strscpy_pad(obj_desc->label, rsp_params->label, 16); return 0; } EXPORT_SYMBOL_GPL(dprc_get_obj); @@ -491,8 +489,7 @@ int dprc_set_obj_irq(struct fsl_mc_io *mc_io, cmd_params->irq_addr = cpu_to_le64(irq_cfg->paddr); cmd_params->irq_num = cpu_to_le32(irq_cfg->irq_num); cmd_params->obj_id = cpu_to_le32(obj_id); - strncpy(cmd_params->obj_type, obj_type, 16); - cmd_params->obj_type[15] = '\0'; + strscpy_pad(cmd_params->obj_type, obj_type, 16); /* send command to mc*/ return mc_send_command(mc_io, ); @@ -564,8 +561,7 @@ int dprc_get_obj_region(struct fsl_mc_io *mc_io, cmd_params = (struct dprc_cmd_get_obj_region *)cmd.params; cmd_params->obj_id = cpu_to_le32(obj_id); cmd_params->region_index = region_index; - strncpy(cmd_params->obj_type, obj_type, 16); - cmd_params->obj_type[15] = '\0'; + strscpy_pad(cmd_params->obj_type, obj_type, 16); /* send command to mc*/ err = mc_send_command(mc_io, ); --- base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c change-id: 20230912-strncpy-drivers-bus-fsl-mc-dprc-c-bc7d818386ec Best regards, -- Justin Stitt
Re: [PATCH] um,ethertap: refactor deprecated strncpy
On Tue, Sep 12, 2023 at 12:36 AM Geert Uytterhoeven
wrote:
>
> Hi Justin,
>
> Thanks for your patch!
>
> On Mon, Sep 11, 2023 at 7:53 PM Justin Stitt wrote:
> > `strncpy` is deprecated for use on NUL-terminated destination strings [1].
> >
> > `gate_buf` should always be NUL-terminated and does not require
> > NUL-padding. It is used as a string arg inside an argv array given to
>
> Can you please explain why it does not require NUL-padding?
> It looks like this buffer is passed eventually to a user space
> application, thus possibly leaking uninitialized stack data.
It looks like it's being passed as a list of command-line arguments in
`run_helper()`. Should this be NUL-padded due to its eventual use in
user space? If we think yes I can send a v2. Thanks for pointing this
out.
>
> > `run_helper()`. Due to this, let's use `strscpy` as it guarantees
> > NUL-terminated on the destination buffer preventing potential buffer
> > overreads [2].
> >
> > This exact invocation was changed from `strcpy` to `strncpy` in commit
> > 7879b1d94badb ("um,ethertap: use strncpy") back in 2015. Let's continue
> > hardening our `str*cpy` apis and use the newer and safer `strscpy`!
> >
> > Link:
> > www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings[1]
> > Link:
> > https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
> > Link: https://github.com/KSPP/linux/issues/90
> > Cc: linux-harden...@vger.kernel.org
> > Cc: Kees Cook
> > Signed-off-by: Justin Stitt
> > ---
> > arch/um/os-Linux/drivers/ethertap_user.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/um/os-Linux/drivers/ethertap_user.c
> > b/arch/um/os-Linux/drivers/ethertap_user.c
> > index 9483021d86dd..3363851a4ae8 100644
> > --- a/arch/um/os-Linux/drivers/ethertap_user.c
> > +++ b/arch/um/os-Linux/drivers/ethertap_user.c
> > @@ -105,7 +105,7 @@ static int etap_tramp(char *dev, char *gate, int
> > control_me,
> > sprintf(data_fd_buf, "%d", data_remote);
> > sprintf(version_buf, "%d", UML_NET_VERSION);
> > if (gate != NULL) {
> > - strncpy(gate_buf, gate, 15);
> > + strscpy(gate_buf, gate, sizeof(gate_buf));
> > args = setup_args;
> > }
> > else args = nosetup_args;
> >
> > ---
> > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
> > change-id:
> > 20230911-strncpy-arch-um-os-linux-drivers-ethertap_user-c-859160d13f59
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> -- Linus Torvalds
Re: [PATCH] x86/tdx: refactor deprecated strncpy
On Mon, Sep 11, 2023 at 11:51 AM Dave Hansen wrote: > > On 9/11/23 11:27, Justin Stitt wrote: > > `strncpy` is deprecated and we should prefer more robust string apis. > > I dunno. It actually seems like a pretty good fit here. > > > In this case, `message.str` is not expected to be NUL-terminated as it > > is simply a buffer of characters residing in a union which allows for > > named fields representing 8 bytes each. There is only one caller of > > `tdx_panic()` and they use a 59-length string for `msg`: > > | const char *msg = "TD misconfiguration: SEPT_VE_DISABLE attribute > > must be set."; > > I'm not really following this logic. > > We need to do the following: > > 1. Make sure not to over write past the end of 'message' > 2. Make sure not to over read past the end of 'msg' > 3. Make sure not to leak stack data into the hypercall registers >in the case of short strings. > > strncpy() does #1, #2 and #3 just fine. Right, to be clear, I do not suspect a bug in the current implementation. Rather, let's move towards a less ambiguous interface for maintainability's sake > > The resulting string does *NOT* need to be NULL-terminated. See the > comment: > > /* VMM assumes '\0' in byte 65, if the message took all 64 bytes */ > > Are there cases where strncpy() doesn't NULL-terminate the string other > than when the buffer is full? > > I actually didn't realize that strncpy() pads its output up to the full > size. I wonder if Kirill used it intentionally or whether he got lucky > here. :) Big reason to use strtomem_pad as it is more obvious about what it does. I'd love more thoughts/testing here.
[PATCH] auxdisplay: panel: refactor deprecated strncpy
`strncpy` is deprecated and as such we should prefer more robust and
less ambiguous interfaces.
In this case, all of `press_str`, `repeat_str` and `release_str` are
explicitly marked as nonstring:
| struct {/* valid when type == INPUT_TYPE_KBD */
| char press_str[sizeof(void *) + sizeof(int)] __nonstring;
| char repeat_str[sizeof(void *) + sizeof(int)] __nonstring;
| char release_str[sizeof(void *) + sizeof(int)] __nonstring;
| } kbd;
... which makes `strtomem_pad` a suitable replacement as it is
functionally the same whilst being more obvious about its behavior.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
[1]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-harden...@vger.kernel.org
Cc: Kees Cook
Signed-off-by: Justin Stitt
---
Note: build-tested
---
drivers/auxdisplay/panel.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/auxdisplay/panel.c b/drivers/auxdisplay/panel.c
index eba04c0de7eb..e20d35bdf5fe 100644
--- a/drivers/auxdisplay/panel.c
+++ b/drivers/auxdisplay/panel.c
@@ -1449,10 +1449,9 @@ static struct logical_input *panel_bind_key(const char
*name, const char *press,
key->rise_time = 1;
key->fall_time = 1;
- strncpy(key->u.kbd.press_str, press, sizeof(key->u.kbd.press_str));
- strncpy(key->u.kbd.repeat_str, repeat, sizeof(key->u.kbd.repeat_str));
- strncpy(key->u.kbd.release_str, release,
- sizeof(key->u.kbd.release_str));
+ strtomem_pad(key->u.kbd.press_str, press, '\0');
+ strtomem_pad(key->u.kbd.repeat_str, repeat, '\0');
+ strtomem_pad(key->u.kbd.release_str, release, '\0');
list_add(>list, _inputs);
return key;
}
---
base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
change-id: 20230911-strncpy-drivers-auxdisplay-panel-c-83bce51f32cb
Best regards,
--
Justin Stitt
[PATCH] ACPI: OSI: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1].
We know `osi->string` is a NUL-terminated string due to its eventual use
in `acpi_install_interface()` and `acpi_remove_interface()` which expect
a `acpi_string` which has been specifically typedef'd as:
| typedef char *acpi_string; /* Null terminated ASCII string */
... and which also has other string functions used on it like `strlen`.
Furthermore, padding is not needed in this instance either.
Due to the reasoning above a suitable replacement is `strscpy` [2] since
it guarantees NUL-termination on the destination buffer and doesn't
unnecessarily NUL-pad.
While there is unlikely to be a buffer overread (or other related bug)
in this case, we should still favor a more robust and less ambiguous
interface.
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
[1]
Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-harden...@vger.kernel.org
Cc: Kees Cook
Signed-off-by: Justin Stitt
---
Note: build-tested
---
drivers/acpi/osi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/acpi/osi.c b/drivers/acpi/osi.c
index d4405e1ca9b9..df9328c850bd 100644
--- a/drivers/acpi/osi.c
+++ b/drivers/acpi/osi.c
@@ -110,7 +110,7 @@ void __init acpi_osi_setup(char *str)
break;
} else if (osi->string[0] == '\0') {
osi->enable = enable;
- strncpy(osi->string, str, OSI_STRING_LENGTH_MAX);
+ strscpy(osi->string, str, OSI_STRING_LENGTH_MAX);
break;
}
}
---
base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
change-id: 20230911-strncpy-drivers-acpi-osi-c-c801b7427987
Best regards,
--
Justin Stitt
[PATCH] x86/tdx: refactor deprecated strncpy
`strncpy` is deprecated and we should prefer more robust string apis. In this case, `message.str` is not expected to be NUL-terminated as it is simply a buffer of characters residing in a union which allows for named fields representing 8 bytes each. There is only one caller of `tdx_panic()` and they use a 59-length string for `msg`: | const char *msg = "TD misconfiguration: SEPT_VE_DISABLE attribute must be set."; Given all this information, let's use `strtomem_pad` as this matches the functionality of `strncpy` in this instances whilst being a more robust and easier to understand interface. Link: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings[1] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-harden...@vger.kernel.org Cc: Kees Cook Cc: Nick Desaulniers Signed-off-by: Justin Stitt --- Note: build-tested --- arch/x86/coco/tdx/tdx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c index 1d6b863c42b0..2e1be592c220 100644 --- a/arch/x86/coco/tdx/tdx.c +++ b/arch/x86/coco/tdx/tdx.c @@ -119,7 +119,7 @@ static void __noreturn tdx_panic(const char *msg) } message; /* VMM assumes '\0' in byte 65, if the message took all 64 bytes */ - strncpy(message.str, msg, 64); + strtomem_pad(message.str, msg, '\0'); args.r8 = message.r8; args.r9 = message.r9; --- base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c change-id: 20230911-strncpy-arch-x86-coco-tdx-tdx-c-98b0b966bb8d Best regards, -- Justin Stitt
[PATCH] xen/efi: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. `efi_loader_signature` has space for 4 bytes. We are copying "Xen" (3 bytes) plus a NUL-byte which makes 4 total bytes. With that being said, there is currently not a bug with the current `strncpy()` implementation in terms of buffer overreads but we should favor a more robust string interface either way. A suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on the destination buffer while being functionally the same in this case. Link: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings[1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-harden...@vger.kernel.org Cc: Kees Cook Signed-off-by: Justin Stitt --- Note: build-tested --- arch/x86/xen/efi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/xen/efi.c b/arch/x86/xen/efi.c index 863d0d6b3edc..7250d0e0e1a9 100644 --- a/arch/x86/xen/efi.c +++ b/arch/x86/xen/efi.c @@ -138,7 +138,7 @@ void __init xen_efi_init(struct boot_params *boot_params) if (efi_systab_xen == NULL) return; - strncpy((char *)_params->efi_info.efi_loader_signature, "Xen", + strscpy((char *)_params->efi_info.efi_loader_signature, "Xen", sizeof(boot_params->efi_info.efi_loader_signature)); boot_params->efi_info.efi_systab = (__u32)__pa(efi_systab_xen); boot_params->efi_info.efi_systab_hi = (__u32)(__pa(efi_systab_xen) >> 32); --- base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c change-id: 20230911-strncpy-arch-x86-xen-efi-c-14292f5a79ee Best regards, -- Justin Stitt
[PATCH] um,ethertap: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1].
`gate_buf` should always be NUL-terminated and does not require
NUL-padding. It is used as a string arg inside an argv array given to
`run_helper()`. Due to this, let's use `strscpy` as it guarantees
NUL-terminated on the destination buffer preventing potential buffer
overreads [2].
This exact invocation was changed from `strcpy` to `strncpy` in commit
7879b1d94badb ("um,ethertap: use strncpy") back in 2015. Let's continue
hardening our `str*cpy` apis and use the newer and safer `strscpy`!
Link:
www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings[1]
Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-harden...@vger.kernel.org
Cc: Kees Cook
Signed-off-by: Justin Stitt
---
arch/um/os-Linux/drivers/ethertap_user.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/um/os-Linux/drivers/ethertap_user.c
b/arch/um/os-Linux/drivers/ethertap_user.c
index 9483021d86dd..3363851a4ae8 100644
--- a/arch/um/os-Linux/drivers/ethertap_user.c
+++ b/arch/um/os-Linux/drivers/ethertap_user.c
@@ -105,7 +105,7 @@ static int etap_tramp(char *dev, char *gate, int control_me,
sprintf(data_fd_buf, "%d", data_remote);
sprintf(version_buf, "%d", UML_NET_VERSION);
if (gate != NULL) {
- strncpy(gate_buf, gate, 15);
+ strscpy(gate_buf, gate, sizeof(gate_buf));
args = setup_args;
}
else args = nosetup_args;
---
base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
change-id:
20230911-strncpy-arch-um-os-linux-drivers-ethertap_user-c-859160d13f59
Best regards,
--
Justin Stitt
