[PATCH] watchdog/perf: reject empty config before the raw event parse

[PATCH] watchdog/perf: reject empty config before the raw event parse

[Bradley Morgan]

Date: Fri, 2 Oct 2026 17:39:13 +0000
Subject: [PATCH] watchdog/perf: reject empty config before the raw event parse

Commit 6164be01f179 ("watchdog/perf: optimize bytes copied and remove
manual NUL-termination") replaced strscpy(buf, str, sizeof(buf)) plus a
manual buf[len] = 0 with strscpy(buf, str, len). strscpy() writes
nothing at all when the count is zero, so on a comma directly after
the "r" prefix, nmi_watchdog=r,1 for example, len is 0, buf stays
uninitialized, and kstrtoull() parses whatever stack garbage is there.

The old code was safe here by accident, strscpy() filled the whole
buffer before buf[len] = 0 overwrote the comma position, so a zero
length just produced an empty string and a clean parse failure.

Treat an empty config the same as an overlong one and return early.

Fixes: 6164be01f179 ("watchdog/perf: optimize bytes copied and remove manual NUL-termination")
Signed-off-by: Bradley Morgan <brads@mainlining.org>
---
 kernel/watchdog_perf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/watchdog_perf.c b/kernel/watchdog_perf.c
index cf05775a96d3..cca0485ba28c 100644
--- a/kernel/watchdog_perf.c
+++ b/kernel/watchdog_perf.c
@@ -301,7 +301,7 @@ void __init hardlockup_config_perf_event(const char *str)
 	} else {
 		unsigned int len = comma - str;
 
-		if (len > sizeof(buf))
+		if (!len || len > sizeof(buf))
 			return;
 
 		strscpy(buf, str, len);
-- 
2.53.0

Re: [PATCH] watchdog/perf: reject empty config before the raw event parse

[Doug Anderson]

Hi,

On Fri, Oct 2, 2026 at 10:46 AM Bradley Morgan <brads@mainlining.org> wrote:
>
> Date: Fri, 2 Oct 2026 17:39:13 +0000
> Subject: [PATCH] watchdog/perf: reject empty config before the raw event parse
>
> Commit 6164be01f179 ("watchdog/perf: optimize bytes copied and remove
> manual NUL-termination") replaced strscpy(buf, str, sizeof(buf)) plus a
> manual buf[len] = 0 with strscpy(buf, str, len). strscpy() writes
> nothing at all when the count is zero, so on a comma directly after
> the "r" prefix, nmi_watchdog=r,1 for example, len is 0, buf stays
> uninitialized, and kstrtoull() parses whatever stack garbage is there.
>
> The old code was safe here by accident, strscpy() filled the whole
> buffer before buf[len] = 0 overwrote the comma position, so a zero
> length just produced an empty string and a clean parse failure.
>
> Treat an empty config the same as an overlong one and return early.
>
> Fixes: 6164be01f179 ("watchdog/perf: optimize bytes copied and remove manual NUL-termination")
> Signed-off-by: Bradley Morgan <brads@mainlining.org>
> ---
>  kernel/watchdog_perf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/watchdog_perf.c b/kernel/watchdog_perf.c
> index cf05775a96d3..cca0485ba28c 100644
> --- a/kernel/watchdog_perf.c
> +++ b/kernel/watchdog_perf.c
> @@ -301,7 +301,7 @@ void __init hardlockup_config_perf_event(const char *str)
>         } else {
>                 unsigned int len = comma - str;
>
> -               if (len > sizeof(buf))
> +               if (!len || len > sizeof(buf))
>                         return;
>
>                 strscpy(buf, str, len);

I fed your patch to my AI, and it wasn't happy with it. While you fix
one corner case, the larger problem is still there.

Image if `str` is "300,panic". Then `comma - str` will be 3. Passing 3
to `strscpy` will copy at most 2 characters so it has room to use the
3rd character as termination. That means `buf` will have "30", not
"300". Oops.

My AI suggests the correct fix is to change the "len >" to "len >="
and then pass "len + 1" to strscpy().

if (len >= sizeof(buf))
  return;

strscpy(buf, str, len + 1);

-Doug

Re: [PATCH] watchdog/perf: reject empty config before the raw event parse

[Bradley Morgan]

On 2 October 2026 23:12:33 BST, Doug Anderson <dianders@chromium.org>
wrote:
>Hi,
>
>On Fri, Oct 2, 2026 at 10:46 AM Bradley Morgan <brads@mainlining.org>
>wrote:
>>
>> Date: Fri, 2 Oct 2026 17:39:13 +0000
>> Subject: [PATCH] watchdog/perf: reject empty config before the raw event
>parse
>>
>> Commit 6164be01f179 ("watchdog/perf: optimize bytes copied and remove
>> manual NUL-termination") replaced strscpy(buf, str, sizeof(buf)) plus a
>> manual buf[len] = 0 with strscpy(buf, str, len). strscpy() writes
>> nothing at all when the count is zero, so on a comma directly after
>> the "r" prefix, nmi_watchdog=r,1 for example, len is 0, buf stays
>> uninitialized, and kstrtoull() parses whatever stack garbage is there.
>>
>> The old code was safe here by accident, strscpy() filled the whole
>> buffer before buf[len] = 0 overwrote the comma position, so a zero
>> length just produced an empty string and a clean parse failure.
>>
>> Treat an empty config the same as an overlong one and return early.
>>
>> Fixes: 6164be01f179 ("watchdog/perf: optimize bytes copied and remove
>manual NUL-termination")
>> Signed-off-by: Bradley Morgan <brads@mainlining.org>
>> ---
>>  kernel/watchdog_perf.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/watchdog_perf.c b/kernel/watchdog_perf.c
>> index cf05775a96d3..cca0485ba28c 100644
>> --- a/kernel/watchdog_perf.c
>> +++ b/kernel/watchdog_perf.c
>> @@ -301,7 +301,7 @@ void __init hardlockup_config_perf_event(const char
>*str)
>>         } else {
>>                 unsigned int len = comma - str;
>>
>> -               if (len > sizeof(buf))
>> +               if (!len || len > sizeof(buf))
>>                         return;
>>
>>                 strscpy(buf, str, len);
>
>I fed your patch to my AI, and it wasn't happy with it. While you fix
>one corner case, the larger problem is still there.
>
>Image if `str` is "300,panic". Then `comma - str` will be 3. Passing 3
>to `strscpy` will copy at most 2 characters so it has room to use the
>3rd character as termination. That means `buf` will have "30", not
>"300". Oops.
>
>My AI suggests the correct fix is to change the "len >" to "len >="
>and then pass "len + 1" to strscpy().
>
>if (len >= sizeof(buf))
>  return;
>
>strscpy(buf, str, len + 1);

Makes sense. Do you think you could put this into a separate patch, or
would you like me to respin? :)

>
>-Doug
>

--- Thanks!
"I'm not a very positive person" - Linus torvalds

Re: [PATCH] watchdog/perf: reject empty config before the raw event parse

[Doug Anderson]

Hi,

On Fri, Oct 2, 2026 at 3:17 PM Bradley Morgan <brads@mainlining.org> wrote:
>
> On 2 October 2026 23:12:33 BST, Doug Anderson <dianders@chromium.org>
> wrote:
> >Hi,
> >
> >On Fri, Oct 2, 2026 at 10:46 AM Bradley Morgan <brads@mainlining.org>
> >wrote:
> >>
> >> Date: Fri, 2 Oct 2026 17:39:13 +0000
> >> Subject: [PATCH] watchdog/perf: reject empty config before the raw event
> >parse
> >>
> >> Commit 6164be01f179 ("watchdog/perf: optimize bytes copied and remove
> >> manual NUL-termination") replaced strscpy(buf, str, sizeof(buf)) plus a
> >> manual buf[len] = 0 with strscpy(buf, str, len). strscpy() writes
> >> nothing at all when the count is zero, so on a comma directly after
> >> the "r" prefix, nmi_watchdog=r,1 for example, len is 0, buf stays
> >> uninitialized, and kstrtoull() parses whatever stack garbage is there.
> >>
> >> The old code was safe here by accident, strscpy() filled the whole
> >> buffer before buf[len] = 0 overwrote the comma position, so a zero
> >> length just produced an empty string and a clean parse failure.
> >>
> >> Treat an empty config the same as an overlong one and return early.
> >>
> >> Fixes: 6164be01f179 ("watchdog/perf: optimize bytes copied and remove
> >manual NUL-termination")
> >> Signed-off-by: Bradley Morgan <brads@mainlining.org>
> >> ---
> >>  kernel/watchdog_perf.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/kernel/watchdog_perf.c b/kernel/watchdog_perf.c
> >> index cf05775a96d3..cca0485ba28c 100644
> >> --- a/kernel/watchdog_perf.c
> >> +++ b/kernel/watchdog_perf.c
> >> @@ -301,7 +301,7 @@ void __init hardlockup_config_perf_event(const char
> >*str)
> >>         } else {
> >>                 unsigned int len = comma - str;
> >>
> >> -               if (len > sizeof(buf))
> >> +               if (!len || len > sizeof(buf))
> >>                         return;
> >>
> >>                 strscpy(buf, str, len);
> >
> >I fed your patch to my AI, and it wasn't happy with it. While you fix
> >one corner case, the larger problem is still there.
> >
> >Image if `str` is "300,panic". Then `comma - str` will be 3. Passing 3
> >to `strscpy` will copy at most 2 characters so it has room to use the
> >3rd character as termination. That means `buf` will have "30", not
> >"300". Oops.
> >
> >My AI suggests the correct fix is to change the "len >" to "len >="
> >and then pass "len + 1" to strscpy().
> >
> >if (len >= sizeof(buf))
> >  return;
> >
> >strscpy(buf, str, len + 1);
>
> Makes sense. Do you think you could put this into a separate patch, or
> would you like me to respin? :)

In the end, it's up to Andrew. If it were me, I'd request you respin.
The patch is currently in Andrew's "provisional" tree
(mm-nonmm-unstable), so if you send a v2 he will just drop the old
patch and replace it with your v2.

-Doug

[PATCH v2] watchdog/perf: fix off by one in the raw event config copy

[Bradley Morgan]

You were right, the truncation was the bigger half of the bug and my
v1 only closed the corner. Fixed both with your len + 1 shape.

Commit 6164be01f179 ("watchdog/perf: optimize bytes copied and remove
manual NUL-termination") replaced strscpy(buf, str, sizeof(buf)) plus a
manual buf[len] = 0 with strscpy(buf, str, len). That count is one less
than the code needs, strscpy() always reserves the last byte of the
destination for the NUL, so the config loses its final digit.
nmi_watchdog=r300,panic for example ends up with buf = "30" and arms
the raw event with the wrong config.

The same count also drops the empty case on the floor, strscpy() with
a zero count writes nothing at all, so nmi_watchdog=r,1 leaves buf
uninitialized and kstrtoull() reads stack garbage.

The old code was safe on both counts by accident, strscpy() filled the
whole buffer before buf[len] = 0 overwrote the comma position, so the
worst outcome was a truncated parse failure.

Pass len + 1 so the copy includes the character the NUL replaces, and
reject len >= sizeof(buf) like the code did before the optimization,
which also makes an empty config a clean parse failure again.

Suggested-by: Doug Anderson <dianders@chromium.org>
Fixes: 6164be01f179 ("watchdog/perf: optimize bytes copied and remove manual NUL-termination")
Signed-off-by: Bradley Morgan <brads@mainlining.org>
---
 kernel/watchdog_perf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/watchdog_perf.c b/kernel/watchdog_perf.c
index cca0485ba28c..a4f677c16b20 100644
--- a/kernel/watchdog_perf.c
+++ b/kernel/watchdog_perf.c
@@ -301,10 +301,10 @@ void __init hardlockup_config_perf_event(const char *str)
 	} else {
 		unsigned int len = comma - str;
 
-		if (!len || len > sizeof(buf))
+		if (len >= sizeof(buf))
 			return;
 
-		strscpy(buf, str, len);
+		strscpy(buf, str, len + 1);
 		if (kstrtoull(buf, 16, &config))
 			return;
 	}
-- 
2.53.0

Re: [PATCH v2] watchdog/perf: fix off by one in the raw event config copy

[Doug Anderson]

Hi,

On Fri, Oct 2, 2026 at 5:06 PM Bradley Morgan <brads@mainlining.org> wrote:
>
> You were right, the truncation was the bigger half of the bug and my
> v1 only closed the corner. Fixed both with your len + 1 shape.

The above should have been "after the cut". Where you have it now,
it'll end up in the commit message.


> Commit 6164be01f179 ("watchdog/perf: optimize bytes copied and remove
> manual NUL-termination") replaced strscpy(buf, str, sizeof(buf)) plus a
> manual buf[len] = 0 with strscpy(buf, str, len). That count is one less
> than the code needs, strscpy() always reserves the last byte of the
> destination for the NUL, so the config loses its final digit.
> nmi_watchdog=r300,panic for example ends up with buf = "30" and arms
> the raw event with the wrong config.
>
> The same count also drops the empty case on the floor, strscpy() with
> a zero count writes nothing at all, so nmi_watchdog=r,1 leaves buf
> uninitialized and kstrtoull() reads stack garbage.
>
> The old code was safe on both counts by accident, strscpy() filled the
> whole buffer before buf[len] = 0 overwrote the comma position, so the
> worst outcome was a truncated parse failure.
>
> Pass len + 1 so the copy includes the character the NUL replaces, and
> reject len >= sizeof(buf) like the code did before the optimization,
> which also makes an empty config a clean parse failure again.
>
> Suggested-by: Doug Anderson <dianders@chromium.org>
> Fixes: 6164be01f179 ("watchdog/perf: optimize bytes copied and remove manual NUL-termination")
> Signed-off-by: Bradley Morgan <brads@mainlining.org>
> ---
>  kernel/watchdog_perf.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/watchdog_perf.c b/kernel/watchdog_perf.c
> index cca0485ba28c..a4f677c16b20 100644
> --- a/kernel/watchdog_perf.c
> +++ b/kernel/watchdog_perf.c
> @@ -301,10 +301,10 @@ void __init hardlockup_config_perf_event(const char *str)
>         } else {
>                 unsigned int len = comma - str;
>
> -               if (!len || len > sizeof(buf))
> +               if (len >= sizeof(buf))

It looks like you sent this atop your previous v1 patch. You should
send your v2 patch as if your v1 patch wasn't applied.

Also, you probably don't need my Suggested-by tag. I just gave review
feedback on v1, which usually doesn't warrant a Suggested-by.

-Doug