[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAFhGd8pGaMxXpsk5gMS98223FW5wq-vQPyt1srVCrP_Fg6Ex9g@mail.gmail.com>
Date: Fri, 11 Oct 2024 14:59:16 -0700
From: Justin Stitt <justinstitt@...gle.com>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: Masami Hiramatsu <mhiramat@...nel.org>, Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
linux-hardening@...r.kernel.org, Kees Cook <kees@...nel.org>
Subject: Re: [PATCH] tracing: replace multiple deprecated strncpy with strscpy
On Tue, Oct 8, 2024 at 6:13 PM Steven Rostedt <rostedt@...dmis.org> wrote:
>
> So this breaks my tests. This is why I have trouble with taking changes
> like this :-(
Shoot.
I think deprecated API cleanups are important but creating more bugs
in the process is not helping anybody. I dropped the ball here... my
patch has an off-by-one.
>
> Before this patch, his worked:
>
> # echo 'common_pid != 0 && common_pid != 120 && common_pid != 1253 && common_pid != 17 && common_pid != 394 && common_pid != 81 && common_pid != 87' > /sys/kernel/tracing/events/sched/sched_switch/filter
Thanks for providing the test case, this made triaging dead simple.
>
> But now it gives an error of:
>
> -bash: echo: write error: Invalid argument
>
> I have to drop this.
In many cases where folks are doing 1) strncpy and 2) manual NUL-byte
assignment, the clear replacement is strscpy. However most of those
cases look like this:
strncpy(dst, src, len);
dst[len-1] = '\0';
and this case was just
strncpy(dst, src, len);
dst[len] = '\0';
Since we have an explicit size check before the first copy, ensuring
@len doesn't overflow @dst, this code is fine but I missed the
off-by-one.
So, assuming I haven't lost your faith, I can send a v2 along the lines of:
1)
strscpy(num_buf, str + s, len + 1);
... or
2)
memcpy(num_buf, str + s, len);
num_buf[len] = 0;
And if you're wondering about option 3: "Don't change anything because
the code works". I'd reiterate that I think it's important to replace
bad ambiguous APIs. There are many cases where folks use strncpy() as
a glorified memcpy because they want the padding behavior, or they use
it on non-null terminated destinations or tons of other "misuses".
Ambiguous code like that poses a real danger to the maintainability of
the codebase and opens threat vectors.
>
> -- Steve
Thanks
Justin
Powered by blists - more mailing lists