On Tue, Jun 13, 2023 at 3:27 PM Kees Cook [off-list ref] wrote:

On Tue, Jun 13, 2023 at 12:41:25AM +0000, Azeem Shaikh wrote:

quoted

strlcpy() reads the entire source buffer first.
This read may exceed the destination size limit.
This is both inefficient and can lead to linear read
overflows if a source string is not NUL-terminated [1].
In an effort to remove strlcpy() completely [2], replace
strlcpy() here with strscpy().

Direct replacement is safe here since return value of -E2BIG
is used to check for truncation instead of sizeof(dest).

This looks technically correct, but I wonder if "< 0" is a better test?

Agreed. "< 0" might more generically represent -errno. Happy to send
over a v2 if you prefer that instead of sticking with this patch.

quoted

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
[2] https://github.com/KSPP/linux/issues/89

Signed-off-by: Azeem Shaikh <redacted>

Either way

Reviewed-by: Kees Cook <redacted>

-Kees

quoted

---
 kernel/trace/trace_boot.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/trace/trace_boot.c b/kernel/trace/trace_boot.c
index 778200dd8ede..5fe525f1b8cc 100644
--- a/kernel/trace/trace_boot.c
+++ b/kernel/trace/trace_boot.c

@@ -31,7 +31,7 @@ trace_boot_set_instance_options(struct trace_array *tr, struct xbc_node *node)

      /* Common ftrace options */
      xbc_node_for_each_array_value(node, "options", anode, p) {
-             if (strlcpy(buf, p, ARRAY_SIZE(buf)) >= ARRAY_SIZE(buf)) {
+             if (strscpy(buf, p, ARRAY_SIZE(buf)) == -E2BIG) {
                      pr_err("String is too long: %s\n", p);
                      continue;
              }

@@ -87,7 +87,7 @@ trace_boot_enable_events(struct trace_array *tr, struct xbc_node *node)
      const char *p;

      xbc_node_for_each_array_value(node, "events", anode, p) {
-             if (strlcpy(buf, p, ARRAY_SIZE(buf)) >= ARRAY_SIZE(buf)) {
+             if (strscpy(buf, p, ARRAY_SIZE(buf)) == -E2BIG) {
                      pr_err("String is too long: %s\n", p);
                      continue;
              }

@@ -486,7 +486,7 @@ trace_boot_init_one_event(struct trace_array *tr, struct xbc_node *gnode,

      p = xbc_node_find_value(enode, "filter", NULL);
      if (p && *p != '\0') {
-             if (strlcpy(buf, p, ARRAY_SIZE(buf)) >= ARRAY_SIZE(buf))
+             if (strscpy(buf, p, ARRAY_SIZE(buf)) == -E2BIG)
                      pr_err("filter string is too long: %s\n", p);
              else if (apply_event_filter(file, buf) < 0)
                      pr_err("Failed to apply filter: %s\n", buf);

@@ -494,7 +494,7 @@ trace_boot_init_one_event(struct trace_array *tr, struct xbc_node *gnode,

      if (IS_ENABLED(CONFIG_HIST_TRIGGERS)) {
              xbc_node_for_each_array_value(enode, "actions", anode, p) {
-                     if (strlcpy(buf, p, ARRAY_SIZE(buf)) >= ARRAY_SIZE(buf))
+                     if (strscpy(buf, p, ARRAY_SIZE(buf)) == -E2BIG)
                              pr_err("action string is too long: %s\n", p);
                      else if (trigger_process_regex(file, buf) < 0)
                              pr_err("Failed to apply an action: %s\n", p);
--

2.41.0.162.gfafddb0af9-goog

--
Kees Cook