Skip to content

Commit

Permalink
kernfs: Convert kernfs_path_from_node_locked() from strlcpy() to strs…
Browse files Browse the repository at this point in the history
…cpy()

One of the last remaining users of strlcpy() in the kernel is
kernfs_path_from_node_locked(), which passes back the problematic "length
we _would_ have copied" return value to indicate truncation.  Convert the
chain of all callers to use the negative return value (some of which
already doing this explicitly). All callers were already also checking
for negative return values, so the risk to missed checks looks very low.

In this analysis, it was found that cgroup1_release_agent() actually
didn't handle the "too large" condition, so this is technically also a
bug fix. :)

Here's the chain of callers, and resolution identifying each one as now
handling the correct return value:

kernfs_path_from_node_locked()
        kernfs_path_from_node()
                pr_cont_kernfs_path()
                        returns void
                kernfs_path()
                        sysfs_warn_dup()
                                return value ignored
                        cgroup_path()
                                blkg_path()
                                        bfq_bic_update_cgroup()
                                                return value ignored
                                TRACE_IOCG_PATH()
                                        return value ignored
                                TRACE_CGROUP_PATH()
                                        return value ignored
                                perf_event_cgroup()
                                        return value ignored
                                task_group_path()
                                        return value ignored
                                damon_sysfs_memcg_path_eq()
                                        return value ignored
                                get_mm_memcg_path()
                                        return value ignored
                                lru_gen_seq_show()
                                        return value ignored
                        cgroup_path_from_kernfs_id()
                                return value ignored
                cgroup_show_path()
                        already converted "too large" error to negative value
                cgroup_path_ns_locked()
                        cgroup_path_ns()
                                bpf_iter_cgroup_show_fdinfo()
                                        return value ignored
                                cgroup1_release_agent()
                                        wasn't checking "too large" error
                        proc_cgroup_show()
                                already converted "too large" to negative value

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Zefan Li <lizefan.x@bytedance.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Waiman Long <longman@redhat.com>
Cc:  <cgroups@vger.kernel.org>
Co-developed-by: Azeem Shaikh <azeemshaikh38@gmail.com>
Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>
Link: https://lore.kernel.org/r/20231116192127.1558276-3-keescook@chromium.org
Signed-off-by: Kees Cook <keescook@chromium.org>
Link: https://lore.kernel.org/r/20231212211741.164376-3-keescook@chromium.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
  • Loading branch information
