strncpy() is deprecated for use on NUL-terminated destination strings [1] and
as such we should prefer more robust and less ambiguous string interfaces.

String copy operations involving manual pointer offset and length
calculations followed by explicit NUL-byte assignments are best changed
to either strscpy or memcpy.

strscpy is not a drop-in replacement as @len would need a one subtracted
from it to avoid truncating the source string.

To not sabotage readability of the current code, use memcpy (retaining
the manual NUL assignment) as this unambiguously describes the desired
behavior.

Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
Link: https://github.com/KSPP/linux/issues/90 [2]
Cc: Kees Cook <keescook@chromium.org>
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
Changes in v2:
- use memcpy instead of strscpy (thanks Steven)
- No longer breaks Steven's test:

$ 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

- Link to v1: https://lore.kernel.org/r/20240930-strncpy-kernel-trace-trace_events_filter-c-v1-1-feed30820b83@google.com
---
 kernel/trace/trace_events_filter.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 0c611b281a5b..78051de581e7 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -1616,7 +1616,7 @@ static int parse_pred(const char *str, void *data,
 				goto err_free;
 			}
 
-			strncpy(num_buf, str + s, len);
+			memcpy(num_buf, str + s, len);
 			num_buf[len] = 0;
 
 			ret = kstrtoul(num_buf, 0, &ip);
@@ -1694,7 +1694,7 @@ static int parse_pred(const char *str, void *data,
 		if (!pred->regex)
 			goto err_mem;
 		pred->regex->len = len;
-		strncpy(pred->regex->pattern, str + s, len);
+		memcpy(pred->regex->pattern, str + s, len);
 		pred->regex->pattern[len] = 0;
 
 	} else if (!strncmp(str + i, "CPUS", 4)) {
@@ -1859,7 +1859,7 @@ static int parse_pred(const char *str, void *data,
 		if (!pred->regex)
 			goto err_mem;
 		pred->regex->len = len;
-		strncpy(pred->regex->pattern, str + s, len);
+		memcpy(pred->regex->pattern, str + s, len);
 		pred->regex->pattern[len] = 0;
 
 		filter_build_regex(pred);
@@ -1919,7 +1919,7 @@ static int parse_pred(const char *str, void *data,
 			goto err_free;
 		}
 
-		strncpy(num_buf, str + s, len);
+		memcpy(num_buf, str + s, len);
 		num_buf[len] = 0;
 
 		/* Make sure it is a value */

---
base-commit: bc83b4d1f08695e85e85d36f7b803da58010161d
change-id: 20240930-strncpy-kernel-trace-trace_events_filter-c-f44a3f848518

Best regards,
--
Justin Stitt <justinstitt@google.com>

On Mon, Oct 14, 2024 at 02:13:14PM -0700, Justin Stitt wrote:
> strncpy() is deprecated for use on NUL-terminated destination strings [1] and
> as such we should prefer more robust and less ambiguous string interfaces.
> 
> String copy operations involving manual pointer offset and length
> calculations followed by explicit NUL-byte assignments are best changed
> to either strscpy or memcpy.
> 
> strscpy is not a drop-in replacement as @len would need a one subtracted
> from it to avoid truncating the source string.
> 
> To not sabotage readability of the current code, use memcpy (retaining
> the manual NUL assignment) as this unambiguously describes the desired
> behavior.

We know the destination must have a NUL-terminated string. Is the src
NUL terminated? Looking at parse_pred(), it seems like no? And we can't
use memtostr_pad() here because the source buffer size isn't known at
compile time. Okay then. And there are no NUL bytes in the "str + s"
span, so yeah, it looks like memcpy() is best.

Reviewed-by: Kees Cook <kees@kernel.org>

-- 
Kees Cook

On Mon, 14 Oct 2024 14:31:29 -0700
Kees Cook <kees@kernel.org> wrote:
> 
> We know the destination must have a NUL-terminated string. Is the src
> NUL terminated? Looking at parse_pred(), it seems like no? And we can't
> use memtostr_pad() here because the source buffer size isn't known at
> compile time. Okay then. And there are no NUL bytes in the "str + s"
> span, so yeah, it looks like memcpy() is best.
> 
> Reviewed-by: Kees Cook <kees@kernel.org>
> 

The code is simply breaking up parts of one string and copying the bits
into other strings. Like the example that broke:

$ 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

It would need to move each of the above tokens "common_pid", "!=", "0", "&&", etc
into their own strings for later processing. The parser finds the start and
end of the location needed to copy, so memcpy() followed by a nul byte is fine.

-- Steve