[PATCH] tracing: Replace deprecated strncpy() with memcpy() for stack_trace_filter_buf

[PATCH] tracing: Replace deprecated strncpy() with memcpy() for stack_trace_filter_buf

[Devaansh Kumar]

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.1

Re: [PATCH] tracing: Replace deprecated strncpy() with memcpy() for stack_trace_filter_buf

[Steven Rostedt]

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;
> -- 

Re: [PATCH] tracing: Replace deprecated strncpy() with memcpy() for stack_trace_filter_buf

[Devaansh Kumar]

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

Re: [PATCH] tracing: Replace deprecated strncpy() with memcpy() for stack_trace_filter_buf

[Mathieu Desnoyers]

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

Re: [PATCH] tracing: Replace deprecated strncpy() with memcpy() for stack_trace_filter_buf

[Devaansh Kumar]

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

Re: [PATCH] tracing: Replace deprecated strncpy() with memcpy() for stack_trace_filter_buf

[Steven Rostedt]

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