Kees Cook authored and Greg Kroah-Hartman committed Dec 15, 2023
1 parent 5b56bf5 commit ff6d413
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 21 deletions.
34 changes: 17 additions & 17 deletions fs/kernfs/dir.c
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ static struct kernfs_node *kernfs_common_ancestor(struct kernfs_node *a,
*
* [3] when @kn_to is %NULL result will be "(null)"
*
* Return: the length of the full path. If the full length is equal to or
* Return: the length of the constructed path. If the path would have been
* greater than @buflen, @buf contains the truncated path with the trailing
* '\0'. On error, -errno is returned.
*/
Expand All @@ -138,16 +138,17 @@ static int kernfs_path_from_node_locked(struct kernfs_node *kn_to,
struct kernfs_node *kn, *common;
const char parent_str[] = "/..";
size_t depth_from, depth_to, len = 0;
ssize_t copied;
int i, j;

if (!kn_to)
return strlcpy(buf, "(null)", buflen);
return strscpy(buf, "(null)", buflen);

if (!kn_from)
kn_from = kernfs_root(kn_to)->kn;

if (kn_from == kn_to)
return strlcpy(buf, "/", buflen);
return strscpy(buf, "/", buflen);

common = kernfs_common_ancestor(kn_from, kn_to);
if (WARN_ON(!common))
Expand All @@ -158,18 +159,19 @@ static int kernfs_path_from_node_locked(struct kernfs_node *kn_to,

buf[0] = '\0';

for (i = 0; i < depth_from; i++)
len += strlcpy(buf + len, parent_str,
len < buflen ? buflen - len : 0);
for (i = 0; i < depth_from; i++) {
copied = strscpy(buf + len, parent_str, buflen - len);
if (copied < 0)
return copied;
len += copied;
}

/* Calculate how many bytes we need for the rest */
for (i = depth_to - 1; i >= 0; i--) {
for (kn = kn_to, j = 0; j < i; j++)
kn = kn->parent;
len += strlcpy(buf + len, "/",
len < buflen ? buflen - len : 0);
len += strlcpy(buf + len, kn->name,
len < buflen ? buflen - len : 0);

len += scnprintf(buf + len, buflen - len, "/%s", kn->name);
}

return len;
Expand Down Expand Up @@ -214,7 +216,7 @@ int kernfs_name(struct kernfs_node *kn, char *buf, size_t buflen)
* path (which includes '..'s) as needed to reach from @from to @to is
* returned.
*
* Return: the length of the full path. If the full length is equal to or
* Return: the length of the constructed path. If the path would have been
* greater than @buflen, @buf contains the truncated path with the trailing
* '\0'. On error, -errno is returned.
*/
Expand Down Expand Up @@ -265,12 +267,10 @@ void pr_cont_kernfs_path(struct kernfs_node *kn)
sz = kernfs_path_from_node(kn, NULL, kernfs_pr_cont_buf,
sizeof(kernfs_pr_cont_buf));
if (sz < 0) {
pr_cont("(error)");
goto out;
}

if (sz >= sizeof(kernfs_pr_cont_buf)) {
pr_cont("(name too long)");
if (sz == -E2BIG)
pr_cont("(name too long)");
else
pr_cont("(error)");
goto out;
}

Expand Down
2 changes: 1 addition & 1 deletion kernel/cgroup/cgroup-v1.c
Original file line number Diff line number Diff line change
Expand Up @@ -802,7 +802,7 @@ void cgroup1_release_agent(struct work_struct *work)
goto out_free;

ret = cgroup_path_ns(cgrp, pathbuf, PATH_MAX, &init_cgroup_ns);
if (ret < 0 || ret >= PATH_MAX)
if (ret < 0)
goto out_free;

argv[0] = agentbuf;
Expand Down
4 changes: 2 additions & 2 deletions kernel/cgroup/cgroup.c
Original file line number Diff line number Diff line change
Expand Up @@ -1893,7 +1893,7 @@ int cgroup_show_path(struct seq_file *sf, struct kernfs_node *kf_node,
len = kernfs_path_from_node(kf_node, ns_cgroup->kn, buf, PATH_MAX);
spin_unlock_irq(&css_set_lock);

if (len >= PATH_MAX)
if (len == -E2BIG)
len = -ERANGE;
else if (len > 0) {
seq_escape(sf, buf, " \t\n\\");
Expand Down Expand Up @@ -6278,7 +6278,7 @@ int proc_cgroup_show(struct seq_file *m, struct pid_namespace *ns,
if (cgroup_on_dfl(cgrp) || !(tsk->flags & PF_EXITING)) {
retval = cgroup_path_ns_locked(cgrp, buf, PATH_MAX,
current->nsproxy->cgroup_ns);
if (retval >= PATH_MAX)
if (retval == -E2BIG)
retval = -ENAMETOOLONG;
if (retval < 0)
goto out_unlock;
Expand Down
2 changes: 1 addition & 1 deletion kernel/cgroup/cpuset.c
Original file line number Diff line number Diff line change
Expand Up @@ -4941,7 +4941,7 @@ int proc_cpuset_show(struct seq_file *m, struct pid_namespace *ns,
retval = cgroup_path_ns(css->cgroup, buf, PATH_MAX,
current->nsproxy->cgroup_ns);
css_put(css);
if (retval >= PATH_MAX)
if (retval == -E2BIG)
retval = -ENAMETOOLONG;
if (retval < 0)
goto out_free;
Expand Down

0 comments on commit ff6d413

Please sign in to comment.