linux-hardening - Re: [PATCH] tracing: replace multiple deprecated strncpy with strscpy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

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