| Message ID | 20241014-strncpy-kernel-trace-trace_events_filter-c-v2-1-d821e81e371e@google.com (mailing list archive) |
|---|---|
| State | Queued |
| Delegated to: | Steven Rostedt |
| Headers | show |
| Series | [v2] tracing: replace multiple deprecated strncpy with memcpy | expand |
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>
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
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 */
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(-) --- base-commit: bc83b4d1f08695e85e85d36f7b803da58010161d change-id: 20240930-strncpy-kernel-trace-trace_events_filter-c-f44a3f848518 Best regards, -- Justin Stitt <justinstitt@google.com>