| Message ID | 20250403191342.1244863-1-devaanshk840@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tracing: Replace deprecated strncpy() with memcpy() for stack_trace_filter_buf | expand |
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(). -- 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
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
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
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;
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(-)