kernel/trace/trace_stack.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
strncpy() is deprecated for NUL-terminated destination buffers and must
be replaced by memcpy() for length bounded buffers.
See issue: https://github.com/KSPP/linux/issues/90
Signed-off-by: Devaansh Kumar <devaanshk840@gmail.com>
---
kernel/trace/trace_stack.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
index 5a48dba912ea..427526fd2afd 100644
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -3,6 +3,7 @@
* Copyright (C) 2008 Steven Rostedt <srostedt@redhat.com>
*
*/
+#include <linux/compiler_attributes.h>
#include <linux/sched/task_stack.h>
#include <linux/stacktrace.h>
#include <linux/security.h>
@@ -537,14 +538,16 @@ stack_trace_sysctl(struct ctl_table *table, int write, void *buffer,
return ret;
}
-static char stack_trace_filter_buf[COMMAND_LINE_SIZE+1] __initdata;
+static char stack_trace_filter_buf[COMMAND_LINE_SIZE+1] __initdata __nonstring;
static __init int enable_stacktrace(char *str)
{
int len;
- if ((len = str_has_prefix(str, "_filter=")))
- strncpy(stack_trace_filter_buf, str + len, COMMAND_LINE_SIZE);
+ len = str_has_prefix(str, "_filter=");
+
+ if (len)
+ memcpy(stack_trace_filter_buf, str + len, sizeof(stack_trace_filter_buf));
stack_tracer_enabled = 1;
return 1;
--
2.47.1On Fri, 4 Apr 2025 00:43:40 +0530
Devaansh Kumar <devaanshk840@gmail.com> wrote:
> @@ -537,14 +538,16 @@ stack_trace_sysctl(struct ctl_table *table, int write, void *buffer,
> return ret;
> }
>
> -static char stack_trace_filter_buf[COMMAND_LINE_SIZE+1] __initdata;
> +static char stack_trace_filter_buf[COMMAND_LINE_SIZE+1] __initdata __nonstring;
>
> static __init int enable_stacktrace(char *str)
> {
> int len;
>
> - if ((len = str_has_prefix(str, "_filter=")))
> - strncpy(stack_trace_filter_buf, str + len, COMMAND_LINE_SIZE);
> + len = str_has_prefix(str, "_filter=");
> +
> + if (len)
> + memcpy(stack_trace_filter_buf, str + len, sizeof(stack_trace_filter_buf));
Hmm, this location looks like it can just use strscpy().
-- Steve
>
> stack_tracer_enabled = 1;
> return 1;
> --
On Fri, 4 Apr 2025 at 01:05, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 4 Apr 2025 00:43:40 +0530
> Devaansh Kumar <devaanshk840@gmail.com> wrote:
>
> > @@ -537,14 +538,16 @@ stack_trace_sysctl(struct ctl_table *table, int write, void *buffer,
> > return ret;
> > }
> >
> > -static char stack_trace_filter_buf[COMMAND_LINE_SIZE+1] __initdata;
> > +static char stack_trace_filter_buf[COMMAND_LINE_SIZE+1] __initdata __nonstring;
> >
> > static __init int enable_stacktrace(char *str)
> > {
> > int len;
> >
> > - if ((len = str_has_prefix(str, "_filter=")))
> > - strncpy(stack_trace_filter_buf, str + len, COMMAND_LINE_SIZE);
> > + len = str_has_prefix(str, "_filter=");
> > +
> > + if (len)
> > + memcpy(stack_trace_filter_buf, str + len, sizeof(stack_trace_filter_buf));
>
> Hmm, this location looks like it can just use strscpy().
Yes strscpy() also works. But since stack_trace_filter_buf is length
bounded, shouldn't memcpy be the right choice?
-- Devaansh Kumar
On 2025-04-04 08:28, Devaansh Kumar wrote:
> On Fri, 4 Apr 2025 at 01:05, Steven Rostedt <rostedt@goodmis.org> wrote:
>>
>> On Fri, 4 Apr 2025 00:43:40 +0530
>> Devaansh Kumar <devaanshk840@gmail.com> wrote:
>>
>>> @@ -537,14 +538,16 @@ stack_trace_sysctl(struct ctl_table *table, int write, void *buffer,
>>> return ret;
>>> }
>>>
>>> -static char stack_trace_filter_buf[COMMAND_LINE_SIZE+1] __initdata;
>>> +static char stack_trace_filter_buf[COMMAND_LINE_SIZE+1] __initdata __nonstring;
>>>
>>> static __init int enable_stacktrace(char *str)
>>> {
>>> int len;
>>>
>>> - if ((len = str_has_prefix(str, "_filter=")))
>>> - strncpy(stack_trace_filter_buf, str + len, COMMAND_LINE_SIZE);
>>> + len = str_has_prefix(str, "_filter=");
>>> +
>>> + if (len)
>>> + memcpy(stack_trace_filter_buf, str + len, sizeof(stack_trace_filter_buf));
>>
>> Hmm, this location looks like it can just use strscpy().
>
> Yes strscpy() also works. But since stack_trace_filter_buf is length
> bounded, shouldn't memcpy be the right choice?
It's not only about the destination, but also about the source length.
AFAIU, turning a strncpy into a memcpy here will overflow reading the
input @str if the input string is smaller than
sizeof(stack_trace_filter_buf) + len.
This can trigger page faults or make KASAN unhappy.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com
On Fri, 4 Apr 2025 08:54:33 -0400 Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > >>> - if ((len = str_has_prefix(str, "_filter="))) > >>> - strncpy(stack_trace_filter_buf, str + len, COMMAND_LINE_SIZE); > >>> + len = str_has_prefix(str, "_filter="); > >>> + > >>> + if (len) > >>> + memcpy(stack_trace_filter_buf, str + len, sizeof(stack_trace_filter_buf)); > >> > >> Hmm, this location looks like it can just use strscpy(). > > > > Yes strscpy() also works. But since stack_trace_filter_buf is length > > bounded, shouldn't memcpy be the right choice? > > It's not only about the destination, but also about the source length. Correct. > > AFAIU, turning a strncpy into a memcpy here will overflow reading the > input @str if the input string is smaller than > sizeof(stack_trace_filter_buf) + len. The old code just read str + len and what was after it until it hit a '\0' or the COMMAND_LINE_SIZE limit. memcpy() always reads COMMAND_LINE_SIZE (which is sizeof(stack_trace_filter_buf)) and will read more of the source "str" than may exist. Which as Mathieu pointed out, is a bug. -- Steve
On Fri, 4 Apr 2025 at 18:24, Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > It's not only about the destination, but also about the source length. > > AFAIU, turning a strncpy into a memcpy here will overflow reading the > input @str if the input string is smaller than > sizeof(stack_trace_filter_buf) + len. > > This can trigger page faults or make KASAN unhappy. I see. I will create a new patch to fix this then
© 2016 - 2026 Red Hat, Inc.